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

If the source subdevice doesn't implement .get_frame_desc, we use a
fallback case where we assume there's a single stream with VC = 0 and DT
based on the mbus format.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/rcar-csi2.c | 113 +++++++++++++++++++----------
 1 file changed, 76 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index b9f83aae725a..8f708196ef49 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -71,10 +71,7 @@ struct rcar_csi2;
 #define FLD_REG				0x1c
 #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
 #define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
-#define FLD_FLD_EN4			BIT(3)
-#define FLD_FLD_EN3			BIT(2)
-#define FLD_FLD_EN2			BIT(1)
-#define FLD_FLD_EN			BIT(0)
+#define FLD_FLD_EN(ch)			BIT(ch)
 
 /* Automatic Standby Control */
 #define ASTBY_REG			0x20
@@ -1066,52 +1063,94 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
 static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
 				     struct v4l2_subdev_state *state)
 {
-	const struct rcar_csi2_format *format;
-	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
-	const struct v4l2_mbus_framefmt *fmt;
+	u32 phycnt, vcdt = 0, vcdt2 = 0;
+	u32 fld = FLD_DET_SEL(1);
+	struct v4l2_mbus_frame_desc source_fd;
+	struct v4l2_subdev_route *route;
 	unsigned int lanes;
-	unsigned int i;
 	int mbps, ret;
+	u8 ch = 0;
 
-	/* Use the format on the sink pad to compute the receiver config. */
-	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
+	ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
+			       priv->remote_pad, &source_fd);
+	if (ret && ret != -ENOIOCTLCMD) {
+		return ret;
+	} else if (ret == -ENOIOCTLCMD) {
+		/* Create a fallback source_fd */
+		struct v4l2_mbus_frame_desc *fd = &source_fd;
+		const struct rcar_csi2_format *format;
+		struct v4l2_mbus_framefmt *fmt;
 
-	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-		fmt->width, fmt->height,
-		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
+		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
+		if (!fmt)
+			return -EINVAL;
 
-	/* Code is validated in set_fmt. */
-	format = rcsi2_code_to_fmt(fmt->code);
-	if (!format)
-		return -EINVAL;
+		format = rcsi2_code_to_fmt(fmt->code);
+		if (!format)
+			return -EINVAL;
 
-	/*
-	 * Enable all supported CSI-2 channels with virtual channel and
-	 * data type matching.
-	 *
-	 * NOTE: It's not possible to get individual datatype for each
-	 *       source virtual channel. Once this is possible in V4L2
-	 *       it should be used here.
-	 */
-	for (i = 0; i < priv->info->num_channels; i++) {
+		memset(fd, 0, sizeof(*fd));
+
+		fd->num_entries = 1;
+		fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+		fd->entry[0].stream = 0;
+		fd->entry[0].pixelcode = fmt->code;
+		fd->entry[0].bus.csi2.vc = 0;
+		fd->entry[0].bus.csi2.dt = format->datatype;
+	}
+
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
+		const struct v4l2_mbus_framefmt *fmt;
+		const struct rcar_csi2_format *format;
+		unsigned int i;
+		u8 vc, dt;
 		u32 vcdt_part;
 
-		if (priv->channel_vc[i] < 0)
-			continue;
+		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(priv->dev,
+				"Failed to find stream from source frame desc\n");
+			return -EPIPE;
+		}
 
-		vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN |
-			VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype);
+		vc = source_entry->bus.csi2.vc;
+		dt = source_entry->bus.csi2.dt;
+
+		vcdt_part = VCDT_SEL_VC(vc) | VCDT_VCDTN_EN |
+			VCDT_SEL_DTN_ON | VCDT_SEL_DT(dt);
 
 		/* Store in correct reg and offset. */
-		if (i < 2)
-			vcdt |= vcdt_part << ((i % 2) * 16);
+		if (ch < 2)
+			vcdt |= vcdt_part << ((ch % 2) * 16);
 		else
-			vcdt2 |= vcdt_part << ((i % 2) * 16);
-	}
+			vcdt2 |= vcdt_part << ((ch % 2) * 16);
+
+		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK,
+						   route->sink_stream);
+		if (!fmt)
+			return -EINVAL;
+
+		dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
+			fmt->width, fmt->height,
+			fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
 
-	if (fmt->field == V4L2_FIELD_ALTERNATE)
-		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
-			| FLD_FLD_EN;
+		/* Code is validated in set_fmt. */
+		format = rcsi2_code_to_fmt(fmt->code);
+		if (!format)
+			return -EINVAL;
+
+		if (fmt->field == V4L2_FIELD_ALTERNATE)
+			fld |= FLD_FLD_EN(ch);
+
+		ch++;
+	}
 
 	/*
 	 * Get the number of active data lanes inspecting the remote mbus

-- 
2.43.0

Hi Tomi,

Thank you for the patch.

On Fri, May 30, 2025 at 04:50:42PM +0300, Tomi Valkeinen wrote:
> Call get_frame_desc to find out VC & DT, for Gen3 platforms, instead of
> hardcoding the VC routing and deducing the DT based on the mbus format.
> 
> If the source subdevice doesn't implement .get_frame_desc, we use a
> fallback case where we assume there's a single stream with VC = 0 and DT
> based on the mbus format.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rcar-csi2.c | 113 +++++++++++++++++++----------
>  1 file changed, 76 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index b9f83aae725a..8f708196ef49 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -71,10 +71,7 @@ struct rcar_csi2;
>  #define FLD_REG				0x1c
>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
>  #define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
> -#define FLD_FLD_EN4			BIT(3)
> -#define FLD_FLD_EN3			BIT(2)
> -#define FLD_FLD_EN2			BIT(1)
> -#define FLD_FLD_EN			BIT(0)
> +#define FLD_FLD_EN(ch)			BIT(ch)
>  
>  /* Automatic Standby Control */
>  #define ASTBY_REG			0x20
> @@ -1066,52 +1063,94 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>  static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
>  				     struct v4l2_subdev_state *state)
>  {
> -	const struct rcar_csi2_format *format;
> -	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> -	const struct v4l2_mbus_framefmt *fmt;
> +	u32 phycnt, vcdt = 0, vcdt2 = 0;
> +	u32 fld = FLD_DET_SEL(1);
> +	struct v4l2_mbus_frame_desc source_fd;
> +	struct v4l2_subdev_route *route;
>  	unsigned int lanes;
> -	unsigned int i;
>  	int mbps, ret;
> +	u8 ch = 0;
>  
> -	/* Use the format on the sink pad to compute the receiver config. */
> -	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
> +	ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
> +			       priv->remote_pad, &source_fd);
> +	if (ret && ret != -ENOIOCTLCMD) {
> +		return ret;
> +	} else if (ret == -ENOIOCTLCMD) {

	if (ret && ret != -ENOIOCTLCMD)
		return ret;

	if (ret == -ENOIOCTLCMD) {

> +		/* Create a fallback source_fd */
> +		struct v4l2_mbus_frame_desc *fd = &source_fd;
> +		const struct rcar_csi2_format *format;
> +		struct v4l2_mbus_framefmt *fmt;
>  
> -	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> -		fmt->width, fmt->height,
> -		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
> +		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
> +		if (!fmt)
> +			return -EINVAL;
>  
> -	/* Code is validated in set_fmt. */
> -	format = rcsi2_code_to_fmt(fmt->code);
> -	if (!format)
> -		return -EINVAL;
> +		format = rcsi2_code_to_fmt(fmt->code);
> +		if (!format)
> +			return -EINVAL;
>  
> -	/*
> -	 * Enable all supported CSI-2 channels with virtual channel and
> -	 * data type matching.
> -	 *
> -	 * NOTE: It's not possible to get individual datatype for each
> -	 *       source virtual channel. Once this is possible in V4L2
> -	 *       it should be used here.
> -	 */
> -	for (i = 0; i < priv->info->num_channels; i++) {
> +		memset(fd, 0, sizeof(*fd));
> +
> +		fd->num_entries = 1;
> +		fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> +		fd->entry[0].stream = 0;
> +		fd->entry[0].pixelcode = fmt->code;
> +		fd->entry[0].bus.csi2.vc = 0;
> +		fd->entry[0].bus.csi2.dt = format->datatype;
> +	}
> +
> +	for_each_active_route(&state->routing, route) {
> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;

const

> +		const struct v4l2_mbus_framefmt *fmt;
> +		const struct rcar_csi2_format *format;
> +		unsigned int i;
> +		u8 vc, dt;
>  		u32 vcdt_part;
>  
> -		if (priv->channel_vc[i] < 0)
> -			continue;
> +		for (i = 0; i < source_fd.num_entries; i++) {
> +			if (source_fd.entry[i].stream == route->sink_stream) {

No need to check the pad ?

> +				source_entry = &source_fd.entry[i];
> +				break;
> +			}
> +		}
> +
> +		if (!source_entry) {
> +			dev_err(priv->dev,
> +				"Failed to find stream from source frame desc\n");
> +			return -EPIPE;
> +		}
>  
> -		vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN |
> -			VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype);
> +		vc = source_entry->bus.csi2.vc;
> +		dt = source_entry->bus.csi2.dt;
> +
> +		vcdt_part = VCDT_SEL_VC(vc) | VCDT_VCDTN_EN |
> +			VCDT_SEL_DTN_ON | VCDT_SEL_DT(dt);

I would drop the vc and dt variables and write

		vcdt_part = VCDT_SEL_VC(source_entry->bus.csi2.vc)
			  | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON
			  | VCDT_SEL_DT(source_entry->bus.csi2.dt);

>  
>  		/* Store in correct reg and offset. */
> -		if (i < 2)
> -			vcdt |= vcdt_part << ((i % 2) * 16);
> +		if (ch < 2)
> +			vcdt |= vcdt_part << ((ch % 2) * 16);
>  		else
> -			vcdt2 |= vcdt_part << ((i % 2) * 16);
> -	}
> +			vcdt2 |= vcdt_part << ((ch % 2) * 16);
> +
> +		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK,
> +						   route->sink_stream);
> +		if (!fmt)
> +			return -EINVAL;
> +
> +		dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> +			fmt->width, fmt->height,
> +			fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>  
> -	if (fmt->field == V4L2_FIELD_ALTERNATE)
> -		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> -			| FLD_FLD_EN;
> +		/* Code is validated in set_fmt. */

Then why don't you drop the !format check ?

> +		format = rcsi2_code_to_fmt(fmt->code);
> +		if (!format)
> +			return -EINVAL;
> +
> +		if (fmt->field == V4L2_FIELD_ALTERNATE)
> +			fld |= FLD_FLD_EN(ch);
> +
> +		ch++;
> +	}
>  
>  	/*
>  	 * Get the number of active data lanes inspecting the remote mbus

-- 
Regards,

Laurent Pinchart

Hi,

On 02/06/2025 16:49, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Fri, May 30, 2025 at 04:50:42PM +0300, Tomi Valkeinen wrote:
>> Call get_frame_desc to find out VC & DT, for Gen3 platforms, instead of
>> hardcoding the VC routing and deducing the DT based on the mbus format.
>>
>> If the source subdevice doesn't implement .get_frame_desc, we use a
>> fallback case where we assume there's a single stream with VC = 0 and DT
>> based on the mbus format.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/renesas/rcar-csi2.c | 113 +++++++++++++++++++----------
>>  1 file changed, 76 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
>> index b9f83aae725a..8f708196ef49 100644
>> --- a/drivers/media/platform/renesas/rcar-csi2.c
>> +++ b/drivers/media/platform/renesas/rcar-csi2.c
>> @@ -71,10 +71,7 @@ struct rcar_csi2;
>>  #define FLD_REG				0x1c
>>  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
>>  #define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
>> -#define FLD_FLD_EN4			BIT(3)
>> -#define FLD_FLD_EN3			BIT(2)
>> -#define FLD_FLD_EN2			BIT(1)
>> -#define FLD_FLD_EN			BIT(0)
>> +#define FLD_FLD_EN(ch)			BIT(ch)
>>  
>>  /* Automatic Standby Control */
>>  #define ASTBY_REG			0x20
>> @@ -1066,52 +1063,94 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>>  static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
>>  				     struct v4l2_subdev_state *state)
>>  {
>> -	const struct rcar_csi2_format *format;
>> -	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>> -	const struct v4l2_mbus_framefmt *fmt;
>> +	u32 phycnt, vcdt = 0, vcdt2 = 0;
>> +	u32 fld = FLD_DET_SEL(1);
>> +	struct v4l2_mbus_frame_desc source_fd;
>> +	struct v4l2_subdev_route *route;
>>  	unsigned int lanes;
>> -	unsigned int i;
>>  	int mbps, ret;
>> +	u8 ch = 0;
>>  
>> -	/* Use the format on the sink pad to compute the receiver config. */
>> -	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
>> +	ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
>> +			       priv->remote_pad, &source_fd);
>> +	if (ret && ret != -ENOIOCTLCMD) {
>> +		return ret;
>> +	} else if (ret == -ENOIOCTLCMD) {
> 
> 	if (ret && ret != -ENOIOCTLCMD)
> 		return ret;
> 
> 	if (ret == -ENOIOCTLCMD) {

Yep, that's cleaner.

>> +		/* Create a fallback source_fd */
>> +		struct v4l2_mbus_frame_desc *fd = &source_fd;
>> +		const struct rcar_csi2_format *format;
>> +		struct v4l2_mbus_framefmt *fmt;
>>  
>> -	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
>> -		fmt->width, fmt->height,
>> -		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>> +		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
>> +		if (!fmt)
>> +			return -EINVAL;
>>  
>> -	/* Code is validated in set_fmt. */
>> -	format = rcsi2_code_to_fmt(fmt->code);
>> -	if (!format)
>> -		return -EINVAL;
>> +		format = rcsi2_code_to_fmt(fmt->code);
>> +		if (!format)
>> +			return -EINVAL;
>>  
>> -	/*
>> -	 * Enable all supported CSI-2 channels with virtual channel and
>> -	 * data type matching.
>> -	 *
>> -	 * NOTE: It's not possible to get individual datatype for each
>> -	 *       source virtual channel. Once this is possible in V4L2
>> -	 *       it should be used here.
>> -	 */
>> -	for (i = 0; i < priv->info->num_channels; i++) {
>> +		memset(fd, 0, sizeof(*fd));
>> +
>> +		fd->num_entries = 1;
>> +		fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>> +		fd->entry[0].stream = 0;
>> +		fd->entry[0].pixelcode = fmt->code;
>> +		fd->entry[0].bus.csi2.vc = 0;
>> +		fd->entry[0].bus.csi2.dt = format->datatype;
>> +	}
>> +
>> +	for_each_active_route(&state->routing, route) {
>> +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
> 
> const

Yep.

>> +		const struct v4l2_mbus_framefmt *fmt;
>> +		const struct rcar_csi2_format *format;
>> +		unsigned int i;
>> +		u8 vc, dt;
>>  		u32 vcdt_part;
>>  
>> -		if (priv->channel_vc[i] < 0)
>> -			continue;
>> +		for (i = 0; i < source_fd.num_entries; i++) {
>> +			if (source_fd.entry[i].stream == route->sink_stream) {
> 
> No need to check the pad ?

We only have a single sink pad in CSI-2.

>> +				source_entry = &source_fd.entry[i];
>> +				break;
>> +			}
>> +		}
>> +
>> +		if (!source_entry) {
>> +			dev_err(priv->dev,
>> +				"Failed to find stream from source frame desc\n");
>> +			return -EPIPE;
>> +		}
>>  
>> -		vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN |
>> -			VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype);
>> +		vc = source_entry->bus.csi2.vc;
>> +		dt = source_entry->bus.csi2.dt;
>> +
>> +		vcdt_part = VCDT_SEL_VC(vc) | VCDT_VCDTN_EN |
>> +			VCDT_SEL_DTN_ON | VCDT_SEL_DT(dt);
> 
> I would drop the vc and dt variables and write
> 
> 		vcdt_part = VCDT_SEL_VC(source_entry->bus.csi2.vc)
> 			  | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON
> 			  | VCDT_SEL_DT(source_entry->bus.csi2.dt);

Yes, I think this is fine too.

>>  
>>  		/* Store in correct reg and offset. */
>> -		if (i < 2)
>> -			vcdt |= vcdt_part << ((i % 2) * 16);
>> +		if (ch < 2)
>> +			vcdt |= vcdt_part << ((ch % 2) * 16);
>>  		else
>> -			vcdt2 |= vcdt_part << ((i % 2) * 16);
>> -	}
>> +			vcdt2 |= vcdt_part << ((ch % 2) * 16);
>> +
>> +		fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK,
>> +						   route->sink_stream);
>> +		if (!fmt)
>> +			return -EINVAL;
>> +
>> +		dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
>> +			fmt->width, fmt->height,
>> +			fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>>  
>> -	if (fmt->field == V4L2_FIELD_ALTERNATE)
>> -		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
>> -			| FLD_FLD_EN;
>> +		/* Code is validated in set_fmt. */
> 
> Then why don't you drop the !format check ?

It's a copy of the existing code. I've been reluctant to change gen3
code, as I can't test. But I can't really see how this could be wrong,
so I'll just drop the check here.

 Tomi

>> +		format = rcsi2_code_to_fmt(fmt->code);
>> +		if (!format)
>> +			return -EINVAL;
>> +
>> +		if (fmt->field == V4L2_FIELD_ALTERNATE)
>> +			fld |= FLD_FLD_EN(ch);
>> +
>> +		ch++;
>> +	}
>>  
>>  	/*
>>  	 * Get the number of active data lanes inspecting the remote mbus
>