Message ID | 1324566720-14073-1-git-send-email-javier.martin@vista-silicon.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1RdkJc-0006cq-2H; Thu, 22 Dec 2011 16:12:16 +0100 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-4) with esmtp id 1RdkJb-0000UE-AS; Thu, 22 Dec 2011 16:12:15 +0100 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752989Ab1LVPMK (ORCPT <rfc822;patchwork@linuxtv.org> + 3 others); Thu, 22 Dec 2011 10:12:10 -0500 Received: from mail-we0-f174.google.com ([74.125.82.174]:56975 "EHLO mail-we0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752246Ab1LVPMJ (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 22 Dec 2011 10:12:09 -0500 Received: by werm1 with SMTP id m1so3397194wer.19 for <linux-media@vger.kernel.org>; Thu, 22 Dec 2011 07:12:08 -0800 (PST) Received: by 10.216.131.150 with SMTP id m22mr14710915wei.8.1324566728428; Thu, 22 Dec 2011 07:12:08 -0800 (PST) Received: from localhost.localdomain (74.51.18.95.dynamic.jazztel.es. [95.18.51.74]) by mx.google.com with ESMTPS id u5sm10072572wbm.2.2011.12.22.07.12.06 (version=TLSv1/SSLv3 cipher=OTHER); Thu, 22 Dec 2011 07:12:07 -0800 (PST) From: Javier Martin <javier.martin@vista-silicon.com> To: linux-media@vger.kernel.org Cc: mchehab@infradead.org, g.liakhovetski@gmx.de, lethal@linux-sh.org, hans.verkuil@cisco.com, s.hauer@pengutronix.de, Javier Martin <javier.martin@vista-silicon.com> Subject: [PATCH v2] media i.MX27 camera: Fix field_count handling. Date: Thu, 22 Dec 2011 16:12:00 +0100 Message-Id: <1324566720-14073-1-git-send-email-javier.martin@vista-silicon.com> X-Mailer: git-send-email 1.7.0.4 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2011.12.22.150018 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_1400_1499 0, BODY_SIZE_2000_LESS 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, __ANY_URI 0, __CP_MEDIA_BODY 0, __CP_URI_IN_BODY 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' X-LSpam-Score: -1.9 (-) X-LSpam-Report: No, score=-1.9 required=5.0 tests=BAYES_00=-1.9 autolearn=ham |
Commit Message
Javier Martin
Dec. 22, 2011, 3:12 p.m. UTC
To properly detect frame loss the driver must keep
track of a frame_count.
Furthermore, field_count use was erroneous because
in progressive format this must be incremented twice.
Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
drivers/media/video/mx2_camera.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
Comments
On Thu, 22 Dec 2011, Javier Martin wrote: > To properly detect frame loss the driver must keep > track of a frame_count. > > Furthermore, field_count use was erroneous because > in progressive format this must be incremented twice. Hm, sorry, why this? I just looked at vivi.c - the version before videobuf2 conversion - and it seems to only increment the count by one. > > Signed-off-by: Javier Martin <javier.martin@vista-silicon.com> > --- > drivers/media/video/mx2_camera.c | 5 ++++- > 1 files changed, 4 insertions(+), 1 deletions(-) > > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c > index ea1f4dc..ca76dd2 100644 > --- a/drivers/media/video/mx2_camera.c > +++ b/drivers/media/video/mx2_camera.c > @@ -255,6 +255,7 @@ struct mx2_camera_dev { > dma_addr_t discard_buffer_dma; > size_t discard_size; > struct mx2_fmt_cfg *emma_prp; > + u32 frame_count; The rule I usually follow, when choosing variable type is the following: does it really have to be fixed bit-width? The positive reply is pretty rare, it comes mostly if (a) the variable is used to store values read from or written to some (fixed-width) hardware registers, or (b) the variable belongs to a fixed ABI, that has to be the same on different (32-bit, 64-bit) systems, like (arguably) ioctl()s, data, transferred over the network or stored on a medium (filesystems,...). This doesn't seem to be the case here, so, I would just use an (unsigned) int. Thanks Guennadi > }; > > /* buffer for one video frame */ > @@ -368,6 +369,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd) > writel(pcdev->csicr1, pcdev->base_csi + CSICR1); > > pcdev->icd = icd; > + pcdev->frame_count = 0; > > dev_info(icd->parent, "Camera driver attached to camera %d\n", > icd->devnum); > @@ -1211,7 +1213,8 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev, > list_del(&vb->queue); > vb->state = state; > do_gettimeofday(&vb->ts); > - vb->field_count++; > + vb->field_count = pcdev->frame_count * 2; > + pcdev->frame_count++; > > wake_up(&vb->done); > } > -- > 1.7.0.4 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
Hi Guennadi, thank you for your comments. On 23 December 2011 00:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Thu, 22 Dec 2011, Javier Martin wrote: > >> To properly detect frame loss the driver must keep >> track of a frame_count. >> >> Furthermore, field_count use was erroneous because >> in progressive format this must be incremented twice. > > Hm, sorry, why this? I just looked at vivi.c - the version before > videobuf2 conversion - and it seems to only increment the count by one. If you look at the videobuf-core code you'll notice that the value assigned to v4l2_buf sequence field is (field_count >> 1): http://lxr.linux.no/#linux+v3.1.6/drivers/media/video/videobuf-core.c#L370 Since mx2_camera driver only supports video formats which are either progressive or provide both fields in the same buffer, this "field_count" must be incremented twice so that the final sequence number is OK. >> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com> >> --- >> drivers/media/video/mx2_camera.c | 5 ++++- >> 1 files changed, 4 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c >> index ea1f4dc..ca76dd2 100644 >> --- a/drivers/media/video/mx2_camera.c >> +++ b/drivers/media/video/mx2_camera.c >> @@ -255,6 +255,7 @@ struct mx2_camera_dev { >> dma_addr_t discard_buffer_dma; >> size_t discard_size; >> struct mx2_fmt_cfg *emma_prp; >> + u32 frame_count; > > The rule I usually follow, when choosing variable type is the following: > does it really have to be fixed bit-width? The positive reply is pretty > rare, it comes mostly if (a) the variable is used to store values read > from or written to some (fixed-width) hardware registers, or (b) the > variable belongs to a fixed ABI, that has to be the same on different > (32-bit, 64-bit) systems, like (arguably) ioctl()s, data, transferred over > the network or stored on a medium (filesystems,...). This doesn't seem to > be the case here, so, I would just use an (unsigned) int. Thanks for the tip. I hadn't thought of it that way. I just saw that v4l2_buf.sequence was a __u32 and I thought it was convenient to use the same type for this variable which is closely related to it Anyway, let me send a second version of the patch since I've just noticed this one doesn't reflect lost frames in the field_count field.
On Fri, 23 Dec 2011, javier Martin wrote: > Hi Guennadi, > thank you for your comments. > > On 23 December 2011 00:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > On Thu, 22 Dec 2011, Javier Martin wrote: > > > >> To properly detect frame loss the driver must keep > >> track of a frame_count. > >> > >> Furthermore, field_count use was erroneous because > >> in progressive format this must be incremented twice. > > > > Hm, sorry, why this? I just looked at vivi.c - the version before > > videobuf2 conversion - and it seems to only increment the count by one. > > If you look at the videobuf-core code you'll notice that the value > assigned to v4l2_buf sequence field is (field_count >> 1): Right, i.e., field-count / 2. So, it really only counts _frames_, not fields, doesn't it? Thanks Guennadi > http://lxr.linux.no/#linux+v3.1.6/drivers/media/video/videobuf-core.c#L370 > > Since mx2_camera driver only supports video formats which are either > progressive or provide both fields in the same buffer, this > "field_count" must be incremented twice so that the final sequence > number is OK. > > >> > >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com> > >> --- > >> drivers/media/video/mx2_camera.c | 5 ++++- > >> 1 files changed, 4 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c > >> index ea1f4dc..ca76dd2 100644 > >> --- a/drivers/media/video/mx2_camera.c > >> +++ b/drivers/media/video/mx2_camera.c > >> @@ -255,6 +255,7 @@ struct mx2_camera_dev { > >> dma_addr_t discard_buffer_dma; > >> size_t discard_size; > >> struct mx2_fmt_cfg *emma_prp; > >> + u32 frame_count; > > > > The rule I usually follow, when choosing variable type is the following: > > does it really have to be fixed bit-width? The positive reply is pretty > > rare, it comes mostly if (a) the variable is used to store values read > > from or written to some (fixed-width) hardware registers, or (b) the > > variable belongs to a fixed ABI, that has to be the same on different > > (32-bit, 64-bit) systems, like (arguably) ioctl()s, data, transferred over > > the network or stored on a medium (filesystems,...). This doesn't seem to > > be the case here, so, I would just use an (unsigned) int. > > Thanks for the tip. I hadn't thought of it that way. I just saw that > v4l2_buf.sequence was a __u32 and I thought it was convenient to use > the same type for this variable which is closely related to it > > Anyway, let me send a second version of the patch since I've just > noticed this one doesn't reflect lost frames in the field_count field. > > -- > Javier Martin > Vista Silicon S.L. > CDTUC - FASE C - Oficina S-345 > Avda de los Castros s/n > 39005- Santander. Cantabria. Spain > +34 942 25 32 60 > www.vista-silicon.com > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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
On 23 December 2011 10:07, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Fri, 23 Dec 2011, javier Martin wrote: > >> Hi Guennadi, >> thank you for your comments. >> >> On 23 December 2011 00:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: >> > On Thu, 22 Dec 2011, Javier Martin wrote: >> > >> >> To properly detect frame loss the driver must keep >> >> track of a frame_count. >> >> >> >> Furthermore, field_count use was erroneous because >> >> in progressive format this must be incremented twice. >> > >> > Hm, sorry, why this? I just looked at vivi.c - the version before >> > videobuf2 conversion - and it seems to only increment the count by one. >> >> If you look at the videobuf-core code you'll notice that the value >> assigned to v4l2_buf sequence field is (field_count >> 1): > > Right, i.e., field-count / 2. So, it really only counts _frames_, not > fields, doesn't it? > Yes, v4l2_buf sequence field counts _frames_ but field_count counts _fields_, that's why I increment field-count twice in my driver.
diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c index ea1f4dc..ca76dd2 100644 --- a/drivers/media/video/mx2_camera.c +++ b/drivers/media/video/mx2_camera.c @@ -255,6 +255,7 @@ struct mx2_camera_dev { dma_addr_t discard_buffer_dma; size_t discard_size; struct mx2_fmt_cfg *emma_prp; + u32 frame_count; }; /* buffer for one video frame */ @@ -368,6 +369,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd) writel(pcdev->csicr1, pcdev->base_csi + CSICR1); pcdev->icd = icd; + pcdev->frame_count = 0; dev_info(icd->parent, "Camera driver attached to camera %d\n", icd->devnum); @@ -1211,7 +1213,8 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev, list_del(&vb->queue); vb->state = state; do_gettimeofday(&vb->ts); - vb->field_count++; + vb->field_count = pcdev->frame_count * 2; + pcdev->frame_count++; wake_up(&vb->done); }