From patchwork Fri Aug 3 18:42:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ezequiel Garcia X-Patchwork-Id: 1272141 Return-Path: X-Original-To: patchwork-linux-media@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id B933FDF25A for ; Fri, 3 Aug 2012 18:43:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752496Ab2HCSmZ (ORCPT ); Fri, 3 Aug 2012 14:42:25 -0400 Received: from mail-gh0-f174.google.com ([209.85.160.174]:64352 "EHLO mail-gh0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751527Ab2HCSmY (ORCPT ); Fri, 3 Aug 2012 14:42:24 -0400 Received: by ghrr11 with SMTP id r11so1199310ghr.19 for ; Fri, 03 Aug 2012 11:42:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=HUaIwpIr+8jgIdmQSXu+3qIvUVMMiv+8J3LL/At1u7E=; b=Lh/wuMG/JHNa32lhNVUT9dVDKcRCYvuVgASSbLo7UCNGSNdz0iBDJX2QtlZkhcsBsE iuLgRFnmk1MG8rC0O1Ts7E99h1kn/aSxW9QzzQmMjanVb0ufDaTJRAmlCNwQz1ZgiHqi fH195cLcnsu3KSth/bhL6mtgYuprDOh2ZW9OTh4IRX6jlIVgCgx+21v5HcwCxzlqOcEw Fs5bDflYl/zqe8JuOnXYBL7OQh3wo1jq3kgHJMIdAiu4z7v0wg1WxYV2fCp+I0eWIb7y 3bFZBzJeNHy0zxfcZBr4tZNrUol//858hUm+N7r/a3LCSLm9aUBVB1FrSkuLb7FVvoBs n2nA== MIME-Version: 1.0 Received: by 10.50.41.201 with SMTP id h9mr12424813igl.37.1344019343800; Fri, 03 Aug 2012 11:42:23 -0700 (PDT) Received: by 10.64.163.98 with HTTP; Fri, 3 Aug 2012 11:42:23 -0700 (PDT) In-Reply-To: References: <1344016352-20302-1-git-send-email-elezegarcia@gmail.com> Date: Fri, 3 Aug 2012 15:42:23 -0300 Message-ID: Subject: Re: [PATCH] em28xx: Fix height setting on non-progressive captures From: Ezequiel Garcia To: Devin Heitmueller Cc: linux-media , Mauro Carvalho Chehab Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Hi Devin, Thanks for answering. On Fri, Aug 3, 2012 at 3:26 PM, Devin Heitmueller wrote: > On Fri, Aug 3, 2012 at 2:11 PM, Ezequiel Garcia wrote: >> On Fri, Aug 3, 2012 at 2:52 PM, Ezequiel Garcia wrote: >>> This was introduced on commit c2a6b54a9: >>> "em28xx: fix: don't do image interlacing on webcams" >>> It is a known bug that has already been reported several times >>> and confirmed by Mauro. >>> Tested by compilation only. >>> >> >> I wonder if it's possible to get an Ack or a Tested-By from any of the >> em28xx owners? > > This shouldn't be accepted upstream without testing at least on x86. > I did make such a change to make it work in my ARM tree, but I don't > fully understand the nature of the change and I'm not completely > confident it's correct for x86 (based on my reading of the datasheet > and how the accumulator field is structured in the em28xx chip). > Also, I actually don't have any progressive devices (I've got probably > a dozen em28xx devices, but they are all interlaced capture), which > made me particularly hesitant to submit this patch. > Wait a minute, unless I completely misunderstood the bug (which is possible), I think this patch is straightforward. By the look of this hunk on commit c2a6b54a: ---------------------------------8<-------------------------- height = height / 2 (or height = height >> 1) as was before, and as my patch is doing. It seems to driver will "merge" the interlaced frames and so the "expected" height is half the real height. I hope I got it right. That said and no matter how straightforward may be, which I'm not sure, I also want the patch to get tested before being accepted. >> Also, Devin: you mentioned in an old mail [1] you had some patches for em28xx, >> but you had no time to put them into shape for submission. >> >> If you want to, send then to me (or the full em28xx tree) and I can >> try to submit >> the patches. > > Yeah, probably not a bad idea. I've been sitting on the tree because > they haven't been tested on any other platforms and some of them are > not necessarily generally suitable for the mainline kernel. And of > course the tree needs to be parsed out into an actual patch series, > and each patch has to be individually validated across multiple > devices to ensure they don't cause breakage (they were tested on an > em2863, but I have no idea if they cause problems on other chips such > as the em2820 or em2880). > > All that said, I'm not really sure what the benefit would be in > sending you the tree if you don't actually have any hardware to test > with. The last thing we need is more crap being sent upstream that is > "compile tested only" since that's where many of the regressions come > from (well meaning people sending completely untested 'cleanup > patches' can cause more harm than good). > Mmm, you're right. I was rather thinking in patches that fixes "obvious" (whatever that may be) things and assuming these patches could get some community testing. So: never mind, bad idea. I've sent enough zero-test patches, which only means more work for Mauro :-( Thanks, Ezequiel. --- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/media/video/em28xx/em28xx-core.c b/drivers/media/video/em28xx/em28xx-core.c index 5b78e19..339fffd 100644 --- a/drivers/media/video/em28xx/em28xx-core.c +++ b/drivers/media/video/em28xx/em28xx-core.c @@ -720,7 +720,10 @@ int em28xx_resolution_set(struct em28xx *dev) { int width, height; width = norm_maxw(dev); - height = norm_maxh(dev) >> 1; + height = norm_maxh(dev); + + if (!dev->progressive) + height >>= norm_maxh(dev); --------------------------------->8-------------------------- It seems to me that for non-progressive the height should just be