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@xxxxxx> 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@xxxxxxxxxxxxxxxxx> > >> --- > >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
