Hi Hans, On Fri, 24 Jul 2015, Hans Verkuil wrote: > Guennadi, > > I want to make a pull request for this patch series. This patch is the only > outstanding one. Right, sorry for the delay. Replying to your explanation below: > Or do you have to review more patches? I only got Acks for patches 1 and > 2. > > Regards, > > Hans > > On 06/22/2015 09:29 AM, Hans Verkuil wrote: > > On 06/22/2015 09:21 AM, Guennadi Liakhovetski wrote: > >> Hi Hans, > >> > >> On Mon, 22 Jun 2015, Hans Verkuil wrote: > >> > >>> On 06/21/2015 07:23 PM, Guennadi Liakhovetski wrote: > >>>> On Mon, 15 Jun 2015, Hans Verkuil wrote: > >>>> > >>>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx> > >>>>> > >>>>> When the standard changes the VACTIVE and VDELAY values need to be updated. > >>>>> > >>>>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx> > >>>>> --- > >>>>> drivers/media/i2c/soc_camera/tw9910.c | 29 ++++++++++++++++++++++++++++- > >>>>> 1 file changed, 28 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c > >>>>> index df66417..e939c24 100644 > >>>>> --- a/drivers/media/i2c/soc_camera/tw9910.c > >>>>> +++ b/drivers/media/i2c/soc_camera/tw9910.c > >>>>> @@ -820,6 +846,7 @@ static int tw9910_video_probe(struct i2c_client *client) > >>>>> "tw9910 Product ID %0x:%0x\n", id, priv->revision); > >>>>> > >>>>> priv->norm = V4L2_STD_NTSC; > >>>>> + priv->scale = &tw9910_ntsc_scales[0]; > >>>> > >>>> Why do you need this? So far everywhere in the code priv->scale is either > >>>> checked or set before use. Don't see why an additional initialisation is > >>>> needed. > >>> > >>> If you just start streaming without explicitly setting up formats (which is > >>> allowed), then priv->scale is still NULL. > >> > >> Yes, it can well be NULL, but it is also unused. Before it is used it will > >> be set, while it is unused it is allowed to stay NULL. > > > > No. If you start streaming without the set_fmt op having been called, then > > s_stream will return an error since priv->scale is NULL. This is wrong. Since > > this driver defaults to NTSC the initial setup should be for NTSC and it should > > be ready for streaming. > > > > So priv->scale *is* used: in s_stream. And it is not necessarily set before use. > > E.g. if you load the driver and run 'v4l2-ctl --stream-out-mmap' you will hit this > > case. It's how I found this bug. > > > > It's a trivial one liner to ensure a valid priv->scale pointer. Yes, you're right, now I see how this can happen. But there's also another possibility: if S_FMT fails priv->scale will be set to NULL. If you then directly start streaming wouldn't the same problem arise? Or is it valid to fail STREAMON after a failed S_FMT? If it is, then of course Acked-by: Guennadi Liakhovetski <g.liakhovetski@xxxxxx> If that's invalid, then maybe a more extensive fix is needed? Thanks Guennadi > > Regards, > > > > Hans > > > >> > >> Thanks > >> Guennadi > >> > >>> V4L2 always assumes that there is some initial format configured, and this line > >>> enables that for this driver (NTSC). > >>> > >>> Regards, > >>> > >>> Hans -- 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
