| Message ID | 20190516011417.10590-5-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rcar-vin: Merge Gen2 and Gen3 file operations | expand |
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'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, 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)
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
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)