Re: [PATCH v5 03/10] media: rcar-csi2: Move {enable|disable}_streams() calls

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sat, Apr 04, 2026 at 02:19:06PM +0200, Niklas Söderlund wrote:
> On 2026-03-18 22:54:35 +0200, Laurent Pinchart wrote:
> > On Wed, Mar 11, 2026 at 03:53:16PM +0200, Tomi Valkeinen wrote:
> > > With multiple streams the operation to enable the CSI-2 hardware and to
> > > call {enable|disable}_streams() on upstream subdev will need to be
> > > handled separately.
> > > 
> > > Prepare for that by moving {enable|disable}_streams() calls out from
> > > rcsi2_start() and rcsi2_stop().
> > > 
> > > On Gen3, a side effect of this change is that if the sink side devices
> > > call .enable_streams() on rcar-csi2 multiple times, the second call will
> > > fail. This is because we always use stream ID 0, so the second call
> > > would attempt to enable the same stream again, leading to an error. In
> > > other words, a normal single-stream setup continues to work, but trying
> > > to use the current driver's custom VC based routing will fail.
> > 
> > I assume this gets addressed later in the series.
> > 
> > > 
> > > On Gen4, this doesn't matter as the rcar-isp behaves in a similar way as
> > > described above, and thus rcar-csi2 will only get a single
> > > .enable_streams() call.
> > > 
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> > > ---
> > >  drivers/media/platform/renesas/rcar-csi2.c | 25 +++++++++++++++----------
> > >  1 file changed, 15 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> > > index 7305cc4a04cb..158fa447e668 100644
> > > --- a/drivers/media/platform/renesas/rcar-csi2.c
> > > +++ b/drivers/media/platform/renesas/rcar-csi2.c
> > > @@ -1822,20 +1822,12 @@ static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
> > >  		return ret;
> > >  	}
> > >  
> > > -	ret = v4l2_subdev_enable_streams(priv->remote, priv->remote_pad,
> > > -					 BIT_ULL(0));
> > > -	if (ret) {
> > > -		rcsi2_enter_standby(priv);
> > > -		return ret;
> > > -	}
> > > -
> > >  	return 0;
> > >  }
> > >  
> > >  static void rcsi2_stop(struct rcar_csi2 *priv)
> > >  {
> > >  	rcsi2_enter_standby(priv);
> > > -	v4l2_subdev_disable_streams(priv->remote, priv->remote_pad, BIT_ULL(0));
> > >  }
> > >  
> > >  static int rcsi2_enable_streams(struct v4l2_subdev *sd,
> > > @@ -1857,6 +1849,14 @@ static int rcsi2_enable_streams(struct v4l2_subdev *sd,
> > >  			return ret;
> > >  	}
> > >  
> > > +	ret = v4l2_subdev_enable_streams(priv->remote, priv->remote_pad,
> > > +					 BIT_ULL(0));
> > > +	if (ret) {
> > > +		if (priv->stream_count == 0)
> > > +			rcsi2_stop(priv);
> > > +		return ret;
> > > +	}
> > > +
> > >  	priv->stream_count += 1;
> > >  
> > >  	return ret;
> > > @@ -1867,7 +1867,7 @@ static int rcsi2_disable_streams(struct v4l2_subdev *sd,
> > >  				 u32 source_pad, u64 source_streams_mask)
> > >  {
> > >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > -	int ret = 0;
> > > +	int ret;
> > >  
> > >  	if (source_streams_mask != 1)
> > >  		return -EINVAL;
> > > @@ -1878,9 +1878,14 @@ static int rcsi2_disable_streams(struct v4l2_subdev *sd,
> > >  	if (priv->stream_count == 1)
> > >  		rcsi2_stop(priv);
> > >  
> > > +	ret = v4l2_subdev_disable_streams(priv->remote, priv->remote_pad,
> > > +					  BIT_ULL(0));
> > > +	if (ret)
> > > +		return ret;
> > > +
> > >  	priv->stream_count -= 1;
> > >  
> > > -	return ret;
> > > +	return 0;
> > >  }
> > 
> > rcsi2_irq_thread() also calls rcsi2_stop(), followed by rcsi2_start().
> > This is to handle errors reported by the AFIFO_OF, ERRSOTHS and
> > ERRSOTSYNCHS interrupts. If the source isn't restarted, such an attempt
> > to recover from errors will likely fail. On the other hand, restarting
> > the source will likely not lead to great results either.
> > 
> > Error handling was introduced in
> > 
> > commit 4ab44ff0841b9a825f9875623d24809d29e37a10
> > Author: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Date:   Thu Apr 11 16:30:58 2019 -0400
> > 
> >     media: rcar-csi2: restart CSI-2 link if error is detected
> > 
> >     Restart the CSI-2 link if the CSI-2 receiver detects an error during
> >     reception. The driver did nothing when a link error happened and the
> >     data flow simply stopped without the user knowing why.
> > 
> >     Change the driver to try and recover from errors by restarting the link
> >     and informing the user that something is not right. For obvious reasons
> >     it's not possible to recover from all errors (video source disconnected
> >     for example) but in such cases the user is at least informed of the
> >     error and the same behavior of the stopped data flow is retained.
> > 
> > Niklas, do you recall anything about the errors you saw ?
> 
> IIRC, the spark for the work was a user report where the wish was to 
> handle detectable errors somehow instead of just silently fail. The 
> first try was this commit where the driver tries to recover. However as 
> you point out it might not be the optimal solution.
> 
> Instead I proposed a way to signal to user-space that there where a link 
> error and let it deal with it [1]. As seen on the date from that work it 
> kind of stalled. I still plan to resume that work once all the cleanups 
> of the R-Car VIN pipeline are done.

Handling it in userspace seems a good idea to me.

> 1.  https://lore.kernel.org/linux-renesas-soc/20211108160220.767586-1-niklas.soderlund%2Brenesas@ragnatech.se/
> 
> > >  
> > >  static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> > > 

-- 
Regards,

Laurent Pinchart