Re: [PATCH v3 03/15] media: rcar-isp: Move {enable|disable}_streams() calls

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

Hi Tomi,

On 2025-09-26 14:22:10 +0300, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/06/2025 12:43, Laurent Pinchart wrote:
> > Hi Tomi,
> > 
> > Thank you for the patch.
> > 
> > On Fri, May 30, 2025 at 04:50:32PM +0300, Tomi Valkeinen wrote:
> >> With multiple streams the operation to enable the ISP 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
> >> risp_start() and risp_stop().
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> >> ---
> >>  drivers/media/platform/renesas/rcar-isp/csisp.c | 18 ++++++++++--------
> >>  1 file changed, 10 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> >> index 8fb2cc3b5650..2337c5d44c40 100644
> >> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> >> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> >> @@ -268,18 +268,11 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> >>  	/* Start ISP. */
> >>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
> >>  
> >> -	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> >> -					 BIT_ULL(0));
> >> -	if (ret)
> >> -		risp_power_off(isp);
> >> -
> >> -	return ret;
> >> +	return 0;
> >>  }
> >>  
> >>  static void risp_stop(struct rcar_isp *isp)
> >>  {
> >> -	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> >> -
> >>  	/* Stop ISP. */
> >>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_STOP);
> >>  
> >> @@ -305,6 +298,13 @@ static int risp_enable_streams(struct v4l2_subdev *sd,
> >>  			return ret;
> >>  	}
> >>  
> >> +	ret = v4l2_subdev_enable_streams(isp->remote, isp->remote_pad,
> >> +					 BIT_ULL(0));
> > 
> > You're now potentially calling v4l2_subdev_disable_streams() multiple
> > times on the same pad and stream, as this call isn't covered by the
> > stream_count check anymore. Is that correct ? Maybe because
> > risp_enable_streams() is guaranteed to never be called multiple times,
> > with stream_count never becoming larger than 1 ? If so that should be
> > explained in the commit message, and stream_count should probably be
> > dropped.
> 
> At this point in the series risp_enable_streams() is called just once,
> from a single VIN. That is, assuming only a single stream is supported.
> In the cover letter I mention that there seems to be some kind of
> attempts for multistreaming in upstream, which breaks during this
> series. My understanding from Niklas was that the custom multistreaming
> doesn't work or at least can be dropped. Niklas, correct me if I'm wrong
> (and if I am wrong, someone else needs to take care of the custom
> multistreaming =).

And as discussed on IRC the main issue for the VIN pipeline will be on 
Gen3 where media graph links are made to model VC-to-VIN as we had no 
streams concept at the time. And that would need to be changed _before_ 
adding streams to instead model the physical busses between the R-Car 
CSI-2 Rx and VIN instances, with all their fun limitations.

If that is not done and streams added to the R-Car CSI-2 driver it would 
do the correct thing, but the VIN driver would still create a lot of 
media links that after streams are not possible to setup in hardware.

> 
> Later in the series risp_enable_streams will be called multiple times.
> Each VIN video node will call it once when enabling.
> 
>  Tomi
> 
> > 
> > Same when stopping.
> > 
> >> +	if (ret) {
> >> +		risp_stop(isp);
> > 
> > This is also not covered by the stream_count, while risp_start() is.
> > 
> >> +		return ret;
> >> +	}
> >> +
> >>  	isp->stream_count += 1;
> >>  
> >>  	return ret;
> >> @@ -322,6 +322,8 @@ static int risp_disable_streams(struct v4l2_subdev *sd,
> >>  	if (!isp->remote)
> >>  		return -ENODEV;
> >>  
> >> +	v4l2_subdev_disable_streams(isp->remote, isp->remote_pad, BIT_ULL(0));
> >> +
> >>  	if (isp->stream_count == 1)
> >>  		risp_stop(isp);
> >>  
> > 
> 

-- 
Kind Regards,
Niklas Söderlund