[RFC PATCH 00/12] media: ov5645: Add support for streams

[RFC PATCH 00/12] media: ov5645: Add support for streams

[Prabhakar]

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

Hi All,

This patch series aims to add the below features,
- Support subdev active state
- Support for streams
- Support for virtual channel
- Code cleanup

Sending this is as an RFC, I've done limited testing and Im seeing issue
when route is updated and wanted some feedback on the implementation.

When route is updated for the sensor with below command, and when start
streaming I get "Dangling sink streams: mask 0x1" error. Should the
corresponding bridge also need to support routes to fix this? or is it
something missing Ive missed in the current implementation.

$ media-ctl -R "'ov5645 0-003c' [1/0->0/1[1]]"

Output after route update:
- entity 4: ov5645 0-003c (2 pads, 1 link, 1 route)
            type V4L2 subdev subtype Sensor flags 0
            device node name /dev/v4l-subdev1
        routes:
                1/0 -> 0/1 [ACTIVE]
        pad0: SOURCE
                [stream:1 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb
                 crop:(0,0)/1920x1080]
                -> "csi-10830400.csi2":0 [ENABLED,IMMUTABLE]
        pad1: SINK,0x8
                [stream:0 fmt:UYVY8_1X16/2592x1944 field:none colorspace:srgb
                 crop:(0,0)/1920x1080]
              
v4l2-compliance log after this patch series:
--------------------------------------------
root@smarc-rzg2l:~# v4l2-compliance -u /dev/v4l-subdev1
v4l2-compliance 1.28.1-5233, 64 bits, 64-bit time_t
v4l2-compliance SHA: fc15e229d9d3 2024-07-23 19:22:15

Compliance test for device /dev/v4l-subdev1:

Driver Info:
        Driver version   : 6.11.0
        Capabilities     : 0x00000002
                Streams Support
        Client Capabilities:[   53.919041] ov5645 0-003c: =================  START STATUS  =================
 0x0000000000000[   53.927271] ov5645 0-003c: ==================  END STATUS  ==================
003
streams interval-uses-which
Required ioctls:
        test VIDIOC_SUDBEV_QUERYCAP: OK
        test invalid ioctls: OK

Allow for multiple opens:
        test second /dev/v4l-subdev1 open: OK
        test VIDIOC_SUBDEV_QUERYCAP: OK
        test for unlimited opens: OK

Debug ioctls:
        test VIDIOC_LOG_STATUS: OK (Not Supported)

Input ioctls:
        test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
        test VIDIOC_ENUMAUDIO: OK (Not Supported)
        test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDIO: OK (Not Supported)
        Inputs: 0 Audio Inputs: 0 Tuners: 0

Output ioctls:
        test VIDIOC_G/S_MODULATOR: OK (Not Supported)
        test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
        test VIDIOC_ENUMAUDOUT: OK (Not Supported)
        test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
        test VIDIOC_G/S_AUDOUT: OK (Not Supported)
        Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
        test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
        test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
        test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
        test VIDIOC_G/S_EDID: OK (Not Supported)

Sub-Device routing ioctls:
        test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK
        test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK

Control ioctls:
        test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
        test VIDIOC_QUERYCTRL: OK
        test VIDIOC_G/S_CTRL: OK
        test VIDIOC_G/S/TRY_EXT_CTRLS: OK
        test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
        test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
        Standard Controls: 12 Private Controls: 0

Format ioctls:
        test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
        test VIDIOC_G/S_PARM: OK (Not Supported)
        test VIDIOC_G_FBUF: OK (Not Supported)
        test VIDIOC_G_FMT: OK (Not Supported)
        test VIDIOC_TRY_FMT: OK (Not Supported)
        test VIDIOC_S_FMT: OK (Not Supported)
        test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
        test Cropping: OK (Not Supported)
        test Composing: OK (Not Supported)
        test Scaling: OK (Not Supported)

Codec ioctls:
        test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
        test VIDIOC_G_ENC_INDEX: OK (Not Supported)
        test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

Buffer ioctls:
        test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
        test CREATE_BUFS maximum buffers: OK
        test VIDIOC_REMOVE_BUFS: OK
        test VIDIOC_EXPBUF: OK (Not Supported)
        test Requests: OK (Not Supported)

Total for device /dev/v4l-subdev1: 47, Succeeded: 47, Failed: 0, Warnings: 0

Cheers,
Prabhakar

Lad Prabhakar (12):
  media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
  media: i2c: ov5645: Use local `dev` pointer for subdev device
    assignment
  media: i2c: ov5645: Enable runtime PM after
    v4l2_async_register_subdev()
  media: i2c: ov5645: Use dev_err_probe instead of dev_err
  media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()
  media: i2c: ov5645: Drop `power_lock` mutex
  media: i2c: ov5645: Use subdev active state
  media: i2c: ov5645: Switch to {enable,disable}_streams
  media: i2c: ov5645: Add internal image sink pad
  media: i2c: ov5645: Report internal routes to userspace
  media: i2c: ov5645: Report streams using frame descriptors
  media: i2c: ov5645: Add virtual channel support

 drivers/media/i2c/ov5645.c | 507 +++++++++++++++++++++++--------------
 1 file changed, 316 insertions(+), 191 deletions(-)

-- 
2.34.1

[RFC PATCH 01/12] media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks

[Prabhakar]

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

The OV5645 sensor exposes controls, so the V4L2_SUBDEV_FL_HAS_EVENTS flag
should be set and implement subscribe_event and unsubscribe_event hooks.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 019979f553b1..6eedd0310b02 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -29,6 +29,7 @@
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -1042,7 +1043,13 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.get_selection = ov5645_get_selection,
 };
 
+static const struct v4l2_subdev_core_ops ov5645_core_ops = {
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_ops ov5645_subdev_ops = {
+	.core = &ov5645_core_ops,
 	.video = &ov5645_video_ops,
 	.pad = &ov5645_subdev_pad_ops,
 };
@@ -1178,7 +1185,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
-	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ov5645->sd.dev = &client->dev;
 	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.34.1

[RFC PATCH 02/12] media: i2c: ov5645: Use local `dev` pointer for subdev device assignment

[Prabhakar]

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

While assigning the subdev device pointer, use the local `dev` pointer
which is already extracted from the `i2c_client` pointer.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 6eedd0310b02..ab3a419df2df 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1187,7 +1187,7 @@ static int ov5645_probe(struct i2c_client *client)
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
 	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
-	ov5645->sd.dev = &client->dev;
+	ov5645->sd.dev = dev;
 	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
 	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
-- 
2.34.1

[RFC PATCH 03/12] media: i2c: ov5645: Enable runtime PM after v4l2_async_register_subdev()

[Prabhakar]

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

To simplify the probe error path, enable runtime PM after the
v4l2_async_register_subdev() call.

This change ensures that runtime PM is only enabled once the subdevice
registration is successful, avoiding unnecessary cleanup in the error
path.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index ab3a419df2df..78b86438c798 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1239,18 +1239,17 @@ static int ov5645_probe(struct i2c_client *client)
 		goto power_down;
 	}
 
-	pm_runtime_set_active(dev);
-	pm_runtime_get_noresume(dev);
-	pm_runtime_enable(dev);
-
 	ov5645_init_state(&ov5645->sd, NULL);
 
 	ret = v4l2_async_register_subdev(&ov5645->sd);
 	if (ret < 0) {
 		dev_err(dev, "could not register v4l2 device\n");
-		goto err_pm_runtime;
+		goto power_down;
 	}
 
+	pm_runtime_set_active(dev);
+	pm_runtime_get_noresume(dev);
+	pm_runtime_enable(dev);
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_mark_last_busy(dev);
@@ -1258,9 +1257,6 @@ static int ov5645_probe(struct i2c_client *client)
 
 	return 0;
 
-err_pm_runtime:
-	pm_runtime_disable(dev);
-	pm_runtime_put_noidle(dev);
 power_down:
 	ov5645_set_power_off(dev);
 free_entity:
-- 
2.34.1

[RFC PATCH 04/12] media: i2c: ov5645: Use dev_err_probe instead of dev_err

[Prabhakar]

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

Drop dev_err() and use the dev_err_probe() helper on probe path.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 74 +++++++++++++++-----------------------
 1 file changed, 28 insertions(+), 46 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 78b86438c798..9e6ff1f1b9ac 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1076,51 +1076,37 @@ static int ov5645_probe(struct i2c_client *client)
 	ov5645->dev = dev;
 
 	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
-	if (!endpoint) {
-		dev_err(dev, "endpoint node not found\n");
-		return -EINVAL;
-	}
+	if (!endpoint)
+		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
 
 	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(endpoint),
 					 &ov5645->ep);
 
 	of_node_put(endpoint);
 
-	if (ret < 0) {
-		dev_err(dev, "parsing endpoint node failed\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "parsing endpoint node failed\n");
 
-	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
-		dev_err(dev, "invalid bus type, must be CSI2\n");
-		return -EINVAL;
-	}
+	if (ov5645->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
+		return dev_err_probe(dev, -EINVAL, "invalid bus type, must be CSI2\n");
 
 	/* get system clock (xclk) */
 	ov5645->xclk = devm_clk_get(dev, NULL);
-	if (IS_ERR(ov5645->xclk)) {
-		dev_err(dev, "could not get xclk");
-		return PTR_ERR(ov5645->xclk);
-	}
+	if (IS_ERR(ov5645->xclk))
+		return dev_err_probe(dev, PTR_ERR(ov5645->xclk), "could not get xclk");
 
 	ret = of_property_read_u32(dev->of_node, "clock-frequency", &xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not get xclk frequency\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "could not get xclk frequency\n");
 
 	/* external clock must be 24MHz, allow 1% tolerance */
-	if (xclk_freq < 23760000 || xclk_freq > 24240000) {
-		dev_err(dev, "external clock frequency %u is not supported\n",
-			xclk_freq);
-		return -EINVAL;
-	}
+	if (xclk_freq < 23760000 || xclk_freq > 24240000)
+		return dev_err_probe(dev, -EINVAL, "external clock frequency %u is not supported\n",
+				     xclk_freq);
 
 	ret = clk_set_rate(ov5645->xclk, xclk_freq);
-	if (ret) {
-		dev_err(dev, "could not set xclk frequency\n");
-		return ret;
-	}
+	if (ret)
+		return dev_err_probe(dev, ret, "could not set xclk frequency\n");
 
 	for (i = 0; i < OV5645_NUM_SUPPLIES; i++)
 		ov5645->supplies[i].supply = ov5645_supply_name[i];
@@ -1131,16 +1117,12 @@ static int ov5645_probe(struct i2c_client *client)
 		return ret;
 
 	ov5645->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_HIGH);
-	if (IS_ERR(ov5645->enable_gpio)) {
-		dev_err(dev, "cannot get enable gpio\n");
-		return PTR_ERR(ov5645->enable_gpio);
-	}
+	if (IS_ERR(ov5645->enable_gpio))
+		return dev_err_probe(dev, PTR_ERR(ov5645->enable_gpio), "cannot get enable gpio\n");
 
 	ov5645->rst_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(ov5645->rst_gpio)) {
-		dev_err(dev, "cannot get reset gpio\n");
-		return PTR_ERR(ov5645->rst_gpio);
-	}
+	if (IS_ERR(ov5645->rst_gpio))
+		return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
 
 	mutex_init(&ov5645->power_lock);
 
@@ -1177,9 +1159,9 @@ static int ov5645_probe(struct i2c_client *client)
 	ov5645->sd.ctrl_handler = &ov5645->ctrls;
 
 	if (ov5645->ctrls.error) {
-		dev_err(dev, "%s: control initialization error %d\n",
-		       __func__, ov5645->ctrls.error);
 		ret = ov5645->ctrls.error;
+		dev_err_probe(dev, ret, "%s: control initialization error %d\n",
+			      __func__, ov5645->ctrls.error);
 		goto free_ctrl;
 	}
 
@@ -1192,7 +1174,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
 	if (ret < 0) {
-		dev_err(dev, "could not register media entity\n");
+		dev_err_probe(dev, ret, "could not register media entity\n");
 		goto free_ctrl;
 	}
 
@@ -1202,14 +1184,14 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
 	if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
-		dev_err(dev, "could not read ID high\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read ID high\n");
 		goto power_down;
 	}
 	ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
 	if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
-		dev_err(dev, "could not read ID low\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read ID low\n");
 		goto power_down;
 	}
 
@@ -1218,24 +1200,24 @@ static int ov5645_probe(struct i2c_client *client)
 	ret = ov5645_read_reg(ov5645, OV5645_AEC_PK_MANUAL,
 			      &ov5645->aec_pk_manual);
 	if (ret < 0) {
-		dev_err(dev, "could not read AEC/AGC mode\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read AEC/AGC mode\n");
 		goto power_down;
 	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG20,
 			      &ov5645->timing_tc_reg20);
 	if (ret < 0) {
-		dev_err(dev, "could not read vflip value\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read vflip value\n");
 		goto power_down;
 	}
 
 	ret = ov5645_read_reg(ov5645, OV5645_TIMING_TC_REG21,
 			      &ov5645->timing_tc_reg21);
 	if (ret < 0) {
-		dev_err(dev, "could not read hflip value\n");
 		ret = -ENODEV;
+		dev_err_probe(dev, ret, "could not read hflip value\n");
 		goto power_down;
 	}
 
@@ -1243,7 +1225,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ret = v4l2_async_register_subdev(&ov5645->sd);
 	if (ret < 0) {
-		dev_err(dev, "could not register v4l2 device\n");
+		dev_err_probe(dev, ret, "could not register v4l2 device\n");
 		goto power_down;
 	}
 
-- 
2.34.1

[RFC PATCH 05/12] media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()

[Prabhakar]

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

Make use v4l2_async_register_subdev_sensor() helper to register
the subdev.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 9e6ff1f1b9ac..45687d004004 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1223,7 +1223,7 @@ static int ov5645_probe(struct i2c_client *client)
 
 	ov5645_init_state(&ov5645->sd, NULL);
 
-	ret = v4l2_async_register_subdev(&ov5645->sd);
+	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "could not register v4l2 device\n");
 		goto power_down;
-- 
2.34.1

[RFC PATCH 06/12] media: i2c: ov5645: Drop `power_lock` mutex

[Prabhakar]

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

Drop mutex while applying controls and just rely on
pm_runtime_get_if_in_use() call.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 45687d004004..25c60afcc0ec 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -106,8 +106,6 @@ struct ov5645 {
 	u8 timing_tc_reg20;
 	u8 timing_tc_reg21;
 
-	struct mutex power_lock; /* lock to protect power state */
-
 	struct gpio_desc *enable_gpio;
 	struct gpio_desc *rst_gpio;
 };
@@ -782,11 +780,8 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
 					     struct ov5645, ctrls);
 	int ret;
 
-	mutex_lock(&ov5645->power_lock);
-	if (!pm_runtime_get_if_in_use(ov5645->dev)) {
-		mutex_unlock(&ov5645->power_lock);
+	if (!pm_runtime_get_if_in_use(ov5645->dev))
 		return 0;
-	}
 
 	switch (ctrl->id) {
 	case V4L2_CID_SATURATION:
@@ -817,7 +812,6 @@ static int ov5645_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	pm_runtime_mark_last_busy(ov5645->dev);
 	pm_runtime_put_autosuspend(ov5645->dev);
-	mutex_unlock(&ov5645->power_lock);
 
 	return ret;
 }
@@ -1124,8 +1118,6 @@ static int ov5645_probe(struct i2c_client *client)
 	if (IS_ERR(ov5645->rst_gpio))
 		return dev_err_probe(dev, PTR_ERR(ov5645->rst_gpio), "cannot get reset gpio\n");
 
-	mutex_init(&ov5645->power_lock);
-
 	v4l2_ctrl_handler_init(&ov5645->ctrls, 9);
 	v4l2_ctrl_new_std(&ov5645->ctrls, &ov5645_ctrl_ops,
 			  V4L2_CID_SATURATION, -4, 4, 1, 0);
@@ -1245,7 +1237,6 @@ static int ov5645_probe(struct i2c_client *client)
 	media_entity_cleanup(&ov5645->sd.entity);
 free_ctrl:
 	v4l2_ctrl_handler_free(&ov5645->ctrls);
-	mutex_destroy(&ov5645->power_lock);
 
 	return ret;
 }
@@ -1262,7 +1253,6 @@ static void ov5645_remove(struct i2c_client *client)
 	if (!pm_runtime_status_suspended(ov5645->dev))
 		ov5645_set_power_off(ov5645->dev);
 	pm_runtime_set_suspended(ov5645->dev);
-	mutex_destroy(&ov5645->power_lock);
 }
 
 static const struct i2c_device_id ov5645_id[] = {
-- 
2.34.1

[RFC PATCH 07/12] media: i2c: ov5645: Use subdev active state

[Prabhakar]

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

Port the ov5645 sensor driver to use the subdev active state.

Move all the format configuration to the subdevice state and simplify
the format handling, locking and initialization.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 109 +++++++++++++------------------------
 1 file changed, 39 insertions(+), 70 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 25c60afcc0ec..9497ec737cb7 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -89,7 +89,6 @@ struct ov5645 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct v4l2_fwnode_endpoint ep;
-	struct v4l2_mbus_framefmt fmt;
 	struct v4l2_rect crop;
 	struct clk *xclk;
 
@@ -850,49 +849,6 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-__ov5645_get_pad_format(struct ov5645 *ov5645,
-			struct v4l2_subdev_state *sd_state,
-			unsigned int pad,
-			enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_format(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &ov5645->fmt;
-	default:
-		return NULL;
-	}
-}
-
-static int ov5645_get_format(struct v4l2_subdev *sd,
-			     struct v4l2_subdev_state *sd_state,
-			     struct v4l2_subdev_format *format)
-{
-	struct ov5645 *ov5645 = to_ov5645(sd);
-
-	format->format = *__ov5645_get_pad_format(ov5645, sd_state,
-						  format->pad,
-						  format->which);
-	return 0;
-}
-
-static struct v4l2_rect *
-__ov5645_get_pad_crop(struct ov5645 *ov5645,
-		      struct v4l2_subdev_state *sd_state,
-		      unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_state_get_crop(sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &ov5645->crop;
-	default:
-		return NULL;
-	}
-}
-
 static int ov5645_set_format(struct v4l2_subdev *sd,
 			     struct v4l2_subdev_state *sd_state,
 			     struct v4l2_subdev_format *format)
@@ -903,33 +859,30 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 	const struct ov5645_mode_info *new_mode;
 	int ret;
 
-	__crop = __ov5645_get_pad_crop(ov5645, sd_state, format->pad,
-				       format->which);
-
+	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
 	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
-			       ARRAY_SIZE(ov5645_mode_info_data),
-			       width, height,
-			       format->format.width, format->format.height);
+					  ARRAY_SIZE(ov5645_mode_info_data),
+					  width, height, format->format.width,
+					  format->format.height);
 
 	__crop->width = new_mode->width;
 	__crop->height = new_mode->height;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		ret = v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
-					     new_mode->pixel_clock);
+		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
+					       new_mode->pixel_clock);
 		if (ret < 0)
 			return ret;
 
-		ret = v4l2_ctrl_s_ctrl(ov5645->link_freq,
-				       new_mode->link_freq);
+		ret = __v4l2_ctrl_s_ctrl(ov5645->link_freq,
+					 new_mode->link_freq);
 		if (ret < 0)
 			return ret;
 
 		ov5645->current_mode = new_mode;
 	}
 
-	__format = __ov5645_get_pad_format(ov5645, sd_state, format->pad,
-					   format->which);
+	__format = v4l2_subdev_state_get_format(sd_state, 0);
 	__format->width = __crop->width;
 	__format->height = __crop->height;
 	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
@@ -944,11 +897,15 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 static int ov5645_init_state(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_state *sd_state)
 {
-	struct v4l2_subdev_format fmt = { 0 };
-
-	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
-	fmt.format.width = 1920;
-	fmt.format.height = 1080;
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_TRY,
+		.pad = 0,
+		.format = {
+			.code = MEDIA_BUS_FMT_UYVY8_1X16,
+			.width = ov5645_mode_info_data[1].width,
+			.height = ov5645_mode_info_data[1].height,
+		},
+	};
 
 	ov5645_set_format(subdev, sd_state, &fmt);
 
@@ -959,25 +916,27 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *sd_state,
 			   struct v4l2_subdev_selection *sel)
 {
-	struct ov5645 *ov5645 = to_ov5645(sd);
-
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	sel->r = *__ov5645_get_pad_crop(ov5645, sd_state, sel->pad,
-					sel->which);
+	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
 	return 0;
 }
 
 static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 {
 	struct ov5645 *ov5645 = to_ov5645(subdev);
+	struct v4l2_subdev_state *state;
 	int ret;
 
+	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
+
 	if (enable) {
 		ret = pm_runtime_resume_and_get(ov5645->dev);
-		if (ret < 0)
+		if (ret < 0) {
+			v4l2_subdev_unlock_state(state);
 			return ret;
+		}
 
 		ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
@@ -988,7 +947,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 				ov5645->current_mode->height);
 			goto err_rpm_put;
 		}
-		ret = v4l2_ctrl_handler_setup(&ov5645->ctrls);
+		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
 		if (ret < 0) {
 			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
 			goto err_rpm_put;
@@ -1013,6 +972,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 		goto stream_off_rpm_put;
 	}
 
+	v4l2_subdev_unlock_state(state);
 	return 0;
 
 err_rpm_put:
@@ -1022,6 +982,7 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
 stream_off_rpm_put:
 	pm_runtime_mark_last_busy(ov5645->dev);
 	pm_runtime_put_autosuspend(ov5645->dev);
+	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
@@ -1032,7 +993,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
 static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.enum_mbus_code = ov5645_enum_mbus_code,
 	.enum_frame_size = ov5645_enum_frame_size,
-	.get_fmt = ov5645_get_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov5645_set_format,
 	.get_selection = ov5645_get_selection,
 };
@@ -1213,12 +1174,17 @@ static int ov5645_probe(struct i2c_client *client)
 		goto power_down;
 	}
 
-	ov5645_init_state(&ov5645->sd, NULL);
+	ov5645->sd.state_lock = ov5645->ctrls.lock;
+	ret = v4l2_subdev_init_finalize(&ov5645->sd);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "subdev init error\n");
+		goto power_down;
+	}
 
 	ret = v4l2_async_register_subdev_sensor(&ov5645->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "could not register v4l2 device\n");
-		goto power_down;
+		goto error_subdev_cleanup;
 	}
 
 	pm_runtime_set_active(dev);
@@ -1231,6 +1197,8 @@ static int ov5645_probe(struct i2c_client *client)
 
 	return 0;
 
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&ov5645->sd);
 power_down:
 	ov5645_set_power_off(dev);
 free_entity:
@@ -1247,6 +1215,7 @@ static void ov5645_remove(struct i2c_client *client)
 	struct ov5645 *ov5645 = to_ov5645(sd);
 
 	v4l2_async_unregister_subdev(&ov5645->sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&ov5645->sd.entity);
 	v4l2_ctrl_handler_free(&ov5645->ctrls);
 	pm_runtime_disable(ov5645->dev);
-- 
2.34.1

[RFC PATCH 08/12] media: i2c: ov5645: Switch to {enable,disable}_streams

[Prabhakar]

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

Switch from s_stream to enable_streams and disable_streams callbacks.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 90 +++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 44 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 9497ec737cb7..dc93514608ee 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -923,71 +923,71 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable)
+static int ov5645_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state, u32 pad,
+				 u64 streams_mask)
 {
-	struct ov5645 *ov5645 = to_ov5645(subdev);
-	struct v4l2_subdev_state *state;
+	struct ov5645 *ov5645 = to_ov5645(sd);
 	int ret;
 
-	state = v4l2_subdev_lock_and_get_active_state(&ov5645->sd);
-
-	if (enable) {
-		ret = pm_runtime_resume_and_get(ov5645->dev);
-		if (ret < 0) {
-			v4l2_subdev_unlock_state(state);
-			return ret;
-		}
+	ret = pm_runtime_resume_and_get(ov5645->dev);
+	if (ret < 0)
+		return ret;
 
-		ret = ov5645_set_register_array(ov5645,
+	ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
 					ov5645->current_mode->data_size);
-		if (ret < 0) {
-			dev_err(ov5645->dev, "could not set mode %dx%d\n",
-				ov5645->current_mode->width,
-				ov5645->current_mode->height);
-			goto err_rpm_put;
-		}
-		ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
-		if (ret < 0) {
-			dev_err(ov5645->dev, "could not sync v4l2 controls\n");
-			goto err_rpm_put;
-		}
-
-		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
-		if (ret < 0)
-			goto err_rpm_put;
-
-		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
-				       OV5645_SYSTEM_CTRL0_START);
-		if (ret < 0)
-			goto err_rpm_put;
-	} else {
-		ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
-		if (ret < 0)
-			goto stream_off_rpm_put;
+	if (ret < 0) {
+		dev_err(ov5645->dev, "could not set mode %dx%d\n",
+			ov5645->current_mode->width,
+			ov5645->current_mode->height);
+		goto err_rpm_put;
+	}
+	ret = __v4l2_ctrl_handler_setup(&ov5645->ctrls);
+	if (ret < 0) {
+		dev_err(ov5645->dev, "could not sync v4l2 controls\n");
+		goto err_rpm_put;
+	}
 
-		ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
-				       OV5645_SYSTEM_CTRL0_STOP);
+	ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x45);
+	if (ret < 0)
+		goto err_rpm_put;
 
-		goto stream_off_rpm_put;
-	}
+	ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+			       OV5645_SYSTEM_CTRL0_START);
+	if (ret < 0)
+		goto err_rpm_put;
 
-	v4l2_subdev_unlock_state(state);
 	return 0;
 
 err_rpm_put:
 	pm_runtime_put_sync(ov5645->dev);
 	return ret;
+}
+
+static int ov5645_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state, u32 pad,
+				  u64 streams_mask)
+{
+	struct ov5645 *ov5645 = to_ov5645(sd);
+	int ret;
+
+	ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40);
+	if (ret < 0)
+		goto rpm_put;
+
+	ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0,
+			       OV5645_SYSTEM_CTRL0_STOP);
 
-stream_off_rpm_put:
+rpm_put:
 	pm_runtime_mark_last_busy(ov5645->dev);
 	pm_runtime_put_autosuspend(ov5645->dev);
-	v4l2_subdev_unlock_state(state);
+
 	return ret;
 }
 
 static const struct v4l2_subdev_video_ops ov5645_video_ops = {
-	.s_stream = ov5645_s_stream,
+	.s_stream = v4l2_subdev_s_stream_helper,
 };
 
 static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
@@ -996,6 +996,8 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = ov5645_set_format,
 	.get_selection = ov5645_get_selection,
+	.enable_streams = ov5645_enable_streams,
+	.disable_streams = ov5645_disable_streams,
 };
 
 static const struct v4l2_subdev_core_ops ov5645_core_ops = {
-- 
2.34.1

[RFC PATCH 09/12] media: i2c: ov5645: Add internal image sink pad

[Prabhakar]

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

Use the newly added internal pad API to expose the internal
configuration of the sensor to userspace in a standard manner.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 100 +++++++++++++++++++++++++++----------
 1 file changed, 75 insertions(+), 25 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index dc93514608ee..e5ec09f44bec 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -60,6 +60,10 @@
 #define OV5645_SDE_SAT_U		0x5583
 #define OV5645_SDE_SAT_V		0x5584
 
+#define OV5645_NATIVE_FORMAT	MEDIA_BUS_FMT_UYVY8_1X16
+#define OV5645_NATIVE_WIDTH	2592
+#define OV5645_NATIVE_HEIGHT	1944
+
 /* regulator supplies */
 static const char * const ov5645_supply_name[] = {
 	"vdddo", /* Digital I/O (1.8V) supply */
@@ -69,6 +73,12 @@ static const char * const ov5645_supply_name[] = {
 
 #define OV5645_NUM_SUPPLIES ARRAY_SIZE(ov5645_supply_name)
 
+enum ov5645_pad_ids {
+	OV5645_PAD_SOURCE,
+	OV5645_PAD_IMAGE,
+	OV5645_NUM_PADS,
+};
+
 struct reg_value {
 	u16 reg;
 	u8 val;
@@ -87,7 +97,7 @@ struct ov5645 {
 	struct i2c_client *i2c_client;
 	struct device *dev;
 	struct v4l2_subdev sd;
-	struct media_pad pad;
+	struct media_pad pads[OV5645_NUM_PADS];
 	struct v4l2_fwnode_endpoint ep;
 	struct v4l2_rect crop;
 	struct clk *xclk;
@@ -838,13 +848,23 @@ static int ov5645_enum_frame_size(struct v4l2_subdev *subdev,
 	if (fse->code != MEDIA_BUS_FMT_UYVY8_1X16)
 		return -EINVAL;
 
-	if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
-		return -EINVAL;
-
-	fse->min_width = ov5645_mode_info_data[fse->index].width;
-	fse->max_width = ov5645_mode_info_data[fse->index].width;
-	fse->min_height = ov5645_mode_info_data[fse->index].height;
-	fse->max_height = ov5645_mode_info_data[fse->index].height;
+	if (fse->pad == OV5645_PAD_IMAGE) {
+		if (fse->index > 0)
+			return -EINVAL;
+
+		fse->min_width = OV5645_NATIVE_WIDTH;
+		fse->max_width = OV5645_NATIVE_WIDTH;
+		fse->min_height = OV5645_NATIVE_HEIGHT;
+		fse->max_height = OV5645_NATIVE_HEIGHT;
+	} else {
+		if (fse->index >= ARRAY_SIZE(ov5645_mode_info_data))
+			return -EINVAL;
+
+		fse->min_width = ov5645_mode_info_data[fse->index].width;
+		fse->max_width = ov5645_mode_info_data[fse->index].width;
+		fse->min_height = ov5645_mode_info_data[fse->index].height;
+		fse->max_height = ov5645_mode_info_data[fse->index].height;
+	}
 
 	return 0;
 }
@@ -855,18 +875,55 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 {
 	struct ov5645 *ov5645 = to_ov5645(sd);
 	struct v4l2_mbus_framefmt *__format;
+	struct v4l2_rect *compose;
 	struct v4l2_rect *__crop;
 	const struct ov5645_mode_info *new_mode;
 	int ret;
 
-	__crop = v4l2_subdev_state_get_crop(sd_state, 0);
+	if (format->pad != OV5645_PAD_SOURCE)
+		return v4l2_subdev_get_fmt(sd, sd_state, format);
+
 	new_mode = v4l2_find_nearest_size(ov5645_mode_info_data,
 					  ARRAY_SIZE(ov5645_mode_info_data),
 					  width, height, format->format.width,
 					  format->format.height);
-
-	__crop->width = new_mode->width;
-	__crop->height = new_mode->height;
+	format->format.code = MEDIA_BUS_FMT_UYVY8_1X16;
+	format->format.width = new_mode->width;
+	format->format.height = new_mode->height;
+	format->format.field = V4L2_FIELD_NONE;
+	format->format.colorspace = V4L2_COLORSPACE_SRGB;
+	format->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
+	format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
+	format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
+
+	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE);
+	*__format = format->format;
+	__format->code = OV5645_NATIVE_FORMAT;
+	__format->width = OV5645_NATIVE_WIDTH;
+	__format->height = OV5645_NATIVE_HEIGHT;
+
+	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE);
+	__crop->width = format->format.width;
+	__crop->height = format->format.height;
+
+	/*
+	 * The compose rectangle models binning, its size is the sensor output
+	 * size.
+	 */
+	compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE);
+	compose->left = 0;
+	compose->top = 0;
+	compose->width = format->format.width;
+	compose->height = format->format.height;
+
+	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE);
+	__crop->left = 0;
+	__crop->top = 0;
+	__crop->width = format->format.width;
+	__crop->height = format->format.height;
+
+	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE);
+	*__format = format->format;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		ret = __v4l2_ctrl_s_ctrl_int64(ov5645->pixel_clock,
@@ -882,14 +939,6 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 		ov5645->current_mode = new_mode;
 	}
 
-	__format = v4l2_subdev_state_get_format(sd_state, 0);
-	__format->width = __crop->width;
-	__format->height = __crop->height;
-	__format->code = MEDIA_BUS_FMT_UYVY8_1X16;
-	__format->field = V4L2_FIELD_NONE;
-	__format->colorspace = V4L2_COLORSPACE_SRGB;
-
-	format->format = *__format;
 
 	return 0;
 }
@@ -899,9 +948,9 @@ static int ov5645_init_state(struct v4l2_subdev *subdev,
 {
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
-		.pad = 0,
+		.pad = OV5645_PAD_SOURCE,
 		.format = {
-			.code = MEDIA_BUS_FMT_UYVY8_1X16,
+			.code = OV5645_NATIVE_FORMAT,
 			.width = ov5645_mode_info_data[1].width,
 			.height = ov5645_mode_info_data[1].height,
 		},
@@ -919,7 +968,7 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
+	sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
 	return 0;
 }
 
@@ -1123,11 +1172,12 @@ static int ov5645_probe(struct i2c_client *client)
 	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
 	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
-	ov5645->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
+	ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
 	ov5645->sd.dev = dev;
 	ov5645->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
-	ret = media_entity_pads_init(&ov5645->sd.entity, 1, &ov5645->pad);
+	ret = media_entity_pads_init(&ov5645->sd.entity, ARRAY_SIZE(ov5645->pads), ov5645->pads);
 	if (ret < 0) {
 		dev_err_probe(dev, ret, "could not register media entity\n");
 		goto free_ctrl;
-- 
2.34.1

[RFC PATCH 10/12] media: i2c: ov5645: Report internal routes to userspace

[Prabhakar]

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

Usage of internal pads creates a route internal to the subdev, and the
V4L2 camera sensor API requires such routes to be reported to userspace.
Create the route in the .init_state() operation.

Internal routing support requires stream support, so set the
V4L2_SUBDEV_FL_HAS_STREAMS flag and switch formats and selection
rectangles access from pads to streams. As the route is immutable,
there's no need to implement the .set_routing() operation, and we can
hardcode the sink and source stream IDs to 0.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index e5ec09f44bec..14010d328e79 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -946,15 +946,36 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 static int ov5645_init_state(struct v4l2_subdev *subdev,
 			     struct v4l2_subdev_state *sd_state)
 {
+	struct v4l2_subdev_route routes[1] = {
+		{
+			.sink_pad = OV5645_PAD_IMAGE,
+			.sink_stream = 0,
+			.source_pad = OV5645_PAD_SOURCE,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE |
+				 V4L2_SUBDEV_ROUTE_FL_IMMUTABLE,
+		},
+	};
+	struct v4l2_subdev_krouting routing = {
+		.len_routes = ARRAY_SIZE(routes),
+		.num_routes = ARRAY_SIZE(routes),
+		.routes = routes,
+	};
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 		.pad = OV5645_PAD_SOURCE,
+		.stream = 0,
 		.format = {
 			.code = OV5645_NATIVE_FORMAT,
 			.width = ov5645_mode_info_data[1].width,
 			.height = ov5645_mode_info_data[1].height,
 		},
 	};
+	int ret;
+
+	ret = v4l2_subdev_set_routing(subdev, sd_state, &routing);
+	if (ret)
+		return ret;
 
 	ov5645_set_format(subdev, sd_state, &fmt);
 
@@ -1171,7 +1192,8 @@ static int ov5645_probe(struct i2c_client *client)
 
 	v4l2_i2c_subdev_init(&ov5645->sd, client, &ov5645_subdev_ops);
 	ov5645->sd.internal_ops = &ov5645_internal_ops;
-	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	ov5645->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS |
+			    V4L2_SUBDEV_FL_STREAMS;
 	ov5645->pads[OV5645_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 	ov5645->pads[OV5645_PAD_IMAGE].flags = MEDIA_PAD_FL_SINK | MEDIA_PAD_FL_INTERNAL;
 	ov5645->sd.dev = dev;
-- 
2.34.1

[RFC PATCH 11/12] media: i2c: ov5645: Report streams using frame descriptors

[Prabhakar]

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

Implement the .get_frame_desc() subdev operation to report information
about streams to the connected CSI-2 receiver. This is required to let
the CSI-2 receiver driver know about virtual channels and data types for
each stream.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 14010d328e79..45762783a19f 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -28,6 +28,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <media/mipi-csi2.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
 #include <media/v4l2-fwnode.h>
@@ -829,6 +830,32 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
 	.s_ctrl = ov5645_s_ctrl,
 };
 
+static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
+				 struct v4l2_mbus_frame_desc *fd)
+{
+	const struct v4l2_mbus_framefmt *fmt;
+	struct v4l2_subdev_state *state;
+
+	if (pad != OV5645_PAD_SOURCE)
+		return -EINVAL;
+
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+	fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
+	v4l2_subdev_unlock_state(state);
+
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+	fd->num_entries = 1;
+
+	memset(fd->entry, 0, sizeof(fd->entry));
+
+	fd->entry[0].pixelcode = fmt->code;
+	fd->entry[0].stream = 0;
+	fd->entry[0].bus.csi2.vc = 0;
+	fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
+
+	return 0;
+}
+
 static int ov5645_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -1061,6 +1088,7 @@ static const struct v4l2_subdev_video_ops ov5645_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
+	.get_frame_desc = ov5645_get_frame_desc,
 	.enum_mbus_code = ov5645_enum_mbus_code,
 	.enum_frame_size = ov5645_enum_frame_size,
 	.get_fmt = v4l2_subdev_get_fmt,
-- 
2.34.1

[RFC PATCH 12/12] media: i2c: ov5645: Add virtual channel support

[Prabhakar]

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

Add routes to configure the virtual channels.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 153 ++++++++++++++++++++++++++++---------
 1 file changed, 116 insertions(+), 37 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index 45762783a19f..cf4a6d8e83e0 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -54,6 +54,7 @@
 #define OV5645_TIMING_TC_REG21		0x3821
 #define		OV5645_SENSOR_MIRROR		BIT(1)
 #define OV5645_MIPI_CTRL00		0x4800
+#define OV5645_REG_DEBUG_MODE		0x4814
 #define OV5645_PRE_ISP_TEST_SETTING_1	0x503d
 #define		OV5645_TEST_PATTERN_MASK	0x3
 #define		OV5645_SET_TEST_PATTERN(x)	((x) & OV5645_TEST_PATTERN_MASK)
@@ -65,6 +66,8 @@
 #define OV5645_NATIVE_WIDTH	2592
 #define OV5645_NATIVE_HEIGHT	1944
 
+#define OV5645_ROUTES_MAX	4
+
 /* regulator supplies */
 static const char * const ov5645_supply_name[] = {
 	"vdddo", /* Digital I/O (1.8V) supply */
@@ -833,25 +836,36 @@ static const struct v4l2_ctrl_ops ov5645_ctrl_ops = {
 static int ov5645_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
 				 struct v4l2_mbus_frame_desc *fd)
 {
-	const struct v4l2_mbus_framefmt *fmt;
 	struct v4l2_subdev_state *state;
+	struct v4l2_subdev_route *route;
+	unsigned int num_routes = 0;
 
 	if (pad != OV5645_PAD_SOURCE)
 		return -EINVAL;
 
 	state = v4l2_subdev_lock_and_get_active_state(sd);
-	fmt = v4l2_subdev_state_get_format(state, OV5645_PAD_SOURCE, 0);
-	v4l2_subdev_unlock_state(state);
 
-	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
-	fd->num_entries = 1;
+	for_each_active_route(&state->routing, route) {
+		struct v4l2_mbus_frame_desc_entry *entry;
+		const struct v4l2_mbus_framefmt *fmt;
+
+		fmt = v4l2_subdev_state_get_format(state, route->source_pad,
+						   route->source_stream);
+
+		entry = &fd->entry[num_routes];
+		entry->stream = num_routes;
+		entry->pixelcode = fmt->code;
 
-	memset(fd->entry, 0, sizeof(fd->entry));
+		entry->bus.csi2.vc = route->source_stream;
+		entry->bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
 
-	fd->entry[0].pixelcode = fmt->code;
-	fd->entry[0].stream = 0;
-	fd->entry[0].bus.csi2.vc = 0;
-	fd->entry[0].bus.csi2.dt = MIPI_CSI2_DT_YUV422_8B;
+		num_routes++;
+	}
+
+	fd->num_entries = num_routes;
+	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
+
+	v4l2_subdev_unlock_state(state);
 
 	return 0;
 }
@@ -923,13 +937,13 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 	format->format.quantization = V4L2_QUANTIZATION_DEFAULT;
 	format->format.xfer_func = V4L2_XFER_FUNC_DEFAULT;
 
-	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE);
+	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_IMAGE, 0);
 	*__format = format->format;
 	__format->code = OV5645_NATIVE_FORMAT;
 	__format->width = OV5645_NATIVE_WIDTH;
 	__format->height = OV5645_NATIVE_HEIGHT;
 
-	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE);
+	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_IMAGE, 0);
 	__crop->width = format->format.width;
 	__crop->height = format->format.height;
 
@@ -937,19 +951,19 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 	 * The compose rectangle models binning, its size is the sensor output
 	 * size.
 	 */
-	compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE);
+	compose = v4l2_subdev_state_get_compose(sd_state, OV5645_PAD_IMAGE, 0);
 	compose->left = 0;
 	compose->top = 0;
 	compose->width = format->format.width;
 	compose->height = format->format.height;
 
-	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE);
+	__crop = v4l2_subdev_state_get_crop(sd_state, OV5645_PAD_SOURCE, format->stream);
 	__crop->left = 0;
 	__crop->top = 0;
 	__crop->width = format->format.width;
 	__crop->height = format->format.height;
 
-	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE);
+	__format = v4l2_subdev_state_get_format(sd_state, OV5645_PAD_SOURCE, format->stream);
 	*__format = format->format;
 
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
@@ -970,43 +984,80 @@ static int ov5645_set_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int ov5645_init_state(struct v4l2_subdev *subdev,
-			     struct v4l2_subdev_state *sd_state)
+static int ov5645_apply_routing(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_krouting *routing)
 {
-	struct v4l2_subdev_route routes[1] = {
-		{
-			.sink_pad = OV5645_PAD_IMAGE,
-			.sink_stream = 0,
-			.source_pad = OV5645_PAD_SOURCE,
-			.source_stream = 0,
-			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE |
-				 V4L2_SUBDEV_ROUTE_FL_IMMUTABLE,
-		},
-	};
-	struct v4l2_subdev_krouting routing = {
-		.len_routes = ARRAY_SIZE(routes),
-		.num_routes = ARRAY_SIZE(routes),
-		.routes = routes,
-	};
 	struct v4l2_subdev_format fmt = {
 		.which = V4L2_SUBDEV_FORMAT_TRY,
 		.pad = OV5645_PAD_SOURCE,
-		.stream = 0,
 		.format = {
 			.code = OV5645_NATIVE_FORMAT,
 			.width = ov5645_mode_info_data[1].width,
 			.height = ov5645_mode_info_data[1].height,
 		},
 	};
+	struct v4l2_subdev_route *route;
 	int ret;
 
-	ret = v4l2_subdev_set_routing(subdev, sd_state, &routing);
+	if (routing->num_routes > 1)
+		routing->num_routes = 1;
+
+	route = &routing->routes[0];
+
+	if (route->sink_stream > 0 || route->source_stream > 3)
+		return -EINVAL;
+
+	ret = v4l2_subdev_routing_validate(sd, routing,
+					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
 	if (ret)
 		return ret;
 
-	ov5645_set_format(subdev, sd_state, &fmt);
+	ret = v4l2_subdev_set_routing(sd, state, routing);
+	if (ret)
+		return ret;
 
-	return 0;
+	fmt.stream = route->source_stream;
+	return ov5645_set_format(sd, state, &fmt);
+}
+
+static int ov5645_init_state(struct v4l2_subdev *subdev,
+			     struct v4l2_subdev_state *sd_state)
+{
+	struct v4l2_subdev_route routes[OV5645_ROUTES_MAX] = {
+		{
+			.sink_pad = OV5645_PAD_IMAGE,
+			.sink_stream = 0,
+			.source_pad = OV5645_PAD_SOURCE,
+			.source_stream = 0,
+			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
+		},
+		{
+			.sink_pad = OV5645_PAD_IMAGE,
+			.sink_stream = 0,
+			.source_pad = OV5645_PAD_SOURCE,
+			.source_stream = 1,
+		},
+		{
+			.sink_pad = OV5645_PAD_IMAGE,
+			.sink_stream = 0,
+			.source_pad = OV5645_PAD_SOURCE,
+			.source_stream = 2,
+		},
+		{
+			.sink_pad = OV5645_PAD_IMAGE,
+			.sink_stream = 0,
+			.source_pad = OV5645_PAD_SOURCE,
+			.source_stream = 3,
+		},
+	};
+	struct v4l2_subdev_krouting routing = {
+		.len_routes = ARRAY_SIZE(routes),
+		.num_routes = ARRAY_SIZE(routes),
+		.routes = routes,
+	};
+
+	return ov5645_apply_routing(subdev, sd_state, &routing);
 }
 
 static int ov5645_get_selection(struct v4l2_subdev *sd,
@@ -1016,14 +1067,29 @@ static int ov5645_get_selection(struct v4l2_subdev *sd,
 	if (sel->target != V4L2_SEL_TGT_CROP)
 		return -EINVAL;
 
-	sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad);
+	sel->r = *v4l2_subdev_state_get_crop(sd_state, sel->pad, sel->stream);
 	return 0;
 }
 
+static int ov5645_set_virtual_channel(struct ov5645 *ov5645, u8 channel)
+{
+	int ret;
+	u8 val;
+
+	ret = ov5645_read_reg(ov5645, OV5645_REG_DEBUG_MODE, &val);
+	if (ret)
+		return ret;
+	val &= ~(3 << 6);
+	val |= (channel << 6);
+
+	return ov5645_write_reg(ov5645, OV5645_REG_DEBUG_MODE, val);
+}
+
 static int ov5645_enable_streams(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *state, u32 pad,
 				 u64 streams_mask)
 {
+	struct v4l2_subdev_route *route = &state->routing.routes[0];
 	struct ov5645 *ov5645 = to_ov5645(sd);
 	int ret;
 
@@ -1031,6 +1097,10 @@ static int ov5645_enable_streams(struct v4l2_subdev *sd,
 	if (ret < 0)
 		return ret;
 
+	ret = ov5645_set_virtual_channel(ov5645, route->source_stream);
+	if (ret)
+		goto err_rpm_put;
+
 	ret = ov5645_set_register_array(ov5645,
 					ov5645->current_mode->data,
 					ov5645->current_mode->data_size);
@@ -1083,6 +1153,14 @@ static int ov5645_disable_streams(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static int ov5645_set_routing(struct v4l2_subdev *sd,
+			      struct v4l2_subdev_state *state,
+			      enum v4l2_subdev_format_whence which,
+			      struct v4l2_subdev_krouting *routing)
+{
+	return ov5645_apply_routing(sd, state, routing);
+}
+
 static const struct v4l2_subdev_video_ops ov5645_video_ops = {
 	.s_stream = v4l2_subdev_s_stream_helper,
 };
@@ -1096,6 +1174,7 @@ static const struct v4l2_subdev_pad_ops ov5645_subdev_pad_ops = {
 	.get_selection = ov5645_get_selection,
 	.enable_streams = ov5645_enable_streams,
 	.disable_streams = ov5645_disable_streams,
+	.set_routing = ov5645_set_routing,
 };
 
 static const struct v4l2_subdev_core_ops ov5645_core_ops = {
-- 
2.34.1

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Tomi Valkeinen]

On 05/09/2024 00:07, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Hi All,
> 
> This patch series aims to add the below features,
> - Support subdev active state
> - Support for streams
> - Support for virtual channel
> - Code cleanup
> 
> Sending this is as an RFC, I've done limited testing and Im seeing issue
> when route is updated and wanted some feedback on the implementation.
> 
> When route is updated for the sensor with below command, and when start
> streaming I get "Dangling sink streams: mask 0x1" error. Should the
> corresponding bridge also need to support routes to fix this? or is it
> something missing Ive missed in the current implementation.
> 
> $ media-ctl -R "'ov5645 0-003c' [1/0->0/1[1]]"
> 
> Output after route update:
> - entity 4: ov5645 0-003c (2 pads, 1 link, 1 route)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev1
>          routes:
>                  1/0 -> 0/1 [ACTIVE]
>          pad0: SOURCE
>                  [stream:1 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb
>                   crop:(0,0)/1920x1080]
>                  -> "csi-10830400.csi2":0 [ENABLED,IMMUTABLE]
>          pad1: SINK,0x8
>                  [stream:0 fmt:UYVY8_1X16/2592x1944 field:none colorspace:srgb
>                   crop:(0,0)/1920x1080]

I think you actually want 1/0->0/0 routing. The error says that the sink 
side device has routing which does not have a stream at stream ID 1, or 
no routing support at all, which implies a single stream at stream ID 0.

  Tomi

>                
> v4l2-compliance log after this patch series:
> --------------------------------------------
> root@smarc-rzg2l:~# v4l2-compliance -u /dev/v4l-subdev1
> v4l2-compliance 1.28.1-5233, 64 bits, 64-bit time_t
> v4l2-compliance SHA: fc15e229d9d3 2024-07-23 19:22:15
> 
> Compliance test for device /dev/v4l-subdev1:
> 
> Driver Info:
>          Driver version   : 6.11.0
>          Capabilities     : 0x00000002
>                  Streams Support
>          Client Capabilities:[   53.919041] ov5645 0-003c: =================  START STATUS  =================
>   0x0000000000000[   53.927271] ov5645 0-003c: ==================  END STATUS  ==================
> 003
> streams interval-uses-which
> Required ioctls:
>          test VIDIOC_SUDBEV_QUERYCAP: OK
>          test invalid ioctls: OK
> 
> Allow for multiple opens:
>          test second /dev/v4l-subdev1 open: OK
>          test VIDIOC_SUBDEV_QUERYCAP: OK
>          test for unlimited opens: OK
> 
> Debug ioctls:
>          test VIDIOC_LOG_STATUS: OK (Not Supported)
> 
> Input ioctls:
>          test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
>          test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>          test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
>          test VIDIOC_ENUMAUDIO: OK (Not Supported)
>          test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
>          test VIDIOC_G/S_AUDIO: OK (Not Supported)
>          Inputs: 0 Audio Inputs: 0 Tuners: 0
> 
> Output ioctls:
>          test VIDIOC_G/S_MODULATOR: OK (Not Supported)
>          test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
>          test VIDIOC_ENUMAUDOUT: OK (Not Supported)
>          test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
>          test VIDIOC_G/S_AUDOUT: OK (Not Supported)
>          Outputs: 0 Audio Outputs: 0 Modulators: 0
> 
> Input/Output configuration ioctls:
>          test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
>          test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
>          test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
>          test VIDIOC_G/S_EDID: OK (Not Supported)
> 
> Sub-Device routing ioctls:
>          test Try VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK
>          test Active VIDIOC_SUBDEV_G_ROUTING/VIDIOC_SUBDEV_S_ROUTING: OK
> 
> Control ioctls:
>          test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
>          test VIDIOC_QUERYCTRL: OK
>          test VIDIOC_G/S_CTRL: OK
>          test VIDIOC_G/S/TRY_EXT_CTRLS: OK
>          test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
>          test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
>          Standard Controls: 12 Private Controls: 0
> 
> Format ioctls:
>          test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK (Not Supported)
>          test VIDIOC_G/S_PARM: OK (Not Supported)
>          test VIDIOC_G_FBUF: OK (Not Supported)
>          test VIDIOC_G_FMT: OK (Not Supported)
>          test VIDIOC_TRY_FMT: OK (Not Supported)
>          test VIDIOC_S_FMT: OK (Not Supported)
>          test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
>          test Cropping: OK (Not Supported)
>          test Composing: OK (Not Supported)
>          test Scaling: OK (Not Supported)
> 
> Codec ioctls:
>          test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
>          test VIDIOC_G_ENC_INDEX: OK (Not Supported)
>          test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
> 
> Buffer ioctls:
>          test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK (Not Supported)
>          test CREATE_BUFS maximum buffers: OK
>          test VIDIOC_REMOVE_BUFS: OK
>          test VIDIOC_EXPBUF: OK (Not Supported)
>          test Requests: OK (Not Supported)
> 
> Total for device /dev/v4l-subdev1: 47, Succeeded: 47, Failed: 0, Warnings: 0
> 
> Cheers,
> Prabhakar
> 
> Lad Prabhakar (12):
>    media: i2c: ov5645: Add V4L2_SUBDEV_FL_HAS_EVENTS and subscribe hooks
>    media: i2c: ov5645: Use local `dev` pointer for subdev device
>      assignment
>    media: i2c: ov5645: Enable runtime PM after
>      v4l2_async_register_subdev()
>    media: i2c: ov5645: Use dev_err_probe instead of dev_err
>    media: i2c: ov5645: Use v4l2_async_register_subdev_sensor()
>    media: i2c: ov5645: Drop `power_lock` mutex
>    media: i2c: ov5645: Use subdev active state
>    media: i2c: ov5645: Switch to {enable,disable}_streams
>    media: i2c: ov5645: Add internal image sink pad
>    media: i2c: ov5645: Report internal routes to userspace
>    media: i2c: ov5645: Report streams using frame descriptors
>    media: i2c: ov5645: Add virtual channel support
> 
>   drivers/media/i2c/ov5645.c | 507 +++++++++++++++++++++++--------------
>   1 file changed, 316 insertions(+), 191 deletions(-)
> 

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Tomi Valkeinen]

On 05/09/2024 09:48, Tomi Valkeinen wrote:
> On 05/09/2024 00:07, Prabhakar wrote:
>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>
>> Hi All,
>>
>> This patch series aims to add the below features,
>> - Support subdev active state
>> - Support for streams
>> - Support for virtual channel
>> - Code cleanup
>>
>> Sending this is as an RFC, I've done limited testing and Im seeing issue
>> when route is updated and wanted some feedback on the implementation.
>>
>> When route is updated for the sensor with below command, and when start
>> streaming I get "Dangling sink streams: mask 0x1" error. Should the
>> corresponding bridge also need to support routes to fix this? or is it
>> something missing Ive missed in the current implementation.
>>
>> $ media-ctl -R "'ov5645 0-003c' [1/0->0/1[1]]"
>>
>> Output after route update:
>> - entity 4: ov5645 0-003c (2 pads, 1 link, 1 route)
>>              type V4L2 subdev subtype Sensor flags 0
>>              device node name /dev/v4l-subdev1
>>          routes:
>>                  1/0 -> 0/1 [ACTIVE]
>>          pad0: SOURCE
>>                  [stream:1 fmt:UYVY8_1X16/1920x1080 field:none 
>> colorspace:srgb
>>                   crop:(0,0)/1920x1080]
>>                  -> "csi-10830400.csi2":0 [ENABLED,IMMUTABLE]
>>          pad1: SINK,0x8
>>                  [stream:0 fmt:UYVY8_1X16/2592x1944 field:none 
>> colorspace:srgb
>>                   crop:(0,0)/1920x1080]
> 
> I think you actually want 1/0->0/0 routing. The error says that the sink 
> side device has routing which does not have a stream at stream ID 1, or 
> no routing support at all, which implies a single stream at stream ID 0.

Looking at patch 12, there's something wrong with the approach here. Are 
you perhaps trying to define the CSI-2 VC with the streams?

If you have a camera with a single image stream coming from an internal 
pad, you should have one hardcoded route, 1/0->0/0. The 
.get_frame_desc() should return the CSI-2 VC (most likely always 0) and 
DT (based on the format) for that stream.

If you also have embedded data, then you'd have another internal pad 
(pad number 2), and the routing would be:

1/0->0/0
2/0->0/1

And here .get_frame_desc() would also handle the second stream, and most 
likely for that stream the VC would also be 0.

The stream ID in the routing table is a software level concept, not 
related to the VC.

Also, the internal pad should use the sensor's native format, not 
UYVY8_1X16.

  Tomi

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Lad, Prabhakar]

Hi Tomi,

Thanks for the feedback.

On Thu, Sep 5, 2024 at 7:48 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 05/09/2024 00:07, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Hi All,
> >
> > This patch series aims to add the below features,
> > - Support subdev active state
> > - Support for streams
> > - Support for virtual channel
> > - Code cleanup
> >
> > Sending this is as an RFC, I've done limited testing and Im seeing issue
> > when route is updated and wanted some feedback on the implementation.
> >
> > When route is updated for the sensor with below command, and when start
> > streaming I get "Dangling sink streams: mask 0x1" error. Should the
> > corresponding bridge also need to support routes to fix this? or is it
> > something missing Ive missed in the current implementation.
> >
> > $ media-ctl -R "'ov5645 0-003c' [1/0->0/1[1]]"
> >
> > Output after route update:
> > - entity 4: ov5645 0-003c (2 pads, 1 link, 1 route)
> >              type V4L2 subdev subtype Sensor flags 0
> >              device node name /dev/v4l-subdev1
> >          routes:
> >                  1/0 -> 0/1 [ACTIVE]
> >          pad0: SOURCE
> >                  [stream:1 fmt:UYVY8_1X16/1920x1080 field:none colorspace:srgb
> >                   crop:(0,0)/1920x1080]
> >                  -> "csi-10830400.csi2":0 [ENABLED,IMMUTABLE]
> >          pad1: SINK,0x8
> >                  [stream:0 fmt:UYVY8_1X16/2592x1944 field:none colorspace:srgb
> >                   crop:(0,0)/1920x1080]
>
> I think you actually want 1/0->0/0 routing.
PAD1 -> Internal image pad Sink
PAD0 -> Source

The above combination works were pad1/stream0 ---> pad0/stream0

> The error says that the sink
> side device has routing which does not have a stream at stream ID 1, or
> no routing support at all, which implies a single stream at stream ID 0.
>
To support virtual channels I have the below routes,

    struct v4l2_subdev_route routes[OV5645_ROUTES_MAX] = {
        {
            .sink_pad = OV5645_PAD_IMAGE,
            .sink_stream = 0,
            .source_pad = OV5645_PAD_SOURCE,
            .source_stream = 0,
            .flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
        },
        {
            .sink_pad = OV5645_PAD_IMAGE,
            .sink_stream = 0,
            .source_pad = OV5645_PAD_SOURCE,
            .source_stream = 1,
        },
        {
            .sink_pad = OV5645_PAD_IMAGE,
            .sink_stream = 0,
            .source_pad = OV5645_PAD_SOURCE,
            .source_stream = 2,
        },
        {
            .sink_pad = OV5645_PAD_IMAGE,
            .sink_stream = 0,
            .source_pad = OV5645_PAD_SOURCE,
            .source_stream = 3,
        },
    };

Where the image pad sink stream0 can connect source stream0/1/2/3

So to switch source stream1 I use the below:
$ media-ctl -R "'ov5645 0-003c' [1/0->0/1[1]]"

With the above I connect sink pad1 stream0 -> source pad0 stream1,
(unless Ive misunderstood the connection graph).

Cheers,
Prabhakar

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Lad, Prabhakar]

Hi Tomi,

On Thu, Sep 5, 2024 at 10:11 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
<snip>
> > I think you actually want 1/0->0/0 routing. The error says that the sink
> > side device has routing which does not have a stream at stream ID 1, or
> > no routing support at all, which implies a single stream at stream ID 0.
>
> Looking at patch 12, there's something wrong with the approach here. Are
> you perhaps trying to define the CSI-2 VC with the streams?
>
Yes, based on the previous feedback received, I am implementing
virtual channels as streams. If this isn't the correct approach can
you please guide me on what we should be using to support virtual
channels?

> If you have a camera with a single image stream coming from an internal
> pad, you should have one hardcoded route, 1/0->0/0. The
> .get_frame_desc() should return the CSI-2 VC (most likely always 0) and
> DT (based on the format) for that stream.
>
Agreed.

> If you also have embedded data, then you'd have another internal pad
> (pad number 2), and the routing would be:
>
> 1/0->0/0
> 2/0->0/1
>
> And here .get_frame_desc() would also handle the second stream, and most
> likely for that stream the VC would also be 0.
>
Ok agreed.

> The stream ID in the routing table is a software level concept, not
> related to the VC.
>
> Also, the internal pad should use the sensor's native format, not
> UYVY8_1X16.
>
Ok, I will update it to MEDIA_BUS_FMT_SBGGR10_1X10.

Cheers,
Prabhakar

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Tomi Valkeinen]

On 05/09/2024 13:27, Lad, Prabhakar wrote:
> Hi Tomi,
> 
> On Thu, Sep 5, 2024 at 10:11 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
> <snip>
>>> I think you actually want 1/0->0/0 routing. The error says that the sink
>>> side device has routing which does not have a stream at stream ID 1, or
>>> no routing support at all, which implies a single stream at stream ID 0.
>>
>> Looking at patch 12, there's something wrong with the approach here. Are
>> you perhaps trying to define the CSI-2 VC with the streams?
>>
> Yes, based on the previous feedback received, I am implementing
> virtual channels as streams. If this isn't the correct approach can
> you please guide me on what we should be using to support virtual
> channels?

Can you describe what kind of hardware you have and how the VCs are 
supposed to be configured and used there?

  Tomi

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Lad, Prabhakar]

Hi Tomi,

On Thu, Sep 5, 2024 at 11:29 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 05/09/2024 13:27, Lad, Prabhakar wrote:
> > Hi Tomi,
> >
> > On Thu, Sep 5, 2024 at 10:11 AM Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> > <snip>
> >>> I think you actually want 1/0->0/0 routing. The error says that the sink
> >>> side device has routing which does not have a stream at stream ID 1, or
> >>> no routing support at all, which implies a single stream at stream ID 0.
> >>
> >> Looking at patch 12, there's something wrong with the approach here. Are
> >> you perhaps trying to define the CSI-2 VC with the streams?
> >>
> > Yes, based on the previous feedback received, I am implementing
> > virtual channels as streams. If this isn't the correct approach can
> > you please guide me on what we should be using to support virtual
> > channels?
>
> Can you describe what kind of hardware you have and how the VCs are
> supposed to be configured and used there?
>

 __________________                       ______________
    _____________________
|                                     |                     |
                |                   |
      |
| OV5645   Sensor        |                     |  RZ/G2L CSI2   |
             |  RZ/G2L CRU                 |
| 1ch out of 4x VC         |---------------> |   4x VC
|-------------->|  1Ch capture out 4xVC  |
|__________________|                      |_____________|
     |____________________|

Cheers,
Prabhakar

Re: [RFC PATCH 00/12] media: ov5645: Add support for streams

[Tomi Valkeinen]

On 05/09/2024 13:51, Lad, Prabhakar wrote:
> Hi Tomi,
> 
> On Thu, Sep 5, 2024 at 11:29 AM Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> On 05/09/2024 13:27, Lad, Prabhakar wrote:
>>> Hi Tomi,
>>>
>>> On Thu, Sep 5, 2024 at 10:11 AM Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>
>>> <snip>
>>>>> I think you actually want 1/0->0/0 routing. The error says that the sink
>>>>> side device has routing which does not have a stream at stream ID 1, or
>>>>> no routing support at all, which implies a single stream at stream ID 0.
>>>>
>>>> Looking at patch 12, there's something wrong with the approach here. Are
>>>> you perhaps trying to define the CSI-2 VC with the streams?
>>>>
>>> Yes, based on the previous feedback received, I am implementing
>>> virtual channels as streams. If this isn't the correct approach can
>>> you please guide me on what we should be using to support virtual
>>> channels?
>>
>> Can you describe what kind of hardware you have and how the VCs are
>> supposed to be configured and used there?
>>
> 
>   __________________                       ______________
>      _____________________
> |                                     |                     |
>                  |                   |
>        |
> | OV5645   Sensor        |                     |  RZ/G2L CSI2   |
>               |  RZ/G2L CRU                 |
> | 1ch out of 4x VC         |---------------> |   4x VC
> |-------------->|  1Ch capture out 4xVC  |
> |__________________|                      |_____________|
>       |____________________|

The formatting there is quite broken, but afaiu you have a standard 
setup with single stream. Then it should be as I wrote earlier: a single 
hardcoded 1/0->0/0 route in the sensor, and .get_frame_desc() returns VC 
0 and a suitable DT for that stream.

When you say "implementing virtual channels as streams", what is the 
feature/requirement you are looking at? Do you have some specific need 
to use other VCs than 0?

  Tomi