Re: [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Laurent,

Thanks for your feedback.

On 2019-05-16 14:26:44 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, May 16, 2019 at 03:14:13AM +0200, Niklas Söderlund wrote:
> > The format is already synced when the subdevice is bound, there is no
> > need to do do it every time the video device is opened.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> 
> This seems a bug fix to me, formats should not be modified at open()
> time according to the V4L2 spec. You may want to add a Fixes: line,
> although I suppose this would go back to the origins of the driver, and
> this series is likely not a candidate for the stable tree, so it may not
> be a good idea.

I agree, a fixes tag could have been useful. But as you deduce this go 
way back to the beginning pre Gen3 support and I don't think it would 
backport easily. As it's not critical I'm fine moving forward without 
such a tag.

> 
> I'm slightly worried of side effects as rvin_s_fmt_vid_cap() calls
> rvin_try_format() which in turn calls v4l2_subdev_call(sd, pad, set_fmt,
> pad_cfg, &format). If you're confident that there's no risk of breakage,

I was worried to, but then I concluded the risk for breakage is the same 
with or without the call to v4l2_subdev_call(..., set_fmt, ...) at bound 
time. The breakage is if the video source format is changed between 
bound to stream on without the user setting the format. The same risk 
exists between open() and stream on before this patch. The user should 
always set the format before it starts to stream. And once I'm daring 
enough to drop the devnode centric support from rcar-vin this will go 
away entirely.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 ---------------------
> >  1 file changed, 25 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index f67cef97b89a3bd4..71651c5a69483367 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -782,38 +782,13 @@ static int rvin_initialize_device(struct file *file)
> >  	struct rvin_dev *vin = video_drvdata(file);
> >  	int ret;
> >  
> > -	struct v4l2_format f = {
> > -		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> > -		.fmt.pix = {
> > -			.width		= vin->format.width,
> > -			.height		= vin->format.height,
> > -			.field		= vin->format.field,
> > -			.colorspace	= vin->format.colorspace,
> > -			.pixelformat	= vin->format.pixelformat,
> > -		},
> > -	};
> > -
> >  	ret = rvin_power_on(vin);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	/*
> > -	 * Try to configure with default parameters. Notice: this is the
> > -	 * very first open, so, we cannot race against other calls,
> > -	 * apart from someone else calling open() simultaneously, but
> > -	 * .host_lock is protecting us against it.
> > -	 */
> > -	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
> > -	if (ret < 0)
> > -		goto esfmt;
> > -
> >  	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> >  
> >  	return 0;
> > -esfmt:
> > -	rvin_power_off(vin);
> > -
> > -	return ret;
> >  }
> >  
> >  static int rvin_open(struct file *file)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund