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));
+	if (ret) {
+		risp_stop(isp);
+		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);
 

-- 
2.43.0

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.

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);
>  

-- 
Regards,

Laurent Pinchart

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.

This should be fine, as if we get multiple calls, we'll try to enable
stream 0 multiple times, and that will fail. So, at this point of the
series, we really only support a single stream, and thus an attempt to
enable multiple VINs would fail here to the second
v4l2_subdev_enable_streams() call.

I'll add a note to the commit description.

> 
> Same when stopping.
> 
>> +	if (ret) {
>> +		risp_stop(isp);
> 
> This is also not covered by the stream_count, while risp_start() is.

Indeed, I'll fix that.

 Tomi

>> +		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);
>>  
>

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 =).

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);
>>  
>

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