[PATCH v2 00/11] media: renesas: rcar-csi2: Use the subdev active state

[PATCH v2 00/11] media: renesas: rcar-csi2: Use the subdev active state

[Jacopo Mondi]

v2->v1:
  - Remove "media: adv748x-csi2: Initialize subdev format"
  - Add "media: adv748x-afe: Use 1X16 media bus code"
  - Tested with CVBS
  - address comments from Laurent and Niklas

As a follow-up to the recently sent
"media: renesas: rcar-csi2: Support multiple streams" series, this smaller
version collects some fixes and implement usage of the subdev active state
to simplify the R-Car CSI-2, ADV748x and MAX9286 drivers implementations.

Tested with GMSL on Eagle V3M
Tested with HDMI on Salvator-X
Tested with CVBS on Salvator-X

Jacopo Mondi (11):
  media: rcar-vin: Fix YUYV8_1X16 handling for CSI-2
  media: rcar-csi2: Disable runtime_pm in probe error
  media: rcar-csi2: Cleanup subdevice in remove()
  media: rcar-csi2: Use the subdev active state
  media: adv748x-csi2: Implement enum_mbus_codes
  media: adv748x-csi2: Validate the image format
  media: adv748x-csi2: Use the subdev active state
  media: adv748x-afe: Use 1X16 media bus code
  media: max9286: Fix enum_mbus_code
  media: max9286: Use the subdev active state
  media: max9286: Use frame interval from subdev state

 drivers/media/i2c/adv748x/adv748x-afe.c       |   4 +-
 drivers/media/i2c/adv748x/adv748x-csi2.c      | 140 ++++++++-----
 drivers/media/i2c/adv748x/adv748x.h           |   1 -
 drivers/media/i2c/max9286.c                   | 189 +++++++-----------
 drivers/media/platform/renesas/rcar-csi2.c    | 141 +++++++------
 .../platform/renesas/rcar-vin/rcar-dma.c      |  16 +-
 6 files changed, 256 insertions(+), 235 deletions(-)

--
2.44.0

[PATCH v2 01/11] media: rcar-vin: Fix YUYV8_1X16 handling for CSI-2

[Jacopo Mondi]

The YUYV8_1X16 and UYVY8_1X16 formats are treated as 'ITU-R
BT.601/BT.1358 16-bit YCbCr-422 input' (YUV16 - 0x5) in the R-Car VIN
driver and are thus disallowed when capturing frames from the R-Car
CSI-2 interface according to the hardware manual.

As the 1X16 format variants are meant to be used with serial busses they
have to be treated as 'YCbCr-422 8-bit data input' (0x1) when capturing
from CSI-2, which is a valid setting for CSI-2.

Commit 78b3f9d75a62 ("media: rcar-vin: Add check that input interface
and format are valid") disallowed capturing YUV16 when using the CSI-2
interface. Fix this by using YUV8_BT601 for YCbCr422 when CSI-2 is in
use.

Fixes: 78b3f9d75a62 ("media: rcar-vin: Add check that input interface and format are valid")
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../media/platform/renesas/rcar-vin/rcar-dma.c   | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
index e2c40abc6d3d..21d5b2815e86 100644
--- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
@@ -742,12 +742,22 @@ static int rvin_setup(struct rvin_dev *vin)
 	 */
 	switch (vin->mbus_code) {
 	case MEDIA_BUS_FMT_YUYV8_1X16:
-		/* BT.601/BT.1358 16bit YCbCr422 */
-		vnmc |= VNMC_INF_YUV16;
+		if (vin->is_csi)
+			/* YCbCr422 8-bit */
+			vnmc |= VNMC_INF_YUV8_BT601;
+		else
+			/* BT.601/BT.1358 16bit YCbCr422 */
+			vnmc |= VNMC_INF_YUV16;
 		input_is_yuv = true;
 		break;
 	case MEDIA_BUS_FMT_UYVY8_1X16:
-		vnmc |= VNMC_INF_YUV16 | VNMC_YCAL;
+		if (vin->is_csi)
+			/* YCbCr422 8-bit */
+			vnmc |= VNMC_INF_YUV8_BT601;
+		else
+			/* BT.601/BT.1358 16bit YCbCr422 */
+			vnmc |= VNMC_INF_YUV16;
+		vnmc |= VNMC_YCAL;
 		input_is_yuv = true;
 		break;
 	case MEDIA_BUS_FMT_UYVY8_2X8:
-- 
2.44.0

[PATCH v2 02/11] media: rcar-csi2: Disable runtime_pm in probe error

[Jacopo Mondi]

Disable pm_runtime in the probe() function error path.

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/rcar-csi2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 582d5e35db0e..249e58c77176 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -1914,12 +1914,14 @@ static int rcsi2_probe(struct platform_device *pdev)
 
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret < 0)
-		goto error_async;
+		goto error_pm_runtime;
 
 	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
 
 	return 0;
 
+error_pm_runtime:
+	pm_runtime_disable(&pdev->dev);
 error_async:
 	v4l2_async_nf_unregister(&priv->notifier);
 	v4l2_async_nf_cleanup(&priv->notifier);
-- 
2.44.0

[PATCH v2 03/11] media: rcar-csi2: Cleanup subdevice in remove()

[Jacopo Mondi]

Cleanup the V4L2 subdevice in the driver's remove function to
ensure its async connection are freed, and guarantee in future that
the subdev active state is cleaned up.

Fixes: 769afd212b16 ("media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver")
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/rcar-csi2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 249e58c77176..2d464e43a5be 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -1938,6 +1938,7 @@ static void rcsi2_remove(struct platform_device *pdev)
 	v4l2_async_nf_unregister(&priv->notifier);
 	v4l2_async_nf_cleanup(&priv->notifier);
 	v4l2_async_unregister_subdev(&priv->subdev);
+	v4l2_subdev_cleanup(&priv->subdev);
 
 	pm_runtime_disable(&pdev->dev);
 
-- 
2.44.0

[PATCH v2 04/11] media: rcar-csi2: Use the subdev active state

[Jacopo Mondi]

Create the subdevice state with v4l2_subdev_init_finalize() and
implement the init_state() operation to guarantee the state is initialized.

Store the current image format in the subdev active state and remove it
from the driver private structure.

To guarantee the same image format is applied to all 4 source pads,
propagate the format from the sink pad to the sources, disallowing
changing format on a source pad.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/platform/renesas/rcar-csi2.c | 138 +++++++++++----------
 1 file changed, 75 insertions(+), 63 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 2d464e43a5be..7a9c4007386f 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -587,7 +587,8 @@ enum rcar_csi2_pads {
 struct rcar_csi2_info {
 	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
 	int (*phy_post_init)(struct rcar_csi2 *priv);
-	int (*start_receiver)(struct rcar_csi2 *priv);
+	int (*start_receiver)(struct rcar_csi2 *priv,
+			      struct v4l2_subdev_state *state);
 	void (*enter_standby)(struct rcar_csi2 *priv);
 	const struct rcsi2_mbps_reg *hsfreqrange;
 	unsigned int csi0clkfreqrange;
@@ -613,8 +614,6 @@ struct rcar_csi2 {
 
 	int channel_vc[4];
 
-	struct mutex lock; /* Protects mf and stream_count. */
-	struct v4l2_mbus_framefmt mf;
 	int stream_count;
 
 	bool cphy;
@@ -808,20 +807,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
 	return 0;
 }
 
-static int rcsi2_start_receiver_gen3(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;
 	unsigned int lanes;
 	unsigned int i;
 	int mbps, ret;
 
+	/* Use the format on the sink pad to compute the receiver config. */
+	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
+
 	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-		priv->mf.width, priv->mf.height,
-		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
+		fmt->width, fmt->height,
+		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
 
 	/* Code is validated in set_fmt. */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	format = rcsi2_code_to_fmt(fmt->code);
 	if (!format)
 		return -EINVAL;
 
@@ -849,11 +853,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
 			vcdt2 |= vcdt_part << ((i % 2) * 16);
 	}
 
-	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
+	if (fmt->field == V4L2_FIELD_ALTERNATE) {
 		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
 			| FLD_FLD_EN;
 
-		if (priv->mf.height == 240)
+		if (fmt->height == 240)
 			fld |= FLD_FLD_NUM(0);
 		else
 			fld |= FLD_FLD_NUM(1);
@@ -1049,15 +1053,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
 	return 0;
 }
 
-static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
+static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
+				    struct v4l2_subdev_state *state)
 {
 	const struct rcar_csi2_format *format;
+	const struct v4l2_mbus_framefmt *fmt;
 	unsigned int lanes;
 	int msps;
 	int ret;
 
-	/* Calculate parameters */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	/* Use the format on the sink pad to compute the receiver config. */
+	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
+	format = rcsi2_code_to_fmt(fmt->code);
 	if (!format)
 		return -EINVAL;
 
@@ -1114,7 +1121,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
 	return 0;
 }
 
-static int rcsi2_start(struct rcar_csi2 *priv)
+static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
 {
 	int ret;
 
@@ -1122,7 +1129,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
 	if (ret < 0)
 		return ret;
 
-	ret = priv->info->start_receiver(priv);
+	ret = priv->info->start_receiver(priv, state);
 	if (ret) {
 		rcsi2_enter_standby(priv);
 		return ret;
@@ -1146,17 +1153,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
 static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
-	mutex_lock(&priv->lock);
+	if (!priv->remote)
+		return -ENODEV;
 
-	if (!priv->remote) {
-		ret = -ENODEV;
-		goto out;
-	}
+	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
 
 	if (enable && priv->stream_count == 0) {
-		ret = rcsi2_start(priv);
+		ret = rcsi2_start(priv, state);
 		if (ret)
 			goto out;
 	} else if (!enable && priv->stream_count == 1) {
@@ -1165,49 +1171,26 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
 
 	priv->stream_count += enable ? 1 : -1;
 out:
-	mutex_unlock(&priv->lock);
+	v4l2_subdev_unlock_state(state);
 
 	return ret;
 }
 
 static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_format *format)
 {
-	struct rcar_csi2 *priv = sd_to_csi2(sd);
-	struct v4l2_mbus_framefmt *framefmt;
-
-	mutex_lock(&priv->lock);
+	if (format->pad > RCAR_CSI2_SINK)
+		return v4l2_subdev_get_fmt(sd, state, format);
 
 	if (!rcsi2_code_to_fmt(format->format.code))
 		format->format.code = rcar_csi2_formats[0].code;
 
-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		priv->mf = format->format;
-	} else {
-		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
-		*framefmt = format->format;
-	}
-
-	mutex_unlock(&priv->lock);
-
-	return 0;
-}
-
-static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
-				struct v4l2_subdev_format *format)
-{
-	struct rcar_csi2 *priv = sd_to_csi2(sd);
-
-	mutex_lock(&priv->lock);
-
-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		format->format = priv->mf;
-	else
-		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
+	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
 
-	mutex_unlock(&priv->lock);
+	/* Propagate the format to the source pads. */
+	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
+		*v4l2_subdev_state_get_format(state, i) = format->format;
 
 	return 0;
 }
@@ -1218,7 +1201,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
 
 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.set_fmt = rcsi2_set_pad_format,
-	.get_fmt = rcsi2_get_pad_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 };
 
 static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
@@ -1226,6 +1209,30 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
 	.pad	= &rcar_csi2_pad_ops,
 };
 
+static int rcsi2_init_state(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *state)
+{
+	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
+		.width		= 1920,
+		.height		= 1080,
+		.code		= MEDIA_BUS_FMT_RGB888_1X24,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
+		.field		= V4L2_FIELD_NONE,
+		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+		.quantization	= V4L2_QUANTIZATION_DEFAULT,
+		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+	};
+
+	for (unsigned int i = RCAR_CSI2_SINK; i < NR_OF_RCAR_CSI2_PAD; i++)
+		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
+	.init_state = rcsi2_init_state,
+};
+
 static irqreturn_t rcsi2_irq(int irq, void *data)
 {
 	struct rcar_csi2 *priv = data;
@@ -1251,14 +1258,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
 
 static irqreturn_t rcsi2_irq_thread(int irq, void *data)
 {
+	struct v4l2_subdev_state *state;
 	struct rcar_csi2 *priv = data;
 
-	mutex_lock(&priv->lock);
+	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
+
 	rcsi2_stop(priv);
 	usleep_range(1000, 2000);
-	if (rcsi2_start(priv))
+	if (rcsi2_start(priv, state))
 		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
-	mutex_unlock(&priv->lock);
+
+	v4l2_subdev_unlock_state(state);
 
 	return IRQ_HANDLED;
 }
@@ -1870,23 +1880,23 @@ static int rcsi2_probe(struct platform_device *pdev)
 
 	priv->dev = &pdev->dev;
 
-	mutex_init(&priv->lock);
 	priv->stream_count = 0;
 
 	ret = rcsi2_probe_resources(priv, pdev);
 	if (ret) {
 		dev_err(priv->dev, "Failed to get resources\n");
-		goto error_mutex;
+		return ret;
 	}
 
 	platform_set_drvdata(pdev, priv);
 
 	ret = rcsi2_parse_dt(priv);
 	if (ret)
-		goto error_mutex;
+		return ret;
 
 	priv->subdev.owner = THIS_MODULE;
 	priv->subdev.dev = &pdev->dev;
+	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
 	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
 	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
 	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
@@ -1912,21 +1922,25 @@ static int rcsi2_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(&pdev->dev);
 
+	ret = v4l2_subdev_init_finalize(&priv->subdev);
+	if (ret)
+		goto error_pm_runtime;
+
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret < 0)
-		goto error_pm_runtime;
+		goto error_subdev;
 
 	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
 
 	return 0;
 
+error_subdev:
+	v4l2_subdev_cleanup(&priv->subdev);
 error_pm_runtime:
 	pm_runtime_disable(&pdev->dev);
 error_async:
 	v4l2_async_nf_unregister(&priv->notifier);
 	v4l2_async_nf_cleanup(&priv->notifier);
-error_mutex:
-	mutex_destroy(&priv->lock);
 
 	return ret;
 }
@@ -1941,8 +1955,6 @@ static void rcsi2_remove(struct platform_device *pdev)
 	v4l2_subdev_cleanup(&priv->subdev);
 
 	pm_runtime_disable(&pdev->dev);
-
-	mutex_destroy(&priv->lock);
 }
 
 static struct platform_driver rcar_csi2_pdrv = {
-- 
2.44.0

Re: [PATCH v2 04/11] media: rcar-csi2: Use the subdev active state

[Niklas Söderlund]

Hi Jacopo,

I really like this patch, the active state is a nice idea. It works as 
expected on Gen3, but produces a NULL pointer dereference when booted on 
Gen4.

[    0.547783] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
[    0.548942] Mem abort info:
[    0.549328]   ESR = 0x0000000096000044
[    0.549813]   EC = 0x25: DABT (current EL), IL = 32 bits
[    0.550495]   SET = 0, FnV = 0
[    0.550889]   EA = 0, S1PTW = 0
[    0.551294]   FSC = 0x04: level 0 translation fault
[    0.551919] Data abort info:
[    0.552291]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
[    0.552993]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
[    0.553660]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    0.554342] [0000000000000010] user address but active_mm is swapper
[    0.555178] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP
[    0.555976] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.9.0-rc2-arm64-renesas-00321-gfba919e351c6 #123
[    0.557150] Hardware name: Renesas White Hawk Single board based on r8a779g2 (DT)
[    0.558093] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    0.558971] pc : rcsi2_init_state+0x40/0x74
[    0.559513] lr : rcsi2_init_state+0x40/0x74
[    0.560043] sp : ffff80008258ba60
[    0.560461] x29: ffff80008258ba60 x28: 0000000000000000 x27: ffff0006bf007538
[    0.561365] x26: ffff000440cd54a0 x25: ffff000440cd5758 x24: ffff8000812dff08
[    0.562265] x23: 0000043800000780 x22: 000000010000100a x21: 0000000000000008
[    0.563166] x20: ffff000440924e00 x19: 0000000000000002 x18: ffffffffffffffff
[    0.564066] x17: ffff00044244bc00 x16: 0000000000000100 x15: 0000000000000028
[    0.564966] x14: 0000000000002104 x13: 0000000000000001 x12: 0000000000000001
[    0.565866] x11: ffff800081af8390 x10: ffff800081a41808 x9 : ffff800082157000
[    0.566766] x8 : 0000000000000004 x7 : ffff800081a41340 x6 : ffff80008210e110
[    0.567667] x5 : ffff0004409d58f8 x4 : ffff800081ae3808 x3 : 0000000000000000
[    0.568567] x2 : 0000000000000000 x1 : 0000000000000002 x0 : 0000000000000000
[    0.569467] Call trace:
[    0.569778]  rcsi2_init_state+0x40/0x74
[    0.570264]  __v4l2_subdev_state_alloc+0x8c/0x110
[    0.570862]  __v4l2_subdev_init_finalize+0x14/0x34
[    0.571467]  rcsi2_probe+0x370/0x508
[    0.571921]  platform_probe+0x64/0xbc
[    0.572389]  really_probe+0xb8/0x294
[    0.572843]  __driver_probe_device+0x74/0x124
[    0.573394]  driver_probe_device+0x3c/0x15c
[    0.573923]  __driver_attach+0xec/0x1c4
[    0.574408]  bus_for_each_dev+0x74/0xcc
[    0.574894]  driver_attach+0x20/0x28
[    0.575347]  bus_add_driver+0xe0/0x1e0
[    0.575821]  driver_register+0x58/0x114
[    0.576307]  __platform_driver_register+0x24/0x2c
[    0.576902]  rcar_csi2_pdrv_init+0x18/0x20
[    0.577425]  do_one_initcall+0x80/0x2e8
[    0.577912]  kernel_init_freeable+0x238/0x34c
[    0.578462]  kernel_init+0x20/0x1c8
[    0.578907]  ret_from_fork+0x10/0x20
[    0.579366] Code: 2a1303e1 aa1403e0 52800002 97ff22d0 (a9017c1f) 
[    0.580136] ---[ end trace 0000000000000000 ]---
[    0.580736] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    0.581705] SMP: stopping secondary CPUs
[    0.582205] Kernel Offset: disabled
[    0.582645] CPU features: 0x0,00000003,80100528,4200700b
[    0.583316] Memory Limit: none
[    0.583703] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---

On 2024-05-06 18:49:32 +0200, Jacopo Mondi wrote:
> Create the subdevice state with v4l2_subdev_init_finalize() and
> implement the init_state() operation to guarantee the state is initialized.
> 
> Store the current image format in the subdev active state and remove it
> from the driver private structure.
> 
> To guarantee the same image format is applied to all 4 source pads,
> propagate the format from the sink pad to the sources, disallowing
> changing format on a source pad.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/renesas/rcar-csi2.c | 138 +++++++++++----------
>  1 file changed, 75 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index 2d464e43a5be..7a9c4007386f 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -587,7 +587,8 @@ enum rcar_csi2_pads {
>  struct rcar_csi2_info {
>  	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
>  	int (*phy_post_init)(struct rcar_csi2 *priv);
> -	int (*start_receiver)(struct rcar_csi2 *priv);
> +	int (*start_receiver)(struct rcar_csi2 *priv,
> +			      struct v4l2_subdev_state *state);
>  	void (*enter_standby)(struct rcar_csi2 *priv);
>  	const struct rcsi2_mbps_reg *hsfreqrange;
>  	unsigned int csi0clkfreqrange;
> @@ -613,8 +614,6 @@ struct rcar_csi2 {
>  
>  	int channel_vc[4];
>  
> -	struct mutex lock; /* Protects mf and stream_count. */
> -	struct v4l2_mbus_framefmt mf;
>  	int stream_count;
>  
>  	bool cphy;
> @@ -808,20 +807,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
>  	return 0;
>  }
>  
> -static int rcsi2_start_receiver_gen3(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;
>  	unsigned int lanes;
>  	unsigned int i;
>  	int mbps, ret;
>  
> +	/* Use the format on the sink pad to compute the receiver config. */
> +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> +
>  	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> -		priv->mf.width, priv->mf.height,
> -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> +		fmt->width, fmt->height,
> +		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>  
>  	/* Code is validated in set_fmt. */
> -	format = rcsi2_code_to_fmt(priv->mf.code);
> +	format = rcsi2_code_to_fmt(fmt->code);
>  	if (!format)
>  		return -EINVAL;
>  
> @@ -849,11 +853,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
>  
> -	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> +	if (fmt->field == V4L2_FIELD_ALTERNATE) {
>  		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
>  			| FLD_FLD_EN;
>  
> -		if (priv->mf.height == 240)
> +		if (fmt->height == 240)
>  			fld |= FLD_FLD_NUM(0);
>  		else
>  			fld |= FLD_FLD_NUM(1);
> @@ -1049,15 +1053,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
>  	return 0;
>  }
>  
> -static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> +static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
> +				    struct v4l2_subdev_state *state)
>  {
>  	const struct rcar_csi2_format *format;
> +	const struct v4l2_mbus_framefmt *fmt;
>  	unsigned int lanes;
>  	int msps;
>  	int ret;
>  
> -	/* Calculate parameters */
> -	format = rcsi2_code_to_fmt(priv->mf.code);
> +	/* Use the format on the sink pad to compute the receiver config. */
> +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> +	format = rcsi2_code_to_fmt(fmt->code);
>  	if (!format)
>  		return -EINVAL;
>  
> @@ -1114,7 +1121,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
>  	return 0;
>  }
>  
> -static int rcsi2_start(struct rcar_csi2 *priv)
> +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
>  {
>  	int ret;
>  
> @@ -1122,7 +1129,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = priv->info->start_receiver(priv);
> +	ret = priv->info->start_receiver(priv, state);
>  	if (ret) {
>  		rcsi2_enter_standby(priv);
>  		return ret;
> @@ -1146,17 +1153,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
>  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
>  
> -	mutex_lock(&priv->lock);
> +	if (!priv->remote)
> +		return -ENODEV;
>  
> -	if (!priv->remote) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
>  
>  	if (enable && priv->stream_count == 0) {
> -		ret = rcsi2_start(priv);
> +		ret = rcsi2_start(priv, state);
>  		if (ret)
>  			goto out;
>  	} else if (!enable && priv->stream_count == 1) {
> @@ -1165,49 +1171,26 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	priv->stream_count += enable ? 1 : -1;
>  out:
> -	mutex_unlock(&priv->lock);
> +	v4l2_subdev_unlock_state(state);
>  
>  	return ret;
>  }
>  
>  static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_state *state,
>  				struct v4l2_subdev_format *format)
>  {
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> -	struct v4l2_mbus_framefmt *framefmt;
> -
> -	mutex_lock(&priv->lock);
> +	if (format->pad > RCAR_CSI2_SINK)
> +		return v4l2_subdev_get_fmt(sd, state, format);
>  
>  	if (!rcsi2_code_to_fmt(format->format.code))
>  		format->format.code = rcar_csi2_formats[0].code;
>  
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		priv->mf = format->format;
> -	} else {
> -		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
> -		*framefmt = format->format;
> -	}
> -
> -	mutex_unlock(&priv->lock);
> -
> -	return 0;
> -}
> -
> -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state,
> -				struct v4l2_subdev_format *format)
> -{
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> -
> -	mutex_lock(&priv->lock);
> -
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		format->format = priv->mf;
> -	else
> -		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
> +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
>  
> -	mutex_unlock(&priv->lock);
> +	/* Propagate the format to the source pads. */
> +	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> +		*v4l2_subdev_state_get_format(state, i) = format->format;
>  
>  	return 0;
>  }
> @@ -1218,7 +1201,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
>  
>  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
>  	.set_fmt = rcsi2_set_pad_format,
> -	.get_fmt = rcsi2_get_pad_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  };
>  
>  static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> @@ -1226,6 +1209,30 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
>  	.pad	= &rcar_csi2_pad_ops,
>  };
>  
> +static int rcsi2_init_state(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state)
> +{
> +	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
> +		.width		= 1920,
> +		.height		= 1080,
> +		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.field		= V4L2_FIELD_NONE,
> +		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +		.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +	};
> +
> +	for (unsigned int i = RCAR_CSI2_SINK; i < NR_OF_RCAR_CSI2_PAD; i++)
> +		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
> +	.init_state = rcsi2_init_state,
> +};
> +
>  static irqreturn_t rcsi2_irq(int irq, void *data)
>  {
>  	struct rcar_csi2 *priv = data;
> @@ -1251,14 +1258,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
>  
>  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
>  {
> +	struct v4l2_subdev_state *state;
>  	struct rcar_csi2 *priv = data;
>  
> -	mutex_lock(&priv->lock);
> +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> +
>  	rcsi2_stop(priv);
>  	usleep_range(1000, 2000);
> -	if (rcsi2_start(priv))
> +	if (rcsi2_start(priv, state))
>  		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> -	mutex_unlock(&priv->lock);
> +
> +	v4l2_subdev_unlock_state(state);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -1870,23 +1880,23 @@ static int rcsi2_probe(struct platform_device *pdev)
>  
>  	priv->dev = &pdev->dev;
>  
> -	mutex_init(&priv->lock);
>  	priv->stream_count = 0;
>  
>  	ret = rcsi2_probe_resources(priv, pdev);
>  	if (ret) {
>  		dev_err(priv->dev, "Failed to get resources\n");
> -		goto error_mutex;
> +		return ret;
>  	}
>  
>  	platform_set_drvdata(pdev, priv);
>  
>  	ret = rcsi2_parse_dt(priv);
>  	if (ret)
> -		goto error_mutex;
> +		return ret;
>  
>  	priv->subdev.owner = THIS_MODULE;
>  	priv->subdev.dev = &pdev->dev;
> +	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
>  	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
>  	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
>  	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
> @@ -1912,21 +1922,25 @@ static int rcsi2_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(&pdev->dev);
>  
> +	ret = v4l2_subdev_init_finalize(&priv->subdev);
> +	if (ret)
> +		goto error_pm_runtime;
> +
>  	ret = v4l2_async_register_subdev(&priv->subdev);
>  	if (ret < 0)
> -		goto error_pm_runtime;
> +		goto error_subdev;
>  
>  	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
>  
>  	return 0;
>  
> +error_subdev:
> +	v4l2_subdev_cleanup(&priv->subdev);
>  error_pm_runtime:
>  	pm_runtime_disable(&pdev->dev);
>  error_async:
>  	v4l2_async_nf_unregister(&priv->notifier);
>  	v4l2_async_nf_cleanup(&priv->notifier);
> -error_mutex:
> -	mutex_destroy(&priv->lock);
>  
>  	return ret;
>  }
> @@ -1941,8 +1955,6 @@ static void rcsi2_remove(struct platform_device *pdev)
>  	v4l2_subdev_cleanup(&priv->subdev);
>  
>  	pm_runtime_disable(&pdev->dev);
> -
> -	mutex_destroy(&priv->lock);
>  }
>  
>  static struct platform_driver rcar_csi2_pdrv = {
> -- 
> 2.44.0
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2 04/11] media: rcar-csi2: Use the subdev active state

[Jacopo Mondi]

Hi Niklas

On Wed, May 08, 2024 at 06:18:53PM GMT, Niklas Söderlund wrote:
> Hi Jacopo,
>
> I really like this patch, the active state is a nice idea. It works as
> expected on Gen3, but produces a NULL pointer dereference when booted on
> Gen4.

Thanks for testing on Gen4, I don't have an hw setup to test with

>
> [    0.547783] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010

I read in probe()

	num_pads = priv->info->use_isp ? 2 : NR_OF_RCAR_CSI2_PAD;
	ret = media_entity_pads_init(&priv->subdev.entity, num_pads,
				     priv->pads);

I presume in gen4 "use_isp = true" and in init_state

	for (unsigned int i = RCAR_CSI2_SINK; i < NR_OF_RCAR_CSI2_PAD; i++)
		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;

NR_OF_RCAR_CSI2_PAD should be replaced with the "num_pads" as well.

> [    0.548942] Mem abort info:
> [    0.549328]   ESR = 0x0000000096000044
> [    0.549813]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    0.550495]   SET = 0, FnV = 0
> [    0.550889]   EA = 0, S1PTW = 0
> [    0.551294]   FSC = 0x04: level 0 translation fault
> [    0.551919] Data abort info:
> [    0.552291]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
> [    0.552993]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> [    0.553660]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [    0.554342] [0000000000000010] user address but active_mm is swapper
> [    0.555178] Internal error: Oops: 0000000096000044 [#1] PREEMPT SMP
> [    0.555976] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.9.0-rc2-arm64-renesas-00321-gfba919e351c6 #123
> [    0.557150] Hardware name: Renesas White Hawk Single board based on r8a779g2 (DT)
> [    0.558093] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    0.558971] pc : rcsi2_init_state+0x40/0x74
> [    0.559513] lr : rcsi2_init_state+0x40/0x74
> [    0.560043] sp : ffff80008258ba60
> [    0.560461] x29: ffff80008258ba60 x28: 0000000000000000 x27: ffff0006bf007538
> [    0.561365] x26: ffff000440cd54a0 x25: ffff000440cd5758 x24: ffff8000812dff08
> [    0.562265] x23: 0000043800000780 x22: 000000010000100a x21: 0000000000000008
> [    0.563166] x20: ffff000440924e00 x19: 0000000000000002 x18: ffffffffffffffff
> [    0.564066] x17: ffff00044244bc00 x16: 0000000000000100 x15: 0000000000000028
> [    0.564966] x14: 0000000000002104 x13: 0000000000000001 x12: 0000000000000001
> [    0.565866] x11: ffff800081af8390 x10: ffff800081a41808 x9 : ffff800082157000
> [    0.566766] x8 : 0000000000000004 x7 : ffff800081a41340 x6 : ffff80008210e110
> [    0.567667] x5 : ffff0004409d58f8 x4 : ffff800081ae3808 x3 : 0000000000000000
> [    0.568567] x2 : 0000000000000000 x1 : 0000000000000002 x0 : 0000000000000000
> [    0.569467] Call trace:
> [    0.569778]  rcsi2_init_state+0x40/0x74
> [    0.570264]  __v4l2_subdev_state_alloc+0x8c/0x110
> [    0.570862]  __v4l2_subdev_init_finalize+0x14/0x34
> [    0.571467]  rcsi2_probe+0x370/0x508
> [    0.571921]  platform_probe+0x64/0xbc
> [    0.572389]  really_probe+0xb8/0x294
> [    0.572843]  __driver_probe_device+0x74/0x124
> [    0.573394]  driver_probe_device+0x3c/0x15c
> [    0.573923]  __driver_attach+0xec/0x1c4
> [    0.574408]  bus_for_each_dev+0x74/0xcc
> [    0.574894]  driver_attach+0x20/0x28
> [    0.575347]  bus_add_driver+0xe0/0x1e0
> [    0.575821]  driver_register+0x58/0x114
> [    0.576307]  __platform_driver_register+0x24/0x2c
> [    0.576902]  rcar_csi2_pdrv_init+0x18/0x20
> [    0.577425]  do_one_initcall+0x80/0x2e8
> [    0.577912]  kernel_init_freeable+0x238/0x34c
> [    0.578462]  kernel_init+0x20/0x1c8
> [    0.578907]  ret_from_fork+0x10/0x20
> [    0.579366] Code: 2a1303e1 aa1403e0 52800002 97ff22d0 (a9017c1f)
> [    0.580136] ---[ end trace 0000000000000000 ]---
> [    0.580736] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    0.581705] SMP: stopping secondary CPUs
> [    0.582205] Kernel Offset: disabled
> [    0.582645] CPU features: 0x0,00000003,80100528,4200700b
> [    0.583316] Memory Limit: none
> [    0.583703] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
>
> On 2024-05-06 18:49:32 +0200, Jacopo Mondi wrote:
> > Create the subdevice state with v4l2_subdev_init_finalize() and
> > implement the init_state() operation to guarantee the state is initialized.
> >
> > Store the current image format in the subdev active state and remove it
> > from the driver private structure.
> >
> > To guarantee the same image format is applied to all 4 source pads,
> > propagate the format from the sink pad to the sources, disallowing
> > changing format on a source pad.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/renesas/rcar-csi2.c | 138 +++++++++++----------
> >  1 file changed, 75 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> > index 2d464e43a5be..7a9c4007386f 100644
> > --- a/drivers/media/platform/renesas/rcar-csi2.c
> > +++ b/drivers/media/platform/renesas/rcar-csi2.c
> > @@ -587,7 +587,8 @@ enum rcar_csi2_pads {
> >  struct rcar_csi2_info {
> >  	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
> >  	int (*phy_post_init)(struct rcar_csi2 *priv);
> > -	int (*start_receiver)(struct rcar_csi2 *priv);
> > +	int (*start_receiver)(struct rcar_csi2 *priv,
> > +			      struct v4l2_subdev_state *state);
> >  	void (*enter_standby)(struct rcar_csi2 *priv);
> >  	const struct rcsi2_mbps_reg *hsfreqrange;
> >  	unsigned int csi0clkfreqrange;
> > @@ -613,8 +614,6 @@ struct rcar_csi2 {
> >
> >  	int channel_vc[4];
> >
> > -	struct mutex lock; /* Protects mf and stream_count. */
> > -	struct v4l2_mbus_framefmt mf;
> >  	int stream_count;
> >
> >  	bool cphy;
> > @@ -808,20 +807,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
> >  	return 0;
> >  }
> >
> > -static int rcsi2_start_receiver_gen3(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;
> >  	unsigned int lanes;
> >  	unsigned int i;
> >  	int mbps, ret;
> >
> > +	/* Use the format on the sink pad to compute the receiver config. */
> > +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> > +
> >  	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > -		priv->mf.width, priv->mf.height,
> > -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +		fmt->width, fmt->height,
> > +		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
> >
> >  	/* Code is validated in set_fmt. */
> > -	format = rcsi2_code_to_fmt(priv->mf.code);
> > +	format = rcsi2_code_to_fmt(fmt->code);
> >  	if (!format)
> >  		return -EINVAL;
> >
> > @@ -849,11 +853,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >
> > -	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> > +	if (fmt->field == V4L2_FIELD_ALTERNATE) {
> >  		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> >  			| FLD_FLD_EN;
> >
> > -		if (priv->mf.height == 240)
> > +		if (fmt->height == 240)
> >  			fld |= FLD_FLD_NUM(0);
> >  		else
> >  			fld |= FLD_FLD_NUM(1);
> > @@ -1049,15 +1053,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
> >  	return 0;
> >  }
> >
> > -static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> > +static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
> > +				    struct v4l2_subdev_state *state)
> >  {
> >  	const struct rcar_csi2_format *format;
> > +	const struct v4l2_mbus_framefmt *fmt;
> >  	unsigned int lanes;
> >  	int msps;
> >  	int ret;
> >
> > -	/* Calculate parameters */
> > -	format = rcsi2_code_to_fmt(priv->mf.code);
> > +	/* Use the format on the sink pad to compute the receiver config. */
> > +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> > +	format = rcsi2_code_to_fmt(fmt->code);
> >  	if (!format)
> >  		return -EINVAL;
> >
> > @@ -1114,7 +1121,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> >  	return 0;
> >  }
> >
> > -static int rcsi2_start(struct rcar_csi2 *priv)
> > +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
> >  {
> >  	int ret;
> >
> > @@ -1122,7 +1129,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	ret = priv->info->start_receiver(priv);
> > +	ret = priv->info->start_receiver(priv, state);
> >  	if (ret) {
> >  		rcsi2_enter_standby(priv);
> >  		return ret;
> > @@ -1146,17 +1153,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
> >  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	struct v4l2_subdev_state *state;
> >  	int ret = 0;
> >
> > -	mutex_lock(&priv->lock);
> > +	if (!priv->remote)
> > +		return -ENODEV;
> >
> > -	if (!priv->remote) {
> > -		ret = -ENODEV;
> > -		goto out;
> > -	}
> > +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> >
> >  	if (enable && priv->stream_count == 0) {
> > -		ret = rcsi2_start(priv);
> > +		ret = rcsi2_start(priv, state);
> >  		if (ret)
> >  			goto out;
> >  	} else if (!enable && priv->stream_count == 1) {
> > @@ -1165,49 +1171,26 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> >
> >  	priv->stream_count += enable ? 1 : -1;
> >  out:
> > -	mutex_unlock(&priv->lock);
> > +	v4l2_subdev_unlock_state(state);
> >
> >  	return ret;
> >  }
> >
> >  static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> > -				struct v4l2_subdev_state *sd_state,
> > +				struct v4l2_subdev_state *state,
> >  				struct v4l2_subdev_format *format)
> >  {
> > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > -	struct v4l2_mbus_framefmt *framefmt;
> > -
> > -	mutex_lock(&priv->lock);
> > +	if (format->pad > RCAR_CSI2_SINK)
> > +		return v4l2_subdev_get_fmt(sd, state, format);
> >
> >  	if (!rcsi2_code_to_fmt(format->format.code))
> >  		format->format.code = rcar_csi2_formats[0].code;
> >
> > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -		priv->mf = format->format;
> > -	} else {
> > -		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
> > -		*framefmt = format->format;
> > -	}
> > -
> > -	mutex_unlock(&priv->lock);
> > -
> > -	return 0;
> > -}
> > -
> > -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> > -				struct v4l2_subdev_state *sd_state,
> > -				struct v4l2_subdev_format *format)
> > -{
> > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > -
> > -	mutex_lock(&priv->lock);
> > -
> > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		format->format = priv->mf;
> > -	else
> > -		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
> > +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
> >
> > -	mutex_unlock(&priv->lock);
> > +	/* Propagate the format to the source pads. */
> > +	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +		*v4l2_subdev_state_get_format(state, i) = format->format;
> >
> >  	return 0;
> >  }
> > @@ -1218,7 +1201,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> >
> >  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> >  	.set_fmt = rcsi2_set_pad_format,
> > -	.get_fmt = rcsi2_get_pad_format,
> > +	.get_fmt = v4l2_subdev_get_fmt,
> >  };
> >
> >  static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> > @@ -1226,6 +1209,30 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> >  	.pad	= &rcar_csi2_pad_ops,
> >  };
> >
> > +static int rcsi2_init_state(struct v4l2_subdev *sd,
> > +			    struct v4l2_subdev_state *state)
> > +{
> > +	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
> > +		.width		= 1920,
> > +		.height		= 1080,
> > +		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> > +		.colorspace	= V4L2_COLORSPACE_SRGB,
> > +		.field		= V4L2_FIELD_NONE,
> > +		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> > +		.quantization	= V4L2_QUANTIZATION_DEFAULT,
> > +		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> > +	};
> > +
> > +	for (unsigned int i = RCAR_CSI2_SINK; i < NR_OF_RCAR_CSI2_PAD; i++)
> > +		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
> > +	.init_state = rcsi2_init_state,
> > +};
> > +
> >  static irqreturn_t rcsi2_irq(int irq, void *data)
> >  {
> >  	struct rcar_csi2 *priv = data;
> > @@ -1251,14 +1258,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
> >
> >  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
> >  {
> > +	struct v4l2_subdev_state *state;
> >  	struct rcar_csi2 *priv = data;
> >
> > -	mutex_lock(&priv->lock);
> > +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> > +
> >  	rcsi2_stop(priv);
> >  	usleep_range(1000, 2000);
> > -	if (rcsi2_start(priv))
> > +	if (rcsi2_start(priv, state))
> >  		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> > -	mutex_unlock(&priv->lock);
> > +
> > +	v4l2_subdev_unlock_state(state);
> >
> >  	return IRQ_HANDLED;
> >  }
> > @@ -1870,23 +1880,23 @@ static int rcsi2_probe(struct platform_device *pdev)
> >
> >  	priv->dev = &pdev->dev;
> >
> > -	mutex_init(&priv->lock);
> >  	priv->stream_count = 0;
> >
> >  	ret = rcsi2_probe_resources(priv, pdev);
> >  	if (ret) {
> >  		dev_err(priv->dev, "Failed to get resources\n");
> > -		goto error_mutex;
> > +		return ret;
> >  	}
> >
> >  	platform_set_drvdata(pdev, priv);
> >
> >  	ret = rcsi2_parse_dt(priv);
> >  	if (ret)
> > -		goto error_mutex;
> > +		return ret;
> >
> >  	priv->subdev.owner = THIS_MODULE;
> >  	priv->subdev.dev = &pdev->dev;
> > +	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
> >  	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
> >  	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
> >  	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
> > @@ -1912,21 +1922,25 @@ static int rcsi2_probe(struct platform_device *pdev)
> >
> >  	pm_runtime_enable(&pdev->dev);
> >
> > +	ret = v4l2_subdev_init_finalize(&priv->subdev);
> > +	if (ret)
> > +		goto error_pm_runtime;
> > +
> >  	ret = v4l2_async_register_subdev(&priv->subdev);
> >  	if (ret < 0)
> > -		goto error_pm_runtime;
> > +		goto error_subdev;
> >
> >  	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> >
> >  	return 0;
> >
> > +error_subdev:
> > +	v4l2_subdev_cleanup(&priv->subdev);
> >  error_pm_runtime:
> >  	pm_runtime_disable(&pdev->dev);
> >  error_async:
> >  	v4l2_async_nf_unregister(&priv->notifier);
> >  	v4l2_async_nf_cleanup(&priv->notifier);
> > -error_mutex:
> > -	mutex_destroy(&priv->lock);
> >
> >  	return ret;
> >  }
> > @@ -1941,8 +1955,6 @@ static void rcsi2_remove(struct platform_device *pdev)
> >  	v4l2_subdev_cleanup(&priv->subdev);
> >
> >  	pm_runtime_disable(&pdev->dev);
> > -
> > -	mutex_destroy(&priv->lock);
> >  }
> >
> >  static struct platform_driver rcar_csi2_pdrv = {
> > --
> > 2.44.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund
>

[PATCH v2.1 04/11] media: rcar-csi2: Use the subdev active state

[Jacopo Mondi]

Create the subdevice state with v4l2_subdev_init_finalize() and
implement the init_state() operation to guarantee the state is initialized.

Store the current image format in the subdev active state and remove it
from the driver private structure.

To guarantee the same image format is applied to all source pads,
propagate the format from the sink pad to the sources, disallowing
changing format on a source pad.

While at it, define an enum member for the number of pads supported by
gen4 and store the number of active pads in the driver's private
structure to handle both gen3 and gen4.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2->v2.1

- Fix gen4 issue reported by Niklas: store the number of pads in the
  driver structure and use it when handling in init_state() and
  set_pad_format() when applying a format to the source pads
---
 drivers/media/platform/renesas/rcar-csi2.c | 153 ++++++++++++---------
 1 file changed, 86 insertions(+), 67 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 2d464e43a5be..8482b155522b 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -579,15 +579,17 @@ enum rcar_csi2_pads {
 	RCAR_CSI2_SINK,
 	RCAR_CSI2_SOURCE_VC0,
 	RCAR_CSI2_SOURCE_VC1,
+	NR_OF_RCAR_CSI2_ISP_PADS = RCAR_CSI2_SOURCE_VC1,
 	RCAR_CSI2_SOURCE_VC2,
 	RCAR_CSI2_SOURCE_VC3,
-	NR_OF_RCAR_CSI2_PAD,
+	NR_OF_RCAR_CSI2_VIN_PADS,
 };

 struct rcar_csi2_info {
 	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
 	int (*phy_post_init)(struct rcar_csi2 *priv);
-	int (*start_receiver)(struct rcar_csi2 *priv);
+	int (*start_receiver)(struct rcar_csi2 *priv,
+			      struct v4l2_subdev_state *state);
 	void (*enter_standby)(struct rcar_csi2 *priv);
 	const struct rcsi2_mbps_reg *hsfreqrange;
 	unsigned int csi0clkfreqrange;
@@ -605,7 +607,8 @@ struct rcar_csi2 {
 	struct reset_control *rstc;

 	struct v4l2_subdev subdev;
-	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
+	struct media_pad pads[NR_OF_RCAR_CSI2_VIN_PADS];
+	unsigned int num_pads;

 	struct v4l2_async_notifier notifier;
 	struct v4l2_subdev *remote;
@@ -613,8 +616,6 @@ struct rcar_csi2 {

 	int channel_vc[4];

-	struct mutex lock; /* Protects mf and stream_count. */
-	struct v4l2_mbus_framefmt mf;
 	int stream_count;

 	bool cphy;
@@ -808,20 +809,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
 	return 0;
 }

-static int rcsi2_start_receiver_gen3(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;
 	unsigned int lanes;
 	unsigned int i;
 	int mbps, ret;

+	/* Use the format on the sink pad to compute the receiver config. */
+	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
+
 	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-		priv->mf.width, priv->mf.height,
-		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
+		fmt->width, fmt->height,
+		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');

 	/* Code is validated in set_fmt. */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	format = rcsi2_code_to_fmt(fmt->code);
 	if (!format)
 		return -EINVAL;

@@ -849,11 +855,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
 			vcdt2 |= vcdt_part << ((i % 2) * 16);
 	}

-	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
+	if (fmt->field == V4L2_FIELD_ALTERNATE) {
 		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
 			| FLD_FLD_EN;

-		if (priv->mf.height == 240)
+		if (fmt->height == 240)
 			fld |= FLD_FLD_NUM(0);
 		else
 			fld |= FLD_FLD_NUM(1);
@@ -1049,15 +1055,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
 	return 0;
 }

-static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
+static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
+				    struct v4l2_subdev_state *state)
 {
 	const struct rcar_csi2_format *format;
+	const struct v4l2_mbus_framefmt *fmt;
 	unsigned int lanes;
 	int msps;
 	int ret;

-	/* Calculate parameters */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	/* Use the format on the sink pad to compute the receiver config. */
+	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
+	format = rcsi2_code_to_fmt(fmt->code);
 	if (!format)
 		return -EINVAL;

@@ -1114,7 +1123,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
 	return 0;
 }

-static int rcsi2_start(struct rcar_csi2 *priv)
+static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
 {
 	int ret;

@@ -1122,7 +1131,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
 	if (ret < 0)
 		return ret;

-	ret = priv->info->start_receiver(priv);
+	ret = priv->info->start_receiver(priv, state);
 	if (ret) {
 		rcsi2_enter_standby(priv);
 		return ret;
@@ -1146,17 +1155,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
 static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	struct v4l2_subdev_state *state;
 	int ret = 0;

-	mutex_lock(&priv->lock);
+	if (!priv->remote)
+		return -ENODEV;

-	if (!priv->remote) {
-		ret = -ENODEV;
-		goto out;
-	}
+	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);

 	if (enable && priv->stream_count == 0) {
-		ret = rcsi2_start(priv);
+		ret = rcsi2_start(priv, state);
 		if (ret)
 			goto out;
 	} else if (!enable && priv->stream_count == 1) {
@@ -1165,49 +1173,28 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)

 	priv->stream_count += enable ? 1 : -1;
 out:
-	mutex_unlock(&priv->lock);
+	v4l2_subdev_unlock_state(state);

 	return ret;
 }

 static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_format *format)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
-	struct v4l2_mbus_framefmt *framefmt;

-	mutex_lock(&priv->lock);
+	if (format->pad > RCAR_CSI2_SINK)
+		return v4l2_subdev_get_fmt(sd, state, format);

 	if (!rcsi2_code_to_fmt(format->format.code))
 		format->format.code = rcar_csi2_formats[0].code;

-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		priv->mf = format->format;
-	} else {
-		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
-		*framefmt = format->format;
-	}
-
-	mutex_unlock(&priv->lock);
-
-	return 0;
-}
-
-static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
-				struct v4l2_subdev_format *format)
-{
-	struct rcar_csi2 *priv = sd_to_csi2(sd);
-
-	mutex_lock(&priv->lock);
+	*v4l2_subdev_state_get_format(state, format->pad) = format->format;

-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		format->format = priv->mf;
-	else
-		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
-
-	mutex_unlock(&priv->lock);
+	/* Propagate the format to the source pads. */
+	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < priv->num_pads; i++)
+		*v4l2_subdev_state_get_format(state, i) = format->format;

 	return 0;
 }
@@ -1218,7 +1205,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {

 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.set_fmt = rcsi2_set_pad_format,
-	.get_fmt = rcsi2_get_pad_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 };

 static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
@@ -1226,6 +1213,32 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
 	.pad	= &rcar_csi2_pad_ops,
 };

+static int rcsi2_init_state(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *state)
+{
+	struct rcar_csi2 *priv = sd_to_csi2(sd);
+
+	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
+		.width		= 1920,
+		.height		= 1080,
+		.code		= MEDIA_BUS_FMT_RGB888_1X24,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
+		.field		= V4L2_FIELD_NONE,
+		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+		.quantization	= V4L2_QUANTIZATION_DEFAULT,
+		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+	};
+
+	for (unsigned int i = RCAR_CSI2_SINK; i < priv->num_pads; i++)
+		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
+	.init_state = rcsi2_init_state,
+};
+
 static irqreturn_t rcsi2_irq(int irq, void *data)
 {
 	struct rcar_csi2 *priv = data;
@@ -1251,14 +1264,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)

 static irqreturn_t rcsi2_irq_thread(int irq, void *data)
 {
+	struct v4l2_subdev_state *state;
 	struct rcar_csi2 *priv = data;

-	mutex_lock(&priv->lock);
+	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
+
 	rcsi2_stop(priv);
 	usleep_range(1000, 2000);
-	if (rcsi2_start(priv))
+	if (rcsi2_start(priv, state))
 		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
-	mutex_unlock(&priv->lock);
+
+	v4l2_subdev_unlock_state(state);

 	return IRQ_HANDLED;
 }
@@ -1851,7 +1867,7 @@ static int rcsi2_probe(struct platform_device *pdev)
 {
 	const struct soc_device_attribute *attr;
 	struct rcar_csi2 *priv;
-	unsigned int i, num_pads;
+	unsigned int i;
 	int ret;

 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -1870,23 +1886,23 @@ static int rcsi2_probe(struct platform_device *pdev)

 	priv->dev = &pdev->dev;

-	mutex_init(&priv->lock);
 	priv->stream_count = 0;

 	ret = rcsi2_probe_resources(priv, pdev);
 	if (ret) {
 		dev_err(priv->dev, "Failed to get resources\n");
-		goto error_mutex;
+		return ret;
 	}

 	platform_set_drvdata(pdev, priv);

 	ret = rcsi2_parse_dt(priv);
 	if (ret)
-		goto error_mutex;
+		return ret;

 	priv->subdev.owner = THIS_MODULE;
 	priv->subdev.dev = &pdev->dev;
+	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
 	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
 	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
 	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
@@ -1896,13 +1912,14 @@ static int rcsi2_probe(struct platform_device *pdev)
 	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	priv->subdev.entity.ops = &rcar_csi2_entity_ops;

-	num_pads = priv->info->use_isp ? 2 : NR_OF_RCAR_CSI2_PAD;
+	priv->num_pads = priv->info->use_isp ? NR_OF_RCAR_CSI2_ISP_PADS
+					     : NR_OF_RCAR_CSI2_VIN_PADS;

 	priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
-	for (i = RCAR_CSI2_SOURCE_VC0; i < num_pads; i++)
+	for (i = RCAR_CSI2_SOURCE_VC0; i < priv->num_pads; i++)
 		priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;

-	ret = media_entity_pads_init(&priv->subdev.entity, num_pads,
+	ret = media_entity_pads_init(&priv->subdev.entity, priv->num_pads,
 				     priv->pads);
 	if (ret)
 		goto error_async;
@@ -1912,21 +1929,25 @@ static int rcsi2_probe(struct platform_device *pdev)

 	pm_runtime_enable(&pdev->dev);

+	ret = v4l2_subdev_init_finalize(&priv->subdev);
+	if (ret)
+		goto error_pm_runtime;
+
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret < 0)
-		goto error_pm_runtime;
+		goto error_subdev;

 	dev_info(priv->dev, "%d lanes found\n", priv->lanes);

 	return 0;

+error_subdev:
+	v4l2_subdev_cleanup(&priv->subdev);
 error_pm_runtime:
 	pm_runtime_disable(&pdev->dev);
 error_async:
 	v4l2_async_nf_unregister(&priv->notifier);
 	v4l2_async_nf_cleanup(&priv->notifier);
-error_mutex:
-	mutex_destroy(&priv->lock);

 	return ret;
 }
@@ -1941,8 +1962,6 @@ static void rcsi2_remove(struct platform_device *pdev)
 	v4l2_subdev_cleanup(&priv->subdev);

 	pm_runtime_disable(&pdev->dev);
-
-	mutex_destroy(&priv->lock);
 }

 static struct platform_driver rcar_csi2_pdrv = {
--
2.44.0

Re: [PATCH v2.1 04/11] media: rcar-csi2: Use the subdev active state

[Niklas Söderlund]

Hi Jacopo,

Thanks for this fix, I will test it once I get some time. One comment 
already.

On 2024-05-09 13:06:51 +0200, Jacopo Mondi wrote:
> Create the subdevice state with v4l2_subdev_init_finalize() and
> implement the init_state() operation to guarantee the state is initialized.
> 
> Store the current image format in the subdev active state and remove it
> from the driver private structure.
> 
> To guarantee the same image format is applied to all source pads,
> propagate the format from the sink pad to the sources, disallowing
> changing format on a source pad.
> 
> While at it, define an enum member for the number of pads supported by
> gen4 and store the number of active pads in the driver's private
> structure to handle both gen3 and gen4.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> v2->v2.1
> 
> - Fix gen4 issue reported by Niklas: store the number of pads in the
>   driver structure and use it when handling in init_state() and
>   set_pad_format() when applying a format to the source pads
> ---
>  drivers/media/platform/renesas/rcar-csi2.c | 153 ++++++++++++---------
>  1 file changed, 86 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> index 2d464e43a5be..8482b155522b 100644
> --- a/drivers/media/platform/renesas/rcar-csi2.c
> +++ b/drivers/media/platform/renesas/rcar-csi2.c
> @@ -579,15 +579,17 @@ enum rcar_csi2_pads {
>  	RCAR_CSI2_SINK,
>  	RCAR_CSI2_SOURCE_VC0,
>  	RCAR_CSI2_SOURCE_VC1,
> +	NR_OF_RCAR_CSI2_ISP_PADS = RCAR_CSI2_SOURCE_VC1,
>  	RCAR_CSI2_SOURCE_VC2,
>  	RCAR_CSI2_SOURCE_VC3,
> -	NR_OF_RCAR_CSI2_PAD,
> +	NR_OF_RCAR_CSI2_VIN_PADS,
>  };

Please don't do this. It's hard to understand the actual values of the 
enum values, and it looks fragile. It's also a hack as the ISP operation 
mode only have the RCAR_CSI2_SINK and RCAR_CSI2_SOURCE_VC0 pads, but 
reading this suggests it also have RCAR_CSI2_SOURCE_VC1.

My preferred solution would be a new helper and not store the number of 
pads in the private data and remove NR_OF_RCAR_CSI2_VIN_PADS. Something 
like,

    static unsigned int rcsi2_num_pads(const struct rcar_csi2 *priv)
    {
	/* Used together with R-Car CSIP, there are one sink and one source pad. */
	if (priv->info->use_isp)
	    return 2;

	/* Used together with R-Car VIN, there are one sink and five source pads. */
	return 5;
    }

> 
>  struct rcar_csi2_info {
>  	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
>  	int (*phy_post_init)(struct rcar_csi2 *priv);
> -	int (*start_receiver)(struct rcar_csi2 *priv);
> +	int (*start_receiver)(struct rcar_csi2 *priv,
> +			      struct v4l2_subdev_state *state);
>  	void (*enter_standby)(struct rcar_csi2 *priv);
>  	const struct rcsi2_mbps_reg *hsfreqrange;
>  	unsigned int csi0clkfreqrange;
> @@ -605,7 +607,8 @@ struct rcar_csi2 {
>  	struct reset_control *rstc;
> 
>  	struct v4l2_subdev subdev;
> -	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> +	struct media_pad pads[NR_OF_RCAR_CSI2_VIN_PADS];
> +	unsigned int num_pads;
> 
>  	struct v4l2_async_notifier notifier;
>  	struct v4l2_subdev *remote;
> @@ -613,8 +616,6 @@ struct rcar_csi2 {
> 
>  	int channel_vc[4];
> 
> -	struct mutex lock; /* Protects mf and stream_count. */
> -	struct v4l2_mbus_framefmt mf;
>  	int stream_count;
> 
>  	bool cphy;
> @@ -808,20 +809,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
>  	return 0;
>  }
> 
> -static int rcsi2_start_receiver_gen3(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;
>  	unsigned int lanes;
>  	unsigned int i;
>  	int mbps, ret;
> 
> +	/* Use the format on the sink pad to compute the receiver config. */
> +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> +
>  	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> -		priv->mf.width, priv->mf.height,
> -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> +		fmt->width, fmt->height,
> +		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
> 
>  	/* Code is validated in set_fmt. */
> -	format = rcsi2_code_to_fmt(priv->mf.code);
> +	format = rcsi2_code_to_fmt(fmt->code);
>  	if (!format)
>  		return -EINVAL;
> 
> @@ -849,11 +855,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
>  			vcdt2 |= vcdt_part << ((i % 2) * 16);
>  	}
> 
> -	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> +	if (fmt->field == V4L2_FIELD_ALTERNATE) {
>  		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
>  			| FLD_FLD_EN;
> 
> -		if (priv->mf.height == 240)
> +		if (fmt->height == 240)
>  			fld |= FLD_FLD_NUM(0);
>  		else
>  			fld |= FLD_FLD_NUM(1);
> @@ -1049,15 +1055,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
>  	return 0;
>  }
> 
> -static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> +static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
> +				    struct v4l2_subdev_state *state)
>  {
>  	const struct rcar_csi2_format *format;
> +	const struct v4l2_mbus_framefmt *fmt;
>  	unsigned int lanes;
>  	int msps;
>  	int ret;
> 
> -	/* Calculate parameters */
> -	format = rcsi2_code_to_fmt(priv->mf.code);
> +	/* Use the format on the sink pad to compute the receiver config. */
> +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> +	format = rcsi2_code_to_fmt(fmt->code);
>  	if (!format)
>  		return -EINVAL;
> 
> @@ -1114,7 +1123,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
>  	return 0;
>  }
> 
> -static int rcsi2_start(struct rcar_csi2 *priv)
> +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
>  {
>  	int ret;
> 
> @@ -1122,7 +1131,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
>  	if (ret < 0)
>  		return ret;
> 
> -	ret = priv->info->start_receiver(priv);
> +	ret = priv->info->start_receiver(priv, state);
>  	if (ret) {
>  		rcsi2_enter_standby(priv);
>  		return ret;
> @@ -1146,17 +1155,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
>  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +	struct v4l2_subdev_state *state;
>  	int ret = 0;
> 
> -	mutex_lock(&priv->lock);
> +	if (!priv->remote)
> +		return -ENODEV;
> 
> -	if (!priv->remote) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> 
>  	if (enable && priv->stream_count == 0) {
> -		ret = rcsi2_start(priv);
> +		ret = rcsi2_start(priv, state);
>  		if (ret)
>  			goto out;
>  	} else if (!enable && priv->stream_count == 1) {
> @@ -1165,49 +1173,28 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> 
>  	priv->stream_count += enable ? 1 : -1;
>  out:
> -	mutex_unlock(&priv->lock);
> +	v4l2_subdev_unlock_state(state);
> 
>  	return ret;
>  }
> 
>  static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state,
> +				struct v4l2_subdev_state *state,
>  				struct v4l2_subdev_format *format)
>  {
>  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> -	struct v4l2_mbus_framefmt *framefmt;
> 
> -	mutex_lock(&priv->lock);
> +	if (format->pad > RCAR_CSI2_SINK)
> +		return v4l2_subdev_get_fmt(sd, state, format);
> 
>  	if (!rcsi2_code_to_fmt(format->format.code))
>  		format->format.code = rcar_csi2_formats[0].code;
> 
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> -		priv->mf = format->format;
> -	} else {
> -		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
> -		*framefmt = format->format;
> -	}
> -
> -	mutex_unlock(&priv->lock);
> -
> -	return 0;
> -}
> -
> -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> -				struct v4l2_subdev_state *sd_state,
> -				struct v4l2_subdev_format *format)
> -{
> -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> -
> -	mutex_lock(&priv->lock);
> +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
> 
> -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		format->format = priv->mf;
> -	else
> -		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
> -
> -	mutex_unlock(&priv->lock);
> +	/* Propagate the format to the source pads. */
> +	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < priv->num_pads; i++)
> +		*v4l2_subdev_state_get_format(state, i) = format->format;
> 
>  	return 0;
>  }
> @@ -1218,7 +1205,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> 
>  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
>  	.set_fmt = rcsi2_set_pad_format,
> -	.get_fmt = rcsi2_get_pad_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  };
> 
>  static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> @@ -1226,6 +1213,32 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
>  	.pad	= &rcar_csi2_pad_ops,
>  };
> 
> +static int rcsi2_init_state(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_state *state)
> +{
> +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> +
> +	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
> +		.width		= 1920,
> +		.height		= 1080,
> +		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.field		= V4L2_FIELD_NONE,
> +		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +		.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +	};
> +
> +	for (unsigned int i = RCAR_CSI2_SINK; i < priv->num_pads; i++)
> +		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
> +	.init_state = rcsi2_init_state,
> +};
> +
>  static irqreturn_t rcsi2_irq(int irq, void *data)
>  {
>  	struct rcar_csi2 *priv = data;
> @@ -1251,14 +1264,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
> 
>  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
>  {
> +	struct v4l2_subdev_state *state;
>  	struct rcar_csi2 *priv = data;
> 
> -	mutex_lock(&priv->lock);
> +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> +
>  	rcsi2_stop(priv);
>  	usleep_range(1000, 2000);
> -	if (rcsi2_start(priv))
> +	if (rcsi2_start(priv, state))
>  		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> -	mutex_unlock(&priv->lock);
> +
> +	v4l2_subdev_unlock_state(state);
> 
>  	return IRQ_HANDLED;
>  }
> @@ -1851,7 +1867,7 @@ static int rcsi2_probe(struct platform_device *pdev)
>  {
>  	const struct soc_device_attribute *attr;
>  	struct rcar_csi2 *priv;
> -	unsigned int i, num_pads;
> +	unsigned int i;
>  	int ret;
> 
>  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> @@ -1870,23 +1886,23 @@ static int rcsi2_probe(struct platform_device *pdev)
> 
>  	priv->dev = &pdev->dev;
> 
> -	mutex_init(&priv->lock);
>  	priv->stream_count = 0;
> 
>  	ret = rcsi2_probe_resources(priv, pdev);
>  	if (ret) {
>  		dev_err(priv->dev, "Failed to get resources\n");
> -		goto error_mutex;
> +		return ret;
>  	}
> 
>  	platform_set_drvdata(pdev, priv);
> 
>  	ret = rcsi2_parse_dt(priv);
>  	if (ret)
> -		goto error_mutex;
> +		return ret;
> 
>  	priv->subdev.owner = THIS_MODULE;
>  	priv->subdev.dev = &pdev->dev;
> +	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
>  	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
>  	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
>  	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
> @@ -1896,13 +1912,14 @@ static int rcsi2_probe(struct platform_device *pdev)
>  	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>  	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
> 
> -	num_pads = priv->info->use_isp ? 2 : NR_OF_RCAR_CSI2_PAD;
> +	priv->num_pads = priv->info->use_isp ? NR_OF_RCAR_CSI2_ISP_PADS
> +					     : NR_OF_RCAR_CSI2_VIN_PADS;
> 
>  	priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> -	for (i = RCAR_CSI2_SOURCE_VC0; i < num_pads; i++)
> +	for (i = RCAR_CSI2_SOURCE_VC0; i < priv->num_pads; i++)
>  		priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> 
> -	ret = media_entity_pads_init(&priv->subdev.entity, num_pads,
> +	ret = media_entity_pads_init(&priv->subdev.entity, priv->num_pads,
>  				     priv->pads);
>  	if (ret)
>  		goto error_async;
> @@ -1912,21 +1929,25 @@ static int rcsi2_probe(struct platform_device *pdev)
> 
>  	pm_runtime_enable(&pdev->dev);
> 
> +	ret = v4l2_subdev_init_finalize(&priv->subdev);
> +	if (ret)
> +		goto error_pm_runtime;
> +
>  	ret = v4l2_async_register_subdev(&priv->subdev);
>  	if (ret < 0)
> -		goto error_pm_runtime;
> +		goto error_subdev;
> 
>  	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> 
>  	return 0;
> 
> +error_subdev:
> +	v4l2_subdev_cleanup(&priv->subdev);
>  error_pm_runtime:
>  	pm_runtime_disable(&pdev->dev);
>  error_async:
>  	v4l2_async_nf_unregister(&priv->notifier);
>  	v4l2_async_nf_cleanup(&priv->notifier);
> -error_mutex:
> -	mutex_destroy(&priv->lock);
> 
>  	return ret;
>  }
> @@ -1941,8 +1962,6 @@ static void rcsi2_remove(struct platform_device *pdev)
>  	v4l2_subdev_cleanup(&priv->subdev);
> 
>  	pm_runtime_disable(&pdev->dev);
> -
> -	mutex_destroy(&priv->lock);
>  }
> 
>  static struct platform_driver rcar_csi2_pdrv = {
> --
> 2.44.0
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH v2.1 04/11] media: rcar-csi2: Use the subdev active state

[Jacopo Mondi]

Hi Niklas

On Thu, May 09, 2024 at 01:24:48PM GMT, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for this fix, I will test it once I get some time. One comment
> already.
>
> On 2024-05-09 13:06:51 +0200, Jacopo Mondi wrote:
> > Create the subdevice state with v4l2_subdev_init_finalize() and
> > implement the init_state() operation to guarantee the state is initialized.
> >
> > Store the current image format in the subdev active state and remove it
> > from the driver private structure.
> >
> > To guarantee the same image format is applied to all source pads,
> > propagate the format from the sink pad to the sources, disallowing
> > changing format on a source pad.
> >
> > While at it, define an enum member for the number of pads supported by
> > gen4 and store the number of active pads in the driver's private
> > structure to handle both gen3 and gen4.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > v2->v2.1
> >
> > - Fix gen4 issue reported by Niklas: store the number of pads in the
> >   driver structure and use it when handling in init_state() and
> >   set_pad_format() when applying a format to the source pads
> > ---
> >  drivers/media/platform/renesas/rcar-csi2.c | 153 ++++++++++++---------
> >  1 file changed, 86 insertions(+), 67 deletions(-)
> >
> > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> > index 2d464e43a5be..8482b155522b 100644
> > --- a/drivers/media/platform/renesas/rcar-csi2.c
> > +++ b/drivers/media/platform/renesas/rcar-csi2.c
> > @@ -579,15 +579,17 @@ enum rcar_csi2_pads {
> >  	RCAR_CSI2_SINK,
> >  	RCAR_CSI2_SOURCE_VC0,
> >  	RCAR_CSI2_SOURCE_VC1,
> > +	NR_OF_RCAR_CSI2_ISP_PADS = RCAR_CSI2_SOURCE_VC1,
> >  	RCAR_CSI2_SOURCE_VC2,
> >  	RCAR_CSI2_SOURCE_VC3,
> > -	NR_OF_RCAR_CSI2_PAD,
> > +	NR_OF_RCAR_CSI2_VIN_PADS,
> >  };
>
> Please don't do this. It's hard to understand the actual values of the
> enum values, and it looks fragile. It's also a hack as the ISP operation
> mode only have the RCAR_CSI2_SINK and RCAR_CSI2_SOURCE_VC0 pads, but
> reading this suggests it also have RCAR_CSI2_SOURCE_VC1.

How does it suggest that ? Isn't the name NR_OF_RCAR_CSI2_ISP_PADS
explicative ?

>
> My preferred solution would be a new helper and not store the number of
> pads in the private data and remove NR_OF_RCAR_CSI2_VIN_PADS. Something
> like,
>
>     static unsigned int rcsi2_num_pads(const struct rcar_csi2 *priv)
>     {
> 	/* Used together with R-Car CSIP, there are one sink and one source pad. */
> 	if (priv->info->use_isp)
> 	    return 2;
>
> 	/* Used together with R-Car VIN, there are one sink and five source pads. */

four souce pads

> 	return 5;
>     }
>

Ok, up to you.

> >
> >  struct rcar_csi2_info {
> >  	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
> >  	int (*phy_post_init)(struct rcar_csi2 *priv);
> > -	int (*start_receiver)(struct rcar_csi2 *priv);
> > +	int (*start_receiver)(struct rcar_csi2 *priv,
> > +			      struct v4l2_subdev_state *state);
> >  	void (*enter_standby)(struct rcar_csi2 *priv);
> >  	const struct rcsi2_mbps_reg *hsfreqrange;
> >  	unsigned int csi0clkfreqrange;
> > @@ -605,7 +607,8 @@ struct rcar_csi2 {
> >  	struct reset_control *rstc;
> >
> >  	struct v4l2_subdev subdev;
> > -	struct media_pad pads[NR_OF_RCAR_CSI2_PAD];
> > +	struct media_pad pads[NR_OF_RCAR_CSI2_VIN_PADS];
> > +	unsigned int num_pads;
> >
> >  	struct v4l2_async_notifier notifier;
> >  	struct v4l2_subdev *remote;
> > @@ -613,8 +616,6 @@ struct rcar_csi2 {
> >
> >  	int channel_vc[4];
> >
> > -	struct mutex lock; /* Protects mf and stream_count. */
> > -	struct v4l2_mbus_framefmt mf;
> >  	int stream_count;
> >
> >  	bool cphy;
> > @@ -808,20 +809,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
> >  	return 0;
> >  }
> >
> > -static int rcsi2_start_receiver_gen3(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;
> >  	unsigned int lanes;
> >  	unsigned int i;
> >  	int mbps, ret;
> >
> > +	/* Use the format on the sink pad to compute the receiver config. */
> > +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> > +
> >  	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > -		priv->mf.width, priv->mf.height,
> > -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > +		fmt->width, fmt->height,
> > +		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
> >
> >  	/* Code is validated in set_fmt. */
> > -	format = rcsi2_code_to_fmt(priv->mf.code);
> > +	format = rcsi2_code_to_fmt(fmt->code);
> >  	if (!format)
> >  		return -EINVAL;
> >
> > @@ -849,11 +855,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
> >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> >  	}
> >
> > -	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> > +	if (fmt->field == V4L2_FIELD_ALTERNATE) {
> >  		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> >  			| FLD_FLD_EN;
> >
> > -		if (priv->mf.height == 240)
> > +		if (fmt->height == 240)
> >  			fld |= FLD_FLD_NUM(0);
> >  		else
> >  			fld |= FLD_FLD_NUM(1);
> > @@ -1049,15 +1055,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
> >  	return 0;
> >  }
> >
> > -static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> > +static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
> > +				    struct v4l2_subdev_state *state)
> >  {
> >  	const struct rcar_csi2_format *format;
> > +	const struct v4l2_mbus_framefmt *fmt;
> >  	unsigned int lanes;
> >  	int msps;
> >  	int ret;
> >
> > -	/* Calculate parameters */
> > -	format = rcsi2_code_to_fmt(priv->mf.code);
> > +	/* Use the format on the sink pad to compute the receiver config. */
> > +	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
> > +	format = rcsi2_code_to_fmt(fmt->code);
> >  	if (!format)
> >  		return -EINVAL;
> >
> > @@ -1114,7 +1123,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> >  	return 0;
> >  }
> >
> > -static int rcsi2_start(struct rcar_csi2 *priv)
> > +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
> >  {
> >  	int ret;
> >
> > @@ -1122,7 +1131,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >  	if (ret < 0)
> >  		return ret;
> >
> > -	ret = priv->info->start_receiver(priv);
> > +	ret = priv->info->start_receiver(priv, state);
> >  	if (ret) {
> >  		rcsi2_enter_standby(priv);
> >  		return ret;
> > @@ -1146,17 +1155,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
> >  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +	struct v4l2_subdev_state *state;
> >  	int ret = 0;
> >
> > -	mutex_lock(&priv->lock);
> > +	if (!priv->remote)
> > +		return -ENODEV;
> >
> > -	if (!priv->remote) {
> > -		ret = -ENODEV;
> > -		goto out;
> > -	}
> > +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> >
> >  	if (enable && priv->stream_count == 0) {
> > -		ret = rcsi2_start(priv);
> > +		ret = rcsi2_start(priv, state);
> >  		if (ret)
> >  			goto out;
> >  	} else if (!enable && priv->stream_count == 1) {
> > @@ -1165,49 +1173,28 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> >
> >  	priv->stream_count += enable ? 1 : -1;
> >  out:
> > -	mutex_unlock(&priv->lock);
> > +	v4l2_subdev_unlock_state(state);
> >
> >  	return ret;
> >  }
> >
> >  static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
> > -				struct v4l2_subdev_state *sd_state,
> > +				struct v4l2_subdev_state *state,
> >  				struct v4l2_subdev_format *format)
> >  {
> >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > -	struct v4l2_mbus_framefmt *framefmt;
> >
> > -	mutex_lock(&priv->lock);
> > +	if (format->pad > RCAR_CSI2_SINK)
> > +		return v4l2_subdev_get_fmt(sd, state, format);
> >
> >  	if (!rcsi2_code_to_fmt(format->format.code))
> >  		format->format.code = rcar_csi2_formats[0].code;
> >
> > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > -		priv->mf = format->format;
> > -	} else {
> > -		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
> > -		*framefmt = format->format;
> > -	}
> > -
> > -	mutex_unlock(&priv->lock);
> > -
> > -	return 0;
> > -}
> > -
> > -static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
> > -				struct v4l2_subdev_state *sd_state,
> > -				struct v4l2_subdev_format *format)
> > -{
> > -	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > -
> > -	mutex_lock(&priv->lock);
> > +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
> >
> > -	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		format->format = priv->mf;
> > -	else
> > -		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
> > -
> > -	mutex_unlock(&priv->lock);
> > +	/* Propagate the format to the source pads. */
> > +	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < priv->num_pads; i++)
> > +		*v4l2_subdev_state_get_format(state, i) = format->format;
> >
> >  	return 0;
> >  }
> > @@ -1218,7 +1205,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {
> >
> >  static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
> >  	.set_fmt = rcsi2_set_pad_format,
> > -	.get_fmt = rcsi2_get_pad_format,
> > +	.get_fmt = v4l2_subdev_get_fmt,
> >  };
> >
> >  static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> > @@ -1226,6 +1213,32 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
> >  	.pad	= &rcar_csi2_pad_ops,
> >  };
> >
> > +static int rcsi2_init_state(struct v4l2_subdev *sd,
> > +			    struct v4l2_subdev_state *state)
> > +{
> > +	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > +	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
> > +		.width		= 1920,
> > +		.height		= 1080,
> > +		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> > +		.colorspace	= V4L2_COLORSPACE_SRGB,
> > +		.field		= V4L2_FIELD_NONE,
> > +		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> > +		.quantization	= V4L2_QUANTIZATION_DEFAULT,
> > +		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> > +	};
> > +
> > +	for (unsigned int i = RCAR_CSI2_SINK; i < priv->num_pads; i++)
> > +		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
> > +	.init_state = rcsi2_init_state,
> > +};
> > +
> >  static irqreturn_t rcsi2_irq(int irq, void *data)
> >  {
> >  	struct rcar_csi2 *priv = data;
> > @@ -1251,14 +1264,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
> >
> >  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
> >  {
> > +	struct v4l2_subdev_state *state;
> >  	struct rcar_csi2 *priv = data;
> >
> > -	mutex_lock(&priv->lock);
> > +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> > +
> >  	rcsi2_stop(priv);
> >  	usleep_range(1000, 2000);
> > -	if (rcsi2_start(priv))
> > +	if (rcsi2_start(priv, state))
> >  		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> > -	mutex_unlock(&priv->lock);
> > +
> > +	v4l2_subdev_unlock_state(state);
> >
> >  	return IRQ_HANDLED;
> >  }
> > @@ -1851,7 +1867,7 @@ static int rcsi2_probe(struct platform_device *pdev)
> >  {
> >  	const struct soc_device_attribute *attr;
> >  	struct rcar_csi2 *priv;
> > -	unsigned int i, num_pads;
> > +	unsigned int i;
> >  	int ret;
> >
> >  	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > @@ -1870,23 +1886,23 @@ static int rcsi2_probe(struct platform_device *pdev)
> >
> >  	priv->dev = &pdev->dev;
> >
> > -	mutex_init(&priv->lock);
> >  	priv->stream_count = 0;
> >
> >  	ret = rcsi2_probe_resources(priv, pdev);
> >  	if (ret) {
> >  		dev_err(priv->dev, "Failed to get resources\n");
> > -		goto error_mutex;
> > +		return ret;
> >  	}
> >
> >  	platform_set_drvdata(pdev, priv);
> >
> >  	ret = rcsi2_parse_dt(priv);
> >  	if (ret)
> > -		goto error_mutex;
> > +		return ret;
> >
> >  	priv->subdev.owner = THIS_MODULE;
> >  	priv->subdev.dev = &pdev->dev;
> > +	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
> >  	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
> >  	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
> >  	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
> > @@ -1896,13 +1912,14 @@ static int rcsi2_probe(struct platform_device *pdev)
> >  	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> >  	priv->subdev.entity.ops = &rcar_csi2_entity_ops;
> >
> > -	num_pads = priv->info->use_isp ? 2 : NR_OF_RCAR_CSI2_PAD;
> > +	priv->num_pads = priv->info->use_isp ? NR_OF_RCAR_CSI2_ISP_PADS
> > +					     : NR_OF_RCAR_CSI2_VIN_PADS;
> >
> >  	priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
> > -	for (i = RCAR_CSI2_SOURCE_VC0; i < num_pads; i++)
> > +	for (i = RCAR_CSI2_SOURCE_VC0; i < priv->num_pads; i++)
> >  		priv->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> >
> > -	ret = media_entity_pads_init(&priv->subdev.entity, num_pads,
> > +	ret = media_entity_pads_init(&priv->subdev.entity, priv->num_pads,
> >  				     priv->pads);
> >  	if (ret)
> >  		goto error_async;
> > @@ -1912,21 +1929,25 @@ static int rcsi2_probe(struct platform_device *pdev)
> >
> >  	pm_runtime_enable(&pdev->dev);
> >
> > +	ret = v4l2_subdev_init_finalize(&priv->subdev);
> > +	if (ret)
> > +		goto error_pm_runtime;
> > +
> >  	ret = v4l2_async_register_subdev(&priv->subdev);
> >  	if (ret < 0)
> > -		goto error_pm_runtime;
> > +		goto error_subdev;
> >
> >  	dev_info(priv->dev, "%d lanes found\n", priv->lanes);
> >
> >  	return 0;
> >
> > +error_subdev:
> > +	v4l2_subdev_cleanup(&priv->subdev);
> >  error_pm_runtime:
> >  	pm_runtime_disable(&pdev->dev);
> >  error_async:
> >  	v4l2_async_nf_unregister(&priv->notifier);
> >  	v4l2_async_nf_cleanup(&priv->notifier);
> > -error_mutex:
> > -	mutex_destroy(&priv->lock);
> >
> >  	return ret;
> >  }
> > @@ -1941,8 +1962,6 @@ static void rcsi2_remove(struct platform_device *pdev)
> >  	v4l2_subdev_cleanup(&priv->subdev);
> >
> >  	pm_runtime_disable(&pdev->dev);
> > -
> > -	mutex_destroy(&priv->lock);
> >  }
> >
> >  static struct platform_driver rcar_csi2_pdrv = {
> > --
> > 2.44.0
> >
>
> --
> Kind Regards,
> Niklas Söderlund

[PATCH v2.2 04/11] media: rcar-csi2: Use the subdev active state

[Jacopo Mondi]

Create the subdevice state with v4l2_subdev_init_finalize() and
implement the init_state() operation to guarantee the state is initialized.

Store the current image format in the subdev active state and remove it
from the driver private structure.

To guarantee the same image format is applied to all source pads,
propagate the format from the sink pad to the sources, disallowing
changing format on a source pad.

To support both gen3 and gen4, which feature a different number of
source pads, introduce an helper function to return the correct number
of pads.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
v2.1->v2.2

- Use an helper function as suggested by Niklas

v2->v2.1

- Fix gen4 issue reported by Niklas: store the number of pads in the
  driver structure and use it when handling in init_state() and
  set_pad_format() when applying a format to the source pads
---
 drivers/media/platform/renesas/rcar-csi2.c | 152 ++++++++++++---------
 1 file changed, 90 insertions(+), 62 deletions(-)

diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
index 2d464e43a5be..c419ddb4c5a2 100644
--- a/drivers/media/platform/renesas/rcar-csi2.c
+++ b/drivers/media/platform/renesas/rcar-csi2.c
@@ -587,7 +587,8 @@ enum rcar_csi2_pads {
 struct rcar_csi2_info {
 	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
 	int (*phy_post_init)(struct rcar_csi2 *priv);
-	int (*start_receiver)(struct rcar_csi2 *priv);
+	int (*start_receiver)(struct rcar_csi2 *priv,
+			      struct v4l2_subdev_state *state);
 	void (*enter_standby)(struct rcar_csi2 *priv);
 	const struct rcsi2_mbps_reg *hsfreqrange;
 	unsigned int csi0clkfreqrange;
@@ -613,8 +614,6 @@ struct rcar_csi2 {

 	int channel_vc[4];

-	struct mutex lock; /* Protects mf and stream_count. */
-	struct v4l2_mbus_framefmt mf;
 	int stream_count;

 	bool cphy;
@@ -632,6 +631,16 @@ static inline struct rcar_csi2 *notifier_to_csi2(struct v4l2_async_notifier *n)
 	return container_of(n, struct rcar_csi2, notifier);
 }

+static unsigned int rcsi2_num_pads(const struct rcar_csi2 *priv)
+{
+	/* Used together with R-Car ISP: one sink and one source pad. */
+	if (priv->info->use_isp)
+		return 2;
+
+	/* Used together with R-Car VIN: one sink and four source pads. */
+	return 5;
+}
+
 static u32 rcsi2_read(struct rcar_csi2 *priv, unsigned int reg)
 {
 	return ioread32(priv->base + reg);
@@ -808,20 +817,25 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
 	return 0;
 }

-static int rcsi2_start_receiver_gen3(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;
 	unsigned int lanes;
 	unsigned int i;
 	int mbps, ret;

+	/* Use the format on the sink pad to compute the receiver config. */
+	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
+
 	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
-		priv->mf.width, priv->mf.height,
-		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
+		fmt->width, fmt->height,
+		fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');

 	/* Code is validated in set_fmt. */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	format = rcsi2_code_to_fmt(fmt->code);
 	if (!format)
 		return -EINVAL;

@@ -849,11 +863,11 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
 			vcdt2 |= vcdt_part << ((i % 2) * 16);
 	}

-	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
+	if (fmt->field == V4L2_FIELD_ALTERNATE) {
 		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
 			| FLD_FLD_EN;

-		if (priv->mf.height == 240)
+		if (fmt->height == 240)
 			fld |= FLD_FLD_NUM(0);
 		else
 			fld |= FLD_FLD_NUM(1);
@@ -1049,15 +1063,18 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
 	return 0;
 }

-static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
+static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
+				    struct v4l2_subdev_state *state)
 {
 	const struct rcar_csi2_format *format;
+	const struct v4l2_mbus_framefmt *fmt;
 	unsigned int lanes;
 	int msps;
 	int ret;

-	/* Calculate parameters */
-	format = rcsi2_code_to_fmt(priv->mf.code);
+	/* Use the format on the sink pad to compute the receiver config. */
+	fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK);
+	format = rcsi2_code_to_fmt(fmt->code);
 	if (!format)
 		return -EINVAL;

@@ -1114,7 +1131,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
 	return 0;
 }

-static int rcsi2_start(struct rcar_csi2 *priv)
+static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
 {
 	int ret;

@@ -1122,7 +1139,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
 	if (ret < 0)
 		return ret;

-	ret = priv->info->start_receiver(priv);
+	ret = priv->info->start_receiver(priv, state);
 	if (ret) {
 		rcsi2_enter_standby(priv);
 		return ret;
@@ -1146,17 +1163,16 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
 static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	struct v4l2_subdev_state *state;
 	int ret = 0;

-	mutex_lock(&priv->lock);
+	if (!priv->remote)
+		return -ENODEV;

-	if (!priv->remote) {
-		ret = -ENODEV;
-		goto out;
-	}
+	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);

 	if (enable && priv->stream_count == 0) {
-		ret = rcsi2_start(priv);
+		ret = rcsi2_start(priv, state);
 		if (ret)
 			goto out;
 	} else if (!enable && priv->stream_count == 1) {
@@ -1165,49 +1181,29 @@ static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)

 	priv->stream_count += enable ? 1 : -1;
 out:
-	mutex_unlock(&priv->lock);
+	v4l2_subdev_unlock_state(state);

 	return ret;
 }

 static int rcsi2_set_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
+				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_format *format)
 {
 	struct rcar_csi2 *priv = sd_to_csi2(sd);
-	struct v4l2_mbus_framefmt *framefmt;
+	unsigned int num_pads = rcsi2_num_pads(priv);

-	mutex_lock(&priv->lock);
+	if (format->pad > RCAR_CSI2_SINK)
+		return v4l2_subdev_get_fmt(sd, state, format);

 	if (!rcsi2_code_to_fmt(format->format.code))
 		format->format.code = rcar_csi2_formats[0].code;

-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		priv->mf = format->format;
-	} else {
-		framefmt = v4l2_subdev_state_get_format(sd_state, 0);
-		*framefmt = format->format;
-	}
-
-	mutex_unlock(&priv->lock);
-
-	return 0;
-}
-
-static int rcsi2_get_pad_format(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
-				struct v4l2_subdev_format *format)
-{
-	struct rcar_csi2 *priv = sd_to_csi2(sd);
-
-	mutex_lock(&priv->lock);
-
-	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE)
-		format->format = priv->mf;
-	else
-		format->format = *v4l2_subdev_state_get_format(sd_state, 0);
+	*v4l2_subdev_state_get_format(state, format->pad) = format->format;

-	mutex_unlock(&priv->lock);
+	/* Propagate the format to the source pads. */
+	for (unsigned int i = RCAR_CSI2_SOURCE_VC0; i < num_pads; i++)
+		*v4l2_subdev_state_get_format(state, i) = format->format;

 	return 0;
 }
@@ -1218,7 +1214,7 @@ static const struct v4l2_subdev_video_ops rcar_csi2_video_ops = {

 static const struct v4l2_subdev_pad_ops rcar_csi2_pad_ops = {
 	.set_fmt = rcsi2_set_pad_format,
-	.get_fmt = rcsi2_get_pad_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 };

 static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
@@ -1226,6 +1222,33 @@ static const struct v4l2_subdev_ops rcar_csi2_subdev_ops = {
 	.pad	= &rcar_csi2_pad_ops,
 };

+static int rcsi2_init_state(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_state *state)
+{
+	struct rcar_csi2 *priv = sd_to_csi2(sd);
+	unsigned int num_pads = rcsi2_num_pads(priv);
+
+	static const struct v4l2_mbus_framefmt rcar_csi2_default_fmt = {
+		.width		= 1920,
+		.height		= 1080,
+		.code		= MEDIA_BUS_FMT_RGB888_1X24,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
+		.field		= V4L2_FIELD_NONE,
+		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+		.quantization	= V4L2_QUANTIZATION_DEFAULT,
+		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+	};
+
+	for (unsigned int i = RCAR_CSI2_SINK; i < num_pads; i++)
+		*v4l2_subdev_state_get_format(state, i) = rcar_csi2_default_fmt;
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops rcar_csi2_internal_ops = {
+	.init_state = rcsi2_init_state,
+};
+
 static irqreturn_t rcsi2_irq(int irq, void *data)
 {
 	struct rcar_csi2 *priv = data;
@@ -1251,14 +1274,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)

 static irqreturn_t rcsi2_irq_thread(int irq, void *data)
 {
+	struct v4l2_subdev_state *state;
 	struct rcar_csi2 *priv = data;

-	mutex_lock(&priv->lock);
+	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
+
 	rcsi2_stop(priv);
 	usleep_range(1000, 2000);
-	if (rcsi2_start(priv))
+	if (rcsi2_start(priv, state))
 		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
-	mutex_unlock(&priv->lock);
+
+	v4l2_subdev_unlock_state(state);

 	return IRQ_HANDLED;
 }
@@ -1870,23 +1896,23 @@ static int rcsi2_probe(struct platform_device *pdev)

 	priv->dev = &pdev->dev;

-	mutex_init(&priv->lock);
 	priv->stream_count = 0;

 	ret = rcsi2_probe_resources(priv, pdev);
 	if (ret) {
 		dev_err(priv->dev, "Failed to get resources\n");
-		goto error_mutex;
+		return ret;
 	}

 	platform_set_drvdata(pdev, priv);

 	ret = rcsi2_parse_dt(priv);
 	if (ret)
-		goto error_mutex;
+		return ret;

 	priv->subdev.owner = THIS_MODULE;
 	priv->subdev.dev = &pdev->dev;
+	priv->subdev.internal_ops = &rcar_csi2_internal_ops;
 	v4l2_subdev_init(&priv->subdev, &rcar_csi2_subdev_ops);
 	v4l2_set_subdevdata(&priv->subdev, &pdev->dev);
 	snprintf(priv->subdev.name, sizeof(priv->subdev.name), "%s %s",
@@ -1896,7 +1922,7 @@ static int rcsi2_probe(struct platform_device *pdev)
 	priv->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
 	priv->subdev.entity.ops = &rcar_csi2_entity_ops;

-	num_pads = priv->info->use_isp ? 2 : NR_OF_RCAR_CSI2_PAD;
+	num_pads = rcsi2_num_pads(priv);

 	priv->pads[RCAR_CSI2_SINK].flags = MEDIA_PAD_FL_SINK;
 	for (i = RCAR_CSI2_SOURCE_VC0; i < num_pads; i++)
@@ -1912,21 +1938,25 @@ static int rcsi2_probe(struct platform_device *pdev)

 	pm_runtime_enable(&pdev->dev);

+	ret = v4l2_subdev_init_finalize(&priv->subdev);
+	if (ret)
+		goto error_pm_runtime;
+
 	ret = v4l2_async_register_subdev(&priv->subdev);
 	if (ret < 0)
-		goto error_pm_runtime;
+		goto error_subdev;

 	dev_info(priv->dev, "%d lanes found\n", priv->lanes);

 	return 0;

+error_subdev:
+	v4l2_subdev_cleanup(&priv->subdev);
 error_pm_runtime:
 	pm_runtime_disable(&pdev->dev);
 error_async:
 	v4l2_async_nf_unregister(&priv->notifier);
 	v4l2_async_nf_cleanup(&priv->notifier);
-error_mutex:
-	mutex_destroy(&priv->lock);

 	return ret;
 }
@@ -1941,8 +1971,6 @@ static void rcsi2_remove(struct platform_device *pdev)
 	v4l2_subdev_cleanup(&priv->subdev);

 	pm_runtime_disable(&pdev->dev);
-
-	mutex_destroy(&priv->lock);
 }

 static struct platform_driver rcar_csi2_pdrv = {
--
2.44.0

[PATCH v2 05/11] media: adv748x-csi2: Implement enum_mbus_codes

[Jacopo Mondi]

Define a list of supported mbus codes for the TXA and TXB CSI-2
transmitters and implement the enum_mbus_code operation.

The TXB transmitter only support YUV422 while the TXA one supports
multiple formats as reported by the chip's manual in section 9.7.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 5b265b722394..4fd6d3a681d5 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -14,6 +14,18 @@
 
 #include "adv748x.h"
 
+static const unsigned int adv748x_csi2_txa_fmts[] = {
+	MEDIA_BUS_FMT_UYVY8_1X16,
+	MEDIA_BUS_FMT_UYVY10_1X20,
+	MEDIA_BUS_FMT_RGB565_1X16,
+	MEDIA_BUS_FMT_RGB666_1X18,
+	MEDIA_BUS_FMT_RGB888_1X24,
+};
+
+static const unsigned int adv748x_csi2_txb_fmts[] = {
+	MEDIA_BUS_FMT_UYVY8_1X16,
+};
+
 int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
 {
 	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
@@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
  * But we must support setting the pad formats for format propagation.
  */
 
+static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
+				       struct v4l2_subdev_state *sd_state,
+				       struct v4l2_subdev_mbus_code_enum *code)
+{
+	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
+	const unsigned int *codes = is_txa(tx) ?
+				    adv748x_csi2_txa_fmts :
+				    adv748x_csi2_txb_fmts;
+	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
+				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
+
+	if (code->pad != ADV748X_CSI2_SOURCE)
+		return -EINVAL;
+
+	if (code->index >= num_fmts)
+		return -EINVAL;
+
+	code->code = codes[code->index];
+
+	return 0;
+}
+
 static struct v4l2_mbus_framefmt *
 adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *sd_state,
@@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
 }
 
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
+	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
 	.get_fmt = adv748x_csi2_get_format,
 	.set_fmt = adv748x_csi2_set_format,
 	.get_mbus_config = adv748x_csi2_get_mbus_config,
-- 
2.44.0

Re: [PATCH v2 05/11] media: adv748x-csi2: Implement enum_mbus_codes

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:33PM +0200, Jacopo Mondi wrote:
> Define a list of supported mbus codes for the TXA and TXB CSI-2
> transmitters and implement the enum_mbus_code operation.
> 
> The TXB transmitter only support YUV422 while the TXA one supports
> multiple formats as reported by the chip's manual in section 9.7.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 5b265b722394..4fd6d3a681d5 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -14,6 +14,18 @@
>  
>  #include "adv748x.h"
>  
> +static const unsigned int adv748x_csi2_txa_fmts[] = {
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +	MEDIA_BUS_FMT_UYVY10_1X20,
> +	MEDIA_BUS_FMT_RGB565_1X16,
> +	MEDIA_BUS_FMT_RGB666_1X18,
> +	MEDIA_BUS_FMT_RGB888_1X24,
> +};
> +
> +static const unsigned int adv748x_csi2_txb_fmts[] = {
> +	MEDIA_BUS_FMT_UYVY8_1X16,
> +};
> +
>  int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
>  {
>  	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> @@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
>   * But we must support setting the pad formats for format propagation.
>   */
>  
> +static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
> +				       struct v4l2_subdev_state *sd_state,
> +				       struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> +	const unsigned int *codes = is_txa(tx) ?
> +				    adv748x_csi2_txa_fmts :
> +				    adv748x_csi2_txb_fmts;
> +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> +
> +	if (code->pad != ADV748X_CSI2_SOURCE)
> +		return -EINVAL;

Any reason to not support enumeration of formats on the sink pad ?

Is the CSI-2 TX pass-through regarding media bus formats ? That is, does
it modify the format between the sink and source pads ? If not, I think
this function should be implemented as

	if (code->pad == ADV748X_CSI2_SINK) {
		if (code->index >= num_fmts)
			return -EINVAL;

		code->code = codes[code->index];
	} else {
		const struct v4l2_msbu_framefmt *fmt;

		if (code->index > 0)
			return -EINVAL;

		/*
		 * The device doesn't modify formats, the same media bus code is
		 * used on the sink and source.
		 */
		fmt = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SINK);
		code->code = fmt->code;
	}

> +
> +	if (code->index >= num_fmts)
> +		return -EINVAL;
> +
> +	code->code = codes[code->index];
> +
> +	return 0;
> +}
> +
>  static struct v4l2_mbus_framefmt *
>  adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
>  			    struct v4l2_subdev_state *sd_state,
> @@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
>  }
>  
>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> +	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
>  	.get_fmt = adv748x_csi2_get_format,
>  	.set_fmt = adv748x_csi2_set_format,
>  	.get_mbus_config = adv748x_csi2_get_mbus_config,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/11] media: adv748x-csi2: Implement enum_mbus_codes

[Jacopo Mondi]

Hi Laurent

On Thu, May 09, 2024 at 03:42:49PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, May 06, 2024 at 06:49:33PM +0200, Jacopo Mondi wrote:
> > Define a list of supported mbus codes for the TXA and TXB CSI-2
> > transmitters and implement the enum_mbus_code operation.
> >
> > The TXB transmitter only support YUV422 while the TXA one supports
> > multiple formats as reported by the chip's manual in section 9.7.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > index 5b265b722394..4fd6d3a681d5 100644
> > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > @@ -14,6 +14,18 @@
> >
> >  #include "adv748x.h"
> >
> > +static const unsigned int adv748x_csi2_txa_fmts[] = {
> > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > +	MEDIA_BUS_FMT_UYVY10_1X20,
> > +	MEDIA_BUS_FMT_RGB565_1X16,
> > +	MEDIA_BUS_FMT_RGB666_1X18,
> > +	MEDIA_BUS_FMT_RGB888_1X24,
> > +};
> > +
> > +static const unsigned int adv748x_csi2_txb_fmts[] = {
> > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > +};
> > +
> >  int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
> >  {
> >  	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> > @@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
> >   * But we must support setting the pad formats for format propagation.
> >   */
> >
> > +static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
> > +				       struct v4l2_subdev_state *sd_state,
> > +				       struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > +	const unsigned int *codes = is_txa(tx) ?
> > +				    adv748x_csi2_txa_fmts :
> > +				    adv748x_csi2_txb_fmts;
> > +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> > +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> > +
> > +	if (code->pad != ADV748X_CSI2_SOURCE)
> > +		return -EINVAL;
>
> Any reason to not support enumeration of formats on the sink pad ?
>
> it modify the format between the sink and source pads ? If not, I think
> this function should be implemented as
>
> 	if (code->pad == ADV748X_CSI2_SINK) {
> 		if (code->index >= num_fmts)
> 			return -EINVAL;
>
> 		code->code = codes[code->index];

I don't think this is correct. The formats I have listed in
adv748x_csi2_txa_fmts and adv748x_csi2_txb_fmts are the CSI-2 output
formats, not the ones accepted on the sink side of the CSI-2 TX

The CSI-2 TX sink pads connects to either the HDMI or AFE subdevices.
The media link represents the internal processing pipeline between the
two frontends and the TXes. The formats accepted on the TX sinks are
then the formats that can be produced by the HDMI/Analog sources the
adv748x is connected to ?

> 	} else {
> 		const struct v4l2_msbu_framefmt *fmt;
>
> 		if (code->index > 0)
> 			return -EINVAL;
>
> 		/*
> 		 * The device doesn't modify formats, the same media bus code is

At the same time the device seems capable of performing format
conversion, but the driver configures it in pass-through mode.

Now, given this configuration, it seems that whatever format is
produced by the HDMI/Analog front-end is reproduced on the CSI-2 Tx
source side. However the two frontends only list

static int adv748x_hdmi_enum_mbus_code(struct v4l2_subdev *sd,
				  struct v4l2_subdev_state *sd_state,
				  struct v4l2_subdev_mbus_code_enum *code)
{
	if (code->index != 0)
		return -EINVAL;

	code->code = MEDIA_BUS_FMT_RGB888_1X24;

	return 0;
}


static int adv748x_afe_enum_mbus_code(struct v4l2_subdev *sd,
				      struct v4l2_subdev_state *sd_state,
				      struct v4l2_subdev_mbus_code_enum *code)
{
	if (code->index != 0)
		return -EINVAL;

	code->code = MEDIA_BUS_FMT_UYVY8_2X8;

	return 0;
}

While I presume many more formats would be possible.

In facts (for analog):
The video standards supported by the video processor include PAL B/PAL
D/PAL I/PAL G/PAL H, PAL 60, PAL M, PAL N, PAL Nc, NTSC M/NTSC J, NTSC
4.43, and SECAM B/SECAM D/SECAM G/SECAM K/SECAM L. The ADV748x can
automatically detect the input video standard and process it
accordingly.

I presume the HDMI standard support more formats than just RGB888 ?

So, as I was not sure on how to handle this, and enumerating formats
on the sink pads (which represent an internal bus connection) was of
little value, I decided to only allow format enumeration on the CSI-2
source pads, as the supported formats are well described by the chip
manual.

What do you think ?

> 		 * used on the sink and source.
> 		 */
> 		fmt = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SINK);
> 		code->code = fmt->code;
> 	}
>
> > +
> > +	if (code->index >= num_fmts)
> > +		return -EINVAL;
> > +
> > +	code->code = codes[code->index];
> > +
> > +	return 0;
> > +}
> > +
> >  static struct v4l2_mbus_framefmt *
> >  adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
> >  			    struct v4l2_subdev_state *sd_state,
> > @@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
> >  }
> >
> >  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> > +	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
> >  	.get_fmt = adv748x_csi2_get_format,
> >  	.set_fmt = adv748x_csi2_set_format,
> >  	.get_mbus_config = adv748x_csi2_get_mbus_config,
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 05/11] media: adv748x-csi2: Implement enum_mbus_codes

[Laurent Pinchart]

Hi Jacopo,

On Thu, May 09, 2024 at 03:44:02PM +0200, Jacopo Mondi wrote:
> On Thu, May 09, 2024 at 03:42:49PM GMT, Laurent Pinchart wrote:
> > On Mon, May 06, 2024 at 06:49:33PM +0200, Jacopo Mondi wrote:
> > > Define a list of supported mbus codes for the TXA and TXB CSI-2
> > > transmitters and implement the enum_mbus_code operation.
> > >
> > > The TXB transmitter only support YUV422 while the TXA one supports
> > > multiple formats as reported by the chip's manual in section 9.7.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/adv748x/adv748x-csi2.c | 35 ++++++++++++++++++++++++
> > >  1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > index 5b265b722394..4fd6d3a681d5 100644
> > > --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> > > @@ -14,6 +14,18 @@
> > >
> > >  #include "adv748x.h"
> > >
> > > +static const unsigned int adv748x_csi2_txa_fmts[] = {
> > > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > > +	MEDIA_BUS_FMT_UYVY10_1X20,
> > > +	MEDIA_BUS_FMT_RGB565_1X16,
> > > +	MEDIA_BUS_FMT_RGB666_1X18,
> > > +	MEDIA_BUS_FMT_RGB888_1X24,
> > > +};
> > > +
> > > +static const unsigned int adv748x_csi2_txb_fmts[] = {
> > > +	MEDIA_BUS_FMT_UYVY8_1X16,
> > > +};
> > > +
> > >  int adv748x_csi2_set_virtual_channel(struct adv748x_csi2 *tx, unsigned int vc)
> > >  {
> > >  	return tx_write(tx, ADV748X_CSI_VC_REF, vc << ADV748X_CSI_VC_REF_SHIFT);
> > > @@ -139,6 +151,28 @@ static const struct v4l2_subdev_video_ops adv748x_csi2_video_ops = {
> > >   * But we must support setting the pad formats for format propagation.
> > >   */
> > >
> > > +static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
> > > +				       struct v4l2_subdev_state *sd_state,
> > > +				       struct v4l2_subdev_mbus_code_enum *code)
> > > +{
> > > +	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> > > +	const unsigned int *codes = is_txa(tx) ?
> > > +				    adv748x_csi2_txa_fmts :
> > > +				    adv748x_csi2_txb_fmts;
> > > +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> > > +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> > > +
> > > +	if (code->pad != ADV748X_CSI2_SOURCE)
> > > +		return -EINVAL;
> >
> > Any reason to not support enumeration of formats on the sink pad ?
> >
> > it modify the format between the sink and source pads ? If not, I think
> > this function should be implemented as
> >
> > 	if (code->pad == ADV748X_CSI2_SINK) {
> > 		if (code->index >= num_fmts)
> > 			return -EINVAL;
> >
> > 		code->code = codes[code->index];
> 
> I don't think this is correct. The formats I have listed in
> adv748x_csi2_txa_fmts and adv748x_csi2_txb_fmts are the CSI-2 output
> formats, not the ones accepted on the sink side of the CSI-2 TX

Ah OK.

> The CSI-2 TX sink pads connects to either the HDMI or AFE subdevices.
> The media link represents the internal processing pipeline between the
> two frontends and the TXes. The formats accepted on the TX sinks are
> then the formats that can be produced by the HDMI/Analog sources the
> adv748x is connected to ?

So the CSI-2 TX performs format conversion ? How about RGB <-> YUV
conversion, is that handled in the source (AFE and HDMI), or in the
CSI-2 TX ?

Let's first discuss what each subdev does, and then we'll look at the
implementation.

> > 	} else {
> > 		const struct v4l2_msbu_framefmt *fmt;
> >
> > 		if (code->index > 0)
> > 			return -EINVAL;
> >
> > 		/*
> > 		 * The device doesn't modify formats, the same media bus code is
> 
> At the same time the device seems capable of performing format
> conversion, but the driver configures it in pass-through mode.
> 
> Now, given this configuration, it seems that whatever format is
> produced by the HDMI/Analog front-end is reproduced on the CSI-2 Tx
> source side. However the two frontends only list
> 
> static int adv748x_hdmi_enum_mbus_code(struct v4l2_subdev *sd,
> 				  struct v4l2_subdev_state *sd_state,
> 				  struct v4l2_subdev_mbus_code_enum *code)
> {
> 	if (code->index != 0)
> 		return -EINVAL;
> 
> 	code->code = MEDIA_BUS_FMT_RGB888_1X24;
> 
> 	return 0;
> }
> 
> 
> static int adv748x_afe_enum_mbus_code(struct v4l2_subdev *sd,
> 				      struct v4l2_subdev_state *sd_state,
> 				      struct v4l2_subdev_mbus_code_enum *code)
> {
> 	if (code->index != 0)
> 		return -EINVAL;
> 
> 	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> 
> 	return 0;
> }
> 
> While I presume many more formats would be possible.
> 
> In facts (for analog):
> The video standards supported by the video processor include PAL B/PAL
> D/PAL I/PAL G/PAL H, PAL 60, PAL M, PAL N, PAL Nc, NTSC M/NTSC J, NTSC
> 4.43, and SECAM B/SECAM D/SECAM G/SECAM K/SECAM L. The ADV748x can
> automatically detect the input video standard and process it
> accordingly.
> 
> I presume the HDMI standard support more formats than just RGB888 ?

Probably. I haven't checked the documentation.

> So, as I was not sure on how to handle this, and enumerating formats
> on the sink pads (which represent an internal bus connection) was of
> little value, I decided to only allow format enumeration on the CSI-2
> source pads, as the supported formats are well described by the chip
> manual.
> 
> What do you think ?

From a CSI-2 TX subdev point of view, the correct thing to do API-wise
is it

- enumerate on the sink pad all the possible formats that can be
  configured on that pad, regardless of how the connected source subdev
  is connected ; and

- enumerate on the source pad all the possible formats that can be
  output using the current sink format (obtained from the state).

We thus need

- a list of all the possible formats the CSI-2 TX can accept on its
  input ; and

- a way to convert those formats to CSI-2 TX output formats, *if* the
  CSI-2 TX supports format conversion.

In case the CSI-2 TX supports format conversion, but the driver doesn't
implement that yet, it's fine enumerating a single format on the source
pad, identical to the current format on the sink pad for now. This can
be extended to other formats in the future when we implement format
conversion in the driver.

> > 		 * used on the sink and source.
> > 		 */
> > 		fmt = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SINK);
> > 		code->code = fmt->code;
> > 	}
> >
> > > +
> > > +	if (code->index >= num_fmts)
> > > +		return -EINVAL;
> > > +
> > > +	code->code = codes[code->index];
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static struct v4l2_mbus_framefmt *
> > >  adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
> > >  			    struct v4l2_subdev_state *sd_state,
> > > @@ -228,6 +262,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
> > >  }
> > >
> > >  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
> > > +	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
> > >  	.get_fmt = adv748x_csi2_get_format,
> > >  	.set_fmt = adv748x_csi2_set_format,
> > >  	.get_mbus_config = adv748x_csi2_get_mbus_config,

-- 
Regards,

Laurent Pinchart

[PATCH v2 06/11] media: adv748x-csi2: Validate the image format

[Jacopo Mondi]

The adv748x-csi2 driver configures the CSI-2 transmitter to
automatically infer the image stream format from the connected
frontend (HDMI or AFE).

Setting a new format on the subdevice hence does not actually control
the CSI-2 output format, but it's only there for the purpose of
pipeline validation.

However, there is currently no validation that the supplied media bus
code is valid and supported by the device.

With the introduction of enum_mbus_codes a list of supported format is
now available, use it to validate that the supplied format is correct
and use the default UYVY8 one if that's not the case.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 4fd6d3a681d5..3f22dc426d7a 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -208,6 +208,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static bool adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
+					  unsigned int code)
+{
+	const unsigned int *codes = is_txa(tx) ?
+				    adv748x_csi2_txa_fmts :
+				    adv748x_csi2_txb_fmts;
+	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
+				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
+
+	for (unsigned int i = 0; i < num_fmts; i++) {
+		if (codes[i] == code)
+			return true;
+	}
+
+	return false;
+}
+
 static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state,
 				   struct v4l2_subdev_format *sdformat)
@@ -217,6 +234,13 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *mbusformat;
 	int ret = 0;
 
+	/*
+	 * Make sure the format is supported, if not default it to
+	 * UYVY8 as it's supported by both TXes.
+	 */
+	if (!adv748x_csi2_is_fmt_supported(tx, sdformat->format.code))
+		sdformat->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
+
 	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
 						 sdformat->which);
 	if (!mbusformat)
-- 
2.44.0

Re: [PATCH v2 06/11] media: adv748x-csi2: Validate the image format

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:34PM +0200, Jacopo Mondi wrote:
> The adv748x-csi2 driver configures the CSI-2 transmitter to
> automatically infer the image stream format from the connected
> frontend (HDMI or AFE).
> 
> Setting a new format on the subdevice hence does not actually control
> the CSI-2 output format, but it's only there for the purpose of
> pipeline validation.
> 
> However, there is currently no validation that the supplied media bus
> code is valid and supported by the device.
> 
> With the introduction of enum_mbus_codes a list of supported format is
> now available, use it to validate that the supplied format is correct
> and use the default UYVY8 one if that's not the case.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 4fd6d3a681d5..3f22dc426d7a 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -208,6 +208,23 @@ static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static bool adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
> +					  unsigned int code)

u32 code to match the data type from the format structure.

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

> +{
> +	const unsigned int *codes = is_txa(tx) ?
> +				    adv748x_csi2_txa_fmts :
> +				    adv748x_csi2_txb_fmts;
> +	size_t num_fmts = is_txa(tx) ? ARRAY_SIZE(adv748x_csi2_txa_fmts)
> +				     : ARRAY_SIZE(adv748x_csi2_txb_fmts);
> +
> +	for (unsigned int i = 0; i < num_fmts; i++) {
> +		if (codes[i] == code)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_state *sd_state,
>  				   struct v4l2_subdev_format *sdformat)
> @@ -217,6 +234,13 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *mbusformat;
>  	int ret = 0;
>  
> +	/*
> +	 * Make sure the format is supported, if not default it to
> +	 * UYVY8 as it's supported by both TXes.
> +	 */
> +	if (!adv748x_csi2_is_fmt_supported(tx, sdformat->format.code))
> +		sdformat->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
> +
>  	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
>  						 sdformat->which);
>  	if (!mbusformat)

-- 
Regards,

Laurent Pinchart

[PATCH v2 07/11] media: adv748x-csi2: Use the subdev active state

[Jacopo Mondi]

Initialize and use the subdev active state to store the subdevice
format.

This simplifies the implementation of the get_fmt and set_fmt pad
operations.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-csi2.c | 107 +++++++++--------------
 drivers/media/i2c/adv748x/adv748x.h      |   1 -
 2 files changed, 42 insertions(+), 66 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
index 3f22dc426d7a..76b5eefdc2a7 100644
--- a/drivers/media/i2c/adv748x/adv748x-csi2.c
+++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
@@ -6,7 +6,6 @@
  */
 
 #include <linux/module.h>
-#include <linux/mutex.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
@@ -71,7 +70,33 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
 
 /* -----------------------------------------------------------------------------
  * v4l2_subdev_internal_ops
- *
+ */
+
+static int adv748x_csi2_init_state(struct v4l2_subdev *sd,
+				   struct v4l2_subdev_state *state)
+{
+	static const struct v4l2_mbus_framefmt adv748x_csi2_default_fmt = {
+		.width		= 1280,
+		.height		= 720,
+		.code		= MEDIA_BUS_FMT_UYVY8_1X16,
+		.colorspace	= V4L2_COLORSPACE_SRGB,
+		.field		= V4L2_FIELD_NONE,
+		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
+		.quantization	= V4L2_QUANTIZATION_DEFAULT,
+		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
+	};
+	struct v4l2_mbus_framefmt *fmt;
+
+	fmt = v4l2_subdev_state_get_format(state, ADV748X_CSI2_SINK);
+	*fmt = adv748x_csi2_default_fmt;
+
+	fmt = v4l2_subdev_state_get_format(state, ADV748X_CSI2_SOURCE);
+	*fmt = adv748x_csi2_default_fmt;
+
+	return 0;
+}
+
+/*
  * We use the internal registered operation to be able to ensure that our
  * incremental subdevices (not connected in the forward path) can be registered
  * against the resulting video path and media device.
@@ -121,6 +146,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
 }
 
 static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
+	.init_state = adv748x_csi2_init_state,
 	.registered = adv748x_csi2_registered,
 };
 
@@ -173,41 +199,6 @@ static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
-			    struct v4l2_subdev_state *sd_state,
-			    unsigned int pad, u32 which)
-{
-	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
-
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_state_get_format(sd_state, pad);
-
-	return &tx->format;
-}
-
-static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
-				   struct v4l2_subdev_state *sd_state,
-				   struct v4l2_subdev_format *sdformat)
-{
-	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
-	struct adv748x_state *state = tx->state;
-	struct v4l2_mbus_framefmt *mbusformat;
-
-	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
-						 sdformat->which);
-	if (!mbusformat)
-		return -EINVAL;
-
-	mutex_lock(&state->mutex);
-
-	sdformat->format = *mbusformat;
-
-	mutex_unlock(&state->mutex);
-
-	return 0;
-}
-
 static bool adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
 					  unsigned int code)
 {
@@ -230,9 +221,10 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_format *sdformat)
 {
 	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
-	struct adv748x_state *state = tx->state;
 	struct v4l2_mbus_framefmt *mbusformat;
-	int ret = 0;
+
+	if (sdformat->pad == ADV748X_CSI2_SOURCE)
+		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
 
 	/*
 	 * Make sure the format is supported, if not default it to
@@ -241,34 +233,14 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
 	if (!adv748x_csi2_is_fmt_supported(tx, sdformat->format.code))
 		sdformat->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
 
-	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
-						 sdformat->which);
-	if (!mbusformat)
-		return -EINVAL;
-
-	mutex_lock(&state->mutex);
-
-	if (sdformat->pad == ADV748X_CSI2_SOURCE) {
-		const struct v4l2_mbus_framefmt *sink_fmt;
-
-		sink_fmt = adv748x_csi2_get_pad_format(sd, sd_state,
-						       ADV748X_CSI2_SINK,
-						       sdformat->which);
-
-		if (!sink_fmt) {
-			ret = -EINVAL;
-			goto unlock;
-		}
-
-		sdformat->format = *sink_fmt;
-	}
-
+	mbusformat = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
 	*mbusformat = sdformat->format;
 
-unlock:
-	mutex_unlock(&state->mutex);
+	/* Propagate format to the source pad. */
+	mbusformat = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SOURCE);
+	*mbusformat = sdformat->format;
 
-	return ret;
+	return 0;
 }
 
 static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
@@ -287,7 +259,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
 
 static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
 	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
-	.get_fmt = adv748x_csi2_get_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = adv748x_csi2_set_format,
 	.get_mbus_config = adv748x_csi2_get_mbus_config,
 };
@@ -379,6 +351,11 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
 	if (ret)
 		goto err_cleanup_subdev;
 
+	tx->sd.state_lock = &state->mutex;
+	ret = v4l2_subdev_init_finalize(&tx->sd);
+	if (ret)
+		goto err_free_ctrl;
+
 	ret = v4l2_async_register_subdev(&tx->sd);
 	if (ret)
 		goto err_free_ctrl;
diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
index d2b5e722e997..9bc0121d0eff 100644
--- a/drivers/media/i2c/adv748x/adv748x.h
+++ b/drivers/media/i2c/adv748x/adv748x.h
@@ -75,7 +75,6 @@ enum adv748x_csi2_pads {
 
 struct adv748x_csi2 {
 	struct adv748x_state *state;
-	struct v4l2_mbus_framefmt format;
 	unsigned int page;
 	unsigned int port;
 	unsigned int num_lanes;
-- 
2.44.0

Re: [PATCH v2 07/11] media: adv748x-csi2: Use the subdev active state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:35PM +0200, Jacopo Mondi wrote:
> Initialize and use the subdev active state to store the subdevice
> format.
> 
> This simplifies the implementation of the get_fmt and set_fmt pad
> operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-csi2.c | 107 +++++++++--------------
>  drivers/media/i2c/adv748x/adv748x.h      |   1 -
>  2 files changed, 42 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-csi2.c b/drivers/media/i2c/adv748x/adv748x-csi2.c
> index 3f22dc426d7a..76b5eefdc2a7 100644
> --- a/drivers/media/i2c/adv748x/adv748x-csi2.c
> +++ b/drivers/media/i2c/adv748x/adv748x-csi2.c
> @@ -6,7 +6,6 @@
>   */
>  
>  #include <linux/module.h>
> -#include <linux/mutex.h>
>  
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> @@ -71,7 +70,33 @@ static int adv748x_csi2_register_link(struct adv748x_csi2 *tx,
>  
>  /* -----------------------------------------------------------------------------
>   * v4l2_subdev_internal_ops
> - *
> + */
> +
> +static int adv748x_csi2_init_state(struct v4l2_subdev *sd,
> +				   struct v4l2_subdev_state *state)
> +{
> +	static const struct v4l2_mbus_framefmt adv748x_csi2_default_fmt = {
> +		.width		= 1280,
> +		.height		= 720,
> +		.code		= MEDIA_BUS_FMT_UYVY8_1X16,
> +		.colorspace	= V4L2_COLORSPACE_SRGB,
> +		.field		= V4L2_FIELD_NONE,
> +		.ycbcr_enc	= V4L2_YCBCR_ENC_DEFAULT,
> +		.quantization	= V4L2_QUANTIZATION_DEFAULT,
> +		.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> +	};
> +	struct v4l2_mbus_framefmt *fmt;
> +
> +	fmt = v4l2_subdev_state_get_format(state, ADV748X_CSI2_SINK);
> +	*fmt = adv748x_csi2_default_fmt;
> +
> +	fmt = v4l2_subdev_state_get_format(state, ADV748X_CSI2_SOURCE);
> +	*fmt = adv748x_csi2_default_fmt;
> +
> +	return 0;
> +}
> +
> +/*
>   * We use the internal registered operation to be able to ensure that our
>   * incremental subdevices (not connected in the forward path) can be registered
>   * against the resulting video path and media device.
> @@ -121,6 +146,7 @@ static int adv748x_csi2_registered(struct v4l2_subdev *sd)
>  }
>  
>  static const struct v4l2_subdev_internal_ops adv748x_csi2_internal_ops = {
> +	.init_state = adv748x_csi2_init_state,
>  	.registered = adv748x_csi2_registered,
>  };
>  
> @@ -173,41 +199,6 @@ static int adv748x_csi2_enum_mbus_code(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -adv748x_csi2_get_pad_format(struct v4l2_subdev *sd,
> -			    struct v4l2_subdev_state *sd_state,
> -			    unsigned int pad, u32 which)
> -{
> -	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> -
> -	if (which == V4L2_SUBDEV_FORMAT_TRY)
> -		return v4l2_subdev_state_get_format(sd_state, pad);
> -
> -	return &tx->format;
> -}
> -
> -static int adv748x_csi2_get_format(struct v4l2_subdev *sd,
> -				   struct v4l2_subdev_state *sd_state,
> -				   struct v4l2_subdev_format *sdformat)
> -{
> -	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> -	struct adv748x_state *state = tx->state;
> -	struct v4l2_mbus_framefmt *mbusformat;
> -
> -	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> -						 sdformat->which);
> -	if (!mbusformat)
> -		return -EINVAL;
> -
> -	mutex_lock(&state->mutex);
> -
> -	sdformat->format = *mbusformat;
> -
> -	mutex_unlock(&state->mutex);
> -
> -	return 0;
> -}
> -
>  static bool adv748x_csi2_is_fmt_supported(struct adv748x_csi2 *tx,
>  					  unsigned int code)
>  {
> @@ -230,9 +221,10 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_format *sdformat)
>  {
>  	struct adv748x_csi2 *tx = adv748x_sd_to_csi2(sd);
> -	struct adv748x_state *state = tx->state;
>  	struct v4l2_mbus_framefmt *mbusformat;
> -	int ret = 0;
> +
> +	if (sdformat->pad == ADV748X_CSI2_SOURCE)
> +		return v4l2_subdev_get_fmt(sd, sd_state, sdformat);
>  
>  	/*
>  	 * Make sure the format is supported, if not default it to
> @@ -241,34 +233,14 @@ static int adv748x_csi2_set_format(struct v4l2_subdev *sd,
>  	if (!adv748x_csi2_is_fmt_supported(tx, sdformat->format.code))
>  		sdformat->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
>  
> -	mbusformat = adv748x_csi2_get_pad_format(sd, sd_state, sdformat->pad,
> -						 sdformat->which);
> -	if (!mbusformat)
> -		return -EINVAL;
> -
> -	mutex_lock(&state->mutex);
> -
> -	if (sdformat->pad == ADV748X_CSI2_SOURCE) {
> -		const struct v4l2_mbus_framefmt *sink_fmt;
> -
> -		sink_fmt = adv748x_csi2_get_pad_format(sd, sd_state,
> -						       ADV748X_CSI2_SINK,
> -						       sdformat->which);
> -
> -		if (!sink_fmt) {
> -			ret = -EINVAL;
> -			goto unlock;
> -		}
> -
> -		sdformat->format = *sink_fmt;
> -	}
> -
> +	mbusformat = v4l2_subdev_state_get_format(sd_state, sdformat->pad);
>  	*mbusformat = sdformat->format;
>  
> -unlock:
> -	mutex_unlock(&state->mutex);
> +	/* Propagate format to the source pad. */
> +	mbusformat = v4l2_subdev_state_get_format(sd_state, ADV748X_CSI2_SOURCE);
> +	*mbusformat = sdformat->format;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
> @@ -287,7 +259,7 @@ static int adv748x_csi2_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad
>  
>  static const struct v4l2_subdev_pad_ops adv748x_csi2_pad_ops = {
>  	.enum_mbus_code = adv748x_csi2_enum_mbus_code,
> -	.get_fmt = adv748x_csi2_get_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>  	.set_fmt = adv748x_csi2_set_format,
>  	.get_mbus_config = adv748x_csi2_get_mbus_config,
>  };
> @@ -379,6 +351,11 @@ int adv748x_csi2_init(struct adv748x_state *state, struct adv748x_csi2 *tx)
>  	if (ret)
>  		goto err_cleanup_subdev;
>  
> +	tx->sd.state_lock = &state->mutex;
> +	ret = v4l2_subdev_init_finalize(&tx->sd);
> +	if (ret)
> +		goto err_free_ctrl;
> +
>  	ret = v4l2_async_register_subdev(&tx->sd);
>  	if (ret)
>  		goto err_free_ctrl;
> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h
> index d2b5e722e997..9bc0121d0eff 100644
> --- a/drivers/media/i2c/adv748x/adv748x.h
> +++ b/drivers/media/i2c/adv748x/adv748x.h
> @@ -75,7 +75,6 @@ enum adv748x_csi2_pads {
>  
>  struct adv748x_csi2 {
>  	struct adv748x_state *state;
> -	struct v4l2_mbus_framefmt format;
>  	unsigned int page;
>  	unsigned int port;
>  	unsigned int num_lanes;

-- 
Regards,

Laurent Pinchart

[PATCH v2 08/11] media: adv748x-afe: Use 1X16 media bus code

[Jacopo Mondi]

Now that the adv748x CSI-2 transmitter drivers validate the supported
formats, it is required for subdevices along the pipeline to produce
and consume the same media bus codes.

The adv748x analog front end driver use the 2X8 variant of the UYVY8
media bus code, while the CSI-2 transmitter use the 1X16 variant, which
is the correct one to use for the serial bus.

Make the adv748x afe use the 1X16 format variant to maintain the
pipeline validation correct.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/adv748x/adv748x-afe.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
index 50d9fbadbe38..5edb3295dc58 100644
--- a/drivers/media/i2c/adv748x/adv748x-afe.c
+++ b/drivers/media/i2c/adv748x/adv748x-afe.c
@@ -114,7 +114,7 @@ static void adv748x_afe_fill_format(struct adv748x_afe *afe,
 {
 	memset(fmt, 0, sizeof(*fmt));
 
-	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	fmt->code = MEDIA_BUS_FMT_UYVY8_1X16;
 	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
 	fmt->field = V4L2_FIELD_ALTERNATE;
 
@@ -337,7 +337,7 @@ static int adv748x_afe_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index != 0)
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
+	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
 
 	return 0;
 }
-- 
2.44.0

Re: [PATCH v2 08/11] media: adv748x-afe: Use 1X16 media bus code

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:36PM +0200, Jacopo Mondi wrote:
> Now that the adv748x CSI-2 transmitter drivers validate the supported
> formats, it is required for subdevices along the pipeline to produce
> and consume the same media bus codes.
> 
> The adv748x analog front end driver use the 2X8 variant of the UYVY8
> media bus code, while the CSI-2 transmitter use the 1X16 variant, which
> is the correct one to use for the serial bus.
> 
> Make the adv748x afe use the 1X16 format variant to maintain the
> pipeline validation correct.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

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

> ---
>  drivers/media/i2c/adv748x/adv748x-afe.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/adv748x/adv748x-afe.c b/drivers/media/i2c/adv748x/adv748x-afe.c
> index 50d9fbadbe38..5edb3295dc58 100644
> --- a/drivers/media/i2c/adv748x/adv748x-afe.c
> +++ b/drivers/media/i2c/adv748x/adv748x-afe.c
> @@ -114,7 +114,7 @@ static void adv748x_afe_fill_format(struct adv748x_afe *afe,
>  {
>  	memset(fmt, 0, sizeof(*fmt));
>  
> -	fmt->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	fmt->code = MEDIA_BUS_FMT_UYVY8_1X16;
>  	fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	fmt->field = V4L2_FIELD_ALTERNATE;
>  
> @@ -337,7 +337,7 @@ static int adv748x_afe_enum_mbus_code(struct v4l2_subdev *sd,
>  	if (code->index != 0)
>  		return -EINVAL;
>  
> -	code->code = MEDIA_BUS_FMT_UYVY8_2X8;
> +	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

[PATCH v2 09/11] media: max9286: Fix enum_mbus_code

[Jacopo Mondi]

The max9286 driver supports multiple output formats but only a single
one is reported through the .enum_mbus_code operation.

Fix that.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index d685d445cf23..5321238cad60 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -914,10 +914,10 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_mbus_code_enum *code)
 {
-	if (code->pad || code->index > 0)
+	if (code->pad || code->index >= ARRAY_SIZE(max9286_formats))
 		return -EINVAL;
 
-	code->code = MEDIA_BUS_FMT_UYVY8_1X16;
+	code->code = max9286_formats[code->index].code;
 
 	return 0;
 }
-- 
2.44.0

[PATCH v2 10/11] media: max9286: Use the subdev active state

[Jacopo Mondi]

Use the subdev active state in the max9286 driver to store the
image format.

Replace the .open() function call with the .init_state() one and
simplify the set/get_pad_fmt() operations.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 130 ++++++++++++------------------------
 1 file changed, 44 insertions(+), 86 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 5321238cad60..7fad190cd9b3 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -19,7 +19,6 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/module.h>
-#include <linux/mutex.h>
 #include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
@@ -198,12 +197,8 @@ struct max9286_priv {
 	struct v4l2_ctrl *pixelrate_ctrl;
 	unsigned int pixelrate;
 
-	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
 	struct v4l2_fract interval;
 
-	/* Protects controls and fmt structures */
-	struct mutex mutex;
-
 	unsigned int nsources;
 	unsigned int source_mask;
 	unsigned int route_mask;
@@ -788,19 +783,22 @@ static void max9286_v4l2_notifier_unregister(struct max9286_priv *priv)
 static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
+	struct v4l2_subdev_state *state;
 	struct max9286_source *source;
 	unsigned int i;
 	bool sync = false;
-	int ret;
+	int ret = 0;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
 
 	if (enable) {
 		const struct v4l2_mbus_framefmt *format;
 
 		/*
-		 * Get the format from the first used sink pad, as all sink
-		 * formats must be identical.
+		 * Get the format from the source pad, as all formats must be
+		 * identical.
 		 */
-		format = &priv->fmt[__ffs(priv->bound_sources)];
+		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
 
 		max9286_set_video_format(priv, format);
 		max9286_set_fsync_period(priv);
@@ -816,12 +814,12 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		for_each_source(priv, source) {
 			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
 			if (ret)
-				return ret;
+				goto unlock;
 		}
 
 		ret = max9286_check_video_links(priv);
 		if (ret)
-			return ret;
+			goto unlock;
 
 		/*
 		 * Wait until frame synchronization is locked.
@@ -842,7 +840,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		if (!sync) {
 			dev_err(&priv->client->dev,
 				"Failed to get frame synchronization\n");
-			return -EXDEV; /* Invalid cross-device link */
+			ret = -EXDEV; /* Invalid cross-device link */
+			goto unlock;
 		}
 
 		/*
@@ -865,7 +864,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		max9286_i2c_mux_close(priv);
 	}
 
-	return 0;
+unlock:
+	v4l2_subdev_unlock_state(state);
+
+	return ret;
 }
 
 static int max9286_get_frame_interval(struct v4l2_subdev *sd,
@@ -922,31 +924,20 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-max9286_get_pad_format(struct max9286_priv *priv,
-		       struct v4l2_subdev_state *sd_state,
-		       unsigned int pad, u32 which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_format(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &priv->fmt[pad];
-	default:
-		return NULL;
-	}
-}
-
 static int max9286_set_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
+			   struct v4l2_subdev_state *state,
 			   struct v4l2_subdev_format *format)
 {
 	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
+	unsigned int pad;
 	unsigned int i;
 
+	/*
+	 * Disable setting format on the source pad: format is propagated
+	 * from the sinks.
+	 */
 	if (format->pad == MAX9286_SRC_PAD)
-		return -EINVAL;
+		return v4l2_subdev_get_fmt(sd, state, format);
 
 	/* Validate the format. */
 	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
@@ -957,42 +948,16 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
 	if (i == ARRAY_SIZE(max9286_formats))
 		format->format.code = max9286_formats[0].code;
 
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
-					 format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	*cfg_fmt = format->format;
-	mutex_unlock(&priv->mutex);
-
-	return 0;
-}
-
-static int max9286_get_fmt(struct v4l2_subdev *sd,
-			   struct v4l2_subdev_state *sd_state,
-			   struct v4l2_subdev_format *format)
-{
-	struct max9286_priv *priv = sd_to_max9286(sd);
-	struct v4l2_mbus_framefmt *cfg_fmt;
-	unsigned int pad = format->pad;
+	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
 
 	/*
-	 * Multiplexed Stream Support: Support link validation by returning the
-	 * format of the first bound link. All links must have the same format,
-	 * as we do not support mixing and matching of cameras connected to the
-	 * max9286.
+	 * Apply the same format on the source pad. As all links must have the
+	 * same format we do so only when the first source format is set.
 	 */
-	if (pad == MAX9286_SRC_PAD)
-		pad = __ffs(priv->bound_sources);
-
-	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
-	if (!cfg_fmt)
-		return -EINVAL;
-
-	mutex_lock(&priv->mutex);
-	format->format = *cfg_fmt;
-	mutex_unlock(&priv->mutex);
+	pad = __ffs(priv->bound_sources);
+	if (pad == format->pad)
+		*v4l2_subdev_state_get_format(state,
+					      MAX9286_SRC_PAD) = format->format;
 
 	return 0;
 }
@@ -1003,7 +968,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
 
 static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
 	.enum_mbus_code = max9286_enum_mbus_code,
-	.get_fmt	= max9286_get_fmt,
+	.get_fmt	= v4l2_subdev_get_fmt,
 	.set_fmt	= max9286_set_fmt,
 	.get_frame_interval = max9286_get_frame_interval,
 	.set_frame_interval = max9286_set_frame_interval,
@@ -1025,26 +990,17 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
 	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
 };
 
-static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
+static int max9286_init_state(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *state)
 {
-	*fmt = max9286_default_format;
-}
-
-static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
-{
-	struct v4l2_mbus_framefmt *format;
-	unsigned int i;
-
-	for (i = 0; i < MAX9286_N_SINKS; i++) {
-		format = v4l2_subdev_state_get_format(fh->state, i);
-		max9286_init_format(format);
-	}
+	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
+		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
 
 	return 0;
 }
 
 static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
-	.open = max9286_open,
+	.init_state = max9286_init_state,
 };
 
 static const struct media_entity_operations max9286_media_ops = {
@@ -1079,10 +1035,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	}
 
 	/* Configure V4L2 for the MAX9286 itself */
-
-	for (i = 0; i < MAX9286_N_SINKS; i++)
-		max9286_init_format(&priv->fmt[i]);
-
 	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
 	priv->sd.internal_ops = &max9286_subdev_internal_ops;
 	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -1109,14 +1061,21 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 	if (ret)
 		goto err_async;
 
+	priv->sd.state_lock = priv->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&priv->sd);
+	if (ret)
+		goto err_async;
+
 	ret = v4l2_async_register_subdev(&priv->sd);
 	if (ret < 0) {
 		dev_err(dev, "Unable to register subdevice\n");
-		goto err_async;
+		goto err_subdev;
 	}
 
 	return 0;
 
+err_subdev:
+	v4l2_subdev_cleanup(&priv->sd);
 err_async:
 	v4l2_ctrl_handler_free(&priv->ctrls);
 	max9286_v4l2_notifier_unregister(priv);
@@ -1126,6 +1085,7 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
 
 static void max9286_v4l2_unregister(struct max9286_priv *priv)
 {
+	v4l2_subdev_cleanup(&priv->sd);
 	v4l2_ctrl_handler_free(&priv->ctrls);
 	v4l2_async_unregister_subdev(&priv->sd);
 	max9286_v4l2_notifier_unregister(priv);
@@ -1629,8 +1589,6 @@ static int max9286_probe(struct i2c_client *client)
 	if (!priv)
 		return -ENOMEM;
 
-	mutex_init(&priv->mutex);
-
 	priv->client = client;
 
 	/* GPIO values default to high */
-- 
2.44.0

Re: [PATCH v2 10/11] media: max9286: Use the subdev active state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:38PM +0200, Jacopo Mondi wrote:
> Use the subdev active state in the max9286 driver to store the
> image format.
> 
> Replace the .open() function call with the .init_state() one and
> simplify the set/get_pad_fmt() operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 130 ++++++++++++------------------------
>  1 file changed, 44 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 5321238cad60..7fad190cd9b3 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -19,7 +19,6 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/module.h>
> -#include <linux/mutex.h>
>  #include <linux/of_graph.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> @@ -198,12 +197,8 @@ struct max9286_priv {
>  	struct v4l2_ctrl *pixelrate_ctrl;
>  	unsigned int pixelrate;
>  
> -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
>  	struct v4l2_fract interval;
>  
> -	/* Protects controls and fmt structures */
> -	struct mutex mutex;
> -
>  	unsigned int nsources;
>  	unsigned int source_mask;
>  	unsigned int route_mask;
> @@ -788,19 +783,22 @@ static void max9286_v4l2_notifier_unregister(struct max9286_priv *priv)
>  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
> +	struct v4l2_subdev_state *state;
>  	struct max9286_source *source;
>  	unsigned int i;
>  	bool sync = false;
> -	int ret;
> +	int ret = 0;
> +
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
>  
>  	if (enable) {
>  		const struct v4l2_mbus_framefmt *format;
>  
>  		/*
> -		 * Get the format from the first used sink pad, as all sink
> -		 * formats must be identical.
> +		 * Get the format from the source pad, as all formats must be
> +		 * identical.
>  		 */
> -		format = &priv->fmt[__ffs(priv->bound_sources)];
> +		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
>  
>  		max9286_set_video_format(priv, format);
>  		max9286_set_fsync_period(priv);
> @@ -816,12 +814,12 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		for_each_source(priv, source) {
>  			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
>  			if (ret)
> -				return ret;
> +				goto unlock;
>  		}
>  
>  		ret = max9286_check_video_links(priv);
>  		if (ret)
> -			return ret;
> +			goto unlock;
>  
>  		/*
>  		 * Wait until frame synchronization is locked.
> @@ -842,7 +840,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		if (!sync) {
>  			dev_err(&priv->client->dev,
>  				"Failed to get frame synchronization\n");
> -			return -EXDEV; /* Invalid cross-device link */
> +			ret = -EXDEV; /* Invalid cross-device link */
> +			goto unlock;
>  		}
>  
>  		/*
> @@ -865,7 +864,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		max9286_i2c_mux_close(priv);
>  	}
>  
> -	return 0;
> +unlock:
> +	v4l2_subdev_unlock_state(state);
> +
> +	return ret;
>  }
>  
>  static int max9286_get_frame_interval(struct v4l2_subdev *sd,
> @@ -922,31 +924,20 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> -static struct v4l2_mbus_framefmt *
> -max9286_get_pad_format(struct max9286_priv *priv,
> -		       struct v4l2_subdev_state *sd_state,
> -		       unsigned int pad, u32 which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_state_get_format(sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &priv->fmt[pad];
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int max9286_set_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> +			   struct v4l2_subdev_state *state,
>  			   struct v4l2_subdev_format *format)
>  {
>  	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> +	unsigned int pad;
>  	unsigned int i;
>  
> +	/*
> +	 * Disable setting format on the source pad: format is propagated
> +	 * from the sinks.
> +	 */
>  	if (format->pad == MAX9286_SRC_PAD)
> -		return -EINVAL;
> +		return v4l2_subdev_get_fmt(sd, state, format);
>  
>  	/* Validate the format. */
>  	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> @@ -957,42 +948,16 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
>  	if (i == ARRAY_SIZE(max9286_formats))
>  		format->format.code = max9286_formats[0].code;
>  
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> -					 format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> -
> -	mutex_lock(&priv->mutex);
> -	*cfg_fmt = format->format;
> -	mutex_unlock(&priv->mutex);
> -
> -	return 0;
> -}
> -
> -static int max9286_get_fmt(struct v4l2_subdev *sd,
> -			   struct v4l2_subdev_state *sd_state,
> -			   struct v4l2_subdev_format *format)
> -{
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -	struct v4l2_mbus_framefmt *cfg_fmt;
> -	unsigned int pad = format->pad;
> +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
>  
>  	/*
> -	 * Multiplexed Stream Support: Support link validation by returning the
> -	 * format of the first bound link. All links must have the same format,
> -	 * as we do not support mixing and matching of cameras connected to the
> -	 * max9286.
> +	 * Apply the same format on the source pad. As all links must have the
> +	 * same format we do so only when the first source format is set.
>  	 */
> -	if (pad == MAX9286_SRC_PAD)
> -		pad = __ffs(priv->bound_sources);
> -
> -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> -	if (!cfg_fmt)
> -		return -EINVAL;
> -
> -	mutex_lock(&priv->mutex);
> -	format->format = *cfg_fmt;
> -	mutex_unlock(&priv->mutex);
> +	pad = __ffs(priv->bound_sources);
> +	if (pad == format->pad)
> +		*v4l2_subdev_state_get_format(state,
> +					      MAX9286_SRC_PAD) = format->format;

In wrote in my review of the previous version:

We don't enforce the requirement that formats on all sink pads must be
identical. Should we fix that ? One way to do so would be to propagate
formats to all the other sinks, in addition to propagating it to the
source.

I haven't seen a reply, maybe you missed the comment.

>  
>  	return 0;
>  }
> @@ -1003,7 +968,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
>  
>  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
>  	.enum_mbus_code = max9286_enum_mbus_code,
> -	.get_fmt	= max9286_get_fmt,
> +	.get_fmt	= v4l2_subdev_get_fmt,
>  	.set_fmt	= max9286_set_fmt,
>  	.get_frame_interval = max9286_get_frame_interval,
>  	.set_frame_interval = max9286_set_frame_interval,
> @@ -1025,26 +990,17 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
>  	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
>  };
>  
> -static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
> +static int max9286_init_state(struct v4l2_subdev *sd,
> +			      struct v4l2_subdev_state *state)
>  {
> -	*fmt = max9286_default_format;
> -}
> -
> -static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> -{
> -	struct v4l2_mbus_framefmt *format;
> -	unsigned int i;
> -
> -	for (i = 0; i < MAX9286_N_SINKS; i++) {
> -		format = v4l2_subdev_state_get_format(fh->state, i);
> -		max9286_init_format(format);
> -	}
> +	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
> +		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
>  
>  	return 0;
>  }
>  
>  static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> -	.open = max9286_open,
> +	.init_state = max9286_init_state,
>  };
>  
>  static const struct media_entity_operations max9286_media_ops = {
> @@ -1079,10 +1035,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	}
>  
>  	/* Configure V4L2 for the MAX9286 itself */
> -
> -	for (i = 0; i < MAX9286_N_SINKS; i++)
> -		max9286_init_format(&priv->fmt[i]);
> -
>  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
>  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
>  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -1109,14 +1061,21 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  	if (ret)
>  		goto err_async;
>  
> +	priv->sd.state_lock = priv->ctrls.lock;
> +	ret = v4l2_subdev_init_finalize(&priv->sd);
> +	if (ret)
> +		goto err_async;
> +
>  	ret = v4l2_async_register_subdev(&priv->sd);
>  	if (ret < 0) {
>  		dev_err(dev, "Unable to register subdevice\n");
> -		goto err_async;
> +		goto err_subdev;
>  	}
>  
>  	return 0;
>  
> +err_subdev:
> +	v4l2_subdev_cleanup(&priv->sd);
>  err_async:
>  	v4l2_ctrl_handler_free(&priv->ctrls);
>  	max9286_v4l2_notifier_unregister(priv);
> @@ -1126,6 +1085,7 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
>  
>  static void max9286_v4l2_unregister(struct max9286_priv *priv)
>  {
> +	v4l2_subdev_cleanup(&priv->sd);
>  	v4l2_ctrl_handler_free(&priv->ctrls);
>  	v4l2_async_unregister_subdev(&priv->sd);
>  	max9286_v4l2_notifier_unregister(priv);
> @@ -1629,8 +1589,6 @@ static int max9286_probe(struct i2c_client *client)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	mutex_init(&priv->mutex);
> -
>  	priv->client = client;
>  
>  	/* GPIO values default to high */

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 10/11] media: max9286: Use the subdev active state

[Jacopo Mondi]

Hi Laurent

On Thu, May 09, 2024 at 05:35:56PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, May 06, 2024 at 06:49:38PM +0200, Jacopo Mondi wrote:
> > Use the subdev active state in the max9286 driver to store the
> > image format.
> >
> > Replace the .open() function call with the .init_state() one and
> > simplify the set/get_pad_fmt() operations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 130 ++++++++++++------------------------
> >  1 file changed, 44 insertions(+), 86 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 5321238cad60..7fad190cd9b3 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -19,7 +19,6 @@
> >  #include <linux/i2c.h>
> >  #include <linux/i2c-mux.h>
> >  #include <linux/module.h>
> > -#include <linux/mutex.h>
> >  #include <linux/of_graph.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> > @@ -198,12 +197,8 @@ struct max9286_priv {
> >  	struct v4l2_ctrl *pixelrate_ctrl;
> >  	unsigned int pixelrate;
> >
> > -	struct v4l2_mbus_framefmt fmt[MAX9286_N_SINKS];
> >  	struct v4l2_fract interval;
> >
> > -	/* Protects controls and fmt structures */
> > -	struct mutex mutex;
> > -
> >  	unsigned int nsources;
> >  	unsigned int source_mask;
> >  	unsigned int route_mask;
> > @@ -788,19 +783,22 @@ static void max9286_v4l2_notifier_unregister(struct max9286_priv *priv)
> >  static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  {
> >  	struct max9286_priv *priv = sd_to_max9286(sd);
> > +	struct v4l2_subdev_state *state;
> >  	struct max9286_source *source;
> >  	unsigned int i;
> >  	bool sync = false;
> > -	int ret;
> > +	int ret = 0;
> > +
> > +	state = v4l2_subdev_lock_and_get_active_state(sd);
> >
> >  	if (enable) {
> >  		const struct v4l2_mbus_framefmt *format;
> >
> >  		/*
> > -		 * Get the format from the first used sink pad, as all sink
> > -		 * formats must be identical.
> > +		 * Get the format from the source pad, as all formats must be
> > +		 * identical.
> >  		 */
> > -		format = &priv->fmt[__ffs(priv->bound_sources)];
> > +		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
> >
> >  		max9286_set_video_format(priv, format);
> >  		max9286_set_fsync_period(priv);
> > @@ -816,12 +814,12 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  		for_each_source(priv, source) {
> >  			ret = v4l2_subdev_call(source->sd, video, s_stream, 1);
> >  			if (ret)
> > -				return ret;
> > +				goto unlock;
> >  		}
> >
> >  		ret = max9286_check_video_links(priv);
> >  		if (ret)
> > -			return ret;
> > +			goto unlock;
> >
> >  		/*
> >  		 * Wait until frame synchronization is locked.
> > @@ -842,7 +840,8 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  		if (!sync) {
> >  			dev_err(&priv->client->dev,
> >  				"Failed to get frame synchronization\n");
> > -			return -EXDEV; /* Invalid cross-device link */
> > +			ret = -EXDEV; /* Invalid cross-device link */
> > +			goto unlock;
> >  		}
> >
> >  		/*
> > @@ -865,7 +864,10 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  		max9286_i2c_mux_close(priv);
> >  	}
> >
> > -	return 0;
> > +unlock:
> > +	v4l2_subdev_unlock_state(state);
> > +
> > +	return ret;
> >  }
> >
> >  static int max9286_get_frame_interval(struct v4l2_subdev *sd,
> > @@ -922,31 +924,20 @@ static int max9286_enum_mbus_code(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > -static struct v4l2_mbus_framefmt *
> > -max9286_get_pad_format(struct max9286_priv *priv,
> > -		       struct v4l2_subdev_state *sd_state,
> > -		       unsigned int pad, u32 which)
> > -{
> > -	switch (which) {
> > -	case V4L2_SUBDEV_FORMAT_TRY:
> > -		return v4l2_subdev_state_get_format(sd_state, pad);
> > -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > -		return &priv->fmt[pad];
> > -	default:
> > -		return NULL;
> > -	}
> > -}
> > -
> >  static int max9286_set_fmt(struct v4l2_subdev *sd,
> > -			   struct v4l2_subdev_state *sd_state,
> > +			   struct v4l2_subdev_state *state,
> >  			   struct v4l2_subdev_format *format)
> >  {
> >  	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > +	unsigned int pad;
> >  	unsigned int i;
> >
> > +	/*
> > +	 * Disable setting format on the source pad: format is propagated
> > +	 * from the sinks.
> > +	 */
> >  	if (format->pad == MAX9286_SRC_PAD)
> > -		return -EINVAL;
> > +		return v4l2_subdev_get_fmt(sd, state, format);
> >
> >  	/* Validate the format. */
> >  	for (i = 0; i < ARRAY_SIZE(max9286_formats); ++i) {
> > @@ -957,42 +948,16 @@ static int max9286_set_fmt(struct v4l2_subdev *sd,
> >  	if (i == ARRAY_SIZE(max9286_formats))
> >  		format->format.code = max9286_formats[0].code;
> >
> > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, format->pad,
> > -					 format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > -
> > -	mutex_lock(&priv->mutex);
> > -	*cfg_fmt = format->format;
> > -	mutex_unlock(&priv->mutex);
> > -
> > -	return 0;
> > -}
> > -
> > -static int max9286_get_fmt(struct v4l2_subdev *sd,
> > -			   struct v4l2_subdev_state *sd_state,
> > -			   struct v4l2_subdev_format *format)
> > -{
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -	struct v4l2_mbus_framefmt *cfg_fmt;
> > -	unsigned int pad = format->pad;
> > +	*v4l2_subdev_state_get_format(state, format->pad) = format->format;
> >
> >  	/*
> > -	 * Multiplexed Stream Support: Support link validation by returning the
> > -	 * format of the first bound link. All links must have the same format,
> > -	 * as we do not support mixing and matching of cameras connected to the
> > -	 * max9286.
> > +	 * Apply the same format on the source pad. As all links must have the
> > +	 * same format we do so only when the first source format is set.
> >  	 */
> > -	if (pad == MAX9286_SRC_PAD)
> > -		pad = __ffs(priv->bound_sources);
> > -
> > -	cfg_fmt = max9286_get_pad_format(priv, sd_state, pad, format->which);
> > -	if (!cfg_fmt)
> > -		return -EINVAL;
> > -
> > -	mutex_lock(&priv->mutex);
> > -	format->format = *cfg_fmt;
> > -	mutex_unlock(&priv->mutex);
> > +	pad = __ffs(priv->bound_sources);
> > +	if (pad == format->pad)
> > +		*v4l2_subdev_state_get_format(state,
> > +					      MAX9286_SRC_PAD) = format->format;
>
> In wrote in my review of the previous version:
>
> We don't enforce the requirement that formats on all sink pads must be
> identical. Should we fix that ? One way to do so would be to propagate
> formats to all the other sinks, in addition to propagating it to the
> source.
>
> I haven't seen a reply, maybe you missed the comment.
>

Sorry, missed that.

I can do it.

> >
> >  	return 0;
> >  }
> > @@ -1003,7 +968,7 @@ static const struct v4l2_subdev_video_ops max9286_video_ops = {
> >
> >  static const struct v4l2_subdev_pad_ops max9286_pad_ops = {
> >  	.enum_mbus_code = max9286_enum_mbus_code,
> > -	.get_fmt	= max9286_get_fmt,
> > +	.get_fmt	= v4l2_subdev_get_fmt,
> >  	.set_fmt	= max9286_set_fmt,
> >  	.get_frame_interval = max9286_get_frame_interval,
> >  	.set_frame_interval = max9286_set_frame_interval,
> > @@ -1025,26 +990,17 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
> >  	.xfer_func	= V4L2_XFER_FUNC_DEFAULT,
> >  };
> >
> > -static void max9286_init_format(struct v4l2_mbus_framefmt *fmt)
> > +static int max9286_init_state(struct v4l2_subdev *sd,
> > +			      struct v4l2_subdev_state *state)
> >  {
> > -	*fmt = max9286_default_format;
> > -}
> > -
> > -static int max9286_open(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh)
> > -{
> > -	struct v4l2_mbus_framefmt *format;
> > -	unsigned int i;
> > -
> > -	for (i = 0; i < MAX9286_N_SINKS; i++) {
> > -		format = v4l2_subdev_state_get_format(fh->state, i);
> > -		max9286_init_format(format);
> > -	}
> > +	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
> > +		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
> >
> >  	return 0;
> >  }
> >
> >  static const struct v4l2_subdev_internal_ops max9286_subdev_internal_ops = {
> > -	.open = max9286_open,
> > +	.init_state = max9286_init_state,
> >  };
> >
> >  static const struct media_entity_operations max9286_media_ops = {
> > @@ -1079,10 +1035,6 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >  	}
> >
> >  	/* Configure V4L2 for the MAX9286 itself */
> > -
> > -	for (i = 0; i < MAX9286_N_SINKS; i++)
> > -		max9286_init_format(&priv->fmt[i]);
> > -
> >  	v4l2_i2c_subdev_init(&priv->sd, priv->client, &max9286_subdev_ops);
> >  	priv->sd.internal_ops = &max9286_subdev_internal_ops;
> >  	priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > @@ -1109,14 +1061,21 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >  	if (ret)
> >  		goto err_async;
> >
> > +	priv->sd.state_lock = priv->ctrls.lock;
> > +	ret = v4l2_subdev_init_finalize(&priv->sd);
> > +	if (ret)
> > +		goto err_async;
> > +
> >  	ret = v4l2_async_register_subdev(&priv->sd);
> >  	if (ret < 0) {
> >  		dev_err(dev, "Unable to register subdevice\n");
> > -		goto err_async;
> > +		goto err_subdev;
> >  	}
> >
> >  	return 0;
> >
> > +err_subdev:
> > +	v4l2_subdev_cleanup(&priv->sd);
> >  err_async:
> >  	v4l2_ctrl_handler_free(&priv->ctrls);
> >  	max9286_v4l2_notifier_unregister(priv);
> > @@ -1126,6 +1085,7 @@ static int max9286_v4l2_register(struct max9286_priv *priv)
> >
> >  static void max9286_v4l2_unregister(struct max9286_priv *priv)
> >  {
> > +	v4l2_subdev_cleanup(&priv->sd);
> >  	v4l2_ctrl_handler_free(&priv->ctrls);
> >  	v4l2_async_unregister_subdev(&priv->sd);
> >  	max9286_v4l2_notifier_unregister(priv);
> > @@ -1629,8 +1589,6 @@ static int max9286_probe(struct i2c_client *client)
> >  	if (!priv)
> >  		return -ENOMEM;
> >
> > -	mutex_init(&priv->mutex);
> > -
> >  	priv->client = client;
> >
> >  	/* GPIO values default to high */
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH v2 11/11] media: max9286: Use frame interval from subdev state

[Jacopo Mondi]

Use the frame interval stored in the subdev state instead of storing
a copy in the driver private structure.

Initialize the frame interval to the special case 0/0 that in the
max9286 driver represents automatic handling of frame sync.

During the startup phase, configure register 0x01 to use automatic
frame sync, to match the subdev state initialiation, instead of calling
max9286_set_fsync_period() which now requires a 'state' argument.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/max9286.c | 59 +++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
index 7fad190cd9b3..6930a98c8965 100644
--- a/drivers/media/i2c/max9286.c
+++ b/drivers/media/i2c/max9286.c
@@ -197,8 +197,6 @@ struct max9286_priv {
 	struct v4l2_ctrl *pixelrate_ctrl;
 	unsigned int pixelrate;
 
-	struct v4l2_fract interval;
-
 	unsigned int nsources;
 	unsigned int source_mask;
 	unsigned int route_mask;
@@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv,
 		      MAX9286_INVVS | MAX9286_HVSRC_D14);
 }
 
-static void max9286_set_fsync_period(struct max9286_priv *priv)
+static void max9286_set_fsync_period(struct max9286_priv *priv,
+				     struct v4l2_subdev_state *state)
 {
+	const struct v4l2_fract *interval;
 	u32 fsync;
 
-	if (!priv->interval.numerator || !priv->interval.denominator) {
+	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
+	if (!interval->numerator || !interval->denominator) {
 		/*
 		 * Special case, a null interval enables automatic FRAMESYNC
 		 * mode. FRAMESYNC is taken from the slowest link.
@@ -591,8 +592,8 @@ static void max9286_set_fsync_period(struct max9286_priv *priv)
 	 * The FRAMESYNC generator is configured with a period expressed as a
 	 * number of PCLK periods.
 	 */
-	fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator,
-			priv->interval.denominator);
+	fsync = div_u64((u64)priv->pixelrate * interval->numerator,
+			interval->denominator);
 
 	dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync,
 		priv->pixelrate);
@@ -801,7 +802,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
 		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
 
 		max9286_set_video_format(priv, format);
-		max9286_set_fsync_period(priv);
+		max9286_set_fsync_period(priv, state);
 
 		/*
 		 * The frame sync between cameras is transmitted across the
@@ -874,19 +875,11 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd,
 				      struct v4l2_subdev_state *sd_state,
 				      struct v4l2_subdev_frame_interval *interval)
 {
-	struct max9286_priv *priv = sd_to_max9286(sd);
-
-	/*
-	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
-	 * subdev active state API.
-	 */
-	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
 	if (interval->pad != MAX9286_SRC_PAD)
 		return -EINVAL;
 
-	interval->interval = priv->interval;
+	interval->interval = *v4l2_subdev_state_get_interval(sd_state,
+							     interval->pad);
 
 	return 0;
 }
@@ -895,19 +888,11 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd,
 				      struct v4l2_subdev_state *sd_state,
 				      struct v4l2_subdev_frame_interval *interval)
 {
-	struct max9286_priv *priv = sd_to_max9286(sd);
-
-	/*
-	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
-	 * subdev active state API.
-	 */
-	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
 	if (interval->pad != MAX9286_SRC_PAD)
 		return -EINVAL;
 
-	priv->interval = interval->interval;
+	*v4l2_subdev_state_get_interval(sd_state,
+					interval->pad) = interval->interval;
 
 	return 0;
 }
@@ -993,9 +978,21 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
 static int max9286_init_state(struct v4l2_subdev *sd,
 			      struct v4l2_subdev_state *state)
 {
+	struct v4l2_fract *interval;
+
 	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
 		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
 
+	/*
+	 * Special case: a null interval enables automatic FRAMESYNC mode.
+	 *
+	 * FRAMESYNC is taken from the slowest link. See register 0x01
+	 * configuration.
+	 */
+	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
+	interval->numerator = 0;
+	interval->denominator = 0;
+
 	return 0;
 }
 
@@ -1142,7 +1139,13 @@ static int max9286_setup(struct max9286_priv *priv)
 	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
 
 	max9286_set_video_format(priv, &max9286_default_format);
-	max9286_set_fsync_period(priv);
+
+	/*
+	 * Use automatic FRAMESYNC mode. FRAMESYNC is taken from the slowest
+	 * link.
+	 */
+	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
+				  MAX9286_FSYNCMETH_AUTO);
 
 	cfg = max9286_read(priv, 0x1c);
 	if (cfg < 0)
-- 
2.44.0

Re: [PATCH v2 11/11] media: max9286: Use frame interval from subdev state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, May 06, 2024 at 06:49:39PM +0200, Jacopo Mondi wrote:
> Use the frame interval stored in the subdev state instead of storing
> a copy in the driver private structure.
> 
> Initialize the frame interval to the special case 0/0 that in the
> max9286 driver represents automatic handling of frame sync.
> 
> During the startup phase, configure register 0x01 to use automatic
> frame sync, to match the subdev state initialiation, instead of calling
> max9286_set_fsync_period() which now requires a 'state' argument.

Given that max9286_set_fsync_period() will be called at stream start
time, is there a need to set the register in max9286_setup() ? If so,

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

If not, you can drop it.

> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/max9286.c | 59 +++++++++++++++++++------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> index 7fad190cd9b3..6930a98c8965 100644
> --- a/drivers/media/i2c/max9286.c
> +++ b/drivers/media/i2c/max9286.c
> @@ -197,8 +197,6 @@ struct max9286_priv {
>  	struct v4l2_ctrl *pixelrate_ctrl;
>  	unsigned int pixelrate;
>  
> -	struct v4l2_fract interval;
> -
>  	unsigned int nsources;
>  	unsigned int source_mask;
>  	unsigned int route_mask;
> @@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv,
>  		      MAX9286_INVVS | MAX9286_HVSRC_D14);
>  }
>  
> -static void max9286_set_fsync_period(struct max9286_priv *priv)
> +static void max9286_set_fsync_period(struct max9286_priv *priv,
> +				     struct v4l2_subdev_state *state)

Depending on whether this series or "media: v4l2-subdev: Provide
const-aware subdev state accessors" gets merged first, this argument
could be const :-) No need to address that for now and add a dependency.

>  {
> +	const struct v4l2_fract *interval;
>  	u32 fsync;
>  
> -	if (!priv->interval.numerator || !priv->interval.denominator) {
> +	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
> +	if (!interval->numerator || !interval->denominator) {
>  		/*
>  		 * Special case, a null interval enables automatic FRAMESYNC
>  		 * mode. FRAMESYNC is taken from the slowest link.
> @@ -591,8 +592,8 @@ static void max9286_set_fsync_period(struct max9286_priv *priv)
>  	 * The FRAMESYNC generator is configured with a period expressed as a
>  	 * number of PCLK periods.
>  	 */
> -	fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator,
> -			priv->interval.denominator);
> +	fsync = div_u64((u64)priv->pixelrate * interval->numerator,
> +			interval->denominator);
>  
>  	dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync,
>  		priv->pixelrate);
> @@ -801,7 +802,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
>  		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
>  
>  		max9286_set_video_format(priv, format);
> -		max9286_set_fsync_period(priv);
> +		max9286_set_fsync_period(priv, state);
>  
>  		/*
>  		 * The frame sync between cameras is transmitted across the
> @@ -874,19 +875,11 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_state *sd_state,
>  				      struct v4l2_subdev_frame_interval *interval)
>  {
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -
> -	/*
> -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> -	 * subdev active state API.
> -	 */
> -	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return -EINVAL;
> -
>  	if (interval->pad != MAX9286_SRC_PAD)
>  		return -EINVAL;
>  
> -	interval->interval = priv->interval;
> +	interval->interval = *v4l2_subdev_state_get_interval(sd_state,
> +							     interval->pad);
>  
>  	return 0;
>  }
> @@ -895,19 +888,11 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd,
>  				      struct v4l2_subdev_state *sd_state,
>  				      struct v4l2_subdev_frame_interval *interval)
>  {
> -	struct max9286_priv *priv = sd_to_max9286(sd);
> -
> -	/*
> -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> -	 * subdev active state API.
> -	 */
> -	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -		return -EINVAL;
> -
>  	if (interval->pad != MAX9286_SRC_PAD)
>  		return -EINVAL;
>  
> -	priv->interval = interval->interval;
> +	*v4l2_subdev_state_get_interval(sd_state,
> +					interval->pad) = interval->interval;
>  
>  	return 0;
>  }
> @@ -993,9 +978,21 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
>  static int max9286_init_state(struct v4l2_subdev *sd,
>  			      struct v4l2_subdev_state *state)
>  {
> +	struct v4l2_fract *interval;
> +
>  	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
>  		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
>  
> +	/*
> +	 * Special case: a null interval enables automatic FRAMESYNC mode.
> +	 *
> +	 * FRAMESYNC is taken from the slowest link. See register 0x01
> +	 * configuration.
> +	 */
> +	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
> +	interval->numerator = 0;
> +	interval->denominator = 0;
> +
>  	return 0;
>  }
>  
> @@ -1142,7 +1139,13 @@ static int max9286_setup(struct max9286_priv *priv)
>  	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
>  
>  	max9286_set_video_format(priv, &max9286_default_format);
> -	max9286_set_fsync_period(priv);
> +
> +	/*
> +	 * Use automatic FRAMESYNC mode. FRAMESYNC is taken from the slowest
> +	 * link.
> +	 */
> +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> +				  MAX9286_FSYNCMETH_AUTO);
>  
>  	cfg = max9286_read(priv, 0x1c);
>  	if (cfg < 0)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 11/11] media: max9286: Use frame interval from subdev state

[Jacopo Mondi]

Hi Laurent

On Thu, May 09, 2024 at 03:51:22PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, May 06, 2024 at 06:49:39PM +0200, Jacopo Mondi wrote:
> > Use the frame interval stored in the subdev state instead of storing
> > a copy in the driver private structure.
> >
> > Initialize the frame interval to the special case 0/0 that in the
> > max9286 driver represents automatic handling of frame sync.
> >
> > During the startup phase, configure register 0x01 to use automatic
> > frame sync, to match the subdev state initialiation, instead of calling
> > max9286_set_fsync_period() which now requires a 'state' argument.
>
> Given that max9286_set_fsync_period() will be called at stream start
> time, is there a need to set the register in max9286_setup() ? If so,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> If not, you can drop it.
>

Ok, I removed it initially then re-introduced in v2

> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/max9286.c | 59 +++++++++++++++++++------------------
> >  1 file changed, 31 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > index 7fad190cd9b3..6930a98c8965 100644
> > --- a/drivers/media/i2c/max9286.c
> > +++ b/drivers/media/i2c/max9286.c
> > @@ -197,8 +197,6 @@ struct max9286_priv {
> >  	struct v4l2_ctrl *pixelrate_ctrl;
> >  	unsigned int pixelrate;
> >
> > -	struct v4l2_fract interval;
> > -
> >  	unsigned int nsources;
> >  	unsigned int source_mask;
> >  	unsigned int route_mask;
> > @@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv,
> >  		      MAX9286_INVVS | MAX9286_HVSRC_D14);
> >  }
> >
> > -static void max9286_set_fsync_period(struct max9286_priv *priv)
> > +static void max9286_set_fsync_period(struct max9286_priv *priv,
> > +				     struct v4l2_subdev_state *state)
>
> Depending on whether this series or "media: v4l2-subdev: Provide
> const-aware subdev state accessors" gets merged first, this argument
> could be const :-) No need to address that for now and add a dependency.
>
> >  {
> > +	const struct v4l2_fract *interval;
> >  	u32 fsync;
> >
> > -	if (!priv->interval.numerator || !priv->interval.denominator) {
> > +	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
> > +	if (!interval->numerator || !interval->denominator) {
> >  		/*
> >  		 * Special case, a null interval enables automatic FRAMESYNC
> >  		 * mode. FRAMESYNC is taken from the slowest link.
> > @@ -591,8 +592,8 @@ static void max9286_set_fsync_period(struct max9286_priv *priv)
> >  	 * The FRAMESYNC generator is configured with a period expressed as a
> >  	 * number of PCLK periods.
> >  	 */
> > -	fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator,
> > -			priv->interval.denominator);
> > +	fsync = div_u64((u64)priv->pixelrate * interval->numerator,
> > +			interval->denominator);
> >
> >  	dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync,
> >  		priv->pixelrate);
> > @@ -801,7 +802,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> >  		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
> >
> >  		max9286_set_video_format(priv, format);
> > -		max9286_set_fsync_period(priv);
> > +		max9286_set_fsync_period(priv, state);
> >
> >  		/*
> >  		 * The frame sync between cameras is transmitted across the
> > @@ -874,19 +875,11 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd,
> >  				      struct v4l2_subdev_state *sd_state,
> >  				      struct v4l2_subdev_frame_interval *interval)
> >  {
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -
> > -	/*
> > -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> > -	 * subdev active state API.
> > -	 */
> > -	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		return -EINVAL;
> > -
> >  	if (interval->pad != MAX9286_SRC_PAD)
> >  		return -EINVAL;
> >
> > -	interval->interval = priv->interval;
> > +	interval->interval = *v4l2_subdev_state_get_interval(sd_state,
> > +							     interval->pad);
> >
> >  	return 0;
> >  }
> > @@ -895,19 +888,11 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd,
> >  				      struct v4l2_subdev_state *sd_state,
> >  				      struct v4l2_subdev_frame_interval *interval)
> >  {
> > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > -
> > -	/*
> > -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> > -	 * subdev active state API.
> > -	 */
> > -	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > -		return -EINVAL;
> > -
> >  	if (interval->pad != MAX9286_SRC_PAD)
> >  		return -EINVAL;
> >
> > -	priv->interval = interval->interval;
> > +	*v4l2_subdev_state_get_interval(sd_state,
> > +					interval->pad) = interval->interval;
> >
> >  	return 0;
> >  }
> > @@ -993,9 +978,21 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
> >  static int max9286_init_state(struct v4l2_subdev *sd,
> >  			      struct v4l2_subdev_state *state)
> >  {
> > +	struct v4l2_fract *interval;
> > +
> >  	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
> >  		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
> >
> > +	/*
> > +	 * Special case: a null interval enables automatic FRAMESYNC mode.
> > +	 *
> > +	 * FRAMESYNC is taken from the slowest link. See register 0x01
> > +	 * configuration.
> > +	 */
> > +	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
> > +	interval->numerator = 0;
> > +	interval->denominator = 0;
> > +
> >  	return 0;
> >  }
> >
> > @@ -1142,7 +1139,13 @@ static int max9286_setup(struct max9286_priv *priv)
> >  	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> >
> >  	max9286_set_video_format(priv, &max9286_default_format);
> > -	max9286_set_fsync_period(priv);
> > +
> > +	/*
> > +	 * Use automatic FRAMESYNC mode. FRAMESYNC is taken from the slowest
> > +	 * link.
> > +	 */
> > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > +				  MAX9286_FSYNCMETH_AUTO);
> >
> >  	cfg = max9286_read(priv, 0x1c);
> >  	if (cfg < 0)
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 11/11] media: max9286: Use frame interval from subdev state

[Laurent Pinchart]

Hi Jacopo,

On Thu, May 09, 2024 at 03:45:11PM +0200, Jacopo Mondi wrote:
> On Thu, May 09, 2024 at 03:51:22PM GMT, Laurent Pinchart wrote:
> > On Mon, May 06, 2024 at 06:49:39PM +0200, Jacopo Mondi wrote:
> > > Use the frame interval stored in the subdev state instead of storing
> > > a copy in the driver private structure.
> > >
> > > Initialize the frame interval to the special case 0/0 that in the
> > > max9286 driver represents automatic handling of frame sync.
> > >
> > > During the startup phase, configure register 0x01 to use automatic
> > > frame sync, to match the subdev state initialiation, instead of calling
> > > max9286_set_fsync_period() which now requires a 'state' argument.
> >
> > Given that max9286_set_fsync_period() will be called at stream start
> > time, is there a need to set the register in max9286_setup() ? If so,
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > If not, you can drop it.
> 
> Ok, I removed it initially then re-introduced in v2

If you drop it, please just record the rationale in the commit message.

> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/max9286.c | 59 +++++++++++++++++++------------------
> > >  1 file changed, 31 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > index 7fad190cd9b3..6930a98c8965 100644
> > > --- a/drivers/media/i2c/max9286.c
> > > +++ b/drivers/media/i2c/max9286.c
> > > @@ -197,8 +197,6 @@ struct max9286_priv {
> > >  	struct v4l2_ctrl *pixelrate_ctrl;
> > >  	unsigned int pixelrate;
> > >
> > > -	struct v4l2_fract interval;
> > > -
> > >  	unsigned int nsources;
> > >  	unsigned int source_mask;
> > >  	unsigned int route_mask;
> > > @@ -571,11 +569,14 @@ static void max9286_set_video_format(struct max9286_priv *priv,
> > >  		      MAX9286_INVVS | MAX9286_HVSRC_D14);
> > >  }
> > >
> > > -static void max9286_set_fsync_period(struct max9286_priv *priv)
> > > +static void max9286_set_fsync_period(struct max9286_priv *priv,
> > > +				     struct v4l2_subdev_state *state)
> >
> > Depending on whether this series or "media: v4l2-subdev: Provide
> > const-aware subdev state accessors" gets merged first, this argument
> > could be const :-) No need to address that for now and add a dependency.
> >
> > >  {
> > > +	const struct v4l2_fract *interval;
> > >  	u32 fsync;
> > >
> > > -	if (!priv->interval.numerator || !priv->interval.denominator) {
> > > +	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
> > > +	if (!interval->numerator || !interval->denominator) {
> > >  		/*
> > >  		 * Special case, a null interval enables automatic FRAMESYNC
> > >  		 * mode. FRAMESYNC is taken from the slowest link.
> > > @@ -591,8 +592,8 @@ static void max9286_set_fsync_period(struct max9286_priv *priv)
> > >  	 * The FRAMESYNC generator is configured with a period expressed as a
> > >  	 * number of PCLK periods.
> > >  	 */
> > > -	fsync = div_u64((u64)priv->pixelrate * priv->interval.numerator,
> > > -			priv->interval.denominator);
> > > +	fsync = div_u64((u64)priv->pixelrate * interval->numerator,
> > > +			interval->denominator);
> > >
> > >  	dev_dbg(&priv->client->dev, "fsync period %u (pclk %u)\n", fsync,
> > >  		priv->pixelrate);
> > > @@ -801,7 +802,7 @@ static int max9286_s_stream(struct v4l2_subdev *sd, int enable)
> > >  		format = v4l2_subdev_state_get_format(state, MAX9286_SRC_PAD);
> > >
> > >  		max9286_set_video_format(priv, format);
> > > -		max9286_set_fsync_period(priv);
> > > +		max9286_set_fsync_period(priv, state);
> > >
> > >  		/*
> > >  		 * The frame sync between cameras is transmitted across the
> > > @@ -874,19 +875,11 @@ static int max9286_get_frame_interval(struct v4l2_subdev *sd,
> > >  				      struct v4l2_subdev_state *sd_state,
> > >  				      struct v4l2_subdev_frame_interval *interval)
> > >  {
> > > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > > -
> > > -	/*
> > > -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> > > -	 * subdev active state API.
> > > -	 */
> > > -	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > -		return -EINVAL;
> > > -
> > >  	if (interval->pad != MAX9286_SRC_PAD)
> > >  		return -EINVAL;
> > >
> > > -	interval->interval = priv->interval;
> > > +	interval->interval = *v4l2_subdev_state_get_interval(sd_state,
> > > +							     interval->pad);
> > >
> > >  	return 0;
> > >  }
> > > @@ -895,19 +888,11 @@ static int max9286_set_frame_interval(struct v4l2_subdev *sd,
> > >  				      struct v4l2_subdev_state *sd_state,
> > >  				      struct v4l2_subdev_frame_interval *interval)
> > >  {
> > > -	struct max9286_priv *priv = sd_to_max9286(sd);
> > > -
> > > -	/*
> > > -	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> > > -	 * subdev active state API.
> > > -	 */
> > > -	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> > > -		return -EINVAL;
> > > -
> > >  	if (interval->pad != MAX9286_SRC_PAD)
> > >  		return -EINVAL;
> > >
> > > -	priv->interval = interval->interval;
> > > +	*v4l2_subdev_state_get_interval(sd_state,
> > > +					interval->pad) = interval->interval;
> > >
> > >  	return 0;
> > >  }
> > > @@ -993,9 +978,21 @@ static const struct v4l2_mbus_framefmt max9286_default_format = {
> > >  static int max9286_init_state(struct v4l2_subdev *sd,
> > >  			      struct v4l2_subdev_state *state)
> > >  {
> > > +	struct v4l2_fract *interval;
> > > +
> > >  	for (unsigned int i = 0; i < MAX9286_N_PADS; i++)
> > >  		*v4l2_subdev_state_get_format(state, i) = max9286_default_format;
> > >
> > > +	/*
> > > +	 * Special case: a null interval enables automatic FRAMESYNC mode.
> > > +	 *
> > > +	 * FRAMESYNC is taken from the slowest link. See register 0x01
> > > +	 * configuration.
> > > +	 */
> > > +	interval = v4l2_subdev_state_get_interval(state, MAX9286_SRC_PAD);
> > > +	interval->numerator = 0;
> > > +	interval->denominator = 0;
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -1142,7 +1139,13 @@ static int max9286_setup(struct max9286_priv *priv)
> > >  	max9286_write(priv, 0x69, (0xf & ~priv->route_mask));
> > >
> > >  	max9286_set_video_format(priv, &max9286_default_format);
> > > -	max9286_set_fsync_period(priv);
> > > +
> > > +	/*
> > > +	 * Use automatic FRAMESYNC mode. FRAMESYNC is taken from the slowest
> > > +	 * link.
> > > +	 */
> > > +	max9286_write(priv, 0x01, MAX9286_FSYNCMODE_INT_HIZ |
> > > +				  MAX9286_FSYNCMETH_AUTO);
> > >
> > >  	cfg = max9286_read(priv, 0x1c);
> > >  	if (cfg < 0)

-- 
Regards,

Laurent Pinchart