Re: [PATCH] media: rcar-csi2: do not update format while streaming

From: "Niklas Söderlund" <niklas.soderlund@ragnatech.se>
To: Dennis Rachui <drachui@de.adit-jv.com>
Cc: Steve Longerbeam <slongerbeam@gmail.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Maxime Ripard <mripard@kernel.org>,
	Sakari Ailus <sakari.ailus@linux.intel.com>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: rcar-csi2: do not update format while streaming
Date: Fri, 9 Jul 2021 16:20:40 +0200	[thread overview]
Message-ID: <YOhbOHnCn9eFgKWG@oden.dyn.berto.se> (raw)
In-Reply-To: <1625750578-108454-1-git-send-email-drachui@de.adit-jv.com>

Hi Dennis,

Thanks for your patch.

On 2021-07-08 15:22:58 +0200, Dennis Rachui wrote:
> Verify that streaming is not active before setting the pad format.
> 
> According to the VIDIOC documentation [1] changes to the active
> format of a media pad via the VIDIOC_SUBDEV_S_FMT ioctl are
> applied to the underlying hardware.
> In rcar-csi2 a format change only applies to hardware, when the
> pipeline is started. While the device is not in use, it is therefore
> okay to update the format.
> 
> However, when the pipeline is active, this leads to a format
> mismatch between driver and device.
> Other applications can query the format with
> VIDIOC_SUBDEV_G_FMT at any time and would be reported
> a format that does not fit the current stream.
> 
> This commit prevents format update while streaming is active
> and returns -EBUSY to user space, as suggested by [1].
> 
> [1] Documentation/userspace-api/media/v4l/vidioc-subdev-g-fmt.rst

I like that this is addressed, but I wonder is this not something that 
should be fixed in the V4L2 core and not in drivers?

> 
> Note: after creation of this commit, it was noticed that Steve
> Longerbeam has a very similar solution in his fork.
> 
> Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
> Cc: Steve Longerbeam <slongerbeam@gmail.com>
> Signed-off-by: Dennis Rachui <drachui@de.adit-jv.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index e28eff0..98152e1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -724,18 +724,37 @@ static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
>  	struct v4l2_mbus_framefmt *framefmt;
> +	int ret = 0;
> +
> +	mutex_lock(&priv->lock);
>  
>  	if (!rcsi2_code_to_fmt(format->format.code))
>  		format->format.code = rcar_csi2_formats[0].code;
>  
>  	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> +
> +		/*
> +		 * Do not apply changes to active format while streaming.
> +		 *
> +		 * Since video streams could be forwarded from sink pad to any
> +		 * source pad (depending on CSI-2 channel routing), all
> +		 * media pads are effected by this rule.
> +		 */
> +		if (priv->stream_count > 0) {
> +			ret = -EBUSY;
> +			goto out;
> +		}
> +
>  		priv->mf = format->format;
>  	} else {
>  		framefmt = v4l2_subdev_get_try_format(sd, sd_state, 0);
>  		*framefmt = format->format;
>  	}
>  
> -	return 0;
> +out:
> +	mutex_unlock(&priv->lock);
> +
> +	return ret;
>  }
>  
>  static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund