* [PATCH v2] media i.MX27 camera: Fix field_count handling.
@ 2011-12-22 15:12 Javier Martin
2011-12-22 23:17 ` Guennadi Liakhovetski
0 siblings, 1 reply; 5+ messages in thread
From: Javier Martin @ 2011-12-22 15:12 UTC (permalink / raw)
To: linux-media
Cc: mchehab, g.liakhovetski, lethal, hans.verkuil, s.hauer, Javier Martin
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(-)
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);
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media i.MX27 camera: Fix field_count handling.
2011-12-22 15:12 [PATCH v2] media i.MX27 camera: Fix field_count handling Javier Martin
@ 2011-12-22 23:17 ` Guennadi Liakhovetski
2011-12-23 7:54 ` javier Martin
0 siblings, 1 reply; 5+ messages in thread
From: Guennadi Liakhovetski @ 2011-12-22 23:17 UTC (permalink / raw)
To: Javier Martin; +Cc: linux-media, mchehab, lethal, hans.verkuil, s.hauer
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media i.MX27 camera: Fix field_count handling.
2011-12-22 23:17 ` Guennadi Liakhovetski
@ 2011-12-23 7:54 ` javier Martin
2011-12-23 9:07 ` Guennadi Liakhovetski
0 siblings, 1 reply; 5+ messages in thread
From: javier Martin @ 2011-12-23 7:54 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: linux-media, mchehab, lethal, hans.verkuil, s.hauer
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.
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media i.MX27 camera: Fix field_count handling.
2011-12-23 7:54 ` javier Martin
@ 2011-12-23 9:07 ` Guennadi Liakhovetski
2011-12-23 10:49 ` javier Martin
0 siblings, 1 reply; 5+ messages in thread
From: Guennadi Liakhovetski @ 2011-12-23 9:07 UTC (permalink / raw)
To: javier Martin; +Cc: linux-media, mchehab, lethal, hans.verkuil, s.hauer
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/
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] media i.MX27 camera: Fix field_count handling.
2011-12-23 9:07 ` Guennadi Liakhovetski
@ 2011-12-23 10:49 ` javier Martin
0 siblings, 0 replies; 5+ messages in thread
From: javier Martin @ 2011-12-23 10:49 UTC (permalink / raw)
To: Guennadi Liakhovetski; +Cc: linux-media, mchehab, lethal, hans.verkuil, s.hauer
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.
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-12-23 10:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-22 15:12 [PATCH v2] media i.MX27 camera: Fix field_count handling Javier Martin
2011-12-22 23:17 ` Guennadi Liakhovetski
2011-12-23 7:54 ` javier Martin
2011-12-23 9:07 ` Guennadi Liakhovetski
2011-12-23 10:49 ` javier Martin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.