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

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

[Dennis Rachui]

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

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

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

[Niklas Söderlund]

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

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

[Laurent Pinchart]

Hi Niklas,

On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> 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?

Some drivers may support format changes during streaming (that's allowed
by the V4L2 API, I'm not sure if it's used anywhere though). While I'd
favour not duplicating the same logic in different (and differently
buggy) ways in drivers, I'm not sure how this could be implemented in a
sane way in the V4L2 core in its current state.

> > 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,

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

Hi Laurent,

On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > 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?
> 
> Some drivers may support format changes during streaming (that's allowed
> by the V4L2 API, I'm not sure if it's used anywhere though). While I'd
> favour not duplicating the same logic in different (and differently
> buggy) ways in drivers, I'm not sure how this could be implemented in a
> sane way in the V4L2 core in its current state.

I understand it's possible from some devices to support to format 
changes during streaming, but as you point out it's the exception and 
not the rule, if used at all.

So my point is if we start to enforce this in drivers we are headed down 
a road where this will be messier to clean up. Would it not make more 
sens to default the V4L2 core to disallow format changes while streaming 
and add a new flag to V4L2_SUBDEV_CAP_ to signal that the subdevice 
supports format changes while streaming?

We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a subdevice 
only supports read-only operations so I think it would not be too hard 
to move this functionality into the core?

> 
> > > 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,
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

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

[Laurent Pinchart]

Hi Niklas,

On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > 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?
> > 
> > Some drivers may support format changes during streaming (that's allowed
> > by the V4L2 API, I'm not sure if it's used anywhere though). While I'd
> > favour not duplicating the same logic in different (and differently
> > buggy) ways in drivers, I'm not sure how this could be implemented in a
> > sane way in the V4L2 core in its current state.
> 
> I understand it's possible from some devices to support to format 
> changes during streaming, but as you point out it's the exception and 
> not the rule, if used at all.
> 
> So my point is if we start to enforce this in drivers we are headed down 
> a road where this will be messier to clean up. Would it not make more 
> sens to default the V4L2 core to disallow format changes while streaming 
> and add a new flag to V4L2_SUBDEV_CAP_ to signal that the subdevice 
> supports format changes while streaming?
> 
> We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a subdevice 
> only supports read-only operations so I think it would not be too hard 
> to move this functionality into the core?

Yes, that's something we could try. The subdev core will then need to
track the streaming state, which may require wrapping the .s_stream()
call. Locking should then also likely be handled by the core. Probably
nothing impossible, but quite a bit of work. Any volunteer ? :-)

> > > > 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,

-- 
Regards,

Laurent Pinchart

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

[Niklas Söderlund]

Hi Laurent,

On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > 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?
> > > 
> > > Some drivers may support format changes during streaming (that's allowed
> > > by the V4L2 API, I'm not sure if it's used anywhere though). While I'd
> > > favour not duplicating the same logic in different (and differently
> > > buggy) ways in drivers, I'm not sure how this could be implemented in a
> > > sane way in the V4L2 core in its current state.
> > 
> > I understand it's possible from some devices to support to format 
> > changes during streaming, but as you point out it's the exception and 
> > not the rule, if used at all.
> > 
> > So my point is if we start to enforce this in drivers we are headed down 
> > a road where this will be messier to clean up. Would it not make more 
> > sens to default the V4L2 core to disallow format changes while streaming 
> > and add a new flag to V4L2_SUBDEV_CAP_ to signal that the subdevice 
> > supports format changes while streaming?
> > 
> > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a subdevice 
> > only supports read-only operations so I think it would not be too hard 
> > to move this functionality into the core?
> 
> Yes, that's something we could try. The subdev core will then need to
> track the streaming state, which may require wrapping the .s_stream()
> call. Locking should then also likely be handled by the core. Probably
> nothing impossible, but quite a bit of work. Any volunteer ? :-)

We already track the stream count in struct media_entity and it's 
incremented/decremented under the media device lock by 
media_pipeline_start() and media_pipeline_stop(). So I don't think it's 
such a hard feature to add.

The large task IMHO is to figure out if we have any subdevice in tree 
that allows format changes while streaming and that would need to set 
this new V4L2_SUBDEV_CAP_ flag.

> 
> > > > > 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,
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

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

[Laurent Pinchart]

Hi Niklas,

On Thu, Jul 15, 2021 at 12:57:37PM +0200, Niklas Söderlund wrote:
> On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> > On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > > 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?
> > > > 
> > > > Some drivers may support format changes during streaming (that's allowed
> > > > by the V4L2 API, I'm not sure if it's used anywhere though). While I'd
> > > > favour not duplicating the same logic in different (and differently
> > > > buggy) ways in drivers, I'm not sure how this could be implemented in a
> > > > sane way in the V4L2 core in its current state.
> > > 
> > > I understand it's possible from some devices to support to format 
> > > changes during streaming, but as you point out it's the exception and 
> > > not the rule, if used at all.
> > > 
> > > So my point is if we start to enforce this in drivers we are headed down 
> > > a road where this will be messier to clean up. Would it not make more 
> > > sens to default the V4L2 core to disallow format changes while streaming 
> > > and add a new flag to V4L2_SUBDEV_CAP_ to signal that the subdevice 
> > > supports format changes while streaming?
> > > 
> > > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a subdevice 
> > > only supports read-only operations so I think it would not be too hard 
> > > to move this functionality into the core?
> > 
> > Yes, that's something we could try. The subdev core will then need to
> > track the streaming state, which may require wrapping the .s_stream()
> > call. Locking should then also likely be handled by the core. Probably
> > nothing impossible, but quite a bit of work. Any volunteer ? :-)
> 
> We already track the stream count in struct media_entity and it's 
> incremented/decremented under the media device lock by 
> media_pipeline_start() and media_pipeline_stop(). So I don't think it's 
> such a hard feature to add.
> 
> The large task IMHO is to figure out if we have any subdevice in tree 
> that allows format changes while streaming and that would need to set 
> this new V4L2_SUBDEV_CAP_ flag.

Many subdevs allow format changes during streaming. The question is
whether any of them do so knowingly, or if they're all buggy :-) I'd be
surprised if there were more than a couple of drivers that actually
support this correctly.

> > > > > > 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,

-- 
Regards,

Laurent Pinchart

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

[Dennis Rachui]

Hi Laurent,

> On Thu, Jul 15, 2021 at 12:57:37PM +0200, Niklas Söderlund wrote:
> > On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> > > On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > > > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > > > 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.rs
> > > > > > > t
> > > > > > 
> > > > > > 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?
> > > > > 
> > > > > Some drivers may support format changes during streaming (that's 
> > > > > allowed by the V4L2 API, I'm not sure if it's used anywhere 
> > > > > though). While I'd favour not duplicating the same logic in 
> > > > > different (and differently
> > > > > buggy) ways in drivers, I'm not sure how this could be 
> > > > > implemented in a sane way in the V4L2 core in its current state.
> > > > 
> > > > I understand it's possible from some devices to support to format 
> > > > changes during streaming, but as you point out it's the exception 
> > > > and not the rule, if used at all.
> > > > 
> > > > So my point is if we start to enforce this in drivers we are 
> > > > headed down a road where this will be messier to clean up. Would 
> > > > it not make more sens to default the V4L2 core to disallow format 
> > > > changes while streaming and add a new flag to V4L2_SUBDEV_CAP_ to 
> > > > signal that the subdevice supports format changes while streaming?
> > > > 
> > > > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a 
> > > > subdevice only supports read-only operations so I think it would 
> > > > not be too hard to move this functionality into the core?
> > > 
> > > Yes, that's something we could try. The subdev core will then need 
> > > to track the streaming state, which may require wrapping the 
> > > .s_stream() call. Locking should then also likely be handled by the 
> > > core. Probably nothing impossible, but quite a bit of work. Any 
> > > volunteer ? :-)
> > 
> > We already track the stream count in struct media_entity and it's 
> > incremented/decremented under the media device lock by
> > media_pipeline_start() and media_pipeline_stop(). So I don't think 
> > it's such a hard feature to add.
> > 
> > The large task IMHO is to figure out if we have any subdevice in tree 
> > that allows format changes while streaming and that would need to set 
> > this new V4L2_SUBDEV_CAP_ flag.
>
> Many subdevs allow format changes during streaming. The question is whether any of them do so knowingly, or if they're all buggy :-) I'd be surprised if there > were more than a couple of drivers that actually support this correctly.

From my perspective, the current stream_count from struct media_entity
would not be sufficient. References should be counted per struct media_pad.
Otherwise, devices that allow multiple parallel streams would block user
space from updating media-pads that are not part of an active media-pipeline.
E.g. in rcar-csi2 this could effect a source pad connected to currently
unused VIN device.

> > > > > > > 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,

--

Regards,

Dennis Rachui

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

[Niklas Söderlund]

Hi Dennis,

On 2021-07-16 16:09:21 +0200, Dennis Rachui wrote:
> Hi Laurent,
> 
> > On Thu, Jul 15, 2021 at 12:57:37PM +0200, Niklas Söderlund wrote:
> > > On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> > > > On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > > > > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > > > > 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.rs
> > > > > > > > t
> > > > > > > 
> > > > > > > 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?
> > > > > > 
> > > > > > Some drivers may support format changes during streaming (that's 
> > > > > > allowed by the V4L2 API, I'm not sure if it's used anywhere 
> > > > > > though). While I'd favour not duplicating the same logic in 
> > > > > > different (and differently
> > > > > > buggy) ways in drivers, I'm not sure how this could be 
> > > > > > implemented in a sane way in the V4L2 core in its current state.
> > > > > 
> > > > > I understand it's possible from some devices to support to format 
> > > > > changes during streaming, but as you point out it's the exception 
> > > > > and not the rule, if used at all.
> > > > > 
> > > > > So my point is if we start to enforce this in drivers we are 
> > > > > headed down a road where this will be messier to clean up. Would 
> > > > > it not make more sens to default the V4L2 core to disallow format 
> > > > > changes while streaming and add a new flag to V4L2_SUBDEV_CAP_ to 
> > > > > signal that the subdevice supports format changes while streaming?
> > > > > 
> > > > > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a 
> > > > > subdevice only supports read-only operations so I think it would 
> > > > > not be too hard to move this functionality into the core?
> > > > 
> > > > Yes, that's something we could try. The subdev core will then need 
> > > > to track the streaming state, which may require wrapping the 
> > > > .s_stream() call. Locking should then also likely be handled by the 
> > > > core. Probably nothing impossible, but quite a bit of work. Any 
> > > > volunteer ? :-)
> > > 
> > > We already track the stream count in struct media_entity and it's 
> > > incremented/decremented under the media device lock by
> > > media_pipeline_start() and media_pipeline_stop(). So I don't think 
> > > it's such a hard feature to add.
> > > 
> > > The large task IMHO is to figure out if we have any subdevice in tree 
> > > that allows format changes while streaming and that would need to set 
> > > this new V4L2_SUBDEV_CAP_ flag.
> >
> > Many subdevs allow format changes during streaming. The question is whether any of them do so knowingly, or if they're all buggy :-) I'd be surprised if there > were more than a couple of drivers that actually support this correctly.
> 
> From my perspective, the current stream_count from struct media_entity
> would not be sufficient. References should be counted per struct media_pad.
> Otherwise, devices that allow multiple parallel streams would block user
> space from updating media-pads that are not part of an active media-pipeline.
> E.g. in rcar-csi2 this could effect a source pad connected to currently
> unused VIN device.

I understand your reasoning, but with the current V4L2 design is this 
really a concern? As s_stream() is not pad/stream aware it acts more or 
less as a big start / stop button.

When starting all enabled media links formats needs to be evaluated 
(something we can't do yet with multiplexed streams..) and all 
subdivides that are part of the media graph are started. We can not 
enable or disable any media links while the pipeline is streaming so I'm 
not sure configuring the format of pads not part of the active capture 
is such a big concern, am I missing something?

> 
> > > > > > > > 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,
> 
> --
> 
> Regards,
> 
> Dennis Rachui
> 

-- 
Regards,
Niklas Söderlund

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

[Dennis Rachui]

Hi Niklas,

> On Fri, Jul 16, 2021 17:02:41 +0200, Niklas Söderlund wrote:
> > On 2021-07-16 16:09:21 +0200, Dennis Rachui wrote:
> > > On Thu, Jul 15, 2021 at 12:57:37PM +0200, Niklas Söderlund wrote:
> > > > On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> > > > > On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > > > > > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > > > > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > > > > > 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.rs
> > > > > > > > > t
> > > > > > > > 
> > > > > > > > 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?
> > > > > > > 
> > > > > > > Some drivers may support format changes during streaming (that's 
> > > > > > > allowed by the V4L2 API, I'm not sure if it's used anywhere 
> > > > > > > though). While I'd favour not duplicating the same logic in 
> > > > > > > different (and differently
> > > > > > > buggy) ways in drivers, I'm not sure how this could be 
> > > > > > > implemented in a sane way in the V4L2 core in its current state.
> > > > > > 
> > > > > > I understand it's possible from some devices to support to format 
> > > > > > changes during streaming, but as you point out it's the exception 
> > > > > > and not the rule, if used at all.
> > > > > > 
> > > > > > So my point is if we start to enforce this in drivers we are 
> > > > > > headed down a road where this will be messier to clean up. Would 
> > > > > > it not make more sens to default the V4L2 core to disallow format 
> > > > > > changes while streaming and add a new flag to V4L2_SUBDEV_CAP_ to 
> > > > > > signal that the subdevice supports format changes while streaming?
> > > > > > 
> > > > > > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a 
> > > > > > subdevice only supports read-only operations so I think it would 
> > > > > > not be too hard to move this functionality into the core?
> > > > > 
> > > > > Yes, that's something we could try. The subdev core will then need 
> > > > > to track the streaming state, which may require wrapping the 
> > > > > .s_stream() call. Locking should then also likely be handled by the 
> > > > > core. Probably nothing impossible, but quite a bit of work. Any 
> > > > > volunteer ? :-)
> > > > 
> > > > We already track the stream count in struct media_entity and it's 
> > > > incremented/decremented under the media device lock by
> > > > media_pipeline_start() and media_pipeline_stop(). So I don't think 
> > > > it's such a hard feature to add.
> > > > 
> > > > The large task IMHO is to figure out if we have any subdevice in tree 
> > > > that allows format changes while streaming and that would need to set 
> > > > this new V4L2_SUBDEV_CAP_ flag.
> > >
> > > Many subdevs allow format changes during streaming. The question is whether any of them do so knowingly, or if they're all buggy :-) I'd be surprised if there
> > > were more than a couple of drivers that actually support this correctly.
> > 
> > From my perspective, the current stream_count from struct media_entity
> > would not be sufficient. References should be counted per struct media_pad.
> > Otherwise, devices that allow multiple parallel streams would block user
> > space from updating media-pads that are not part of an active media-pipeline.
> > E.g. in rcar-csi2 this could effect a source pad connected to currently
> > unused VIN device.
>
> I understand your reasoning, but with the current V4L2 design is this 
> really a concern? As s_stream() is not pad/stream aware it acts more or 
> less as a big start / stop button.
>
> When starting all enabled media links formats needs to be evaluated 
> (something we can't do yet with multiplexed streams..) and all 
> subdivides that are part of the media graph are started. We can not 
> enable or disable any media links while the pipeline is streaming so I'm 
> not sure configuring the format of pads not part of the active capture 
> is such a big concern, am I missing something?

I missed the fact that media links can not be enabled / disabled while
the pipeline is streaming. So I agree that preventing format updates while
the entity is busy is no big concern. Userspace applications should be aware
of this just as they must be aware that link enable / disable only works
while the pipeline is not running.

> 
> > > > > > > > > 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,

--
Regards,
Dennis Rachui

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

[Laurent Pinchart]

Hi Dennis,

On Fri, Jul 16, 2021 at 04:09:21PM +0200, Dennis Rachui wrote:
> > On Thu, Jul 15, 2021 at 12:57:37PM +0200, Niklas Söderlund wrote:
> > > On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> > > > On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > > > > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > > > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > > > > 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.rs
> > > > > > > > t
> > > > > > > 
> > > > > > > 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?
> > > > > > 
> > > > > > Some drivers may support format changes during streaming (that's 
> > > > > > allowed by the V4L2 API, I'm not sure if it's used anywhere 
> > > > > > though). While I'd favour not duplicating the same logic in 
> > > > > > different (and differently
> > > > > > buggy) ways in drivers, I'm not sure how this could be 
> > > > > > implemented in a sane way in the V4L2 core in its current state.
> > > > > 
> > > > > I understand it's possible from some devices to support to format 
> > > > > changes during streaming, but as you point out it's the exception 
> > > > > and not the rule, if used at all.
> > > > > 
> > > > > So my point is if we start to enforce this in drivers we are 
> > > > > headed down a road where this will be messier to clean up. Would 
> > > > > it not make more sens to default the V4L2 core to disallow format 
> > > > > changes while streaming and add a new flag to V4L2_SUBDEV_CAP_ to 
> > > > > signal that the subdevice supports format changes while streaming?
> > > > > 
> > > > > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a 
> > > > > subdevice only supports read-only operations so I think it would 
> > > > > not be too hard to move this functionality into the core?
> > > > 
> > > > Yes, that's something we could try. The subdev core will then need 
> > > > to track the streaming state, which may require wrapping the 
> > > > .s_stream() call. Locking should then also likely be handled by the 
> > > > core. Probably nothing impossible, but quite a bit of work. Any 
> > > > volunteer ? :-)
> > > 
> > > We already track the stream count in struct media_entity and it's 
> > > incremented/decremented under the media device lock by
> > > media_pipeline_start() and media_pipeline_stop(). So I don't think 
> > > it's such a hard feature to add.
> > > 
> > > The large task IMHO is to figure out if we have any subdevice in tree 
> > > that allows format changes while streaming and that would need to set 
> > > this new V4L2_SUBDEV_CAP_ flag.
> >
> > Many subdevs allow format changes during streaming. The question is
> > whether any of them do so knowingly, or if they're all buggy :-) I'd
> > be surprised if there > were more than a couple of drivers that
> > actually support this correctly.
> 
> From my perspective, the current stream_count from struct media_entity
> would not be sufficient. References should be counted per struct media_pad.
> Otherwise, devices that allow multiple parallel streams would block user
> space from updating media-pads that are not part of an active media-pipeline.
> E.g. in rcar-csi2 this could effect a source pad connected to currently
> unused VIN device.

We're working on moving the information to pads, see "[PATCH v7 06/27]
media: entity: Move the pipeline from entity to pads"
(https://lore.kernel.org/linux-media/20210524104408.599645-7-tomi.valkeinen@ideasonboard.com/).
Does this address your concern ?

> > > > > > > > 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,

-- 
Regards,

Laurent Pinchart

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

[Dennis Rachui]

Hi Laurent,

> On Fri, Jul 16, 2021 18:07:01 +0300, Laurent Pinchart wrote:
> > On Fri, Jul 16, 2021 at 04:09:21PM +0200, Dennis Rachui wrote:
> > > On Thu, Jul 15, 2021 at 12:57:37PM +0200, Niklas Söderlund wrote:
> > > > On 2021-07-14 21:40:51 +0300, Laurent Pinchart wrote:
> > > > > On Tue, Jul 13, 2021 at 11:42:17AM +0200, Niklas Söderlund wrote:
> > > > > > On 2021-07-11 01:42:44 +0300, Laurent Pinchart wrote:
> > > > > > > On Fri, Jul 09, 2021 at 04:20:40PM +0200, Niklas Söderlund wrote:
> > > > > > > > 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.rs
> > > > > > > > > t
> > > > > > > > 
> > > > > > > > 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?
> > > > > > > 
> > > > > > > Some drivers may support format changes during streaming (that's 
> > > > > > > allowed by the V4L2 API, I'm not sure if it's used anywhere 
> > > > > > > though). While I'd favour not duplicating the same logic in 
> > > > > > > different (and differently
> > > > > > > buggy) ways in drivers, I'm not sure how this could be 
> > > > > > > implemented in a sane way in the V4L2 core in its current state.
> > > > > > 
> > > > > > I understand it's possible from some devices to support to format 
> > > > > > changes during streaming, but as you point out it's the exception 
> > > > > > and not the rule, if used at all.
> > > > > > 
> > > > > > So my point is if we start to enforce this in drivers we are 
> > > > > > headed down a road where this will be messier to clean up. Would 
> > > > > > it not make more sens to default the V4L2 core to disallow format 
> > > > > > changes while streaming and add a new flag to V4L2_SUBDEV_CAP_ to 
> > > > > > signal that the subdevice supports format changes while streaming?
> > > > > > 
> > > > > > We already have V4L2_SUBDEV_CAP_RO_SUBDEV to signal that a 
> > > > > > subdevice only supports read-only operations so I think it would 
> > > > > > not be too hard to move this functionality into the core?
> > > > > 
> > > > > Yes, that's something we could try. The subdev core will then need 
> > > > > to track the streaming state, which may require wrapping the 
> > > > > .s_stream() call. Locking should then also likely be handled by the 
> > > > > core. Probably nothing impossible, but quite a bit of work. Any 
> > > > > volunteer ? :-)
> > > > 
> > > > We already track the stream count in struct media_entity and it's 
> > > > incremented/decremented under the media device lock by
> > > > media_pipeline_start() and media_pipeline_stop(). So I don't think 
> > > > it's such a hard feature to add.
> > > > 
> > > > The large task IMHO is to figure out if we have any subdevice in tree 
> > > > that allows format changes while streaming and that would need to set 
> > > > this new V4L2_SUBDEV_CAP_ flag.
> > >
> > > Many subdevs allow format changes during streaming. The question is
> > > whether any of them do so knowingly, or if they're all buggy :-) I'd
> > > be surprised if there > were more than a couple of drivers that
> > > actually support this correctly.
> > 
> > From my perspective, the current stream_count from struct media_entity
> > would not be sufficient. References should be counted per struct media_pad.
> > Otherwise, devices that allow multiple parallel streams would block user
> > space from updating media-pads that are not part of an active media-pipeline.
> > E.g. in rcar-csi2 this could effect a source pad connected to currently
> > unused VIN device.
>
> We're working on moving the information to pads, see "[PATCH v7 06/27]
> media: entity: Move the pipeline from entity to pads"
> (https://lore.kernel.org/linux-media/20210524104408.599645-7-tomi.valkeinen@ideasonboard.com/).
> Does this address your concern ?

Since media links cannot be enabled / disabled while streaming, my concern
is resolved. Anyways, the patch set you shared looks promising for better
V4L2 integration of devices with independent parallel streaming capability.

> > > > > > > > > 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,

-- 
Regards,
Dennis Rachui