Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
routing and deducing the DT based on the mbus format.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
 1 file changed, 77 insertions(+), 31 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
index a04cbf96b809..887d8eb21a3a 100644
--- a/drivers/media/platform/renesas/rcar-isp/csisp.c
+++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
@@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
 	pm_runtime_put(isp->dev);
 }
 
-static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
+static int risp_configure_routing(struct rcar_isp *isp,
+				  struct v4l2_subdev_state *state)
 {
-	const struct v4l2_mbus_framefmt *fmt;
-	const struct rcar_isp_format *format;
-	unsigned int vc;
-	u32 sel_csi = 0;
+	struct v4l2_mbus_frame_desc source_fd;
+	struct v4l2_subdev_route *route;
 	int ret;
 
-	fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
-	if (!fmt)
-		return -EINVAL;
+	ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
+			       isp->remote_pad, &source_fd);
+	if (ret)
+		return ret;
 
-	format = risp_code_to_fmt(fmt->code);
-	if (!format) {
-		dev_err(isp->dev, "Unsupported bus format\n");
-		return -EINVAL;
+	/* Clear the channel registers */
+	for (unsigned int ch = 0; ch < 12; ++ch) {
+		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
+		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
 	}
 
+	/* Clear the proc mode registers */
+	for (unsigned int dt = 0; dt < 64; ++dt)
+		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);
+
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
+		const struct rcar_isp_format *format;
+		const struct v4l2_mbus_framefmt *fmt;
+		unsigned int i;
+		u8 vc, dt, ch;
+		u32 v;
+
+		for (i = 0; i < source_fd.num_entries; i++) {
+			if (source_fd.entry[i].stream == route->sink_stream) {
+				source_entry = &source_fd.entry[i];
+				break;
+			}
+		}
+
+		if (!source_entry) {
+			dev_err(isp->dev,
+				"Failed to find stream from source frame desc\n");
+			return -EPIPE;
+		}
+
+		vc = source_entry->bus.csi2.vc;
+		dt = source_entry->bus.csi2.dt;
+		/* Channels 4 - 11 go to VIN */
+		ch = route->source_pad - 1 + 4;
+
+		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
+						   route->sink_stream);
+		if (!fmt)
+			return -EINVAL;
+
+		format = risp_code_to_fmt(fmt->code);
+		if (!format) {
+			dev_err(isp->dev, "Unsupported bus format\n");
+			return -EINVAL;
+		}
+
+		/* VC Filtering */
+		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
+
+		/* DT Filtering */
+		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
+			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
+
+		/* Proc mode */
+		v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
+		v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
+		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);
+	}
+
+	return 0;
+}
+
+static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
+{
+	u32 sel_csi = 0;
+	int ret;
+
 	ret = risp_power_on(isp);
 	if (ret) {
 		dev_err(isp->dev, "Failed to power on ISP\n");
@@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
 	risp_write_cs(isp, ISPINPUTSEL0_REG,
 		      risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
 
-	/* Configure Channel Selector. */
-	for (vc = 0; vc < 4; vc++) {
-		u8 ch = vc + 4;
-		u8 dt = format->datatype;
-
-		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
-		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
-			      ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
-			      ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
-			      ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
-			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
-	}
-
-	/* Setup processing method. */
-	risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
-		      ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
-		      ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
-		      ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
-		      ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
+	ret = risp_configure_routing(isp, state);
+	if (ret)
+		return ret;
 
 	/* Start ISP. */
 	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);

-- 
2.43.0

Hi Tomi,

Thanks for your work.

On 2025-05-30 16:50:40 +0300, Tomi Valkeinen wrote:
> Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
> routing and deducing the DT based on the mbus format.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
>  1 file changed, 77 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index a04cbf96b809..887d8eb21a3a 100644
> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> @@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
>  	pm_runtime_put(isp->dev);
>  }
>  
> -static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> +static int risp_configure_routing(struct rcar_isp *isp,
> +				  struct v4l2_subdev_state *state)
>  {
> -	const struct v4l2_mbus_framefmt *fmt;
> -	const struct rcar_isp_format *format;
> -	unsigned int vc;
> -	u32 sel_csi = 0;
> +	struct v4l2_mbus_frame_desc source_fd;
> +	struct v4l2_subdev_route *route;
>  	int ret;
>  
> -	fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
> -	if (!fmt)
> -		return -EINVAL;
> +	ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
> +			       isp->remote_pad, &source_fd);
> +	if (ret)
> +		return ret;
>  
> -	format = risp_code_to_fmt(fmt->code);
> -	if (!format) {
> -		dev_err(isp->dev, "Unsupported bus format\n");
> -		return -EINVAL;
> +	/* Clear the channel registers */
> +	for (unsigned int ch = 0; ch < 12; ++ch) {
> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
>  	}
>  
> +	/* Clear the proc mode registers */
> +	for (unsigned int dt = 0; dt < 64; ++dt)
> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);

I agree with Laurent's comments, do we really need to clear these 
registers?

> +
> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
> +		const struct rcar_isp_format *format;
> +		const struct v4l2_mbus_framefmt *fmt;
> +		unsigned int i;
> +		u8 vc, dt, ch;
> +		u32 v;
> +
> +		for (i = 0; i < source_fd.num_entries; i++) {
> +			if (source_fd.entry[i].stream == route->sink_stream) {
> +				source_entry = &source_fd.entry[i];
> +				break;
> +			}
> +		}
> +
> +		if (!source_entry) {
> +			dev_err(isp->dev,
> +				"Failed to find stream from source frame desc\n");
> +			return -EPIPE;
> +		}
> +
> +		vc = source_entry->bus.csi2.vc;
> +		dt = source_entry->bus.csi2.dt;
> +		/* Channels 4 - 11 go to VIN */
> +		ch = route->source_pad - 1 + 4;
> +
> +		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
> +						   route->sink_stream);
> +		if (!fmt)
> +			return -EINVAL;
> +
> +		format = risp_code_to_fmt(fmt->code);
> +		if (!format) {
> +			dev_err(isp->dev, "Unsupported bus format\n");
> +			return -EINVAL;
> +		}
> +
> +		/* VC Filtering */
> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> +
> +		/* DT Filtering */
> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> +			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> +
> +		/* Proc mode */
> +		v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
> +		v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);

Also as Laurent suggested I thin it would be nicer to build these 
registers up in a local variable and do the writes once outside the 
loop. That way the clearing of the register will take care of itself ;-)

> +	}
> +
> +	return 0;
> +}
> +
> +static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> +{
> +	u32 sel_csi = 0;
> +	int ret;
> +
>  	ret = risp_power_on(isp);
>  	if (ret) {
>  		dev_err(isp->dev, "Failed to power on ISP\n");
> @@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>  	risp_write_cs(isp, ISPINPUTSEL0_REG,
>  		      risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
>  
> -	/* Configure Channel Selector. */
> -	for (vc = 0; vc < 4; vc++) {
> -		u8 ch = vc + 4;
> -		u8 dt = format->datatype;
> -
> -		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> -		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> -			      ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> -			      ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> -			      ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> -			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> -	}
> -
> -	/* Setup processing method. */
> -	risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
> +	ret = risp_configure_routing(isp, state);
> +	if (ret)
> +		return ret;
>  
>  	/* Start ISP. */
>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
> 
> -- 
> 2.43.0
> 

-- 
Kind Regards,
Niklas Söderlund

Hi,

On 06/06/2025 15:20, Niklas Söderlund wrote:
> Hi Tomi,
> 
> Thanks for your work.
> 
> On 2025-05-30 16:50:40 +0300, Tomi Valkeinen wrote:
>> Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
>> routing and deducing the DT based on the mbus format.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
>>  1 file changed, 77 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> index a04cbf96b809..887d8eb21a3a 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> @@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
>>  	pm_runtime_put(isp->dev);
>>  }
>>  
>> -static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> +static int risp_configure_routing(struct rcar_isp *isp,
>> +				  struct v4l2_subdev_state *state)
>>  {
>> -	const struct v4l2_mbus_framefmt *fmt;
>> -	const struct rcar_isp_format *format;
>> -	unsigned int vc;
>> -	u32 sel_csi = 0;
>> +	struct v4l2_mbus_frame_desc source_fd;
>> +	struct v4l2_subdev_route *route;
>>  	int ret;
>>  
>> -	fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
>> -	if (!fmt)
>> -		return -EINVAL;
>> +	ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
>> +			       isp->remote_pad, &source_fd);
>> +	if (ret)
>> +		return ret;
>>  
>> -	format = risp_code_to_fmt(fmt->code);
>> -	if (!format) {
>> -		dev_err(isp->dev, "Unsupported bus format\n");
>> -		return -EINVAL;
>> +	/* Clear the channel registers */
>> +	for (unsigned int ch = 0; ch < 12; ++ch) {
>> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
>> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
>>  	}
>>  
>> +	/* Clear the proc mode registers */
>> +	for (unsigned int dt = 0; dt < 64; ++dt)
>> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);
> 
> I agree with Laurent's comments, do we really need to clear these 
> registers?
> 
>> +
>> +	for_each_active_route(&state->routing, route) {
>> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
>> +		const struct rcar_isp_format *format;
>> +		const struct v4l2_mbus_framefmt *fmt;
>> +		unsigned int i;
>> +		u8 vc, dt, ch;
>> +		u32 v;
>> +
>> +		for (i = 0; i < source_fd.num_entries; i++) {
>> +			if (source_fd.entry[i].stream == route->sink_stream) {
>> +				source_entry = &source_fd.entry[i];
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!source_entry) {
>> +			dev_err(isp->dev,
>> +				"Failed to find stream from source frame desc\n");
>> +			return -EPIPE;
>> +		}
>> +
>> +		vc = source_entry->bus.csi2.vc;
>> +		dt = source_entry->bus.csi2.dt;
>> +		/* Channels 4 - 11 go to VIN */
>> +		ch = route->source_pad - 1 + 4;
>> +
>> +		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
>> +						   route->sink_stream);
>> +		if (!fmt)
>> +			return -EINVAL;
>> +
>> +		format = risp_code_to_fmt(fmt->code);
>> +		if (!format) {
>> +			dev_err(isp->dev, "Unsupported bus format\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* VC Filtering */
>> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
>> +
>> +		/* DT Filtering */
>> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
>> +			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
>> +
>> +		/* Proc mode */
>> +		v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
>> +		v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
>> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);
> 
> Also as Laurent suggested I thin it would be nicer to build these 
> registers up in a local variable and do the writes once outside the 
> loop. That way the clearing of the register will take care of itself ;-)

I can do that, but I'm not sure I see the need from performance
perspective. We're talking about a few register reads & writes, once,
when enabling the first stream.

Would the code be more readable? I'm not sure... Here we do all the
writes related to a single stream in one iteration, which is nice.

But I don't have strong opinions either way.

 Tomi

>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> +{
>> +	u32 sel_csi = 0;
>> +	int ret;
>> +
>>  	ret = risp_power_on(isp);
>>  	if (ret) {
>>  		dev_err(isp->dev, "Failed to power on ISP\n");
>> @@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>>  	risp_write_cs(isp, ISPINPUTSEL0_REG,
>>  		      risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
>>  
>> -	/* Configure Channel Selector. */
>> -	for (vc = 0; vc < 4; vc++) {
>> -		u8 ch = vc + 4;
>> -		u8 dt = format->datatype;
>> -
>> -		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
>> -		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
>> -			      ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
>> -			      ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
>> -			      ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
>> -			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
>> -	}
>> -
>> -	/* Setup processing method. */
>> -	risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
>> +	ret = risp_configure_routing(isp, state);
>> +	if (ret)
>> +		return ret;
>>  
>>  	/* Start ISP. */
>>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
>>
>> -- 
>> 2.43.0
>>
> 

Hi Tomi,

Thank you for the patch.

On Fri, May 30, 2025 at 04:50:40PM +0300, Tomi Valkeinen wrote:
> Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
> routing and deducing the DT based on the mbus format.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
>  1 file changed, 77 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
> index a04cbf96b809..887d8eb21a3a 100644
> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
> @@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
>  	pm_runtime_put(isp->dev);
>  }
>  
> -static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> +static int risp_configure_routing(struct rcar_isp *isp,
> +				  struct v4l2_subdev_state *state)
>  {
> -	const struct v4l2_mbus_framefmt *fmt;
> -	const struct rcar_isp_format *format;
> -	unsigned int vc;
> -	u32 sel_csi = 0;
> +	struct v4l2_mbus_frame_desc source_fd;
> +	struct v4l2_subdev_route *route;
>  	int ret;
>  
> -	fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
> -	if (!fmt)
> -		return -EINVAL;
> +	ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
> +			       isp->remote_pad, &source_fd);
> +	if (ret)
> +		return ret;
>  
> -	format = risp_code_to_fmt(fmt->code);
> -	if (!format) {
> -		dev_err(isp->dev, "Unsupported bus format\n");
> -		return -EINVAL;
> +	/* Clear the channel registers */
> +	for (unsigned int ch = 0; ch < 12; ++ch) {

A macro for the number of channels would be nice.

> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
>  	}
>  
> +	/* Clear the proc mode registers */
> +	for (unsigned int dt = 0; dt < 64; ++dt)
> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);

Do we really need to clear those ? These registers seem to be used to
select how to process a particular DT, likely to allow overriding the
default processing method. 0 means RAW8, so it's not a magic disable
value as far as I can tell. I think we can leave the registers as-is.

> +
> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
> +		const struct rcar_isp_format *format;
> +		const struct v4l2_mbus_framefmt *fmt;
> +		unsigned int i;
> +		u8 vc, dt, ch;
> +		u32 v;
> +
> +		for (i = 0; i < source_fd.num_entries; i++) {
> +			if (source_fd.entry[i].stream == route->sink_stream) {
> +				source_entry = &source_fd.entry[i];
> +				break;
> +			}
> +		}
> +
> +		if (!source_entry) {
> +			dev_err(isp->dev,
> +				"Failed to find stream from source frame desc\n");

Isn't it rather "Failed to find source frame desc for stream" ?

> +			return -EPIPE;
> +		}
> +
> +		vc = source_entry->bus.csi2.vc;
> +		dt = source_entry->bus.csi2.dt;
> +		/* Channels 4 - 11 go to VIN */
> +		ch = route->source_pad - 1 + 4;
> +
> +		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
> +						   route->sink_stream);
> +		if (!fmt)
> +			return -EINVAL;
> +
> +		format = risp_code_to_fmt(fmt->code);
> +		if (!format) {
> +			dev_err(isp->dev, "Unsupported bus format\n");
> +			return -EINVAL;
> +		}
> +
> +		/* VC Filtering */
> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> +
> +		/* DT Filtering */
> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> +			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> +
> +		/* Proc mode */
> +		v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
> +		v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);

If we want to minimize the register writes, we could store the
ISPPROCMODE_DT_REG values in a local variable and write all of them in
one go. Possible/probably overkill.

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

> +	}
> +
> +	return 0;
> +}
> +
> +static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
> +{
> +	u32 sel_csi = 0;
> +	int ret;
> +
>  	ret = risp_power_on(isp);
>  	if (ret) {
>  		dev_err(isp->dev, "Failed to power on ISP\n");
> @@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>  	risp_write_cs(isp, ISPINPUTSEL0_REG,
>  		      risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
>  
> -	/* Configure Channel Selector. */
> -	for (vc = 0; vc < 4; vc++) {
> -		u8 ch = vc + 4;
> -		u8 dt = format->datatype;
> -
> -		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
> -		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
> -			      ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
> -			      ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
> -			      ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
> -			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
> -	}
> -
> -	/* Setup processing method. */
> -	risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
> -		      ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
> +	ret = risp_configure_routing(isp, state);
> +	if (ret)
> +		return ret;
>  
>  	/* Start ISP. */
>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);

-- 
Regards,

Laurent Pinchart

Hi,

On 02/06/2025 16:22, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, May 30, 2025 at 04:50:40PM +0300, Tomi Valkeinen wrote:
>> Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
>> routing and deducing the DT based on the mbus format.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
>>  1 file changed, 77 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> index a04cbf96b809..887d8eb21a3a 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> @@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
>>  	pm_runtime_put(isp->dev);
>>  }
>>  
>> -static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> +static int risp_configure_routing(struct rcar_isp *isp,
>> +				  struct v4l2_subdev_state *state)
>>  {
>> -	const struct v4l2_mbus_framefmt *fmt;
>> -	const struct rcar_isp_format *format;
>> -	unsigned int vc;
>> -	u32 sel_csi = 0;
>> +	struct v4l2_mbus_frame_desc source_fd;
>> +	struct v4l2_subdev_route *route;
>>  	int ret;
>>  
>> -	fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
>> -	if (!fmt)
>> -		return -EINVAL;
>> +	ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
>> +			       isp->remote_pad, &source_fd);
>> +	if (ret)
>> +		return ret;
>>  
>> -	format = risp_code_to_fmt(fmt->code);
>> -	if (!format) {
>> -		dev_err(isp->dev, "Unsupported bus format\n");
>> -		return -EINVAL;
>> +	/* Clear the channel registers */
>> +	for (unsigned int ch = 0; ch < 12; ++ch) {
> 
> A macro for the number of channels would be nice.

Sure, will add.

>> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
>> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
>>  	}
>>  
>> +	/* Clear the proc mode registers */
>> +	for (unsigned int dt = 0; dt < 64; ++dt)
>> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);
> 
> Do we really need to clear those ? These registers seem to be used to
> select how to process a particular DT, likely to allow overriding the
> default processing method. 0 means RAW8, so it's not a magic disable
> value as far as I can tell. I think we can leave the registers as-is.

Yes, I think you are right. I'll drop the clear.

>> +
>> +	for_each_active_route(&state->routing, route) {
>> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
>> +		const struct rcar_isp_format *format;
>> +		const struct v4l2_mbus_framefmt *fmt;
>> +		unsigned int i;
>> +		u8 vc, dt, ch;
>> +		u32 v;
>> +
>> +		for (i = 0; i < source_fd.num_entries; i++) {
>> +			if (source_fd.entry[i].stream == route->sink_stream) {
>> +				source_entry = &source_fd.entry[i];
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!source_entry) {
>> +			dev_err(isp->dev,
>> +				"Failed to find stream from source frame desc\n");
> 
> Isn't it rather "Failed to find source frame desc for stream" ?

I think it's kind of the same thing... But perhaps your version is
better, as, indeed, the result we're getting is the frame desc entry.

>> +			return -EPIPE;
>> +		}
>> +
>> +		vc = source_entry->bus.csi2.vc;
>> +		dt = source_entry->bus.csi2.dt;
>> +		/* Channels 4 - 11 go to VIN */
>> +		ch = route->source_pad - 1 + 4;
>> +
>> +		fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
>> +						   route->sink_stream);
>> +		if (!fmt)
>> +			return -EINVAL;
>> +
>> +		format = risp_code_to_fmt(fmt->code);
>> +		if (!format) {
>> +			dev_err(isp->dev, "Unsupported bus format\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		/* VC Filtering */
>> +		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
>> +
>> +		/* DT Filtering */
>> +		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
>> +			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
>> +
>> +		/* Proc mode */
>> +		v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
>> +		v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
>> +		risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);
> 
> If we want to minimize the register writes, we could store the
> ISPPROCMODE_DT_REG values in a local variable and write all of them in
> one go. Possible/probably overkill.

Yes, I don't think that would be visible in any performance measurement.
If it is, I'm fine with doing it later. I'd rather avoid any
optimizations at this point of time, as it's quite a challenge to get
both the gen3 & gen4 converted.

Tomi

> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> +{
>> +	u32 sel_csi = 0;
>> +	int ret;
>> +
>>  	ret = risp_power_on(isp);
>>  	if (ret) {
>>  		dev_err(isp->dev, "Failed to power on ISP\n");
>> @@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>>  	risp_write_cs(isp, ISPINPUTSEL0_REG,
>>  		      risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
>>  
>> -	/* Configure Channel Selector. */
>> -	for (vc = 0; vc < 4; vc++) {
>> -		u8 ch = vc + 4;
>> -		u8 dt = format->datatype;
>> -
>> -		risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
>> -		risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
>> -			      ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
>> -			      ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
>> -			      ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
>> -			      ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
>> -	}
>> -
>> -	/* Setup processing method. */
>> -	risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
>> -		      ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
>> +	ret = risp_configure_routing(isp, state);
>> +	if (ret)
>> +		return ret;
>>  
>>  	/* Start ISP. */
>>  	risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
>