[PATCH 0/3] media: platform: rzg2l-cru: CSI-2 and CRU enhancements

[PATCH 0/3] media: platform: rzg2l-cru: CSI-2 and CRU enhancements

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Hi All,

This patch series aims to add the below:
- Retrieve virtual channel from remote subdev
- Support to capture 8bit Bayer formats.

Cheers,
Prabhakar

Lad Prabhakar (3):
  media: platform: rzg2l-cru: rzg2l-csi2: Implement .get_frame_desc()
  media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel
    information
  media: platform: rzg2l-cru: Add support to capture 8bit raw sRGB

 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  5 ++
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   | 19 ++++-
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     |  9 ++-
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 80 +++++++++++++++++--
 4 files changed, 102 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH 1/3] media: platform: rzg2l-cru: rzg2l-csi2: Implement .get_frame_desc()

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The RZ/G2L CRU requires information about which VCx to process data from,
among the four available VCs. To obtain this information, the
.get_frame_desc() routine is implemented. This routine, in turn, calls
.get_frame_desc() on the remote sensor connected to the CSI2 bridge.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../media/platform/renesas/rzg2l-cru/rzg2l-csi2.c    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index c7fdee347ac8..a7e4a0c109da 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -582,6 +582,17 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				     struct v4l2_mbus_frame_desc *fd)
+{
+	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
+
+	if (!csi2->remote_source)
+		return -EINVAL;
+
+	return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc, pad, fd);
+}
+
 static const struct v4l2_subdev_video_ops rzg2l_csi2_video_ops = {
 	.s_stream = rzg2l_csi2_s_stream,
 	.pre_streamon = rzg2l_csi2_pre_streamon,
@@ -593,6 +604,7 @@ static const struct v4l2_subdev_pad_ops rzg2l_csi2_pad_ops = {
 	.enum_frame_size = rzg2l_csi2_enum_frame_size,
 	.set_fmt = rzg2l_csi2_set_format,
 	.get_fmt = v4l2_subdev_get_fmt,
+	.get_frame_desc = rzg2l_csi2_get_frame_desc,
 };
 
 static const struct v4l2_subdev_ops rzg2l_csi2_subdev_ops = {
-- 
2.34.1

[PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
virtual channel should be processed from the four available VCs. To
retrieve this information from the connected subdevice, the
.get_frame_desc() function is called.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index bbf4674f888d..6101a070e785 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
 	spin_unlock_irqrestore(&cru->qlock, flags);
 }
 
+static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
+{
+	struct v4l2_mbus_frame_desc fd = { };
+	struct media_pad *pad;
+	int ret;
+
+	pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);
+	if (IS_ERR(pad))
+		return PTR_ERR(pad);
+
+	ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
+			       pad->index, &fd);
+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		return ret;
+	/* If remote subdev does not implement .get_frame_desc default to VC0. */
+	if (ret == -ENOIOCTLCMD)
+		return 0;
+
+	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
+		return -EINVAL;
+
+	return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;
+}
+
 int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
 {
 	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
 	unsigned long flags;
 	int ret;
 
+	ret = rzg2l_cru_get_virtual_channel(cru);
+	if (ret < 0)
+		return ret;
+	cru->csi.channel = ret;
+
 	spin_lock_irqsave(&cru->qlock, flags);
 
 	/* Select a video input */
-- 
2.34.1

[PATCH 3/3] media: platform: rzg2l-cru: Add support to capture 8bit raw sRGB

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Add support to capture 8bit Bayer formats.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  5 ++
 .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   |  7 ++-
 .../platform/renesas/rzg2l-cru/rzg2l-ip.c     |  9 +++-
 .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 51 ++++++++++++++++---
 4 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
index 174760239548..83c664cb0929 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
@@ -58,6 +58,11 @@ struct rzg2l_cru_ip {
 	struct v4l2_subdev *remote;
 };
 
+enum rzg2l_cru_fmt {
+	RZG2L_YUV = 0,
+	RZG2L_RAW_BAYER,
+	RZG2L_USER_DEFINED,
+};
 /**
  * struct rzg2l_cru_dev - Renesas CRU device structure
  * @dev:		(OF) device
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
index a7e4a0c109da..b14c92cd7c18 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
@@ -18,6 +18,7 @@
 #include <linux/sys_soc.h>
 #include <linux/units.h>
 
+#include <media/mipi-csi2.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -188,7 +189,11 @@ struct rzg2l_csi2_format {
 };
 
 static const struct rzg2l_csi2_format rzg2l_csi2_formats[] = {
-	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
 };
 
 static inline struct rzg2l_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
index ac8ebae4ed07..5f60be92ea85 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/delay.h>
+#include <media/mipi-csi2.h>
 #include "rzg2l-cru.h"
 
 struct rzg2l_cru_ip_format {
@@ -15,7 +16,11 @@ struct rzg2l_cru_ip_format {
 };
 
 static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
-	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 },
+	{ .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
+	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
 };
 
 enum rzg2l_csi2_pads {
@@ -149,7 +154,7 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index != 0)
 		return -EINVAL;
 
-	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
+	if (!rzg2l_cru_ip_code_to_fmt(fse->code))
 		return -EINVAL;
 
 	fse->min_width = RZG2L_CRU_MIN_INPUT_WIDTH;
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index 6101a070e785..66a0b80e8da7 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -15,6 +15,7 @@
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
 
+#include <media/mipi-csi2.h>
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-dma-contig.h>
 
@@ -78,6 +79,7 @@
 #define ICnMC				0x208
 #define ICnMC_CSCTHR			BIT(5)
 #define ICnMC_INF_YUV8_422		(0x1e << 16)
+#define ICnMC_INF_RAW8			(0x2a << 16)
 #define ICnMC_INF_USER			(0x30 << 16)
 #define ICnMC_VCSEL(x)			((x) << 22)
 #define ICnMC_INF_MASK			GENMASK(21, 16)
@@ -203,6 +205,10 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
 
 	switch (fmt.format.code) {
 	case MEDIA_BUS_FMT_UYVY8_1X16:
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
 		break;
 	default:
 		return -EPIPE;
@@ -300,7 +306,7 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
 	rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
 }
 
-static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
+static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, enum rzg2l_cru_fmt *input_fmt,
 				 struct v4l2_mbus_framefmt *ip_sd_fmt)
 {
 	u32 icnmc;
@@ -308,11 +314,18 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
 	switch (ip_sd_fmt->code) {
 	case MEDIA_BUS_FMT_UYVY8_1X16:
 		icnmc = ICnMC_INF_YUV8_422;
-		*input_is_yuv = true;
+		*input_fmt = RZG2L_YUV;
+		break;
+	case MEDIA_BUS_FMT_SBGGR8_1X8:
+	case MEDIA_BUS_FMT_SGBRG8_1X8:
+	case MEDIA_BUS_FMT_SGRBG8_1X8:
+	case MEDIA_BUS_FMT_SRGGB8_1X8:
+		icnmc = ICnMC_INF_RAW8;
+		*input_fmt = RZG2L_RAW_BAYER;
 		break;
 	default:
-		*input_is_yuv = false;
 		icnmc = ICnMC_INF_USER;
+		*input_fmt = RZG2L_USER_DEFINED;
 		break;
 	}
 
@@ -327,17 +340,23 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
 static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
 					   struct v4l2_mbus_framefmt *ip_sd_fmt)
 {
-	bool output_is_yuv = false;
-	bool input_is_yuv = false;
+	enum rzg2l_cru_fmt input_fmt, output_fmt;
 	u32 icndmr;
 
-	rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt);
+	rzg2l_cru_csi2_setup(cru, &input_fmt, ip_sd_fmt);
 
 	/* Output format */
 	switch (cru->format.pixelformat) {
 	case V4L2_PIX_FMT_UYVY:
 		icndmr = ICnDMR_YCMODE_UYVY;
-		output_is_yuv = true;
+		output_fmt = RZG2L_YUV;
+		break;
+	case V4L2_PIX_FMT_SBGGR8:
+	case V4L2_PIX_FMT_SGBRG8:
+	case V4L2_PIX_FMT_SGRBG8:
+	case V4L2_PIX_FMT_SRGGB8:
+		icndmr = 0;
+		output_fmt = RZG2L_RAW_BAYER;
 		break;
 	default:
 		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
@@ -346,7 +365,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
 	}
 
 	/* If input and output use same colorspace, do bypass mode */
-	if (output_is_yuv == input_is_yuv)
+	if (input_fmt == output_fmt)
 		rzg2l_cru_write(cru, ICnMC,
 				rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
 	else
@@ -809,6 +828,22 @@ static const struct v4l2_format_info rzg2l_cru_formats[] = {
 		.format = V4L2_PIX_FMT_UYVY,
 		.bpp[0] = 2,
 	},
+	{
+		.format = V4L2_PIX_FMT_SBGGR8,
+		.bpp[0] = 1,
+	},
+	{
+		.format = V4L2_PIX_FMT_SGBRG8,
+		.bpp[0] = 1,
+	},
+	{
+		.format = V4L2_PIX_FMT_SGRBG8,
+		.bpp[0] = 1,
+	},
+	{
+		.format = V4L2_PIX_FMT_SRGGB8,
+		.bpp[0] = 1,
+	},
 };
 
 const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)
-- 
2.34.1

Re: [PATCH 1/3] media: platform: rzg2l-cru: rzg2l-csi2: Implement .get_frame_desc()

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Sep 06, 2024 at 06:39:45PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The RZ/G2L CRU requires information about which VCx to process data from,
> among the four available VCs. To obtain this information, the
> .get_frame_desc() routine is implemented. This routine, in turn, calls
> .get_frame_desc() on the remote sensor connected to the CSI2 bridge.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../media/platform/renesas/rzg2l-cru/rzg2l-csi2.c    | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index c7fdee347ac8..a7e4a0c109da 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -582,6 +582,17 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +				     struct v4l2_mbus_frame_desc *fd)
> +{
> +	struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
> +
> +	if (!csi2->remote_source)
> +		return -EINVAL;

Maybe -ENODEV ?

> +
> +	return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc, pad, fd);
> +}
> +

I wonder if we should implement a wrapper around .get_frame_desc() that
would automatically forward the call to the source if .get_frame_desc()
isn't implemented by a subdev. That's not a requirement for this series,

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

>  static const struct v4l2_subdev_video_ops rzg2l_csi2_video_ops = {
>  	.s_stream = rzg2l_csi2_s_stream,
>  	.pre_streamon = rzg2l_csi2_pre_streamon,
> @@ -593,6 +604,7 @@ static const struct v4l2_subdev_pad_ops rzg2l_csi2_pad_ops = {
>  	.enum_frame_size = rzg2l_csi2_enum_frame_size,
>  	.set_fmt = rzg2l_csi2_set_format,
>  	.get_fmt = v4l2_subdev_get_fmt,
> +	.get_frame_desc = rzg2l_csi2_get_frame_desc,
>  };
>  
>  static const struct v4l2_subdev_ops rzg2l_csi2_subdev_ops = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
> virtual channel should be processed from the four available VCs. To
> retrieve this information from the connected subdevice, the
> .get_frame_desc() function is called.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index bbf4674f888d..6101a070e785 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
>  	spin_unlock_irqrestore(&cru->qlock, flags);
>  }
>  
> +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> +{
> +	struct v4l2_mbus_frame_desc fd = { };
> +	struct media_pad *pad;
> +	int ret;
> +
> +	pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);

It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding
the pad number. That would require moving rzg2l_csi2_pads to the shared
header. You can do that on top.

An now that I've said that, is it really the source pad you need here ?

> +	if (IS_ERR(pad))
> +		return PTR_ERR(pad);

Can this happen, or would the pipeline fail to validate ? I think you
can set the MUST_CONNECT flag on the sink pad, then you'll have a
guarantee something will be connected.

> +
> +	ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> +			       pad->index, &fd);
> +	if (ret < 0 && ret != -ENOIOCTLCMD)

Printing an error message would help debugging.

> +		return ret;
> +	/* If remote subdev does not implement .get_frame_desc default to VC0. */
> +	if (ret == -ENOIOCTLCMD)
> +		return 0;
> +
> +	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)

An error message would help here too I think.

> +		return -EINVAL;
> +
> +	return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;

I think you should return an error if fd.num_entries is 0, that
shouldn't happen.

> +}
> +
>  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
>  {
>  	struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
>  	unsigned long flags;
>  	int ret;
>  
> +	ret = rzg2l_cru_get_virtual_channel(cru);
> +	if (ret < 0)
> +		return ret;
> +	cru->csi.channel = ret;

How about passing the value to the function that needs it, instead of
storing it in cru->csi.channel ? You can do that on top and drop the
csi.channel field.

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

> +
>  	spin_lock_irqsave(&cru->qlock, flags);
>  
>  	/* Select a video input */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] media: platform: rzg2l-cru: Add support to capture 8bit raw sRGB

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Sep 06, 2024 at 06:39:47PM +0100, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Add support to capture 8bit Bayer formats.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../platform/renesas/rzg2l-cru/rzg2l-cru.h    |  5 ++
>  .../platform/renesas/rzg2l-cru/rzg2l-csi2.c   |  7 ++-
>  .../platform/renesas/rzg2l-cru/rzg2l-ip.c     |  9 +++-
>  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 51 ++++++++++++++++---
>  4 files changed, 61 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> index 174760239548..83c664cb0929 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-cru.h
> @@ -58,6 +58,11 @@ struct rzg2l_cru_ip {
>  	struct v4l2_subdev *remote;
>  };
>  
> +enum rzg2l_cru_fmt {
> +	RZG2L_YUV = 0,
> +	RZG2L_RAW_BAYER,
> +	RZG2L_USER_DEFINED,
> +};
>  /**
>   * struct rzg2l_cru_dev - Renesas CRU device structure
>   * @dev:		(OF) device
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> index a7e4a0c109da..b14c92cd7c18 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> @@ -18,6 +18,7 @@
>  #include <linux/sys_soc.h>
>  #include <linux/units.h>
>  
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -188,7 +189,11 @@ struct rzg2l_csi2_format {
>  };
>  
>  static const struct rzg2l_csi2_format rzg2l_csi2_formats[] = {
> -	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 },
> +	{ .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> +	{ .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> +	{ .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> +	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
>  };
>  
>  static inline struct rzg2l_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> index ac8ebae4ed07..5f60be92ea85 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-ip.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <media/mipi-csi2.h>

I would add a blank line on both sides.

>  #include "rzg2l-cru.h"
>  
>  struct rzg2l_cru_ip_format {
> @@ -15,7 +16,11 @@ struct rzg2l_cru_ip_format {
>  };
>  
>  static const struct rzg2l_cru_ip_format rzg2l_cru_ip_formats[] = {
> -	{ .code = MEDIA_BUS_FMT_UYVY8_1X16,	.datatype = 0x1e, .bpp = 16 },
> +	{ .code = MEDIA_BUS_FMT_UYVY8_1X16, .datatype = MIPI_CSI2_DT_YUV422_8B, .bpp = 16 },
> +	{ .code = MEDIA_BUS_FMT_SBGGR8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> +	{ .code = MEDIA_BUS_FMT_SGBRG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> +	{ .code = MEDIA_BUS_FMT_SGRBG8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
> +	{ .code = MEDIA_BUS_FMT_SRGGB8_1X8, .datatype = MIPI_CSI2_DT_RAW8, .bpp = 8, },
>  };
>  
>  enum rzg2l_csi2_pads {
> @@ -149,7 +154,7 @@ static int rzg2l_cru_ip_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fse->index != 0)
>  		return -EINVAL;
>  
> -	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
> +	if (!rzg2l_cru_ip_code_to_fmt(fse->code))
>  		return -EINVAL;
>  
>  	fse->min_width = RZG2L_CRU_MIN_INPUT_WIDTH;
> diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> index 6101a070e785..66a0b80e8da7 100644
> --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> @@ -15,6 +15,7 @@
>  #include <linux/delay.h>
>  #include <linux/pm_runtime.h>
>  
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-dma-contig.h>
>  
> @@ -78,6 +79,7 @@
>  #define ICnMC				0x208
>  #define ICnMC_CSCTHR			BIT(5)
>  #define ICnMC_INF_YUV8_422		(0x1e << 16)
> +#define ICnMC_INF_RAW8			(0x2a << 16)
>  #define ICnMC_INF_USER			(0x30 << 16)

It looks like the INF field contains the MIPI data type. I would replace
this with

#define ICnMC_INF(x)			((x) << 16)

and use it as

	ICnMC_INF(MIPI_CSI2_DT_RAW8)

>  #define ICnMC_VCSEL(x)			((x) << 22)
>  #define ICnMC_INF_MASK			GENMASK(21, 16)
> @@ -203,6 +205,10 @@ static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
>  
>  	switch (fmt.format.code) {
>  	case MEDIA_BUS_FMT_UYVY8_1X16:
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
>  		break;
>  	default:
>  		return -EPIPE;
> @@ -300,7 +306,7 @@ static void rzg2l_cru_initialize_axi(struct rzg2l_cru_dev *cru)
>  	rzg2l_cru_write(cru, AMnAXIATTR, amnaxiattr);
>  }
>  
> -static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
> +static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, enum rzg2l_cru_fmt *input_fmt,
>  				 struct v4l2_mbus_framefmt *ip_sd_fmt)
>  {
>  	u32 icnmc;
> @@ -308,11 +314,18 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
>  	switch (ip_sd_fmt->code) {
>  	case MEDIA_BUS_FMT_UYVY8_1X16:
>  		icnmc = ICnMC_INF_YUV8_422;
> -		*input_is_yuv = true;
> +		*input_fmt = RZG2L_YUV;
> +		break;
> +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> +		icnmc = ICnMC_INF_RAW8;
> +		*input_fmt = RZG2L_RAW_BAYER;
>  		break;
>  	default:
> -		*input_is_yuv = false;
>  		icnmc = ICnMC_INF_USER;
> +		*input_fmt = RZG2L_USER_DEFINED;
>  		break;
>  	}
>  
> @@ -327,17 +340,23 @@ static void rzg2l_cru_csi2_setup(struct rzg2l_cru_dev *cru, bool *input_is_yuv,
>  static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  					   struct v4l2_mbus_framefmt *ip_sd_fmt)
>  {
> -	bool output_is_yuv = false;
> -	bool input_is_yuv = false;
> +	enum rzg2l_cru_fmt input_fmt, output_fmt;
>  	u32 icndmr;
>  
> -	rzg2l_cru_csi2_setup(cru, &input_is_yuv, ip_sd_fmt);
> +	rzg2l_cru_csi2_setup(cru, &input_fmt, ip_sd_fmt);
>  
>  	/* Output format */
>  	switch (cru->format.pixelformat) {
>  	case V4L2_PIX_FMT_UYVY:
>  		icndmr = ICnDMR_YCMODE_UYVY;
> -		output_is_yuv = true;
> +		output_fmt = RZG2L_YUV;
> +		break;
> +	case V4L2_PIX_FMT_SBGGR8:
> +	case V4L2_PIX_FMT_SGBRG8:
> +	case V4L2_PIX_FMT_SGRBG8:
> +	case V4L2_PIX_FMT_SRGGB8:
> +		icndmr = 0;
> +		output_fmt = RZG2L_RAW_BAYER;
>  		break;
>  	default:
>  		dev_err(cru->dev, "Invalid pixelformat (0x%x)\n",
> @@ -346,7 +365,7 @@ static int rzg2l_cru_initialize_image_conv(struct rzg2l_cru_dev *cru,
>  	}
>  
>  	/* If input and output use same colorspace, do bypass mode */
> -	if (output_is_yuv == input_is_yuv)
> +	if (input_fmt == output_fmt)
>  		rzg2l_cru_write(cru, ICnMC,
>  				rzg2l_cru_read(cru, ICnMC) | ICnMC_CSCTHR);
>  	else
> @@ -809,6 +828,22 @@ static const struct v4l2_format_info rzg2l_cru_formats[] = {
>  		.format = V4L2_PIX_FMT_UYVY,
>  		.bpp[0] = 2,
>  	},
> +	{
> +		.format = V4L2_PIX_FMT_SBGGR8,
> +		.bpp[0] = 1,
> +	},
> +	{
> +		.format = V4L2_PIX_FMT_SGBRG8,
> +		.bpp[0] = 1,
> +	},
> +	{
> +		.format = V4L2_PIX_FMT_SGRBG8,
> +		.bpp[0] = 1,
> +	},
> +	{
> +		.format = V4L2_PIX_FMT_SRGGB8,
> +		.bpp[0] = 1,
> +	},
>  };

I think all of this could be simplified if you add the format to the
rzg2l_cru_ip_format structure, as well as a rzg2l_cru_fmt member, and
used rzg2l_cru_ip_format through the driver.
rzg2l_cru_initialize_image_conv() would look up the rzg2l_cru_ip_format
entry based on the code, and would pass that to rzg2l_cru_csi2_setup(),
which wouldn't have to return an input_fmt. You could even add the
icndmr value to the structure too, to avoid a switch/case.

>  
>  const struct v4l2_format_info *rzg2l_cru_format_from_pixel(u32 format)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media: platform: rzg2l-cru: rzg2l-csi2: Implement .get_frame_desc()

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Fri, Sep 6, 2024 at 11:38 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Sep 06, 2024 at 06:39:45PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The RZ/G2L CRU requires information about which VCx to process data from,
> > among the four available VCs. To obtain this information, the
> > .get_frame_desc() routine is implemented. This routine, in turn, calls
> > .get_frame_desc() on the remote sensor connected to the CSI2 bridge.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../media/platform/renesas/rzg2l-cru/rzg2l-csi2.c    | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > index c7fdee347ac8..a7e4a0c109da 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-csi2.c
> > @@ -582,6 +582,17 @@ static int rzg2l_csi2_enum_frame_size(struct v4l2_subdev *sd,
> >       return 0;
> >  }
> >
> > +static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> > +                                  struct v4l2_mbus_frame_desc *fd)
> > +{
> > +     struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
> > +
> > +     if (!csi2->remote_source)
> > +             return -EINVAL;
>
> Maybe -ENODEV ?
>
Agreed.

> > +
> > +     return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc, pad, fd);
> > +}
> > +
>
> I wonder if we should implement a wrapper around .get_frame_desc() that
> would automatically forward the call to the source if .get_frame_desc()
> isn't implemented by a subdev. That's not a requirement for this series,
>
That would be helpful.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >  static const struct v4l2_subdev_video_ops rzg2l_csi2_video_ops = {
> >       .s_stream = rzg2l_csi2_s_stream,
> >       .pre_streamon = rzg2l_csi2_pre_streamon,
> > @@ -593,6 +604,7 @@ static const struct v4l2_subdev_pad_ops rzg2l_csi2_pad_ops = {
> >       .enum_frame_size = rzg2l_csi2_enum_frame_size,
> >       .set_fmt = rzg2l_csi2_set_format,
> >       .get_fmt = v4l2_subdev_get_fmt,
> > +     .get_frame_desc = rzg2l_csi2_get_frame_desc,
> >  };
> >
> >  static const struct v4l2_subdev_ops rzg2l_csi2_subdev_ops = {
>

Cheers,
Prabhakar

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
> > virtual channel should be processed from the four available VCs. To
> > retrieve this information from the connected subdevice, the
> > .get_frame_desc() function is called.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > index bbf4674f888d..6101a070e785 100644
> > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> >       spin_unlock_irqrestore(&cru->qlock, flags);
> >  }
> >
> > +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> > +{
> > +     struct v4l2_mbus_frame_desc fd = { };
> > +     struct media_pad *pad;
> > +     int ret;
> > +
> > +     pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);
>
> It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding
> the pad number. That would require moving rzg2l_csi2_pads to the shared
> header. You can do that on top.
>
With the below comment we dont need to move rzg2l_csi2_pads into the
shared header.

> An now that I've said that, is it really the source pad you need here ?
>
Ouch you are right.

> > +     if (IS_ERR(pad))
> > +             return PTR_ERR(pad);
>
> Can this happen, or would the pipeline fail to validate ? I think you
> can set the MUST_CONNECT flag on the sink pad, then you'll have a
> guarantee something will be connected.
>
After adding the MUST_CONNECT flag, I wouldn't need the  above
media_pad_remote_pad_unique()...

> > +
> > +     ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> > +                            pad->index, &fd);
... and here I can use '0' instead or do you prefer RZG2L_CRU_IP_SINK
(I say because we are calling into remote subdev of IP which is CSI so
the RZG2L_CRU_IP_SINK wont make sense)?

> > +     if (ret < 0 && ret != -ENOIOCTLCMD)
>
> Printing an error message would help debugging.
>
OK, I will add.

> > +             return ret;
> > +     /* If remote subdev does not implement .get_frame_desc default to VC0. */
> > +     if (ret == -ENOIOCTLCMD)
> > +             return 0;
> > +
> > +     if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>
> An error message would help here too I think.
>
OK, I will add.

> > +             return -EINVAL;
> > +
> > +     return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;
>
> I think you should return an error if fd.num_entries is 0, that
> shouldn't happen.
>
OK, I will add.

> > +}
> > +
> >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> >  {
> >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> >       unsigned long flags;
> >       int ret;
> >
> > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > +     if (ret < 0)
> > +             return ret;
> > +     cru->csi.channel = ret;
>
> How about passing the value to the function that needs it, instead of
> storing it in cru->csi.channel ? You can do that on top and drop the
> csi.channel field.
>
OK, let me check if this can be done.

Cheers,
Prabhakar

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Lad, Prabhakar]

Hi Laurent,

On Sat, Sep 7, 2024 at 10:09 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
>
> Hi Laurent,
>
> Thank you for the review.
>
> On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
<snip>
> > > +
> > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> > >  {
> > >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> > >       unsigned long flags;
> > >       int ret;
> > >
> > > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     cru->csi.channel = ret;
> >
> > How about passing the value to the function that needs it, instead of
> > storing it in cru->csi.channel ? You can do that on top and drop the
> > csi.channel field.
> >
> OK, let me check if this can be done.
>
The virtual channel number is programmed in rzg2l_cru_csi2_setup() [0]
call, below is the code flow of the call. This code flow is called by
spinlock held.

rzg2l_cru_start_image_processing()
    rzg2l_cru_initialize_image_conv()
        rzg2l_cru_csi2_setup()

When rzg2l_cru_get_virtual_channel() is called directly in
rzg2l_cru_csi2_setup() function we get "BUG: Invalid wait context"
(when PROVE_LOCKING is enabled) this is due to we do a mutex lock as
part of v4l2_subdev_lock_and_get_active_state() in get_frame_desc.

So probably I'll leave this as it is now.

[0] https://elixir.bootlin.com/linux/v6.10.8/source/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c#L311

Cheers,
Prabhakar

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Laurent Pinchart]

Hi Prabhakar,

On Sat, Sep 07, 2024 at 10:09:10PM +0100, Lad, Prabhakar wrote:
> On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart wrote:
> > On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
> > > virtual channel should be processed from the four available VCs. To
> > > retrieve this information from the connected subdevice, the
> > > .get_frame_desc() function is called.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > index bbf4674f888d..6101a070e785 100644
> > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> > >       spin_unlock_irqrestore(&cru->qlock, flags);
> > >  }
> > >
> > > +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> > > +{
> > > +     struct v4l2_mbus_frame_desc fd = { };
> > > +     struct media_pad *pad;
> > > +     int ret;
> > > +
> > > +     pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);
> >
> > It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding
> > the pad number. That would require moving rzg2l_csi2_pads to the shared
> > header. You can do that on top.
>
> With the below comment we dont need to move rzg2l_csi2_pads into the
> shared header.
> 
> > An now that I've said that, is it really the source pad you need here ?
>
> Ouch you are right.
> 
> > > +     if (IS_ERR(pad))
> > > +             return PTR_ERR(pad);
> >
> > Can this happen, or would the pipeline fail to validate ? I think you
> > can set the MUST_CONNECT flag on the sink pad, then you'll have a
> > guarantee something will be connected.
>
> After adding the MUST_CONNECT flag, I wouldn't need the  above
> media_pad_remote_pad_unique()...
> 
> > > +
> > > +     ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> > > +                            pad->index, &fd);
>
> ... and here I can use '0' instead

Can you ? You need to call the operation on the pad of the connected
entity that is connected to tbe sink pad of the IP entity. That would be
the source pad of the CSI-2 RX in this case, but it can't be hardcoded
as it could also bethe source pad of a parallel sensor (once support for
that will be implemented). I think you therefore need to keep the
media_pad_remote_pad_unique() call.

> or do you prefer RZG2L_CRU_IP_SINK
> (I say because we are calling into remote subdev of IP which is CSI so
> the RZG2L_CRU_IP_SINK wont make sense)?
> 
> > > +     if (ret < 0 && ret != -ENOIOCTLCMD)
> >
> > Printing an error message would help debugging.
> >
> OK, I will add.
> 
> > > +             return ret;
> > > +     /* If remote subdev does not implement .get_frame_desc default to VC0. */
> > > +     if (ret == -ENOIOCTLCMD)
> > > +             return 0;
> > > +
> > > +     if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> >
> > An error message would help here too I think.
> >
> OK, I will add.
> 
> > > +             return -EINVAL;
> > > +
> > > +     return fd.num_entries ? fd.entry[0].bus.csi2.vc : 0;
> >
> > I think you should return an error if fd.num_entries is 0, that
> > shouldn't happen.
> >
> OK, I will add.
> 
> > > +}
> > > +
> > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> > >  {
> > >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> > >       unsigned long flags;
> > >       int ret;
> > >
> > > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > > +     if (ret < 0)
> > > +             return ret;
> > > +     cru->csi.channel = ret;
> >
> > How about passing the value to the function that needs it, instead of
> > storing it in cru->csi.channel ? You can do that on top and drop the
> > csi.channel field.
> >
> OK, let me check if this can be done.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Lad, Prabhakar]

Hi Laurent,

On Sun, Sep 8, 2024 at 11:39 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Sat, Sep 07, 2024 at 10:09:10PM +0100, Lad, Prabhakar wrote:
> > On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart wrote:
> > > On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
> > > > virtual channel should be processed from the four available VCs. To
> > > > retrieve this information from the connected subdevice, the
> > > > .get_frame_desc() function is called.
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > ---
> > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
> > > >  1 file changed, 29 insertions(+)
> > > >
> > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > index bbf4674f888d..6101a070e785 100644
> > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> > > >       spin_unlock_irqrestore(&cru->qlock, flags);
> > > >  }
> > > >
> > > > +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> > > > +{
> > > > +     struct v4l2_mbus_frame_desc fd = { };
> > > > +     struct media_pad *pad;
> > > > +     int ret;
> > > > +
> > > > +     pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);
> > >
> > > It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding
> > > the pad number. That would require moving rzg2l_csi2_pads to the shared
> > > header. You can do that on top.
> >
> > With the below comment we dont need to move rzg2l_csi2_pads into the
> > shared header.
> >
> > > An now that I've said that, is it really the source pad you need here ?
> >
> > Ouch you are right.
> >
> > > > +     if (IS_ERR(pad))
> > > > +             return PTR_ERR(pad);
> > >
> > > Can this happen, or would the pipeline fail to validate ? I think you
> > > can set the MUST_CONNECT flag on the sink pad, then you'll have a
> > > guarantee something will be connected.
> >
> > After adding the MUST_CONNECT flag, I wouldn't need the  above
> > media_pad_remote_pad_unique()...
> >
> > > > +
> > > > +     ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> > > > +                            pad->index, &fd);
> >
> > ... and here I can use '0' instead
>
> Can you ? You need to call the operation on the pad of the connected
> entity that is connected to tbe sink pad of the IP entity. That would be
> the source pad of the CSI-2 RX in this case, but it can't be hardcoded
> as it could also bethe source pad of a parallel sensor (once support for
> that will be implemented). I think you therefore need to keep the
> media_pad_remote_pad_unique() call.
>
media pipeline for RZ/G2L is [0].

Calling media_pad_remote_pad_unique() with sink pad of IP entity will
return source pad of CSI-Rx, where the pad index will be '1', passing
pad index '1' to .get_frame_desc to CSI subdev and then OV5645 subdev
would return -EINVAL.

I need to update patch [1] instead of just forwarding the pad index to
remote subdev I'll need to do the same as done IP subdev ie in CSI
subdev call media_pad_remote_pad_unique() on sink pad of CSI which
will return OV5465 source pad, and this will have a pad index of '0'.
The CSI get_frame_desc() will look something like below:

static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
                     struct v4l2_mbus_frame_desc *fd)
{
    struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
    struct media_pad *remote_pad;

    if (!csi2->remote_source)
        return -ENODEV;

    remote_pad = media_pad_remote_pad_unique(&csi2->pads[RZG2L_CSI2_SINK]);
    if (IS_ERR(remote_pad)) {
        dev_err(csi2->dev, "can't get source pad of %s (%ld)\n",
            csi2->remote_source->name, PTR_ERR(remote_pad));
        return PTR_ERR(remote_pad);
    }
    return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc,
remote_pad->index, fd);
}

For the parallel input case I plan to implement something similar to
R-Car vin with bool flag 'is_csi' where we skip calling
'rzg2l_cru_get_virtual_channel'.

[0] https://postimg.cc/rz0vSMLC
[1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240906173947.282402-2-prabhakar.mahadev-lad.rj@bp.renesas.com/

Cheers,
Prabhakar

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Laurent Pinchart]

On Sun, Sep 08, 2024 at 09:23:52PM +0100, Lad, Prabhakar wrote:
> On Sat, Sep 7, 2024 at 10:09 PM Lad, Prabhakar wrote:
> > On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart wrote:
>
> <snip>
>
> > > > +
> > > >  int rzg2l_cru_start_image_processing(struct rzg2l_cru_dev *cru)
> > > >  {
> > > >       struct v4l2_mbus_framefmt *fmt = rzg2l_cru_ip_get_src_fmt(cru);
> > > >       unsigned long flags;
> > > >       int ret;
> > > >
> > > > +     ret = rzg2l_cru_get_virtual_channel(cru);
> > > > +     if (ret < 0)
> > > > +             return ret;
> > > > +     cru->csi.channel = ret;
> > >
> > > How about passing the value to the function that needs it, instead of
> > > storing it in cru->csi.channel ? You can do that on top and drop the
> > > csi.channel field.
> >
> > OK, let me check if this can be done.
>
> The virtual channel number is programmed in rzg2l_cru_csi2_setup() [0]
> call, below is the code flow of the call. This code flow is called by
> spinlock held.
> 
> rzg2l_cru_start_image_processing()
>     rzg2l_cru_initialize_image_conv()
>         rzg2l_cru_csi2_setup()
> 
> When rzg2l_cru_get_virtual_channel() is called directly in
> rzg2l_cru_csi2_setup() function we get "BUG: Invalid wait context"
> (when PROVE_LOCKING is enabled) this is due to we do a mutex lock as
> part of v4l2_subdev_lock_and_get_active_state() in get_frame_desc.

I didn't mean calling rzg2l_cru_get_virtual_channel() from
rzg2l_cru_csi2_setup(), but passing the virtual channel number from
rzg2l_cru_start_image_processing() to rzg2l_cru_initialize_image_conv()
and then to rzg2l_cru_csi2_setup().

> So probably I'll leave this as it is now.
> 
> [0] https://elixir.bootlin.com/linux/v6.10.8/source/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c#L311

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] media: platform: rzg2l-cru: rzg2l-video: Retrieve virtual channel information

[Laurent Pinchart]

Hi Prabhakar,

On Mon, Sep 09, 2024 at 10:57:59AM +0100, Lad, Prabhakar wrote:
> On Sun, Sep 8, 2024 at 11:39 PM Laurent Pinchart wrote:
> > On Sat, Sep 07, 2024 at 10:09:10PM +0100, Lad, Prabhakar wrote:
> > > On Sat, Sep 7, 2024 at 12:10 AM Laurent Pinchart wrote:
> > > > On Fri, Sep 06, 2024 at 06:39:46PM +0100, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > The RZ/G2L CRU needs to configure the ICnMC.VCSEL bits to specify which
> > > > > virtual channel should be processed from the four available VCs. To
> > > > > retrieve this information from the connected subdevice, the
> > > > > .get_frame_desc() function is called.
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > > ---
> > > > >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 29 +++++++++++++++++++
> > > > >  1 file changed, 29 insertions(+)
> > > > >
> > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > > index bbf4674f888d..6101a070e785 100644
> > > > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
> > > > > @@ -433,12 +433,41 @@ void rzg2l_cru_stop_image_processing(struct rzg2l_cru_dev *cru)
> > > > >       spin_unlock_irqrestore(&cru->qlock, flags);
> > > > >  }
> > > > >
> > > > > +static int rzg2l_cru_get_virtual_channel(struct rzg2l_cru_dev *cru)
> > > > > +{
> > > > > +     struct v4l2_mbus_frame_desc fd = { };
> > > > > +     struct media_pad *pad;
> > > > > +     int ret;
> > > > > +
> > > > > +     pad = media_pad_remote_pad_unique(&cru->ip.pads[1]);
> > > >
> > > > It would be nice to use RZG2L_CRU_IP_SOURCE here instead of hardcoding
> > > > the pad number. That would require moving rzg2l_csi2_pads to the shared
> > > > header. You can do that on top.
> > >
> > > With the below comment we dont need to move rzg2l_csi2_pads into the
> > > shared header.
> > >
> > > > An now that I've said that, is it really the source pad you need here ?
> > >
> > > Ouch you are right.
> > >
> > > > > +     if (IS_ERR(pad))
> > > > > +             return PTR_ERR(pad);
> > > >
> > > > Can this happen, or would the pipeline fail to validate ? I think you
> > > > can set the MUST_CONNECT flag on the sink pad, then you'll have a
> > > > guarantee something will be connected.
> > >
> > > After adding the MUST_CONNECT flag, I wouldn't need the  above
> > > media_pad_remote_pad_unique()...
> > >
> > > > > +
> > > > > +     ret = v4l2_subdev_call(cru->ip.remote, pad, get_frame_desc,
> > > > > +                            pad->index, &fd);
> > >
> > > ... and here I can use '0' instead
> >
> > Can you ? You need to call the operation on the pad of the connected
> > entity that is connected to tbe sink pad of the IP entity. That would be
> > the source pad of the CSI-2 RX in this case, but it can't be hardcoded
> > as it could also bethe source pad of a parallel sensor (once support for
> > that will be implemented). I think you therefore need to keep the
> > media_pad_remote_pad_unique() call.
>
> media pipeline for RZ/G2L is [0].
> 
> Calling media_pad_remote_pad_unique() with sink pad of IP entity will
> return source pad of CSI-Rx, where the pad index will be '1', passing
> pad index '1' to .get_frame_desc to CSI subdev and then OV5645 subdev
> would return -EINVAL.

Why does it return -EINVAL ?

> I need to update patch [1] instead of just forwarding the pad index to
> remote subdev I'll need to do the same as done IP subdev ie in CSI
> subdev call media_pad_remote_pad_unique() on sink pad of CSI which
> will return OV5465 source pad, and this will have a pad index of '0'.

Ah, that's why it returns -EINVAL :-)

You're right, the pad number can't be propagated as-is.

> The CSI get_frame_desc() will look something like below:
> 
> static int rzg2l_csi2_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
>                      struct v4l2_mbus_frame_desc *fd)
> {
>     struct rzg2l_csi2 *csi2 = sd_to_csi2(sd);
>     struct media_pad *remote_pad;
> 
>     if (!csi2->remote_source)
>         return -ENODEV;
> 
>     remote_pad = media_pad_remote_pad_unique(&csi2->pads[RZG2L_CSI2_SINK]);
>     if (IS_ERR(remote_pad)) {
>         dev_err(csi2->dev, "can't get source pad of %s (%ld)\n",
>             csi2->remote_source->name, PTR_ERR(remote_pad));
>         return PTR_ERR(remote_pad);
>     }
>     return v4l2_subdev_call(csi2->remote_source, pad, get_frame_desc,
> remote_pad->index, fd);
> }
> 
> For the parallel input case I plan to implement something similar to
> R-Car vin with bool flag 'is_csi' where we skip calling
> 'rzg2l_cru_get_virtual_channel'.
> 
> [0] https://postimg.cc/rz0vSMLC
> [1] https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240906173947.282402-2-prabhakar.mahadev-lad.rj@bp.renesas.com/

-- 
Regards,

Laurent Pinchart