[PATCH v2 0/3] DCMI bridge support

[PATCH v2 0/3] DCMI bridge support

[Hugues Fruchet]

This patch serie allows to connect non-parallel camera sensor to
DCMI thanks to a bridge connected in between such as STMIPID02 [1].

Media controller support is introduced first, then support of
several sub-devices within pipeline with dynamic linking
between them.
In order to keep backward compatibility with applications
relying on V4L2 interface only, format set on video node
is propagated to all sub-devices connected to camera interface.

[1] https://www.spinics.net/lists/devicetree/msg278002.html

===========
= history =
===========
version 2:
  - Fix bus_info not consistent between media and V4L:
    https://www.spinics.net/lists/arm-kernel/msg717676.html
  - Propagation of format set on video node to the sub-devices
    chain connected on camera interface

version 1:
  - Initial submission

Hugues Fruchet (3):
  media: stm32-dcmi: improve sensor subdev naming
  media: stm32-dcmi: add media controller support
  media: stm32-dcmi: add support of several sub-devices

 drivers/media/platform/Kconfig            |   2 +-
 drivers/media/platform/stm32/stm32-dcmi.c | 317 +++++++++++++++++++++++++-----
 2 files changed, 266 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] media: stm32-dcmi: improve sensor subdev naming

[Hugues Fruchet]

Add a new "sensor" field to dcmi struct instead of
reusing entity->subdev to address sensor subdev.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/platform/stm32/stm32-dcmi.c | 37 ++++++++++++++++---------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index b9dad0a..7a4d559 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -151,6 +151,7 @@ struct stm32_dcmi {
 	unsigned int			num_of_sd_framesizes;
 	struct dcmi_framesize		sd_framesize;
 	struct v4l2_rect		sd_bounds;
+	struct v4l2_subdev		*sensor;
 
 	/* Protect this data structure */
 	struct mutex			lock;
@@ -595,7 +596,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	}
 
 	/* Enable stream on the sub device */
-	ret = v4l2_subdev_call(dcmi->entity.subdev, video, s_stream, 1);
+	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 1);
 	if (ret && ret != -ENOIOCTLCMD) {
 		dev_err(dcmi->dev, "%s: Failed to start streaming, subdev streamon error",
 			__func__);
@@ -685,7 +686,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 
 err_subdev_streamoff:
-	v4l2_subdev_call(dcmi->entity.subdev, video, s_stream, 0);
+	v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
 
 err_pm_put:
 	pm_runtime_put(dcmi->dev);
@@ -713,7 +714,7 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
 	int ret;
 
 	/* Disable stream on the sub device */
-	ret = v4l2_subdev_call(dcmi->entity.subdev, video, s_stream, 0);
+	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
 	if (ret && ret != -ENOIOCTLCMD)
 		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
 			__func__, ret);
@@ -857,7 +858,7 @@ static int dcmi_try_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f,
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad, set_fmt,
+	ret = v4l2_subdev_call(dcmi->sensor, pad, set_fmt,
 			       &pad_cfg, &format);
 	if (ret < 0)
 		return ret;
@@ -934,7 +935,7 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
 	mf->width = sd_framesize.width;
 	mf->height = sd_framesize.height;
 
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad,
+	ret = v4l2_subdev_call(dcmi->sensor, pad,
 			       set_fmt, NULL, &format);
 	if (ret < 0)
 		return ret;
@@ -991,7 +992,7 @@ static int dcmi_get_sensor_format(struct stm32_dcmi *dcmi,
 	};
 	int ret;
 
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad, get_fmt, NULL, &fmt);
+	ret = v4l2_subdev_call(dcmi->sensor, pad, get_fmt, NULL, &fmt);
 	if (ret)
 		return ret;
 
@@ -1020,7 +1021,7 @@ static int dcmi_set_sensor_format(struct stm32_dcmi *dcmi,
 	}
 
 	v4l2_fill_mbus_format(&format.format, pix, sd_fmt->mbus_code);
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad, set_fmt,
+	ret = v4l2_subdev_call(dcmi->sensor, pad, set_fmt,
 			       &pad_cfg, &format);
 	if (ret < 0)
 		return ret;
@@ -1043,7 +1044,7 @@ static int dcmi_get_sensor_bounds(struct stm32_dcmi *dcmi,
 	/*
 	 * Get sensor bounds first
 	 */
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad, get_selection,
+	ret = v4l2_subdev_call(dcmi->sensor, pad, get_selection,
 			       NULL, &bounds);
 	if (!ret)
 		*r = bounds.r;
@@ -1224,7 +1225,7 @@ static int dcmi_enum_framesizes(struct file *file, void *fh,
 
 	fse.code = sd_fmt->mbus_code;
 
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad, enum_frame_size,
+	ret = v4l2_subdev_call(dcmi->sensor, pad, enum_frame_size,
 			       NULL, &fse);
 	if (ret)
 		return ret;
@@ -1241,7 +1242,7 @@ static int dcmi_g_parm(struct file *file, void *priv,
 {
 	struct stm32_dcmi *dcmi = video_drvdata(file);
 
-	return v4l2_g_parm_cap(video_devdata(file), dcmi->entity.subdev, p);
+	return v4l2_g_parm_cap(video_devdata(file), dcmi->sensor, p);
 }
 
 static int dcmi_s_parm(struct file *file, void *priv,
@@ -1249,7 +1250,7 @@ static int dcmi_s_parm(struct file *file, void *priv,
 {
 	struct stm32_dcmi *dcmi = video_drvdata(file);
 
-	return v4l2_s_parm_cap(video_devdata(file), dcmi->entity.subdev, p);
+	return v4l2_s_parm_cap(video_devdata(file), dcmi->sensor, p);
 }
 
 static int dcmi_enum_frameintervals(struct file *file, void *fh,
@@ -1271,7 +1272,7 @@ static int dcmi_enum_frameintervals(struct file *file, void *fh,
 
 	fie.code = sd_fmt->mbus_code;
 
-	ret = v4l2_subdev_call(dcmi->entity.subdev, pad,
+	ret = v4l2_subdev_call(dcmi->sensor, pad,
 			       enum_frame_interval, NULL, &fie);
 	if (ret)
 		return ret;
@@ -1291,7 +1292,7 @@ MODULE_DEVICE_TABLE(of, stm32_dcmi_of_match);
 static int dcmi_open(struct file *file)
 {
 	struct stm32_dcmi *dcmi = video_drvdata(file);
-	struct v4l2_subdev *sd = dcmi->entity.subdev;
+	struct v4l2_subdev *sd = dcmi->sensor;
 	int ret;
 
 	if (mutex_lock_interruptible(&dcmi->lock))
@@ -1322,7 +1323,7 @@ static int dcmi_open(struct file *file)
 static int dcmi_release(struct file *file)
 {
 	struct stm32_dcmi *dcmi = video_drvdata(file);
-	struct v4l2_subdev *sd = dcmi->entity.subdev;
+	struct v4l2_subdev *sd = dcmi->sensor;
 	bool fh_singular;
 	int ret;
 
@@ -1433,7 +1434,7 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
 {
 	const struct dcmi_format *sd_fmts[ARRAY_SIZE(dcmi_formats)];
 	unsigned int num_fmts = 0, i, j;
-	struct v4l2_subdev *subdev = dcmi->entity.subdev;
+	struct v4l2_subdev *subdev = dcmi->sensor;
 	struct v4l2_subdev_mbus_code_enum mbus_code = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
@@ -1479,7 +1480,7 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
 static int dcmi_framesizes_init(struct stm32_dcmi *dcmi)
 {
 	unsigned int num_fsize = 0;
-	struct v4l2_subdev *subdev = dcmi->entity.subdev;
+	struct v4l2_subdev *subdev = dcmi->sensor;
 	struct v4l2_subdev_frame_size_enum fse = {
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 		.code = dcmi->sd_format->mbus_code,
@@ -1526,7 +1527,7 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
 	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
 	int ret;
 
-	dcmi->vdev->ctrl_handler = dcmi->entity.subdev->ctrl_handler;
+	dcmi->vdev->ctrl_handler = dcmi->sensor->ctrl_handler;
 	ret = dcmi_formats_init(dcmi);
 	if (ret) {
 		dev_err(dcmi->dev, "No supported mediabus format found\n");
@@ -1582,7 +1583,7 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
 
 	dev_dbg(dcmi->dev, "Subdev %s bound\n", subdev->name);
 
-	dcmi->entity.subdev = subdev;
+	dcmi->sensor = subdev;
 
 	return 0;
 }
-- 
2.7.4

Re: [PATCH v2 1/3] media: stm32-dcmi: improve sensor subdev naming

[Sakari Ailus]

Hi Hugues,

On Tue, Jun 11, 2019 at 10:48:30AM +0200, Hugues Fruchet wrote:
> Add a new "sensor" field to dcmi struct instead of
> reusing entity->subdev to address sensor subdev.

The purpose of the struct binding image source's async subdev as well as
related information is to allow associating the two. This patch breaks
that. If your device can support a single sensor, it might not be a big
deal. The end result remains somewhat inconsistent as subdev specific
information is spread across struct stm32_dcmi and struct
dcmi_graph_entity.

In general you don't need to know the sensor as you can always find it
using media_entity_remote_pad(). This driver is a little different though
as it could presumably continue to work without MC. Was that the intent?

On a side note: struct dcmi_graph_entity does NOT have struct
v4l2_async_subdev as its first member. Please fix that and prepend the fix
to this set.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 1/3] media: stm32-dcmi: improve sensor subdev naming

[Hugues FRUCHET]

Hi Sakari,

On 6/20/19 5:26 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Tue, Jun 11, 2019 at 10:48:30AM +0200, Hugues Fruchet wrote:
>> Add a new "sensor" field to dcmi struct instead of
>> reusing entity->subdev to address sensor subdev.
As discussed on IRC, fixed in v3,
> 
> The purpose of the struct binding image source's async subdev as well as
> related information is to allow associating the two. This patch breaks
> that. If your device can support a single sensor, it might not be a big
> deal. The end result remains somewhat inconsistent as subdev specific
> information is spread across struct stm32_dcmi and struct
> dcmi_graph_entity.
As discussed on IRC, fixed in v3,

> 
> In general you don't need to know the sensor as you can always find it
> using media_entity_remote_pad(). This driver is a little different though
> as it could presumably continue to work without MC. Was that the intent?
> 
> On a side note: struct dcmi_graph_entity does NOT have struct
> v4l2_async_subdev as its first member. Please fix that and prepend the fix
> to this set.
> 
As discussed on IRC, fixed in v3,

BR,
Hugues.

[PATCH v2 2/3] media: stm32-dcmi: add media controller support

[Hugues Fruchet]

Add media controller support to dcmi.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/platform/Kconfig            |  2 +-
 drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++--------
 2 files changed, 63 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 8a19654..de7e21f 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF
 
 config VIDEO_STM32_DCMI
 	tristate "STM32 Digital Camera Memory Interface (DCMI) support"
-	depends on VIDEO_V4L2 && OF
+	depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER
 	depends on ARCH_STM32 || COMPILE_TEST
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_FWNODE
diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 7a4d559..3a69783 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -170,6 +170,9 @@ struct stm32_dcmi {
 
 	/* Ensure DMA operations atomicity */
 	struct mutex			dma_lock;
+
+	struct media_device		mdev;
+	struct media_pad		vid_cap_pad;
 };
 
 static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
@@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
 		dev_err(dcmi->dev, "Could not get sensor bounds\n");
 		return ret;
 	}
-
 	ret = dcmi_set_default_fmt(dcmi);
 	if (ret) {
 		dev_err(dcmi->dev, "Could not set default format\n");
 		return ret;
 	}
 
-	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
-	if (ret) {
-		dev_err(dcmi->dev, "Failed to register video device\n");
-		return ret;
-	}
-
-	dev_dbg(dcmi->dev, "Device registered as %s\n",
-		video_device_node_name(dcmi->vdev));
 	return 0;
 }
 
@@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
 	return 0;
 }
 
+static void dcmi_graph_deinit(struct stm32_dcmi *dcmi)
+{
+	v4l2_async_notifier_unregister(&dcmi->notifier);
+	v4l2_async_notifier_cleanup(&dcmi->notifier);
+}
+
 static int dcmi_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev)
 
 	q = &dcmi->queue;
 
+	dcmi->v4l2_dev.mdev = &dcmi->mdev;
+
+	/* Initialize media device */
+	strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model));
+	snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info),
+		 "platform:%s", DRV_NAME);
+	dcmi->mdev.dev = &pdev->dev;
+	media_device_init(&dcmi->mdev);
+
+	/* Register the media device */
+	ret = media_device_register(&dcmi->mdev);
+	if (ret) {
+		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
+			ret);
+		goto err_media_device_cleanup;
+	}
+
 	/* Initialize the top-level structure */
 	ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev);
 	if (ret)
-		goto err_dma_release;
+		goto err_media_device_unregister;
 
 	dcmi->vdev = video_device_alloc();
 	if (!dcmi->vdev) {
@@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev)
 				  V4L2_CAP_READWRITE;
 	video_set_drvdata(dcmi->vdev, dcmi);
 
+	/* Media entity pads */
+	dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK;
+	ret = media_entity_pads_init(&dcmi->vdev->entity,
+				     1, &dcmi->vid_cap_pad);
+	if (ret) {
+		dev_err(dcmi->dev, "Failed to init media entity pad\n");
+		goto err_device_unregister;
+	}
+	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
+
+	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
+	if (ret) {
+		dev_err(dcmi->dev, "Failed to register video device\n");
+		goto err_media_entity_cleanup;
+	}
+
+	dev_dbg(dcmi->dev, "Device registered as %s\n",
+		video_device_node_name(dcmi->vdev));
+
 	/* Buffer queue */
 	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
@@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev)
 	ret = vb2_queue_init(q);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "Failed to initialize vb2 queue\n");
-		goto err_device_release;
+		goto err_media_entity_cleanup;
 	}
 
 	ret = dcmi_graph_init(dcmi);
 	if (ret < 0)
-		goto err_device_release;
+		goto err_media_entity_cleanup;
 
 	/* Reset device */
 	ret = reset_control_assert(dcmi->rstc);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to assert the reset line\n");
-		goto err_cleanup;
+		goto err_graph_deinit;
 	}
 
 	usleep_range(3000, 5000);
@@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev)
 	ret = reset_control_deassert(dcmi->rstc);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to deassert the reset line\n");
-		goto err_cleanup;
+		goto err_graph_deinit;
 	}
 
 	dev_info(&pdev->dev, "Probe done\n");
@@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_cleanup:
-	v4l2_async_notifier_cleanup(&dcmi->notifier);
-err_device_release:
-	video_device_release(dcmi->vdev);
+err_graph_deinit:
+	dcmi_graph_deinit(dcmi);
+err_media_entity_cleanup:
+	media_entity_cleanup(&dcmi->vdev->entity);
 err_device_unregister:
 	v4l2_device_unregister(&dcmi->v4l2_dev);
-err_dma_release:
+err_media_device_unregister:
+	media_device_unregister(&dcmi->mdev);
+err_media_device_cleanup:
+	media_device_cleanup(&dcmi->mdev);
 	dma_release_channel(dcmi->dma_chan);
 
 	return ret;
@@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
-	v4l2_async_notifier_unregister(&dcmi->notifier);
-	v4l2_async_notifier_cleanup(&dcmi->notifier);
+	dcmi_graph_deinit(dcmi);
+	media_entity_cleanup(&dcmi->vdev->entity);
 	v4l2_device_unregister(&dcmi->v4l2_dev);
+	media_device_unregister(&dcmi->mdev);
+	media_device_cleanup(&dcmi->mdev);
 
 	dma_release_channel(dcmi->dma_chan);
 
-- 
2.7.4

Re: [PATCH v2 2/3] media: stm32-dcmi: add media controller support

[Hans Verkuil]

On 6/11/19 10:48 AM, Hugues Fruchet wrote:
> Add media controller support to dcmi.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/platform/Kconfig            |  2 +-
>  drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++--------
>  2 files changed, 63 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 8a19654..de7e21f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF
>  
>  config VIDEO_STM32_DCMI
>  	tristate "STM32 Digital Camera Memory Interface (DCMI) support"
> -	depends on VIDEO_V4L2 && OF
> +	depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER
>  	depends on ARCH_STM32 || COMPILE_TEST
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_FWNODE
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 7a4d559..3a69783 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -170,6 +170,9 @@ struct stm32_dcmi {
>  
>  	/* Ensure DMA operations atomicity */
>  	struct mutex			dma_lock;
> +
> +	struct media_device		mdev;
> +	struct media_pad		vid_cap_pad;
>  };
>  
>  static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
> @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>  		dev_err(dcmi->dev, "Could not get sensor bounds\n");
>  		return ret;
>  	}
> -
>  	ret = dcmi_set_default_fmt(dcmi);
>  	if (ret) {
>  		dev_err(dcmi->dev, "Could not set default format\n");
>  		return ret;
>  	}
>  
> -	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
> -	if (ret) {
> -		dev_err(dcmi->dev, "Failed to register video device\n");
> -		return ret;
> -	}

Why was this moved to probe()? Off-hand I see no reason for that.

Regards,

	Hans

> -
> -	dev_dbg(dcmi->dev, "Device registered as %s\n",
> -		video_device_node_name(dcmi->vdev));
>  	return 0;
>  }
>  
> @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>  	return 0;
>  }
>  
> +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi)
> +{
> +	v4l2_async_notifier_unregister(&dcmi->notifier);
> +	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +}
> +
>  static int dcmi_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev)
>  
>  	q = &dcmi->queue;
>  
> +	dcmi->v4l2_dev.mdev = &dcmi->mdev;
> +
> +	/* Initialize media device */
> +	strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model));
> +	snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info),
> +		 "platform:%s", DRV_NAME);
> +	dcmi->mdev.dev = &pdev->dev;
> +	media_device_init(&dcmi->mdev);
> +
> +	/* Register the media device */
> +	ret = media_device_register(&dcmi->mdev);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
> +			ret);
> +		goto err_media_device_cleanup;
> +	}
> +
>  	/* Initialize the top-level structure */
>  	ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev);
>  	if (ret)
> -		goto err_dma_release;
> +		goto err_media_device_unregister;
>  
>  	dcmi->vdev = video_device_alloc();
>  	if (!dcmi->vdev) {
> @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev)
>  				  V4L2_CAP_READWRITE;
>  	video_set_drvdata(dcmi->vdev, dcmi);
>  
> +	/* Media entity pads */
> +	dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK;
> +	ret = media_entity_pads_init(&dcmi->vdev->entity,
> +				     1, &dcmi->vid_cap_pad);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to init media entity pad\n");
> +		goto err_device_unregister;
> +	}
> +	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
> +
> +	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register video device\n");
> +		goto err_media_entity_cleanup;
> +	}
> +
> +	dev_dbg(dcmi->dev, "Device registered as %s\n",
> +		video_device_node_name(dcmi->vdev));
> +
>  	/* Buffer queue */
>  	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
> @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev)
>  	ret = vb2_queue_init(q);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to initialize vb2 queue\n");
> -		goto err_device_release;
> +		goto err_media_entity_cleanup;
>  	}
>  
>  	ret = dcmi_graph_init(dcmi);
>  	if (ret < 0)
> -		goto err_device_release;
> +		goto err_media_entity_cleanup;
>  
>  	/* Reset device */
>  	ret = reset_control_assert(dcmi->rstc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to assert the reset line\n");
> -		goto err_cleanup;
> +		goto err_graph_deinit;
>  	}
>  
>  	usleep_range(3000, 5000);
> @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev)
>  	ret = reset_control_deassert(dcmi->rstc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to deassert the reset line\n");
> -		goto err_cleanup;
> +		goto err_graph_deinit;
>  	}
>  
>  	dev_info(&pdev->dev, "Probe done\n");
> @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err_cleanup:
> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
> -err_device_release:
> -	video_device_release(dcmi->vdev);
> +err_graph_deinit:
> +	dcmi_graph_deinit(dcmi);
> +err_media_entity_cleanup:
> +	media_entity_cleanup(&dcmi->vdev->entity);
>  err_device_unregister:
>  	v4l2_device_unregister(&dcmi->v4l2_dev);
> -err_dma_release:
> +err_media_device_unregister:
> +	media_device_unregister(&dcmi->mdev);
> +err_media_device_cleanup:
> +	media_device_cleanup(&dcmi->mdev);
>  	dma_release_channel(dcmi->dma_chan);
>  
>  	return ret;
> @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev)
>  
>  	pm_runtime_disable(&pdev->dev);
>  
> -	v4l2_async_notifier_unregister(&dcmi->notifier);
> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +	dcmi_graph_deinit(dcmi);
> +	media_entity_cleanup(&dcmi->vdev->entity);
>  	v4l2_device_unregister(&dcmi->v4l2_dev);
> +	media_device_unregister(&dcmi->mdev);
> +	media_device_cleanup(&dcmi->mdev);
>  
>  	dma_release_channel(dcmi->dma_chan);
>  
> 

Re: [PATCH v2 2/3] media: stm32-dcmi: add media controller support

[Hugues FRUCHET]

Hi Hans,

On 6/20/19 2:01 PM, Hans Verkuil wrote:
> On 6/11/19 10:48 AM, Hugues Fruchet wrote:
>> Add media controller support to dcmi.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/platform/Kconfig            |  2 +-
>>   drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++--------
>>   2 files changed, 63 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index 8a19654..de7e21f 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF
>>   
>>   config VIDEO_STM32_DCMI
>>   	tristate "STM32 Digital Camera Memory Interface (DCMI) support"
>> -	depends on VIDEO_V4L2 && OF
>> +	depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER
>>   	depends on ARCH_STM32 || COMPILE_TEST
>>   	select VIDEOBUF2_DMA_CONTIG
>>   	select V4L2_FWNODE
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
>> index 7a4d559..3a69783 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -170,6 +170,9 @@ struct stm32_dcmi {
>>   
>>   	/* Ensure DMA operations atomicity */
>>   	struct mutex			dma_lock;
>> +
>> +	struct media_device		mdev;
>> +	struct media_pad		vid_cap_pad;
>>   };
>>   
>>   static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
>> @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>   		dev_err(dcmi->dev, "Could not get sensor bounds\n");
>>   		return ret;
>>   	}
>> -
>>   	ret = dcmi_set_default_fmt(dcmi);
>>   	if (ret) {
>>   		dev_err(dcmi->dev, "Could not set default format\n");
>>   		return ret;
>>   	}
>>   
>> -	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
>> -	if (ret) {
>> -		dev_err(dcmi->dev, "Failed to register video device\n");
>> -		return ret;
>> -	}
> 
> Why was this moved to probe()? Off-hand I see no reason for that.
> 
> Regards,
> 
> 	Hans
> 

I need to do that otherwise the dcmi_graph_notify_bound() subdevice pad 
link code crashes:
  +	/*
  +	 * Link this sub-device to DCMI, it could be
  +	 * a parallel camera sensor or a bridge
  +	 */
  +	src_pad = media_entity_get_fwnode_pad(&subdev->entity,
  +					      subdev->fwnode,
  +					      MEDIA_PAD_FL_SOURCE);
  +
  +	ret = media_create_pad_link(&subdev->entity, src_pad,
  +				    &dcmi->vdev->entity, 0,
  +				    MEDIA_LNK_FL_IMMUTABLE |
  +				    MEDIA_LNK_FL_ENABLED);
see https://www.spinics.net/lists/linux-media/msg153120.html.


>> -
>> -	dev_dbg(dcmi->dev, "Device registered as %s\n",
>> -		video_device_node_name(dcmi->vdev));
>>   	return 0;
>>   }
>>   
>> @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>>   	return 0;
>>   }
>>   
>> +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi)
>> +{
>> +	v4l2_async_notifier_unregister(&dcmi->notifier);
>> +	v4l2_async_notifier_cleanup(&dcmi->notifier);
>> +}
>> +
>>   static int dcmi_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *np = pdev->dev.of_node;
>> @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev)
>>   
>>   	q = &dcmi->queue;
>>   
>> +	dcmi->v4l2_dev.mdev = &dcmi->mdev;
>> +
>> +	/* Initialize media device */
>> +	strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model));
>> +	snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info),
>> +		 "platform:%s", DRV_NAME);
>> +	dcmi->mdev.dev = &pdev->dev;
>> +	media_device_init(&dcmi->mdev);
>> +
>> +	/* Register the media device */
>> +	ret = media_device_register(&dcmi->mdev);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
>> +			ret);
>> +		goto err_media_device_cleanup;
>> +	}
>> +
>>   	/* Initialize the top-level structure */
>>   	ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev);
>>   	if (ret)
>> -		goto err_dma_release;
>> +		goto err_media_device_unregister;
>>   
>>   	dcmi->vdev = video_device_alloc();
>>   	if (!dcmi->vdev) {
>> @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev)
>>   				  V4L2_CAP_READWRITE;
>>   	video_set_drvdata(dcmi->vdev, dcmi);
>>   
>> +	/* Media entity pads */
>> +	dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK;
>> +	ret = media_entity_pads_init(&dcmi->vdev->entity,
>> +				     1, &dcmi->vid_cap_pad);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to init media entity pad\n");
>> +		goto err_device_unregister;
>> +	}
>> +	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
>> +
>> +	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to register video device\n");
>> +		goto err_media_entity_cleanup;
>> +	}
>> +
>> +	dev_dbg(dcmi->dev, "Device registered as %s\n",
>> +		video_device_node_name(dcmi->vdev));
>> +
>>   	/* Buffer queue */
>>   	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>   	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
>> @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev)
>>   	ret = vb2_queue_init(q);
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to initialize vb2 queue\n");
>> -		goto err_device_release;
>> +		goto err_media_entity_cleanup;
>>   	}
>>   
>>   	ret = dcmi_graph_init(dcmi);
>>   	if (ret < 0)
>> -		goto err_device_release;
>> +		goto err_media_entity_cleanup;
>>   
>>   	/* Reset device */
>>   	ret = reset_control_assert(dcmi->rstc);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Failed to assert the reset line\n");
>> -		goto err_cleanup;
>> +		goto err_graph_deinit;
>>   	}
>>   
>>   	usleep_range(3000, 5000);
>> @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev)
>>   	ret = reset_control_deassert(dcmi->rstc);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Failed to deassert the reset line\n");
>> -		goto err_cleanup;
>> +		goto err_graph_deinit;
>>   	}
>>   
>>   	dev_info(&pdev->dev, "Probe done\n");
>> @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev)
>>   
>>   	return 0;
>>   
>> -err_cleanup:
>> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
>> -err_device_release:
>> -	video_device_release(dcmi->vdev);
>> +err_graph_deinit:
>> +	dcmi_graph_deinit(dcmi);
>> +err_media_entity_cleanup:
>> +	media_entity_cleanup(&dcmi->vdev->entity);
>>   err_device_unregister:
>>   	v4l2_device_unregister(&dcmi->v4l2_dev);
>> -err_dma_release:
>> +err_media_device_unregister:
>> +	media_device_unregister(&dcmi->mdev);
>> +err_media_device_cleanup:
>> +	media_device_cleanup(&dcmi->mdev);
>>   	dma_release_channel(dcmi->dma_chan);
>>   
>>   	return ret;
>> @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev)
>>   
>>   	pm_runtime_disable(&pdev->dev);
>>   
>> -	v4l2_async_notifier_unregister(&dcmi->notifier);
>> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
>> +	dcmi_graph_deinit(dcmi);
>> +	media_entity_cleanup(&dcmi->vdev->entity);
>>   	v4l2_device_unregister(&dcmi->v4l2_dev);
>> +	media_device_unregister(&dcmi->mdev);
>> +	media_device_cleanup(&dcmi->mdev);
>>   
>>   	dma_release_channel(dcmi->dma_chan);
>>   
>>
> 

BR,
Hugues.

Re: [PATCH v2 2/3] media: stm32-dcmi: add media controller support

[Sakari Ailus]

Hi Hugues,

On Tue, Jun 11, 2019 at 10:48:31AM +0200, Hugues Fruchet wrote:
> Add media controller support to dcmi.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/platform/Kconfig            |  2 +-
>  drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++--------
>  2 files changed, 63 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 8a19654..de7e21f 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF
>  
>  config VIDEO_STM32_DCMI
>  	tristate "STM32 Digital Camera Memory Interface (DCMI) support"
> -	depends on VIDEO_V4L2 && OF
> +	depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER

Ok, if the intent is to require MC from now on, then I think you could
simply rely on media_entity_remote_pad() in finding the image source.

>  	depends on ARCH_STM32 || COMPILE_TEST
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_FWNODE
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 7a4d559..3a69783 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -170,6 +170,9 @@ struct stm32_dcmi {
>  
>  	/* Ensure DMA operations atomicity */
>  	struct mutex			dma_lock;
> +
> +	struct media_device		mdev;
> +	struct media_pad		vid_cap_pad;
>  };
>  
>  static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
> @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>  		dev_err(dcmi->dev, "Could not get sensor bounds\n");
>  		return ret;
>  	}
> -
>  	ret = dcmi_set_default_fmt(dcmi);
>  	if (ret) {
>  		dev_err(dcmi->dev, "Could not set default format\n");
>  		return ret;
>  	}
>  
> -	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
> -	if (ret) {
> -		dev_err(dcmi->dev, "Failed to register video device\n");
> -		return ret;
> -	}
> -
> -	dev_dbg(dcmi->dev, "Device registered as %s\n",
> -		video_device_node_name(dcmi->vdev));
>  	return 0;
>  }
>  
> @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>  	return 0;
>  }
>  
> +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi)
> +{
> +	v4l2_async_notifier_unregister(&dcmi->notifier);
> +	v4l2_async_notifier_cleanup(&dcmi->notifier);

I'd just leave the calls where they are now. This doesn't improve
readability of the code, rather the opposite.

> +}
> +
>  static int dcmi_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev)
>  
>  	q = &dcmi->queue;
>  
> +	dcmi->v4l2_dev.mdev = &dcmi->mdev;
> +
> +	/* Initialize media device */
> +	strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model));
> +	snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info),
> +		 "platform:%s", DRV_NAME);
> +	dcmi->mdev.dev = &pdev->dev;
> +	media_device_init(&dcmi->mdev);
> +
> +	/* Register the media device */
> +	ret = media_device_register(&dcmi->mdev);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
> +			ret);
> +		goto err_media_device_cleanup;
> +	}
> +
>  	/* Initialize the top-level structure */
>  	ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev);
>  	if (ret)
> -		goto err_dma_release;
> +		goto err_media_device_unregister;
>  
>  	dcmi->vdev = video_device_alloc();
>  	if (!dcmi->vdev) {
> @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev)
>  				  V4L2_CAP_READWRITE;
>  	video_set_drvdata(dcmi->vdev, dcmi);
>  
> +	/* Media entity pads */
> +	dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK;
> +	ret = media_entity_pads_init(&dcmi->vdev->entity,
> +				     1, &dcmi->vid_cap_pad);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to init media entity pad\n");
> +		goto err_device_unregister;
> +	}
> +	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
> +
> +	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register video device\n");
> +		goto err_media_entity_cleanup;
> +	}
> +
> +	dev_dbg(dcmi->dev, "Device registered as %s\n",
> +		video_device_node_name(dcmi->vdev));
> +
>  	/* Buffer queue */
>  	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
> @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev)
>  	ret = vb2_queue_init(q);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "Failed to initialize vb2 queue\n");
> -		goto err_device_release;
> +		goto err_media_entity_cleanup;
>  	}
>  
>  	ret = dcmi_graph_init(dcmi);
>  	if (ret < 0)
> -		goto err_device_release;
> +		goto err_media_entity_cleanup;
>  
>  	/* Reset device */
>  	ret = reset_control_assert(dcmi->rstc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to assert the reset line\n");
> -		goto err_cleanup;
> +		goto err_graph_deinit;
>  	}
>  
>  	usleep_range(3000, 5000);
> @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev)
>  	ret = reset_control_deassert(dcmi->rstc);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to deassert the reset line\n");
> -		goto err_cleanup;
> +		goto err_graph_deinit;
>  	}
>  
>  	dev_info(&pdev->dev, "Probe done\n");
> @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -err_cleanup:
> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
> -err_device_release:
> -	video_device_release(dcmi->vdev);
> +err_graph_deinit:
> +	dcmi_graph_deinit(dcmi);
> +err_media_entity_cleanup:
> +	media_entity_cleanup(&dcmi->vdev->entity);
>  err_device_unregister:
>  	v4l2_device_unregister(&dcmi->v4l2_dev);
> -err_dma_release:
> +err_media_device_unregister:
> +	media_device_unregister(&dcmi->mdev);
> +err_media_device_cleanup:
> +	media_device_cleanup(&dcmi->mdev);
>  	dma_release_channel(dcmi->dma_chan);
>  
>  	return ret;
> @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev)
>  
>  	pm_runtime_disable(&pdev->dev);
>  
> -	v4l2_async_notifier_unregister(&dcmi->notifier);
> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +	dcmi_graph_deinit(dcmi);
> +	media_entity_cleanup(&dcmi->vdev->entity);
>  	v4l2_device_unregister(&dcmi->v4l2_dev);
> +	media_device_unregister(&dcmi->mdev);

Please unregister the media device first before unregistering anything else
it depends on (i.e. async notifier or the entity).

> +	media_device_cleanup(&dcmi->mdev);
>  
>  	dma_release_channel(dcmi->dma_chan);
>  

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 2/3] media: stm32-dcmi: add media controller support

[Hugues FRUCHET]

Hi Sakari,

On 6/20/19 6:13 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Tue, Jun 11, 2019 at 10:48:31AM +0200, Hugues Fruchet wrote:
>> Add media controller support to dcmi.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/platform/Kconfig            |  2 +-
>>   drivers/media/platform/stm32/stm32-dcmi.c | 83 +++++++++++++++++++++++--------
>>   2 files changed, 63 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index 8a19654..de7e21f 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -121,7 +121,7 @@ config VIDEO_S3C_CAMIF
>>   
>>   config VIDEO_STM32_DCMI
>>   	tristate "STM32 Digital Camera Memory Interface (DCMI) support"
>> -	depends on VIDEO_V4L2 && OF
>> +	depends on VIDEO_V4L2 && OF && MEDIA_CONTROLLER
> 
> Ok, if the intent is to require MC from now on, then I think you could
> simply rely on media_entity_remote_pad() in finding the image source.
> 
>>   	depends on ARCH_STM32 || COMPILE_TEST
>>   	select VIDEOBUF2_DMA_CONTIG
>>   	select V4L2_FWNODE
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
>> index 7a4d559..3a69783 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -170,6 +170,9 @@ struct stm32_dcmi {
>>   
>>   	/* Ensure DMA operations atomicity */
>>   	struct mutex			dma_lock;
>> +
>> +	struct media_device		mdev;
>> +	struct media_pad		vid_cap_pad;
>>   };
>>   
>>   static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
>> @@ -1545,21 +1548,12 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>   		dev_err(dcmi->dev, "Could not get sensor bounds\n");
>>   		return ret;
>>   	}
>> -
>>   	ret = dcmi_set_default_fmt(dcmi);
>>   	if (ret) {
>>   		dev_err(dcmi->dev, "Could not set default format\n");
>>   		return ret;
>>   	}
>>   
>> -	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
>> -	if (ret) {
>> -		dev_err(dcmi->dev, "Failed to register video device\n");
>> -		return ret;
>> -	}
>> -
>> -	dev_dbg(dcmi->dev, "Device registered as %s\n",
>> -		video_device_node_name(dcmi->vdev));
>>   	return 0;
>>   }
>>   
>> @@ -1648,6 +1642,12 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>>   	return 0;
>>   }
>>   
>> +static void dcmi_graph_deinit(struct stm32_dcmi *dcmi)
>> +{
>> +	v4l2_async_notifier_unregister(&dcmi->notifier);
>> +	v4l2_async_notifier_cleanup(&dcmi->notifier);
> 
> I'd just leave the calls where they are now. This doesn't improve
> readability of the code, rather the opposite.
> 

fixed in v3.

>> +}
>> +
>>   static int dcmi_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *np = pdev->dev.of_node;
>> @@ -1752,10 +1752,27 @@ static int dcmi_probe(struct platform_device *pdev)
>>   
>>   	q = &dcmi->queue;
>>   
>> +	dcmi->v4l2_dev.mdev = &dcmi->mdev;
>> +
>> +	/* Initialize media device */
>> +	strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model));
>> +	snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info),
>> +		 "platform:%s", DRV_NAME);
>> +	dcmi->mdev.dev = &pdev->dev;
>> +	media_device_init(&dcmi->mdev);
>> +
>> +	/* Register the media device */
>> +	ret = media_device_register(&dcmi->mdev);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
>> +			ret);
>> +		goto err_media_device_cleanup;
>> +	}
>> +
>>   	/* Initialize the top-level structure */
>>   	ret = v4l2_device_register(&pdev->dev, &dcmi->v4l2_dev);
>>   	if (ret)
>> -		goto err_dma_release;
>> +		goto err_media_device_unregister;
>>   
>>   	dcmi->vdev = video_device_alloc();
>>   	if (!dcmi->vdev) {
>> @@ -1775,6 +1792,25 @@ static int dcmi_probe(struct platform_device *pdev)
>>   				  V4L2_CAP_READWRITE;
>>   	video_set_drvdata(dcmi->vdev, dcmi);
>>   
>> +	/* Media entity pads */
>> +	dcmi->vid_cap_pad.flags = MEDIA_PAD_FL_SINK;
>> +	ret = media_entity_pads_init(&dcmi->vdev->entity,
>> +				     1, &dcmi->vid_cap_pad);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to init media entity pad\n");
>> +		goto err_device_unregister;
>> +	}
>> +	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
>> +
>> +	ret = video_register_device(dcmi->vdev, VFL_TYPE_GRABBER, -1);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to register video device\n");
>> +		goto err_media_entity_cleanup;
>> +	}
>> +
>> +	dev_dbg(dcmi->dev, "Device registered as %s\n",
>> +		video_device_node_name(dcmi->vdev));
>> +
>>   	/* Buffer queue */
>>   	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>   	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
>> @@ -1790,18 +1826,18 @@ static int dcmi_probe(struct platform_device *pdev)
>>   	ret = vb2_queue_init(q);
>>   	if (ret < 0) {
>>   		dev_err(&pdev->dev, "Failed to initialize vb2 queue\n");
>> -		goto err_device_release;
>> +		goto err_media_entity_cleanup;
>>   	}
>>   
>>   	ret = dcmi_graph_init(dcmi);
>>   	if (ret < 0)
>> -		goto err_device_release;
>> +		goto err_media_entity_cleanup;
>>   
>>   	/* Reset device */
>>   	ret = reset_control_assert(dcmi->rstc);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Failed to assert the reset line\n");
>> -		goto err_cleanup;
>> +		goto err_graph_deinit;
>>   	}
>>   
>>   	usleep_range(3000, 5000);
>> @@ -1809,7 +1845,7 @@ static int dcmi_probe(struct platform_device *pdev)
>>   	ret = reset_control_deassert(dcmi->rstc);
>>   	if (ret) {
>>   		dev_err(&pdev->dev, "Failed to deassert the reset line\n");
>> -		goto err_cleanup;
>> +		goto err_graph_deinit;
>>   	}
>>   
>>   	dev_info(&pdev->dev, "Probe done\n");
>> @@ -1820,13 +1856,16 @@ static int dcmi_probe(struct platform_device *pdev)
>>   
>>   	return 0;
>>   
>> -err_cleanup:
>> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
>> -err_device_release:
>> -	video_device_release(dcmi->vdev);
>> +err_graph_deinit:
>> +	dcmi_graph_deinit(dcmi);
>> +err_media_entity_cleanup:
>> +	media_entity_cleanup(&dcmi->vdev->entity);
>>   err_device_unregister:
>>   	v4l2_device_unregister(&dcmi->v4l2_dev);
>> -err_dma_release:
>> +err_media_device_unregister:
>> +	media_device_unregister(&dcmi->mdev);
>> +err_media_device_cleanup:
>> +	media_device_cleanup(&dcmi->mdev);
>>   	dma_release_channel(dcmi->dma_chan);
>>   
>>   	return ret;
>> @@ -1838,9 +1877,11 @@ static int dcmi_remove(struct platform_device *pdev)
>>   
>>   	pm_runtime_disable(&pdev->dev);
>>   
>> -	v4l2_async_notifier_unregister(&dcmi->notifier);
>> -	v4l2_async_notifier_cleanup(&dcmi->notifier);
>> +	dcmi_graph_deinit(dcmi);
>> +	media_entity_cleanup(&dcmi->vdev->entity);
>>   	v4l2_device_unregister(&dcmi->v4l2_dev);
>> +	media_device_unregister(&dcmi->mdev);
> 
> Please unregister the media device first before unregistering anything else
> it depends on (i.e. async notifier or the entity).
> 
Media device registry is dropped in v3 to not expose media controller
interfaces to userspace as discussed here:
https://www.spinics.net/lists/linux-media/msg153417.html

>> +	media_device_cleanup(&dcmi->mdev);
>>   
>>   	dma_release_channel(dcmi->dma_chan);
>>   
> 

BR,
Hugues.

[PATCH v2 3/3] media: stm32-dcmi: add support of several sub-devices

[Hugues Fruchet]

Add support of several sub-devices within pipeline instead
of a single one.
This allows to support a CSI-2 camera sensor connected
through a CSI-2 to parallel bridge.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/platform/stm32/stm32-dcmi.c | 207 +++++++++++++++++++++++++++---
 1 file changed, 189 insertions(+), 18 deletions(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 3a69783..144912f 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -173,6 +173,7 @@ struct stm32_dcmi {
 
 	struct media_device		mdev;
 	struct media_pad		vid_cap_pad;
+	struct media_pipeline		pipeline;
 };
 
 static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
@@ -584,6 +585,135 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
 	spin_unlock_irq(&dcmi->irqlock);
 }
 
+static struct media_entity *dcmi_find_sensor(struct stm32_dcmi *dcmi)
+{
+	struct media_entity *entity = &dcmi->vdev->entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+
+	/* Walk searching for entity having no sink */
+	while (1) {
+		pad = &entity->pads[0];
+
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_entity_remote_pad(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+	}
+
+	return entity;
+}
+
+static int dcmi_pipeline_s_fmt(struct stm32_dcmi *dcmi,
+			       struct v4l2_subdev_pad_config *pad_cfg,
+			       struct v4l2_subdev_format *format)
+{
+	struct media_entity *entity = &dcmi->sensor->entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *sink_pad = NULL;
+	struct media_pad *src_pad = NULL;
+	struct media_pad *pad = NULL;
+	struct v4l2_subdev_format fmt = *format;
+	bool found = false;
+	int ret;
+
+	/*
+	 * Starting from sensor subdevice, walk within
+	 * pipeline and set format on each subdevice
+	 */
+	while (1) {
+		unsigned int i;
+
+		/* Search if current entity has a source pad */
+		for (i = 0; i < entity->num_pads; i++) {
+			pad = &entity->pads[i];
+			if (pad->flags & MEDIA_PAD_FL_SOURCE) {
+				src_pad = pad;
+				found = true;
+				break;
+			}
+		}
+		if (!found)
+			break;
+
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		/* Propagate format on sink pad if any, otherwise source pad */
+		if (sink_pad)
+			pad = sink_pad;
+
+		dev_dbg(dcmi->dev, "%s[%d] pad format set to 0x%x %ux%u\n",
+			subdev->name, pad->index, format->format.code,
+			format->format.width, format->format.height);
+
+		fmt.pad = pad->index;
+		ret = v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
+		if (ret < 0)
+			return ret;
+
+		/* Walk to next entity */
+		sink_pad = media_entity_remote_pad(src_pad);
+		if (!sink_pad || !is_media_entity_v4l2_subdev(sink_pad->entity))
+			break;
+
+		entity = sink_pad->entity;
+	}
+	*format = fmt;
+
+	return 0;
+}
+
+static int dcmi_pipeline_s_stream(struct stm32_dcmi *dcmi, int state)
+{
+	struct media_entity *entity = &dcmi->vdev->entity;
+	struct v4l2_subdev *subdev;
+	struct media_pad *pad;
+	int ret;
+
+	/* Start/stop all entities within pipeline */
+	while (1) {
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_entity_remote_pad(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+		ret = v4l2_subdev_call(subdev, video, s_stream, state);
+		if (ret < 0 && ret != -ENOIOCTLCMD) {
+			dev_err(dcmi->dev, "%s: %s failed to %s streaming (%d)\n",
+				__func__, subdev->name,
+				state ? "start" : "stop", ret);
+			return ret;
+		}
+
+		dev_dbg(dcmi->dev, "%s is %s\n",
+			subdev->name, state ? "started" : "stopped");
+	}
+
+	return 0;
+}
+
+static int dcmi_pipeline_start(struct stm32_dcmi *dcmi)
+{
+	return dcmi_pipeline_s_stream(dcmi, 1);
+}
+
+static void dcmi_pipeline_stop(struct stm32_dcmi *dcmi)
+{
+	dcmi_pipeline_s_stream(dcmi, 0);
+}
+
 static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 {
 	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
@@ -598,14 +728,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err_release_buffers;
 	}
 
-	/* Enable stream on the sub device */
-	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 1);
-	if (ret && ret != -ENOIOCTLCMD) {
-		dev_err(dcmi->dev, "%s: Failed to start streaming, subdev streamon error",
-			__func__);
+	ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
+	if (ret < 0) {
+		dev_err(dcmi->dev, "%s: Failed to start streaming, media pipeline start error (%d)\n",
+			__func__, ret);
 		goto err_pm_put;
 	}
 
+	ret = dcmi_pipeline_start(dcmi);
+	if (ret)
+		goto err_media_pipeline_stop;
+
 	spin_lock_irq(&dcmi->irqlock);
 
 	/* Set bus width */
@@ -677,7 +810,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (ret) {
 		dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
 			__func__);
-		goto err_subdev_streamoff;
+		goto err_pipeline_stop;
 	}
 
 	/* Enable interruptions */
@@ -688,8 +821,11 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	return 0;
 
-err_subdev_streamoff:
-	v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
+err_pipeline_stop:
+	dcmi_pipeline_stop(dcmi);
+
+err_media_pipeline_stop:
+	media_pipeline_stop(&dcmi->vdev->entity);
 
 err_pm_put:
 	pm_runtime_put(dcmi->dev);
@@ -714,13 +850,10 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
 {
 	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
 	struct dcmi_buf *buf, *node;
-	int ret;
 
-	/* Disable stream on the sub device */
-	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
-	if (ret && ret != -ENOIOCTLCMD)
-		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
-			__func__, ret);
+	dcmi_pipeline_stop(dcmi);
+
+	media_pipeline_stop(&dcmi->vdev->entity);
 
 	spin_lock_irq(&dcmi->irqlock);
 
@@ -938,8 +1071,7 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
 	mf->width = sd_framesize.width;
 	mf->height = sd_framesize.height;
 
-	ret = v4l2_subdev_call(dcmi->sensor, pad,
-			       set_fmt, NULL, &format);
+	ret = dcmi_pipeline_s_fmt(dcmi, NULL, &format);
 	if (ret < 0)
 		return ret;
 
@@ -1530,7 +1662,19 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
 	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
 	int ret;
 
+	/*
+	 * Now that the graph is complete,
+	 * we search for the camera sensor subdevice
+	 * in order to expose it through V4L2 interface
+	 */
+	dcmi->sensor = media_entity_to_v4l2_subdev(dcmi_find_sensor(dcmi));
+	if (!dcmi->sensor) {
+		dev_err(dcmi->dev, "No camera sensor subdevice found\n");
+		return -ENODEV;
+	}
+
 	dcmi->vdev->ctrl_handler = dcmi->sensor->ctrl_handler;
+
 	ret = dcmi_formats_init(dcmi);
 	if (ret) {
 		dev_err(dcmi->dev, "No supported mediabus format found\n");
@@ -1574,12 +1718,30 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
 				   struct v4l2_async_subdev *asd)
 {
 	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
+	unsigned int ret;
+	int src_pad;
 
 	dev_dbg(dcmi->dev, "Subdev %s bound\n", subdev->name);
 
-	dcmi->sensor = subdev;
+	/*
+	 * Link this sub-device to DCMI, it could be
+	 * a parallel camera sensor or a bridge
+	 */
+	src_pad = media_entity_get_fwnode_pad(&subdev->entity,
+					      subdev->fwnode,
+					      MEDIA_PAD_FL_SOURCE);
+
+	ret = media_create_pad_link(&subdev->entity, src_pad,
+				    &dcmi->vdev->entity, 0,
+				    MEDIA_LNK_FL_IMMUTABLE |
+				    MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		dev_err(dcmi->dev, "Failed to create media pad link with subdev %s\n",
+			subdev->name);
+	else
+		dev_dbg(dcmi->dev, "DCMI is now linked to %s\n", subdev->name);
 
-	return 0;
+	return ret;
 }
 
 static const struct v4l2_async_notifier_operations dcmi_graph_notify_ops = {
@@ -1639,6 +1801,15 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
 		return ret;
 	}
 
+	/* Register all the subdev nodes */
+	ret = v4l2_device_register_subdev_nodes(&dcmi->v4l2_dev);
+	if (ret) {
+		dev_err(dcmi->dev, "Failed to register subdev nodes\n");
+		v4l2_async_notifier_unregister(&dcmi->notifier);
+		of_node_put(dcmi->entity.node);
+		return ret;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH v2 3/3] media: stm32-dcmi: add support of several sub-devices

[Sakari Ailus]

Hi Hugues,

Thank you for the update.

On Tue, Jun 11, 2019 at 10:48:32AM +0200, Hugues Fruchet wrote:
> Add support of several sub-devices within pipeline instead
> of a single one.
> This allows to support a CSI-2 camera sensor connected
> through a CSI-2 to parallel bridge.
> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/platform/stm32/stm32-dcmi.c | 207 +++++++++++++++++++++++++++---
>  1 file changed, 189 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 3a69783..144912f 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -173,6 +173,7 @@ struct stm32_dcmi {
>  
>  	struct media_device		mdev;
>  	struct media_pad		vid_cap_pad;
> +	struct media_pipeline		pipeline;
>  };
>  
>  static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
> @@ -584,6 +585,135 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
>  	spin_unlock_irq(&dcmi->irqlock);
>  }
>  
> +static struct media_entity *dcmi_find_sensor(struct stm32_dcmi *dcmi)

You generally should be only concerned with the next entity connected to the
one you're in control of, not the rest of the pipeline.

> +{
> +	struct media_entity *entity = &dcmi->vdev->entity;
> +	struct v4l2_subdev *subdev;
> +	struct media_pad *pad;
> +
> +	/* Walk searching for entity having no sink */
> +	while (1) {
> +		pad = &entity->pads[0];
> +
> +		subdev = media_entity_to_v4l2_subdev(entity);
> +
> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +			break;
> +
> +		pad = media_entity_remote_pad(pad);
> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> +			break;
> +
> +		entity = pad->entity;
> +	}
> +
> +	return entity;
> +}
> +
> +static int dcmi_pipeline_s_fmt(struct stm32_dcmi *dcmi,
> +			       struct v4l2_subdev_pad_config *pad_cfg,
> +			       struct v4l2_subdev_format *format)
> +{
> +	struct media_entity *entity = &dcmi->sensor->entity;
> +	struct v4l2_subdev *subdev;
> +	struct media_pad *sink_pad = NULL;
> +	struct media_pad *src_pad = NULL;
> +	struct media_pad *pad = NULL;
> +	struct v4l2_subdev_format fmt = *format;
> +	bool found = false;
> +	int ret;
> +
> +	/*
> +	 * Starting from sensor subdevice, walk within
> +	 * pipeline and set format on each subdevice
> +	 */
> +	while (1) {
> +		unsigned int i;
> +
> +		/* Search if current entity has a source pad */
> +		for (i = 0; i < entity->num_pads; i++) {
> +			pad = &entity->pads[i];
> +			if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> +				src_pad = pad;
> +				found = true;
> +				break;
> +			}
> +		}
> +		if (!found)
> +			break;
> +
> +		subdev = media_entity_to_v4l2_subdev(entity);
> +
> +		/* Propagate format on sink pad if any, otherwise source pad */
> +		if (sink_pad)
> +			pad = sink_pad;
> +
> +		dev_dbg(dcmi->dev, "%s[%d] pad format set to 0x%x %ux%u\n",
> +			subdev->name, pad->index, format->format.code,
> +			format->format.width, format->format.height);
> +
> +		fmt.pad = pad->index;
> +		ret = v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);

Generally speaking, on MC-centric devices, the user space needs to
configure the pipeline. The driver's responsibility is to validate it
(through the link_validate media entity and subdev pad ops). I.e. set_fmt
is only used through the subdev nodes.

> +		if (ret < 0)
> +			return ret;
> +
> +		/* Walk to next entity */
> +		sink_pad = media_entity_remote_pad(src_pad);
> +		if (!sink_pad || !is_media_entity_v4l2_subdev(sink_pad->entity))
> +			break;
> +
> +		entity = sink_pad->entity;
> +	}
> +	*format = fmt;
> +
> +	return 0;
> +}
> +
> +static int dcmi_pipeline_s_stream(struct stm32_dcmi *dcmi, int state)
> +{
> +	struct media_entity *entity = &dcmi->vdev->entity;
> +	struct v4l2_subdev *subdev;
> +	struct media_pad *pad;
> +	int ret;
> +
> +	/* Start/stop all entities within pipeline */
> +	while (1) {
> +		pad = &entity->pads[0];
> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
> +			break;
> +
> +		pad = media_entity_remote_pad(pad);
> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> +			break;
> +
> +		entity = pad->entity;
> +		subdev = media_entity_to_v4l2_subdev(entity);
> +
> +		ret = v4l2_subdev_call(subdev, video, s_stream, state);

Please only call this on the next upstream sub-device. See e.g. the
ipu3-cio2 or omap3isp driver for an example.

> +		if (ret < 0 && ret != -ENOIOCTLCMD) {
> +			dev_err(dcmi->dev, "%s: %s failed to %s streaming (%d)\n",
> +				__func__, subdev->name,
> +				state ? "start" : "stop", ret);
> +			return ret;
> +		}
> +
> +		dev_dbg(dcmi->dev, "%s is %s\n",
> +			subdev->name, state ? "started" : "stopped");
> +	}
> +
> +	return 0;
> +}
> +
> +static int dcmi_pipeline_start(struct stm32_dcmi *dcmi)
> +{
> +	return dcmi_pipeline_s_stream(dcmi, 1);
> +}
> +
> +static void dcmi_pipeline_stop(struct stm32_dcmi *dcmi)
> +{
> +	dcmi_pipeline_s_stream(dcmi, 0);
> +}
> +
>  static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
> @@ -598,14 +728,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		goto err_release_buffers;
>  	}
>  
> -	/* Enable stream on the sub device */
> -	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 1);
> -	if (ret && ret != -ENOIOCTLCMD) {
> -		dev_err(dcmi->dev, "%s: Failed to start streaming, subdev streamon error",
> -			__func__);
> +	ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
> +	if (ret < 0) {
> +		dev_err(dcmi->dev, "%s: Failed to start streaming, media pipeline start error (%d)\n",
> +			__func__, ret);
>  		goto err_pm_put;
>  	}
>  
> +	ret = dcmi_pipeline_start(dcmi);
> +	if (ret)
> +		goto err_media_pipeline_stop;
> +
>  	spin_lock_irq(&dcmi->irqlock);
>  
>  	/* Set bus width */
> @@ -677,7 +810,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret) {
>  		dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
>  			__func__);
> -		goto err_subdev_streamoff;
> +		goto err_pipeline_stop;
>  	}
>  
>  	/* Enable interruptions */
> @@ -688,8 +821,11 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	return 0;
>  
> -err_subdev_streamoff:
> -	v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
> +err_pipeline_stop:
> +	dcmi_pipeline_stop(dcmi);
> +
> +err_media_pipeline_stop:
> +	media_pipeline_stop(&dcmi->vdev->entity);
>  
>  err_pm_put:
>  	pm_runtime_put(dcmi->dev);
> @@ -714,13 +850,10 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
>  	struct dcmi_buf *buf, *node;
> -	int ret;
>  
> -	/* Disable stream on the sub device */
> -	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
> -	if (ret && ret != -ENOIOCTLCMD)
> -		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
> -			__func__, ret);
> +	dcmi_pipeline_stop(dcmi);
> +
> +	media_pipeline_stop(&dcmi->vdev->entity);
>  
>  	spin_lock_irq(&dcmi->irqlock);
>  
> @@ -938,8 +1071,7 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
>  	mf->width = sd_framesize.width;
>  	mf->height = sd_framesize.height;
>  
> -	ret = v4l2_subdev_call(dcmi->sensor, pad,
> -			       set_fmt, NULL, &format);
> +	ret = dcmi_pipeline_s_fmt(dcmi, NULL, &format);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1530,7 +1662,19 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>  	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
>  	int ret;
>  
> +	/*
> +	 * Now that the graph is complete,
> +	 * we search for the camera sensor subdevice
> +	 * in order to expose it through V4L2 interface
> +	 */
> +	dcmi->sensor = media_entity_to_v4l2_subdev(dcmi_find_sensor(dcmi));
> +	if (!dcmi->sensor) {
> +		dev_err(dcmi->dev, "No camera sensor subdevice found\n");
> +		return -ENODEV;
> +	}
> +
>  	dcmi->vdev->ctrl_handler = dcmi->sensor->ctrl_handler;
> +
>  	ret = dcmi_formats_init(dcmi);
>  	if (ret) {
>  		dev_err(dcmi->dev, "No supported mediabus format found\n");
> @@ -1574,12 +1718,30 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd)
>  {
>  	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
> +	unsigned int ret;
> +	int src_pad;
>  
>  	dev_dbg(dcmi->dev, "Subdev %s bound\n", subdev->name);
>  
> -	dcmi->sensor = subdev;
> +	/*
> +	 * Link this sub-device to DCMI, it could be
> +	 * a parallel camera sensor or a bridge
> +	 */
> +	src_pad = media_entity_get_fwnode_pad(&subdev->entity,
> +					      subdev->fwnode,
> +					      MEDIA_PAD_FL_SOURCE);
> +
> +	ret = media_create_pad_link(&subdev->entity, src_pad,
> +				    &dcmi->vdev->entity, 0,
> +				    MEDIA_LNK_FL_IMMUTABLE |
> +				    MEDIA_LNK_FL_ENABLED);
> +	if (ret)
> +		dev_err(dcmi->dev, "Failed to create media pad link with subdev %s\n",
> +			subdev->name);
> +	else
> +		dev_dbg(dcmi->dev, "DCMI is now linked to %s\n", subdev->name);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct v4l2_async_notifier_operations dcmi_graph_notify_ops = {
> @@ -1639,6 +1801,15 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>  		return ret;
>  	}
>  
> +	/* Register all the subdev nodes */
> +	ret = v4l2_device_register_subdev_nodes(&dcmi->v4l2_dev);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register subdev nodes\n");
> +		v4l2_async_notifier_unregister(&dcmi->notifier);
> +		of_node_put(dcmi->entity.node);
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  

-- 
regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 3/3] media: stm32-dcmi: add support of several sub-devices

[Hugues FRUCHET]

Hi Sakari,

On 6/20/19 5:54 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> Thank you for the update.
> 
> On Tue, Jun 11, 2019 at 10:48:32AM +0200, Hugues Fruchet wrote:
>> Add support of several sub-devices within pipeline instead
>> of a single one.
>> This allows to support a CSI-2 camera sensor connected
>> through a CSI-2 to parallel bridge.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/platform/stm32/stm32-dcmi.c | 207 +++++++++++++++++++++++++++---
>>   1 file changed, 189 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
>> index 3a69783..144912f 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -173,6 +173,7 @@ struct stm32_dcmi {
>>   
>>   	struct media_device		mdev;
>>   	struct media_pad		vid_cap_pad;
>> +	struct media_pipeline		pipeline;
>>   };
>>   
>>   static inline struct stm32_dcmi *notifier_to_dcmi(struct v4l2_async_notifier *n)
>> @@ -584,6 +585,135 @@ static void dcmi_buf_queue(struct vb2_buffer *vb)
>>   	spin_unlock_irq(&dcmi->irqlock);
>>   }
>>   
>> +static struct media_entity *dcmi_find_sensor(struct stm32_dcmi *dcmi)
> 
> You generally should be only concerned with the next entity connected to the
> one you're in control of, not the rest of the pipeline.

This was discussed with Laurent here:
https://www.spinics.net/lists/linux-media/msg153417.html
and it's OK because DCMI is a video node and we are not
exposing media controller interface to userspace.

> 
>> +{
>> +	struct media_entity *entity = &dcmi->vdev->entity;
>> +	struct v4l2_subdev *subdev;
>> +	struct media_pad *pad;
>> +
>> +	/* Walk searching for entity having no sink */
>> +	while (1) {
>> +		pad = &entity->pads[0];
>> +
>> +		subdev = media_entity_to_v4l2_subdev(entity);
>> +
>> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
>> +			break;
>> +
>> +		pad = media_entity_remote_pad(pad);
>> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>> +			break;
>> +
>> +		entity = pad->entity;
>> +	}
>> +
>> +	return entity;
>> +}
>> +
>> +static int dcmi_pipeline_s_fmt(struct stm32_dcmi *dcmi,
>> +			       struct v4l2_subdev_pad_config *pad_cfg,
>> +			       struct v4l2_subdev_format *format)
>> +{
>> +	struct media_entity *entity = &dcmi->sensor->entity;
>> +	struct v4l2_subdev *subdev;
>> +	struct media_pad *sink_pad = NULL;
>> +	struct media_pad *src_pad = NULL;
>> +	struct media_pad *pad = NULL;
>> +	struct v4l2_subdev_format fmt = *format;
>> +	bool found = false;
>> +	int ret;
>> +
>> +	/*
>> +	 * Starting from sensor subdevice, walk within
>> +	 * pipeline and set format on each subdevice
>> +	 */
>> +	while (1) {
>> +		unsigned int i;
>> +
>> +		/* Search if current entity has a source pad */
>> +		for (i = 0; i < entity->num_pads; i++) {
>> +			pad = &entity->pads[i];
>> +			if (pad->flags & MEDIA_PAD_FL_SOURCE) {
>> +				src_pad = pad;
>> +				found = true;
>> +				break;
>> +			}
>> +		}
>> +		if (!found)
>> +			break;
>> +
>> +		subdev = media_entity_to_v4l2_subdev(entity);
>> +
>> +		/* Propagate format on sink pad if any, otherwise source pad */
>> +		if (sink_pad)
>> +			pad = sink_pad;
>> +
>> +		dev_dbg(dcmi->dev, "%s[%d] pad format set to 0x%x %ux%u\n",
>> +			subdev->name, pad->index, format->format.code,
>> +			format->format.width, format->format.height);
>> +
>> +		fmt.pad = pad->index;
>> +		ret = v4l2_subdev_call(subdev, pad, set_fmt, pad_cfg, &fmt);
> 
> Generally speaking, on MC-centric devices, the user space needs to
> configure the pipeline. The driver's responsibility is to validate it
> (through the link_validate media entity and subdev pad ops). I.e. set_fmt
> is only used through the subdev nodes.
> 
ditto

>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		/* Walk to next entity */
>> +		sink_pad = media_entity_remote_pad(src_pad);
>> +		if (!sink_pad || !is_media_entity_v4l2_subdev(sink_pad->entity))
>> +			break;
>> +
>> +		entity = sink_pad->entity;
>> +	}
>> +	*format = fmt;
>> +
>> +	return 0;
>> +}
>> +
>> +static int dcmi_pipeline_s_stream(struct stm32_dcmi *dcmi, int state)
>> +{
>> +	struct media_entity *entity = &dcmi->vdev->entity;
>> +	struct v4l2_subdev *subdev;
>> +	struct media_pad *pad;
>> +	int ret;
>> +
>> +	/* Start/stop all entities within pipeline */
>> +	while (1) {
>> +		pad = &entity->pads[0];
>> +		if (!(pad->flags & MEDIA_PAD_FL_SINK))
>> +			break;
>> +
>> +		pad = media_entity_remote_pad(pad);
>> +		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
>> +			break;
>> +
>> +		entity = pad->entity;
>> +		subdev = media_entity_to_v4l2_subdev(entity);
>> +
>> +		ret = v4l2_subdev_call(subdev, video, s_stream, state);
> 
> Please only call this on the next upstream sub-device. See e.g. the
> ipu3-cio2 or omap3isp driver for an example.
> 
ditto

>> +		if (ret < 0 && ret != -ENOIOCTLCMD) {
>> +			dev_err(dcmi->dev, "%s: %s failed to %s streaming (%d)\n",
>> +				__func__, subdev->name,
>> +				state ? "start" : "stop", ret);
>> +			return ret;
>> +		}
>> +
>> +		dev_dbg(dcmi->dev, "%s is %s\n",
>> +			subdev->name, state ? "started" : "stopped");
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dcmi_pipeline_start(struct stm32_dcmi *dcmi)
>> +{
>> +	return dcmi_pipeline_s_stream(dcmi, 1);
>> +}
>> +
>> +static void dcmi_pipeline_stop(struct stm32_dcmi *dcmi)
>> +{
>> +	dcmi_pipeline_s_stream(dcmi, 0);
>> +}
>> +
>>   static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   {
>>   	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
>> @@ -598,14 +728,17 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   		goto err_release_buffers;
>>   	}
>>   
>> -	/* Enable stream on the sub device */
>> -	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 1);
>> -	if (ret && ret != -ENOIOCTLCMD) {
>> -		dev_err(dcmi->dev, "%s: Failed to start streaming, subdev streamon error",
>> -			__func__);
>> +	ret = media_pipeline_start(&dcmi->vdev->entity, &dcmi->pipeline);
>> +	if (ret < 0) {
>> +		dev_err(dcmi->dev, "%s: Failed to start streaming, media pipeline start error (%d)\n",
>> +			__func__, ret);
>>   		goto err_pm_put;
>>   	}
>>   
>> +	ret = dcmi_pipeline_start(dcmi);
>> +	if (ret)
>> +		goto err_media_pipeline_stop;
>> +
>>   	spin_lock_irq(&dcmi->irqlock);
>>   
>>   	/* Set bus width */
>> @@ -677,7 +810,7 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	if (ret) {
>>   		dev_err(dcmi->dev, "%s: Start streaming failed, cannot start capture\n",
>>   			__func__);
>> -		goto err_subdev_streamoff;
>> +		goto err_pipeline_stop;
>>   	}
>>   
>>   	/* Enable interruptions */
>> @@ -688,8 +821,11 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   
>>   	return 0;
>>   
>> -err_subdev_streamoff:
>> -	v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
>> +err_pipeline_stop:
>> +	dcmi_pipeline_stop(dcmi);
>> +
>> +err_media_pipeline_stop:
>> +	media_pipeline_stop(&dcmi->vdev->entity);
>>   
>>   err_pm_put:
>>   	pm_runtime_put(dcmi->dev);
>> @@ -714,13 +850,10 @@ static void dcmi_stop_streaming(struct vb2_queue *vq)
>>   {
>>   	struct stm32_dcmi *dcmi = vb2_get_drv_priv(vq);
>>   	struct dcmi_buf *buf, *node;
>> -	int ret;
>>   
>> -	/* Disable stream on the sub device */
>> -	ret = v4l2_subdev_call(dcmi->sensor, video, s_stream, 0);
>> -	if (ret && ret != -ENOIOCTLCMD)
>> -		dev_err(dcmi->dev, "%s: Failed to stop streaming, subdev streamoff error (%d)\n",
>> -			__func__, ret);
>> +	dcmi_pipeline_stop(dcmi);
>> +
>> +	media_pipeline_stop(&dcmi->vdev->entity);
>>   
>>   	spin_lock_irq(&dcmi->irqlock);
>>   
>> @@ -938,8 +1071,7 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
>>   	mf->width = sd_framesize.width;
>>   	mf->height = sd_framesize.height;
>>   
>> -	ret = v4l2_subdev_call(dcmi->sensor, pad,
>> -			       set_fmt, NULL, &format);
>> +	ret = dcmi_pipeline_s_fmt(dcmi, NULL, &format);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> @@ -1530,7 +1662,19 @@ static int dcmi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>   	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
>>   	int ret;
>>   
>> +	/*
>> +	 * Now that the graph is complete,
>> +	 * we search for the camera sensor subdevice
>> +	 * in order to expose it through V4L2 interface
>> +	 */
>> +	dcmi->sensor = media_entity_to_v4l2_subdev(dcmi_find_sensor(dcmi));
>> +	if (!dcmi->sensor) {
>> +		dev_err(dcmi->dev, "No camera sensor subdevice found\n");
>> +		return -ENODEV;
>> +	}
>> +
>>   	dcmi->vdev->ctrl_handler = dcmi->sensor->ctrl_handler;
>> +
>>   	ret = dcmi_formats_init(dcmi);
>>   	if (ret) {
>>   		dev_err(dcmi->dev, "No supported mediabus format found\n");
>> @@ -1574,12 +1718,30 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
>>   				   struct v4l2_async_subdev *asd)
>>   {
>>   	struct stm32_dcmi *dcmi = notifier_to_dcmi(notifier);
>> +	unsigned int ret;
>> +	int src_pad;
>>   
>>   	dev_dbg(dcmi->dev, "Subdev %s bound\n", subdev->name);
>>   
>> -	dcmi->sensor = subdev;
>> +	/*
>> +	 * Link this sub-device to DCMI, it could be
>> +	 * a parallel camera sensor or a bridge
>> +	 */
>> +	src_pad = media_entity_get_fwnode_pad(&subdev->entity,
>> +					      subdev->fwnode,
>> +					      MEDIA_PAD_FL_SOURCE);
>> +
>> +	ret = media_create_pad_link(&subdev->entity, src_pad,
>> +				    &dcmi->vdev->entity, 0,
>> +				    MEDIA_LNK_FL_IMMUTABLE |
>> +				    MEDIA_LNK_FL_ENABLED);
>> +	if (ret)
>> +		dev_err(dcmi->dev, "Failed to create media pad link with subdev %s\n",
>> +			subdev->name);
>> +	else
>> +		dev_dbg(dcmi->dev, "DCMI is now linked to %s\n", subdev->name);
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static const struct v4l2_async_notifier_operations dcmi_graph_notify_ops = {
>> @@ -1639,6 +1801,15 @@ static int dcmi_graph_init(struct stm32_dcmi *dcmi)
>>   		return ret;
>>   	}
>>   
>> +	/* Register all the subdev nodes */
>> +	ret = v4l2_device_register_subdev_nodes(&dcmi->v4l2_dev);
>> +	if (ret) {
>> +		dev_err(dcmi->dev, "Failed to register subdev nodes\n");
>> +		v4l2_async_notifier_unregister(&dcmi->notifier);
>> +		of_node_put(dcmi->entity.node);
>> +		return ret;
>> +	}
>> +
>>   	return 0;
>>   }
>>   
> 

BR,
Hugues.

Re: [PATCH v2 0/3] DCMI bridge support

[Sakari Ailus]

Hi Hugues,

On Tue, Jun 11, 2019 at 10:48:29AM +0200, Hugues Fruchet wrote:
> This patch serie allows to connect non-parallel camera sensor to
> DCMI thanks to a bridge connected in between such as STMIPID02 [1].
> 
> Media controller support is introduced first, then support of
> several sub-devices within pipeline with dynamic linking
> between them.
> In order to keep backward compatibility with applications
> relying on V4L2 interface only, format set on video node
> is propagated to all sub-devices connected to camera interface.
> 
> [1] https://www.spinics.net/lists/devicetree/msg278002.html

General notes on the set, not related to any single patch:

- Where's the sub-device representing the bridge itself?

- As the driver becomes MC-centric, crop configuration takes place through
  V4L2 sub-device interface, not through the video device node.

- Same goes for accessing sensor configuration: it does not take place
  through video node but through the sub-device nodes.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 0/3] DCMI bridge support

[Hugues FRUCHET]

Hi Sakari,

 > - Where's the sub-device representing the bridge itself?
This is pointed by [1]: drivers/media/i2c/st-mipid02.c

 > - As the driver becomes MC-centric, crop configuration takes place
through
 >   V4L2 sub-device interface, not through the video device node.
 > - Same goes for accessing sensor configuration: it does not take place
 >   through video node but through the sub-device nodes.

Our objective is to be able to support either a simple parallel sensor
or a CSI-2 sensor connected through a bridge without any changes on 
userspace side because no additional processing or conversion involved, 
only deserialisation is m.
With the proposed set of patches, we succeeded to do so, the same 
non-regression tests campaign is passed with OV5640 parallel sensor 
(STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with 
D3 mezzanine board).

We don't want driver to be MC-centric, media controller support was 
required only to get access to the set of functions needed to link and
walk trough subdevices: media_create_pad_link(), 
media_entity_remote_pad(), etc...

We did a try with the v1 version of this patchset, delegating subdevices 
handling to userspace, by using media-controller, but this require to 
configure first the pipeline for each single change of resolution and 
format before making any capture using v4l2-ctl or GStreamer, quite 
heavy in fact.
Benjamin did another try using new libcamera codebase, but even for a 
basic capture use-case, negotiation code is quite tricky in order to
match the right subdevices bus format to the required V4L2 format.
Moreover, it was not clear how to call libcamera library prior to any
v4l2-ctl or GStreamer calls.

Adding 100 lines of code into DCMI to well configure resolution and 
formats fixes the point and allows us to keep backward compatibility
as per our objective, so it seems far more reasonable to us to do so
even if DCMI controls more than the subdevice it is connected to.
Moreover we found similar code in other video interfaces code like 
qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole 
pipeline, so it seems to us quite natural to go this way.

To summarize, if we cannot do the negotiation within kernel, delegating
this to userspace implies far more complexity and breaks compatibility
with existing applications without adding new functionalities.

Having all that in mind, what should be reconsidered in your opinion 
Sakari ? Do you have some alternatives ?

Best regards,
Hugues.


On 6/20/19 6:17 PM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Tue, Jun 11, 2019 at 10:48:29AM +0200, Hugues Fruchet wrote:
>> This patch serie allows to connect non-parallel camera sensor to
>> DCMI thanks to a bridge connected in between such as STMIPID02 [1].
>>
>> Media controller support is introduced first, then support of
>> several sub-devices within pipeline with dynamic linking
>> between them.
>> In order to keep backward compatibility with applications
>> relying on V4L2 interface only, format set on video node
>> is propagated to all sub-devices connected to camera interface.
>>
>> [1] https://www.spinics.net/lists/devicetree/msg278002.html
> 
> General notes on the set, not related to any single patch:
> 
> - Where's the sub-device representing the bridge itself?
> 
> - As the driver becomes MC-centric, crop configuration takes place through
>    V4L2 sub-device interface, not through the video device node.
> 
> - Same goes for accessing sensor configuration: it does not take place
>    through video node but through the sub-device nodes.
> 

Re: [PATCH v2 0/3] DCMI bridge support

[Laurent Pinchart]

Hi Hugues,

On Mon, Jun 24, 2019 at 10:10:05AM +0000, Hugues FRUCHET wrote:
> Hi Sakari,
> 
>  > - Where's the sub-device representing the bridge itself?
> This is pointed by [1]: drivers/media/i2c/st-mipid02.c
> 
>  > - As the driver becomes MC-centric, crop configuration takes place
> through
>  >   V4L2 sub-device interface, not through the video device node.
>  > - Same goes for accessing sensor configuration: it does not take place
>  >   through video node but through the sub-device nodes.
> 
> Our objective is to be able to support either a simple parallel sensor
> or a CSI-2 sensor connected through a bridge without any changes on 
> userspace side because no additional processing or conversion involved, 
> only deserialisation is m.
> With the proposed set of patches, we succeeded to do so, the same 
> non-regression tests campaign is passed with OV5640 parallel sensor 
> (STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with 
> D3 mezzanine board).
> 
> We don't want driver to be MC-centric, media controller support was 
> required only to get access to the set of functions needed to link and
> walk trough subdevices: media_create_pad_link(), 
> media_entity_remote_pad(), etc...
> 
> We did a try with the v1 version of this patchset, delegating subdevices 
> handling to userspace, by using media-controller, but this require to 
> configure first the pipeline for each single change of resolution and 
> format before making any capture using v4l2-ctl or GStreamer, quite 
> heavy in fact.
> Benjamin did another try using new libcamera codebase, but even for a 
> basic capture use-case, negotiation code is quite tricky in order to
> match the right subdevices bus format to the required V4L2 format.

Why would it be trickier in userspace than in the kernel ? The V4L2
subdev operations are more or less expose verbatim through the subdev
userspace API.

> Moreover, it was not clear how to call libcamera library prior to any
> v4l2-ctl or GStreamer calls.

libcamera isn't meant to be called before v4l2-ctl or GStreamer.
Applications are supposed to be based directly on libcamera, or, for
existing userspace APIs such as V4L2 or GStreamer, compatibility layers
are supposed to be developed. For V4L2 it will take the form of a
LD_PRELOAD-able .so that will intercept the V4L2 API calls, making most
V4L2 applications work with libcamera unmodified (I said most as 100%
compatibility will likely not be achievable). For GStreamer it will take
the form of a GStreamer libcamera element that will replace the V4L2
source element.

> Adding 100 lines of code into DCMI to well configure resolution and 
> formats fixes the point and allows us to keep backward compatibility
> as per our objective, so it seems far more reasonable to us to do so
> even if DCMI controls more than the subdevice it is connected to.
> Moreover we found similar code in other video interfaces code like 
> qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole 
> pipeline, so it seems to us quite natural to go this way.

I can't comment on the qcom-camss driver as I'm not aware of its
internals, but where have you found such code in the Xilinx V4L2 drivers
?

> To summarize, if we cannot do the negotiation within kernel, delegating
> this to userspace implies far more complexity and breaks compatibility
> with existing applications without adding new functionalities.
> 
> Having all that in mind, what should be reconsidered in your opinion 
> Sakari ? Do you have some alternatives ?

First of all, let's note that your patch series performs to related but
still independent changes: it enables MC support, *and* enables the V4L2
subdev userspace API. The former is clearly needed and will allow you to
use the MC API internally in the kernel, simplifying pipeline traversal.
The latter then enables the V4L2 subdev userspace API, moving the
pipeline configuration responsibility to userspace.

You could in theory move to the MC API inside the kernel, without
enabling support for the V4L2 subdev userspace API. Configuring the
pipeline and propagating the formats would then be the responsibility of
the kernel driver. However, this will limit your driver to the
following:

- Fully linear pipelines only (single sensor)
- No support for controls implemented by multiple entities in the
  pipeline (for instance controls that would exist in both the sensor
  and the bridge, such as gains)
- No proper support for scaling configuration if multiple components in
  the pipeline can scale

Are you willing to set those limitations in stone and give up on
supporting those features ?

> On 6/20/19 6:17 PM, Sakari Ailus wrote:
> > On Tue, Jun 11, 2019 at 10:48:29AM +0200, Hugues Fruchet wrote:
> >> This patch serie allows to connect non-parallel camera sensor to
> >> DCMI thanks to a bridge connected in between such as STMIPID02 [1].
> >>
> >> Media controller support is introduced first, then support of
> >> several sub-devices within pipeline with dynamic linking
> >> between them.
> >> In order to keep backward compatibility with applications
> >> relying on V4L2 interface only, format set on video node
> >> is propagated to all sub-devices connected to camera interface.
> >>
> >> [1] https://www.spinics.net/lists/devicetree/msg278002.html
> > 
> > General notes on the set, not related to any single patch:
> > 
> > - Where's the sub-device representing the bridge itself?
> > 
> > - As the driver becomes MC-centric, crop configuration takes place through
> >    V4L2 sub-device interface, not through the video device node.
> > 
> > - Same goes for accessing sensor configuration: it does not take place
> >    through video node but through the sub-device nodes.
> > 

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] DCMI bridge support

[Hugues FRUCHET]

Hi Laurent,

Thanks for reviewing,

On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> Hi Hugues,
> 
> On Mon, Jun 24, 2019 at 10:10:05AM +0000, Hugues FRUCHET wrote:
>> Hi Sakari,
>>
>>   > - Where's the sub-device representing the bridge itself?
>> This is pointed by [1]: drivers/media/i2c/st-mipid02.c
>>
>>   > - As the driver becomes MC-centric, crop configuration takes place
>> through
>>   >   V4L2 sub-device interface, not through the video device node.
>>   > - Same goes for accessing sensor configuration: it does not take place
>>   >   through video node but through the sub-device nodes.
>>
>> Our objective is to be able to support either a simple parallel sensor
>> or a CSI-2 sensor connected through a bridge without any changes on
>> userspace side because no additional processing or conversion involved,
>> only deserialisation is m.
>> With the proposed set of patches, we succeeded to do so, the same
>> non-regression tests campaign is passed with OV5640 parallel sensor
>> (STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with
>> D3 mezzanine board).
>>
>> We don't want driver to be MC-centric, media controller support was
>> required only to get access to the set of functions needed to link and
>> walk trough subdevices: media_create_pad_link(),
>> media_entity_remote_pad(), etc...
>>
>> We did a try with the v1 version of this patchset, delegating subdevices
>> handling to userspace, by using media-controller, but this require to
>> configure first the pipeline for each single change of resolution and
>> format before making any capture using v4l2-ctl or GStreamer, quite
>> heavy in fact.
>> Benjamin did another try using new libcamera codebase, but even for a
>> basic capture use-case, negotiation code is quite tricky in order to
>> match the right subdevices bus format to the required V4L2 format.
> 
> Why would it be trickier in userspace than in the kernel ? The V4L2
> subdev operations are more or less expose verbatim through the subdev
> userspace API.
> 
>> Moreover, it was not clear how to call libcamera library prior to any
>> v4l2-ctl or GStreamer calls.
> 
> libcamera isn't meant to be called before v4l2-ctl or GStreamer.
> Applications are supposed to be based directly on libcamera, or, for
> existing userspace APIs such as V4L2 or GStreamer, compatibility layers
> are supposed to be developed. For V4L2 it will take the form of a
> LD_PRELOAD-able .so that will intercept the V4L2 API calls, making most
> V4L2 applications work with libcamera unmodified (I said most as 100%
> compatibility will likely not be achievable). For GStreamer it will take
> the form of a GStreamer libcamera element that will replace the V4L2
> source element.
> 
>> Adding 100 lines of code into DCMI to well configure resolution and
>> formats fixes the point and allows us to keep backward compatibility
>> as per our objective, so it seems far more reasonable to us to do so
>> even if DCMI controls more than the subdevice it is connected to.
>> Moreover we found similar code in other video interfaces code like
>> qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole
>> pipeline, so it seems to us quite natural to go this way.
> 
> I can't comment on the qcom-camss driver as I'm not aware of its
> internals, but where have you found such code in the Xilinx V4L2 drivers
> ?
For ex. in xilinx/xilinx-dma.c, stream on/off is propagated to all 
subdevices within pipeline:
  * Walk the entities chain starting at the pipeline output video node 
static int xvip_pipeline_start_stop(struct xvip_pipeline *pipe, bool start)

Same for qcom/camss/camss-video.c:
static int video_start_streaming(struct vb2_queue *q, unsigned int count)

For resolution/format, in exynos4-is/fimc-capture.c:
static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
...
	while (1) {
...
		/* set format on all pipeline subdevs */
		while (me != &fimc->vid_cap.subdev.entity) {
...
			ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);

> 
>> To summarize, if we cannot do the negotiation within kernel, delegating
>> this to userspace implies far more complexity and breaks compatibility
>> with existing applications without adding new functionalities.
>>
>> Having all that in mind, what should be reconsidered in your opinion
>> Sakari ? Do you have some alternatives ?
> 
> First of all, let's note that your patch series performs to related but
> still independent changes: it enables MC support, *and* enables the V4L2
> subdev userspace API. The former is clearly needed and will allow you to
> use the MC API internally in the kernel, simplifying pipeline traversal.
> The latter then enables the V4L2 subdev userspace API, moving the
> pipeline configuration responsibility to userspace.
> 
> You could in theory move to the MC API inside the kernel, without
> enabling support for the V4L2 subdev userspace API. Configuring the
> pipeline and propagating the formats would then be the responsibility of
> the kernel driver.

Yes this is exactly what we want to do.
If I understand well, to disable the V4L2 subdev userspace API, I just 
have to remove the media device registry:
-	/* Register the media device */
-	ret = media_device_register(&dcmi->mdev);
-	if (ret) {
-		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
-			ret);
-		goto err_media_device_cleanup;
-	}
Do you see any additional things to do ?


> However, this will limit your driver to the
> following:
> 
> - Fully linear pipelines only (single sensor)
> - No support for controls implemented by multiple entities in the
>    pipeline (for instance controls that would exist in both the sensor
>    and the bridge, such as gains)
> - No proper support for scaling configuration if multiple components in
>    the pipeline can scale
> 
> Are you willing to set those limitations in stone and give up on
> supporting those features ?
> 

The involved hardware do not have those features, no need of extra 
functionalities to be exposed to userspace, so this is fine.


I'll push a v3 with this change and the other fixes related to Sakari 
and Hans comments.

Please Sakari & Hans, also comment on that change that we can converge 
on v3.


Best regards,
Hugues.

Re: [PATCH v2 0/3] DCMI bridge support

[Laurent Pinchart]

Hi Hugues,

On Thu, Jun 27, 2019 at 12:38:40PM +0000, Hugues FRUCHET wrote:
> On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> > On Mon, Jun 24, 2019 at 10:10:05AM +0000, Hugues FRUCHET wrote:
> >> Hi Sakari,
> >>
> >>> - Where's the sub-device representing the bridge itself?
> >>
> >> This is pointed by [1]: drivers/media/i2c/st-mipid02.c
> >>
> >>> - As the driver becomes MC-centric, crop configuration takes place through
> >>>   V4L2 sub-device interface, not through the video device node.
> >>> - Same goes for accessing sensor configuration: it does not take place
> >>>   through video node but through the sub-device nodes.
> >>
> >> Our objective is to be able to support either a simple parallel sensor
> >> or a CSI-2 sensor connected through a bridge without any changes on
> >> userspace side because no additional processing or conversion involved,
> >> only deserialisation is m.
> >> With the proposed set of patches, we succeeded to do so, the same
> >> non-regression tests campaign is passed with OV5640 parallel sensor
> >> (STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with
> >> D3 mezzanine board).
> >>
> >> We don't want driver to be MC-centric, media controller support was
> >> required only to get access to the set of functions needed to link and
> >> walk trough subdevices: media_create_pad_link(),
> >> media_entity_remote_pad(), etc...
> >>
> >> We did a try with the v1 version of this patchset, delegating subdevices
> >> handling to userspace, by using media-controller, but this require to
> >> configure first the pipeline for each single change of resolution and
> >> format before making any capture using v4l2-ctl or GStreamer, quite
> >> heavy in fact.
> >> Benjamin did another try using new libcamera codebase, but even for a
> >> basic capture use-case, negotiation code is quite tricky in order to
> >> match the right subdevices bus format to the required V4L2 format.
> > 
> > Why would it be trickier in userspace than in the kernel ? The V4L2
> > subdev operations are more or less expose verbatim through the subdev
> > userspace API.
> > 
> >> Moreover, it was not clear how to call libcamera library prior to any
> >> v4l2-ctl or GStreamer calls.
> > 
> > libcamera isn't meant to be called before v4l2-ctl or GStreamer.
> > Applications are supposed to be based directly on libcamera, or, for
> > existing userspace APIs such as V4L2 or GStreamer, compatibility layers
> > are supposed to be developed. For V4L2 it will take the form of a
> > LD_PRELOAD-able .so that will intercept the V4L2 API calls, making most
> > V4L2 applications work with libcamera unmodified (I said most as 100%
> > compatibility will likely not be achievable). For GStreamer it will take
> > the form of a GStreamer libcamera element that will replace the V4L2
> > source element.
> > 
> >> Adding 100 lines of code into DCMI to well configure resolution and
> >> formats fixes the point and allows us to keep backward compatibility
> >> as per our objective, so it seems far more reasonable to us to do so
> >> even if DCMI controls more than the subdevice it is connected to.
> >> Moreover we found similar code in other video interfaces code like
> >> qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole
> >> pipeline, so it seems to us quite natural to go this way.
> > 
> > I can't comment on the qcom-camss driver as I'm not aware of its
> > internals, but where have you found such code in the Xilinx V4L2 drivers
> > ?
> 
> For ex. in xilinx/xilinx-dma.c, stream on/off is propagated to all 
> subdevices within pipeline:
>   * Walk the entities chain starting at the pipeline output video node 
> static int xvip_pipeline_start_stop(struct xvip_pipeline *pipe, bool start)
> 
> Same for qcom/camss/camss-video.c:
> static int video_start_streaming(struct vb2_queue *q, unsigned int count)

For stream start/stop, that's expected. Userspace only controls the
stream start/stop on the video node, and the kernel propagates that
along the pipeline. There is no VIDIOC_STREAMON or VIDIOC_STREAMOFF
ioctl exposed to userspace for V4L2 subdevs. What is not propagated in
the kernel for MC-centric devices is the pipeline configuration (formats
and selection rectangles).

> For resolution/format, in exynos4-is/fimc-capture.c:
> static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
> ...
> 	while (1) {
> ...
> 		/* set format on all pipeline subdevs */
> 		while (me != &fimc->vid_cap.subdev.entity) {
> ...
> 			ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);

As explained below, propagating formats is fine for video node-centric
drivers, but comes with limitations.

> >> To summarize, if we cannot do the negotiation within kernel, delegating
> >> this to userspace implies far more complexity and breaks compatibility
> >> with existing applications without adding new functionalities.
> >>
> >> Having all that in mind, what should be reconsidered in your opinion
> >> Sakari ? Do you have some alternatives ?
> > 
> > First of all, let's note that your patch series performs to related but
> > still independent changes: it enables MC support, *and* enables the V4L2
> > subdev userspace API. The former is clearly needed and will allow you to
> > use the MC API internally in the kernel, simplifying pipeline traversal.
> > The latter then enables the V4L2 subdev userspace API, moving the
> > pipeline configuration responsibility to userspace.
> > 
> > You could in theory move to the MC API inside the kernel, without
> > enabling support for the V4L2 subdev userspace API. Configuring the
> > pipeline and propagating the formats would then be the responsibility of
> > the kernel driver.
> 
> Yes this is exactly what we want to do.
> If I understand well, to disable the V4L2 subdev userspace API, I just 
> have to remove the media device registry:
> 
> -	/* Register the media device */
> -	ret = media_device_register(&dcmi->mdev);
> -	if (ret) {
> -		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
> -			ret);
> -		goto err_media_device_cleanup;
> -	}
> 
> Do you see any additional things to do ?

That should be it. Note that in that case pipeline configuration has to
be handled by the master driver (DCMI in this case), the external
subdevs involved (such as the CSI-2 to parallel bridge) must not handle
any propagation of formats or selection rectangles.

> > However, this will limit your driver to the
> > following:
> > 
> > - Fully linear pipelines only (single sensor)
> > - No support for controls implemented by multiple entities in the
> >    pipeline (for instance controls that would exist in both the sensor
> >    and the bridge, such as gains)
> > - No proper support for scaling configuration if multiple components in
> >    the pipeline can scale
> > 
> > Are you willing to set those limitations in stone and give up on
> > supporting those features ?
> > 
> 
> The involved hardware do not have those features, no need of extra 
> functionalities to be exposed to userspace, so this is fine.
> 
> I'll push a v3 with this change and the other fixes related to Sakari 
> and Hans comments.
> 
> Please Sakari & Hans, also comment on that change that we can converge 
> on v3.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] DCMI bridge support

[Sakari Ailus]

Hi Laurent,

On Thu, Jun 27, 2019 at 04:38:24PM +0300, Laurent Pinchart wrote:
> Hi Hugues,
> 
> On Thu, Jun 27, 2019 at 12:38:40PM +0000, Hugues FRUCHET wrote:
> > On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> > > On Mon, Jun 24, 2019 at 10:10:05AM +0000, Hugues FRUCHET wrote:
> > >> Hi Sakari,
> > >>
> > >>> - Where's the sub-device representing the bridge itself?
> > >>
> > >> This is pointed by [1]: drivers/media/i2c/st-mipid02.c
> > >>
> > >>> - As the driver becomes MC-centric, crop configuration takes place through
> > >>>   V4L2 sub-device interface, not through the video device node.
> > >>> - Same goes for accessing sensor configuration: it does not take place
> > >>>   through video node but through the sub-device nodes.
> > >>
> > >> Our objective is to be able to support either a simple parallel sensor
> > >> or a CSI-2 sensor connected through a bridge without any changes on
> > >> userspace side because no additional processing or conversion involved,
> > >> only deserialisation is m.
> > >> With the proposed set of patches, we succeeded to do so, the same
> > >> non-regression tests campaign is passed with OV5640 parallel sensor
> > >> (STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with
> > >> D3 mezzanine board).
> > >>
> > >> We don't want driver to be MC-centric, media controller support was
> > >> required only to get access to the set of functions needed to link and
> > >> walk trough subdevices: media_create_pad_link(),
> > >> media_entity_remote_pad(), etc...
> > >>
> > >> We did a try with the v1 version of this patchset, delegating subdevices
> > >> handling to userspace, by using media-controller, but this require to
> > >> configure first the pipeline for each single change of resolution and
> > >> format before making any capture using v4l2-ctl or GStreamer, quite
> > >> heavy in fact.
> > >> Benjamin did another try using new libcamera codebase, but even for a
> > >> basic capture use-case, negotiation code is quite tricky in order to
> > >> match the right subdevices bus format to the required V4L2 format.
> > > 
> > > Why would it be trickier in userspace than in the kernel ? The V4L2
> > > subdev operations are more or less expose verbatim through the subdev
> > > userspace API.
> > > 
> > >> Moreover, it was not clear how to call libcamera library prior to any
> > >> v4l2-ctl or GStreamer calls.
> > > 
> > > libcamera isn't meant to be called before v4l2-ctl or GStreamer.
> > > Applications are supposed to be based directly on libcamera, or, for
> > > existing userspace APIs such as V4L2 or GStreamer, compatibility layers
> > > are supposed to be developed. For V4L2 it will take the form of a
> > > LD_PRELOAD-able .so that will intercept the V4L2 API calls, making most
> > > V4L2 applications work with libcamera unmodified (I said most as 100%
> > > compatibility will likely not be achievable). For GStreamer it will take
> > > the form of a GStreamer libcamera element that will replace the V4L2
> > > source element.
> > > 
> > >> Adding 100 lines of code into DCMI to well configure resolution and
> > >> formats fixes the point and allows us to keep backward compatibility
> > >> as per our objective, so it seems far more reasonable to us to do so
> > >> even if DCMI controls more than the subdevice it is connected to.
> > >> Moreover we found similar code in other video interfaces code like
> > >> qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole
> > >> pipeline, so it seems to us quite natural to go this way.
> > > 
> > > I can't comment on the qcom-camss driver as I'm not aware of its
> > > internals, but where have you found such code in the Xilinx V4L2 drivers
> > > ?
> > 
> > For ex. in xilinx/xilinx-dma.c, stream on/off is propagated to all 
> > subdevices within pipeline:
> >   * Walk the entities chain starting at the pipeline output video node 
> > static int xvip_pipeline_start_stop(struct xvip_pipeline *pipe, bool start)
> > 
> > Same for qcom/camss/camss-video.c:
> > static int video_start_streaming(struct vb2_queue *q, unsigned int count)
> 
> For stream start/stop, that's expected. Userspace only controls the
> stream start/stop on the video node, and the kernel propagates that
> along the pipeline. There is no VIDIOC_STREAMON or VIDIOC_STREAMOFF
> ioctl exposed to userspace for V4L2 subdevs. What is not propagated in
> the kernel for MC-centric devices is the pipeline configuration (formats
> and selection rectangles).
> 
> > For resolution/format, in exynos4-is/fimc-capture.c:
> > static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
> > ...
> > 	while (1) {
> > ...
> > 		/* set format on all pipeline subdevs */
> > 		while (me != &fimc->vid_cap.subdev.entity) {
> > ...
> > 			ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);
> 
> As explained below, propagating formats is fine for video node-centric
> drivers, but comes with limitations.
> 
> > >> To summarize, if we cannot do the negotiation within kernel, delegating
> > >> this to userspace implies far more complexity and breaks compatibility
> > >> with existing applications without adding new functionalities.
> > >>
> > >> Having all that in mind, what should be reconsidered in your opinion
> > >> Sakari ? Do you have some alternatives ?
> > > 
> > > First of all, let's note that your patch series performs to related but
> > > still independent changes: it enables MC support, *and* enables the V4L2
> > > subdev userspace API. The former is clearly needed and will allow you to
> > > use the MC API internally in the kernel, simplifying pipeline traversal.
> > > The latter then enables the V4L2 subdev userspace API, moving the
> > > pipeline configuration responsibility to userspace.
> > > 
> > > You could in theory move to the MC API inside the kernel, without
> > > enabling support for the V4L2 subdev userspace API. Configuring the
> > > pipeline and propagating the formats would then be the responsibility of
> > > the kernel driver.
> > 
> > Yes this is exactly what we want to do.
> > If I understand well, to disable the V4L2 subdev userspace API, I just 
> > have to remove the media device registry:
> > 
> > -	/* Register the media device */
> > -	ret = media_device_register(&dcmi->mdev);
> > -	if (ret) {
> > -		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
> > -			ret);
> > -		goto err_media_device_cleanup;
> > -	}
> > 
> > Do you see any additional things to do ?
> 
> That should be it. Note that in that case pipeline configuration has to
> be handled by the master driver (DCMI in this case), the external
> subdevs involved (such as the CSI-2 to parallel bridge) must not handle
> any propagation of formats or selection rectangles.

I wonder what we'd do in the case when someone needs to connect something
else to the pipeline, such as a sensor with more than one sub-device, or a
flash or a lens controller.

For future-proofness, I'd just use MC for hardware that may be part of a
complex pipeline. In this case, if you think backwards compatibility is
important (and for most hardware it probably is), I don't think there are
perfect solutions if your existing driver is not MC-enabled.

A reasonable compromise would be to add a Kconfig option that allows
enabling MC. This way you can provide backwards compatibility and allow
making use of the full potential of the hardware. That's also why hardware
that may be part of a non-trivial MC pipeline should start with MC-enabled
so we wouldn't run into this.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 0/3] DCMI bridge support

[Laurent Pinchart]

Hi Sakari,

On Fri, Jul 05, 2019 at 10:55:22AM +0300, Sakari Ailus wrote:
> On Thu, Jun 27, 2019 at 04:38:24PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 27, 2019 at 12:38:40PM +0000, Hugues FRUCHET wrote:
> >> On 6/26/19 7:25 PM, Laurent Pinchart wrote:
> >>> On Mon, Jun 24, 2019 at 10:10:05AM +0000, Hugues FRUCHET wrote:
> >>>> Hi Sakari,
> >>>>
> >>>>> - Where's the sub-device representing the bridge itself?
> >>>>
> >>>> This is pointed by [1]: drivers/media/i2c/st-mipid02.c
> >>>>
> >>>>> - As the driver becomes MC-centric, crop configuration takes place through
> >>>>>   V4L2 sub-device interface, not through the video device node.
> >>>>> - Same goes for accessing sensor configuration: it does not take place
> >>>>>   through video node but through the sub-device nodes.
> >>>>
> >>>> Our objective is to be able to support either a simple parallel sensor
> >>>> or a CSI-2 sensor connected through a bridge without any changes on
> >>>> userspace side because no additional processing or conversion involved,
> >>>> only deserialisation is m.
> >>>> With the proposed set of patches, we succeeded to do so, the same
> >>>> non-regression tests campaign is passed with OV5640 parallel sensor
> >>>> (STM32MP1 evaluation board) or OV5640 CSI-2 sensor (Avenger96 board with
> >>>> D3 mezzanine board).
> >>>>
> >>>> We don't want driver to be MC-centric, media controller support was
> >>>> required only to get access to the set of functions needed to link and
> >>>> walk trough subdevices: media_create_pad_link(),
> >>>> media_entity_remote_pad(), etc...
> >>>>
> >>>> We did a try with the v1 version of this patchset, delegating subdevices
> >>>> handling to userspace, by using media-controller, but this require to
> >>>> configure first the pipeline for each single change of resolution and
> >>>> format before making any capture using v4l2-ctl or GStreamer, quite
> >>>> heavy in fact.
> >>>> Benjamin did another try using new libcamera codebase, but even for a
> >>>> basic capture use-case, negotiation code is quite tricky in order to
> >>>> match the right subdevices bus format to the required V4L2 format.
> >>> 
> >>> Why would it be trickier in userspace than in the kernel ? The V4L2
> >>> subdev operations are more or less expose verbatim through the subdev
> >>> userspace API.
> >>> 
> >>>> Moreover, it was not clear how to call libcamera library prior to any
> >>>> v4l2-ctl or GStreamer calls.
> >>> 
> >>> libcamera isn't meant to be called before v4l2-ctl or GStreamer.
> >>> Applications are supposed to be based directly on libcamera, or, for
> >>> existing userspace APIs such as V4L2 or GStreamer, compatibility layers
> >>> are supposed to be developed. For V4L2 it will take the form of a
> >>> LD_PRELOAD-able .so that will intercept the V4L2 API calls, making most
> >>> V4L2 applications work with libcamera unmodified (I said most as 100%
> >>> compatibility will likely not be achievable). For GStreamer it will take
> >>> the form of a GStreamer libcamera element that will replace the V4L2
> >>> source element.
> >>> 
> >>>> Adding 100 lines of code into DCMI to well configure resolution and
> >>>> formats fixes the point and allows us to keep backward compatibility
> >>>> as per our objective, so it seems far more reasonable to us to do so
> >>>> even if DCMI controls more than the subdevice it is connected to.
> >>>> Moreover we found similar code in other video interfaces code like
> >>>> qcom/camss/camss.c and xilinx/xilinx-dma.c, controlling the whole
> >>>> pipeline, so it seems to us quite natural to go this way.
> >>> 
> >>> I can't comment on the qcom-camss driver as I'm not aware of its
> >>> internals, but where have you found such code in the Xilinx V4L2 drivers
> >>> ?
> >> 
> >> For ex. in xilinx/xilinx-dma.c, stream on/off is propagated to all 
> >> subdevices within pipeline:
> >>   * Walk the entities chain starting at the pipeline output video node 
> >> static int xvip_pipeline_start_stop(struct xvip_pipeline *pipe, bool start)
> >> 
> >> Same for qcom/camss/camss-video.c:
> >> static int video_start_streaming(struct vb2_queue *q, unsigned int count)
> > 
> > For stream start/stop, that's expected. Userspace only controls the
> > stream start/stop on the video node, and the kernel propagates that
> > along the pipeline. There is no VIDIOC_STREAMON or VIDIOC_STREAMOFF
> > ioctl exposed to userspace for V4L2 subdevs. What is not propagated in
> > the kernel for MC-centric devices is the pipeline configuration (formats
> > and selection rectangles).
> > 
> >> For resolution/format, in exynos4-is/fimc-capture.c:
> >> static int fimc_pipeline_try_format(struct fimc_ctx *ctx,
> >> ...
> >> 	while (1) {
> >> ...
> >> 		/* set format on all pipeline subdevs */
> >> 		while (me != &fimc->vid_cap.subdev.entity) {
> >> ...
> >> 			ret = v4l2_subdev_call(sd, pad, set_fmt, NULL, &sfmt);
> > 
> > As explained below, propagating formats is fine for video node-centric
> > drivers, but comes with limitations.
> > 
> >>>> To summarize, if we cannot do the negotiation within kernel, delegating
> >>>> this to userspace implies far more complexity and breaks compatibility
> >>>> with existing applications without adding new functionalities.
> >>>>
> >>>> Having all that in mind, what should be reconsidered in your opinion
> >>>> Sakari ? Do you have some alternatives ?
> >>> 
> >>> First of all, let's note that your patch series performs to related but
> >>> still independent changes: it enables MC support, *and* enables the V4L2
> >>> subdev userspace API. The former is clearly needed and will allow you to
> >>> use the MC API internally in the kernel, simplifying pipeline traversal.
> >>> The latter then enables the V4L2 subdev userspace API, moving the
> >>> pipeline configuration responsibility to userspace.
> >>> 
> >>> You could in theory move to the MC API inside the kernel, without
> >>> enabling support for the V4L2 subdev userspace API. Configuring the
> >>> pipeline and propagating the formats would then be the responsibility of
> >>> the kernel driver.
> >> 
> >> Yes this is exactly what we want to do.
> >> If I understand well, to disable the V4L2 subdev userspace API, I just 
> >> have to remove the media device registry:
> >> 
> >> -	/* Register the media device */
> >> -	ret = media_device_register(&dcmi->mdev);
> >> -	if (ret) {
> >> -		dev_err(dcmi->dev, "Failed to register media device (%d)\n",
> >> -			ret);
> >> -		goto err_media_device_cleanup;
> >> -	}
> >> 
> >> Do you see any additional things to do ?
> > 
> > That should be it. Note that in that case pipeline configuration has to
> > be handled by the master driver (DCMI in this case), the external
> > subdevs involved (such as the CSI-2 to parallel bridge) must not handle
> > any propagation of formats or selection rectangles.
> 
> I wonder what we'd do in the case when someone needs to connect something
> else to the pipeline, such as a sensor with more than one sub-device, or a
> flash or a lens controller.
> 
> For future-proofness, I'd just use MC for hardware that may be part of a
> complex pipeline. In this case, if you think backwards compatibility is
> important (and for most hardware it probably is), I don't think there are
> perfect solutions if your existing driver is not MC-enabled.

Oh, I fully agree with you, which is why I mentioned in another e-mail
that using a video node-centric approach would come with limitations,
such as not being able to support more complex pipelines, ever.

> A reasonable compromise would be to add a Kconfig option that allows
> enabling MC. This way you can provide backwards compatibility and allow
> making use of the full potential of the hardware. That's also why hardware
> that may be part of a non-trivial MC pipeline should start with MC-enabled
> so we wouldn't run into this.

I really don't like this, as it introduces additional complexity. My
recommendation is to go for an MC-centric approach. Going for a video
node-centric approach is really shooting oneself in the foot regarding
future extensions. But that being said, if there's a strong desire to go
for foot self-shooting, the way to go is explained above.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/3] DCMI bridge support

[Sakari Ailus]

Hi Laurent,

On Fri, Jul 05, 2019 at 11:04:24AM +0300, Laurent Pinchart wrote:

...

> > A reasonable compromise would be to add a Kconfig option that allows
> > enabling MC. This way you can provide backwards compatibility and allow
> > making use of the full potential of the hardware. That's also why hardware
> > that may be part of a non-trivial MC pipeline should start with MC-enabled
> > so we wouldn't run into this.
> 
> I really don't like this, as it introduces additional complexity. My
> recommendation is to go for an MC-centric approach. Going for a video
> node-centric approach is really shooting oneself in the foot regarding
> future extensions. But that being said, if there's a strong desire to go
> for foot self-shooting, the way to go is explained above.

Well, there's nothing that can be done anymore as this has already
happened: this is an existing driver in the mainline kernel. Unless you
have a time machine of some sort, of course. :-) The choice is now really
between breaking existing applications (plain V4L2) and supporting new
functionality (MC-centric), so if you need both, I don't really see another
choice than a Kconfig option.

On the other hand, if we know there are no existing users that could not
support the MC-centric view of the device, we could just change the driver
API and forget the Kconfig option. It'd be much more simple that way
indeed. But I don'k know what's the case.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com