Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Ulrich Hecht <uli@fpond.eu>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	Ulrich Hecht <uli+renesas@fpond.eu>
Subject: Re: [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device
Date: Thu, 16 May 2019 15:25:29 +0200
Message-ID: <20190516132529.GE31788@bigcity.dyn.berto.se> (raw)
In-Reply-To: <20190516112644.GB14820@pendragon.ideasonboard.com>

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

  reply index

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
2019-05-16  1:14 ` [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}() Niklas Söderlund
2019-05-16  7:23   ` Geert Uytterhoeven
2019-05-16 11:32     ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable} Niklas Söderlund
2019-05-16  7:27   ` Geert Uytterhoeven
2019-05-16 11:34     ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device Niklas Söderlund
2019-05-16 11:21   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening " Niklas Söderlund
2019-05-16 11:26   ` Laurent Pinchart
2019-05-16 13:25     ` Niklas Söderlund [this message]
2019-05-16  1:14 ` [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers Niklas Söderlund
2019-05-16 11:39   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice Niklas Söderlund
2019-05-16  7:40   ` Ulrich Hecht
2019-05-16 11:44   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open() Niklas Söderlund
2019-05-16 11:45   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
2019-05-16 11:48   ` Laurent Pinchart

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190516132529.GE31788@bigcity.dyn.berto.se \
    --to=niklas.soderlund@ragnatech.se \
    --cc=laurent.pinchart@ideasonboard.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=uli+renesas@fpond.eu \
    --cc=uli@fpond.eu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox