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

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

On Wed, Mar 11, 2026 at 03:53:15PM +0200, 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().
> 
> On Gen4, a side effect of this change is that if the sink side devices
> call .enable_streams() on rcar-isp 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.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/media/platform/renesas/rcar-isp/csisp.c | 27 ++++++++++++++++---------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index 8fb2cc3b5650..58a9a3bd9f75 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);
>  
> @@ -291,7 +284,7 @@ static int risp_enable_streams(struct v4l2_subdev *sd,
>  			       u64 source_streams_mask)
>  {
>  	struct rcar_isp *isp = sd_to_isp(sd);
> -	int ret = 0;
> +	int ret;
>  
>  	if (source_streams_mask != 1)
>  		return -EINVAL;
> @@ -305,9 +298,17 @@ 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) {
> +		if (isp->stream_count == 0)
> +			risp_stop(isp);
> +		return ret;
> +	}
> +
>  	isp->stream_count += 1;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int risp_disable_streams(struct v4l2_subdev *sd,
> @@ -315,6 +316,7 @@ static int risp_disable_streams(struct v4l2_subdev *sd,
>  				u64 source_streams_mask)
>  {
>  	struct rcar_isp *isp = sd_to_isp(sd);
> +	int ret;
>  
>  	if (source_streams_mask != 1)
>  		return -EINVAL;
> @@ -322,6 +324,11 @@ static int risp_disable_streams(struct v4l2_subdev *sd,
>  	if (!isp->remote)
>  		return -ENODEV;
>  
> +	ret = v4l2_subdev_disable_streams(isp->remote, isp->remote_pad,
> +					  BIT_ULL(0));
> +	if (ret)
> +		return ret;
> +
>  	if (isp->stream_count == 1)
>  		risp_stop(isp);
>  
> 

-- 
Regards,

Laurent Pinchart