Em Mon, 25 Apr 2016 14:42:31 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> > So, I would go to the following enclosed patch.
>
> Looks good to me. Acked below. Amazing how many bugs one can make in one
> simple patch...
Applied, thanks!
Yeah, simple patches are harder than complex ones ;)
>
> >
> > Ezequiel,
> >
> > Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
> > As the device seems to support setting the fps, you should be adding
> > support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.
> >
> > On both ioctls, the driver should return the actual framerate used.
> > So, you'll need to add a code that would convert from the 15 possible
> > framerate converter register settings to v4l2_fract.
> >
> >>
> >>> + i = 14; /* 25 fps */
> >>> + else
> >>> + i = std_625_50[fps];
> >>> + } else {
> >>> + if (unlikely(i > ARRAY_SIZE(std_525_60)))
> >>> + i = 0; /* 30 fps */
> >>> + else
> >>> + i = std_525_60[fps];
> >>> + }
> >>>
> >>> return map[i];
> >>> }
> >>>
> >>
> >> Regards,
> >>
> >> Hans
> >
> > Thanks,
> > Mauro
> >
> > -
> >
> > [media] tw686x: Don't go past array
> >
> > Depending on the compiler version, currently it produces the
> > following warnings:
> > tw686x-video.c: In function 'tw686x_video_init':
> > tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> >
> > This is actually bogus with the current code, as it currently
> > hardcodes the framerate to 30 frames/sec, however a potential
> > use after the array size could happen when the driver adds support
> > for setting the framerate. So, fix it.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx>
>
> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> >
> > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> > index 118e9fac9f28..9468fda69f3d 100644
> > --- a/drivers/media/pci/tw686x/tw686x-video.c
> > +++ b/drivers/media/pci/tw686x/tw686x-video.c
> > @@ -61,8 +61,17 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
> > 8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
> > };
> >
> > - unsigned int i =
> > - (std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> > + unsigned int i;
> > +
> > + if (std & V4L2_STD_525_60) {
> > + if (fps > ARRAY_SIZE(std_525_60))
> > + fps = 30;
> > + i = std_525_60[fps];
> > + } else {
> > + if (fps > ARRAY_SIZE(std_625_50))
> > + fps = 25;
> > + i = std_625_50[fps];
> > + }
> >
> > return map[i];
> > }
> >
> >
> > --
> > 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
> >
>
--
Thanks,
Mauro
--
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
