[PATCH v7 0/5] Use V4L2 mbus config for conveying MEI CSI link frequency

[PATCH v7 0/5] Use V4L2 mbus config for conveying MEI CSI link frequency

[Sakari Ailus]

Hi folks,

This set adds a few helpers for obtaining the link frequency from the V4L2
mbus config for devices that don't need to provide an UAPI to change it,
and finally add support for the Intel IVSC CSI device.

since v6:

- Remove comments on #else / #endif, it's trivial.

- Add a patch to convert the ipu6 driver.

since v5:

- Only support pad-based operation with CONFIG_MEDIA_CONTROLLER (1st and
  2nd patches).

since v4:

- Rework documentation a little.

- Remove wrong alignment change in 2nd patch.

- Move link_freq field after the type field in struct v4l2_mbus_config.

since v3:

- Add back missing ret I accidentally removed rather than moved to the 2nd
  patch.

since v2:

- Switch to V4L2 mbus config for conveying the link frequency.

since v1:

- Add a new 64-bit integer control V4L2_CID_CUR_LINK_FREQ instead of
  re-using V4L2_CID_LINK_FREQ.

Sakari Ailus (5):
  media: v4l: Support passing media pad argument to v4l2_get_link_freq()
  media: v4l: Support obtaining link frequency via get_mbus_config
  media: Documentation: Update link frequency driver documentation
  media: ivsc: csi: Obtain link frequency from the media pad
  media: intel/ipu6: Obtain link frequency from a sub-device

 Documentation/driver-api/media/tx-rx.rst      |  4 ++
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +---
 drivers/media/pci/intel/ivsc/mei_csi.c        | 72 +++++++------------
 drivers/media/v4l2-core/v4l2-common.c         | 26 ++++++-
 include/media/v4l2-common.h                   | 19 ++++-
 include/media/v4l2-mediabus.h                 |  2 +
 6 files changed, 73 insertions(+), 62 deletions(-)


base-commit: d1028b5748164e3ddd6d2bb0bbceee846ed2fc71
-- 
2.39.5

[PATCH v7 1/5] media: v4l: Support passing media pad argument to v4l2_get_link_freq()

[Sakari Ailus]

v4l2_get_link_freq() accepts a V4L2 control handler for now, but it needs
to take struct media_pad argument in order to obtain the link frequency
using get_mbus_config() pad op. Prepare for this by allowing struct
media_pad as well.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 21 ++++++++++++++++++---
 include/media/v4l2-common.h           | 19 ++++++++++++++++---
 2 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 0a2f4f0d0a07..9fe74c7e064f 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -466,8 +466,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
 }
 EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
 
-s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
-		       unsigned int div)
+s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
+			      unsigned int mul, unsigned int div)
 {
 	struct v4l2_ctrl *ctrl;
 	s64 freq;
@@ -502,7 +502,22 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
 
 	return freq > 0 ? freq : -EINVAL;
 }
-EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
+EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
+			     unsigned int div)
+{
+	struct v4l2_subdev *sd;
+
+	sd = media_entity_to_v4l2_subdev(pad->entity);
+	if (!sd)
+		return -ENODEV;
+
+	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
+}
+EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
+#endif /* CONFIG_MEDIA_CONTROLLER */
 
 /*
  * Simplify a fraction using a simple continued fraction decomposition. The
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index 63ad36f04f72..fda903bb3674 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -525,7 +525,8 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
 /**
  * v4l2_get_link_freq - Get link rate from transmitter
  *
- * @handler: The transmitter's control handler
+ * @pad: The transmitter's media pad (or control handler for non-MC users or
+ *	 compatibility reasons, don't use in new code)
  * @mul: The multiplier between pixel rate and link frequency. Bits per pixel on
  *	 D-PHY, samples per clock on parallel. 0 otherwise.
  * @div: The divisor between pixel rate and link frequency. Number of data lanes
@@ -541,8 +542,20 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
  * * %-ENOENT: Link frequency or pixel rate control not found
  * * %-EINVAL: Invalid link frequency value
  */
-s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
-		       unsigned int div);
+#ifdef CONFIG_MEDIA_CONTROLLER
+#define v4l2_get_link_freq(pad, mul, div)				\
+	_Generic(pad,							\
+		 struct media_pad *: __v4l2_get_link_freq_pad,		\
+		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
+	(pad, mul, div)
+s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
+			     unsigned int div);
+#else
+#define v4l2_get_link_freq(handler, mul, div)		\
+	__v4l2_get_link_freq_ctrl(handler, mul, div)
+#endif
+s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
+			      unsigned int mul, unsigned int div);
 
 void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
 		unsigned int n_terms, unsigned int threshold);
-- 
2.39.5

[PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Sakari Ailus]

Add link_freq field to struct v4l2_mbus_config in order to pass the link
frequency to the receiving sub-device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
 include/media/v4l2-mediabus.h         |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 9fe74c7e064f..88fbd5608f00 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
 s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
 			     unsigned int div)
 {
+	struct v4l2_mbus_config mbus_config = {};
 	struct v4l2_subdev *sd;
+	int ret;
 
 	sd = media_entity_to_v4l2_subdev(pad->entity);
-	if (!sd)
-		return -ENODEV;
+	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
+			       &mbus_config);
+	if (ret < 0 && ret != -ENOIOCTLCMD)
+		return ret;
 
-	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
+	return mbus_config.link_freq ?:
+		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
 }
 EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
 #endif /* CONFIG_MEDIA_CONTROLLER */
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 5bce6e423e94..cc5f776dc662 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -148,6 +148,7 @@ enum v4l2_mbus_type {
 /**
  * struct v4l2_mbus_config - media bus configuration
  * @type: interface type
+ * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
  * @bus: bus configuration data structure
  * @bus.parallel: embedded &struct v4l2_mbus_config_parallel.
  *		  Used if the bus is parallel or BT.656.
@@ -162,6 +163,7 @@ enum v4l2_mbus_type {
  */
 struct v4l2_mbus_config {
 	enum v4l2_mbus_type type;
+	u64 link_freq;
 	union {
 		struct v4l2_mbus_config_parallel parallel;
 		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
-- 
2.39.5

[PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Sakari Ailus]

Add the get_mbus_config() as the means for conveying the link frequency
towards the receiver drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/driver-api/media/tx-rx.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
index dd09484df1d3..1430c330fd52 100644
--- a/Documentation/driver-api/media/tx-rx.rst
+++ b/Documentation/driver-api/media/tx-rx.rst
@@ -49,6 +49,10 @@ Link frequency
 The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
 receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
 
+On devices where the link frequency isn't configurable, the link_freq field of
+struct v4l2_mbus_config is recommended over controls for conveying the link
+frequency to the downstream driver in the pipeline.
+
 ``.enable_streams()`` and ``.disable_streams()`` callbacks
 ^^^^^^^^^^^^^^^^^^^^^^^^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.39.5

[PATCH v7 4/5] media: ivsc: csi: Obtain link frequency from the media pad

[Sakari Ailus]

Support the use of the media pad for obtaining the link frequency.
Similarly, call the v4l2_get_link_freq() on the media pad, not on the
remote's control handler.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ivsc/mei_csi.c | 72 +++++++++-----------------
 1 file changed, 25 insertions(+), 47 deletions(-)

diff --git a/drivers/media/pci/intel/ivsc/mei_csi.c b/drivers/media/pci/intel/ivsc/mei_csi.c
index 2a9c12c975ca..9241c95fb025 100644
--- a/drivers/media/pci/intel/ivsc/mei_csi.c
+++ b/drivers/media/pci/intel/ivsc/mei_csi.c
@@ -35,8 +35,6 @@
 
 #define MEI_CSI_ENTITY_NAME "Intel IVSC CSI"
 
-#define MEI_CSI_LINK_FREQ_400MHZ 400000000ULL
-
 /* the 5s used here is based on experiment */
 #define CSI_CMD_TIMEOUT (5 * HZ)
 /* to setup CSI-2 link an extra delay needed and determined experimentally */
@@ -121,14 +119,13 @@ struct mei_csi {
 	struct mutex lock;
 
 	struct v4l2_subdev subdev;
-	struct v4l2_subdev *remote;
+	struct media_pad *remote;
 	struct v4l2_async_notifier notifier;
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *freq_ctrl;
 	struct v4l2_ctrl *privacy_ctrl;
 	/* lock for v4l2 controls */
 	struct mutex ctrl_lock;
-	unsigned int remote_pad;
 	/* start streaming or not */
 	int streaming;
 
@@ -147,10 +144,6 @@ static const struct v4l2_mbus_framefmt mei_csi_format_mbus_default = {
 	.field = V4L2_FIELD_NONE,
 };
 
-static s64 link_freq_menu_items[] = {
-	MEI_CSI_LINK_FREQ_400MHZ
-};
-
 static inline struct mei_csi *notifier_to_csi(struct v4l2_async_notifier *n)
 {
 	return container_of(n, struct mei_csi, notifier);
@@ -286,11 +279,13 @@ static void mei_csi_rx(struct mei_cl_device *cldev)
 static int mei_csi_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct mei_csi *csi = sd_to_csi(sd);
+	struct v4l2_subdev *remote_sd =
+		media_entity_to_v4l2_subdev(csi->remote->entity);
 	s64 freq;
 	int ret;
 
 	if (enable && csi->streaming == 0) {
-		freq = v4l2_get_link_freq(csi->remote->ctrl_handler, 0, 0);
+		freq = v4l2_get_link_freq(csi->remote, 0, 0);
 		if (freq < 0) {
 			dev_err(&csi->cldev->dev,
 				"error %lld, invalid link_freq\n", freq);
@@ -309,11 +304,11 @@ static int mei_csi_set_stream(struct v4l2_subdev *sd, int enable)
 		if (ret < 0)
 			goto err_switch;
 
-		ret = v4l2_subdev_call(csi->remote, video, s_stream, 1);
+		ret = v4l2_subdev_call(remote_sd, video, s_stream, 1);
 		if (ret)
 			goto err_switch;
 	} else if (!enable && csi->streaming == 1) {
-		v4l2_subdev_call(csi->remote, video, s_stream, 0);
+		v4l2_subdev_call(remote_sd, video, s_stream, 0);
 
 		/* switch CSI-2 link to IVSC */
 		ret = csi_set_link_owner(csi, CSI_LINK_IVSC);
@@ -470,34 +465,29 @@ static int mei_csi_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int mei_csi_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+static int mei_csi_get_mbus_config(struct v4l2_subdev *sd, unsigned int pad,
+				   struct v4l2_mbus_config *mbus_config)
 {
-	struct mei_csi *csi = ctrl_to_csi(ctrl);
+	struct mei_csi *csi = sd_to_csi(sd);
+	unsigned int i;
 	s64 freq;
 
-	if (ctrl->id == V4L2_CID_LINK_FREQ) {
-		if (!csi->remote)
-			return -EINVAL;
-
-		freq = v4l2_get_link_freq(csi->remote->ctrl_handler, 0, 0);
-		if (freq < 0) {
-			dev_err(&csi->cldev->dev,
-				"error %lld, invalid link_freq\n", freq);
-			return -EINVAL;
-		}
-
-		link_freq_menu_items[0] = freq;
-		ctrl->val = 0;
+	mbus_config->type = V4L2_MBUS_CSI2_DPHY;
+	for (i = 0; i < V4L2_MBUS_CSI2_MAX_DATA_LANES; i++)
+		mbus_config->bus.mipi_csi2.data_lanes[i] = i + 1;
+	mbus_config->bus.mipi_csi2.num_data_lanes = csi->nr_of_lanes;
 
-		return 0;
+	freq = v4l2_get_link_freq(csi->remote, 0, 0);
+	if (freq < 0) {
+		dev_err(&csi->cldev->dev,
+			"error %lld, invalid link_freq\n", freq);
+		return -EINVAL;
 	}
 
-	return -EINVAL;
-}
+	mbus_config->link_freq = csi->link_freq = freq;
 
-static const struct v4l2_ctrl_ops mei_csi_ctrl_ops = {
-	.g_volatile_ctrl = mei_csi_g_volatile_ctrl,
-};
+	return 0;
+}
 
 static const struct v4l2_subdev_video_ops mei_csi_video_ops = {
 	.s_stream = mei_csi_set_stream,
@@ -506,6 +496,7 @@ static const struct v4l2_subdev_video_ops mei_csi_video_ops = {
 static const struct v4l2_subdev_pad_ops mei_csi_pad_ops = {
 	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = mei_csi_set_fmt,
+	.get_mbus_config = mei_csi_get_mbus_config,
 };
 
 static const struct v4l2_subdev_ops mei_csi_subdev_ops = {
@@ -533,8 +524,7 @@ static int mei_csi_notify_bound(struct v4l2_async_notifier *notifier,
 	if (pad < 0)
 		return pad;
 
-	csi->remote = subdev;
-	csi->remote_pad = pad;
+	csi->remote = &subdev->entity.pads[pad];
 
 	return media_create_pad_link(&subdev->entity, pad,
 				     &csi->subdev.entity, CSI_PAD_SINK,
@@ -558,28 +548,16 @@ static const struct v4l2_async_notifier_operations mei_csi_notify_ops = {
 
 static int mei_csi_init_controls(struct mei_csi *csi)
 {
-	u32 max;
 	int ret;
 
 	mutex_init(&csi->ctrl_lock);
 
-	ret = v4l2_ctrl_handler_init(&csi->ctrl_handler, 2);
+	ret = v4l2_ctrl_handler_init(&csi->ctrl_handler, 1);
 	if (ret)
 		return ret;
 
 	csi->ctrl_handler.lock = &csi->ctrl_lock;
 
-	max = ARRAY_SIZE(link_freq_menu_items) - 1;
-	csi->freq_ctrl = v4l2_ctrl_new_int_menu(&csi->ctrl_handler,
-						&mei_csi_ctrl_ops,
-						V4L2_CID_LINK_FREQ,
-						max,
-						0,
-						link_freq_menu_items);
-	if (csi->freq_ctrl)
-		csi->freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY |
-					 V4L2_CTRL_FLAG_VOLATILE;
-
 	csi->privacy_ctrl = v4l2_ctrl_new_std(&csi->ctrl_handler, NULL,
 					      V4L2_CID_PRIVACY, 0, 1, 1, 0);
 	if (csi->privacy_ctrl)
-- 
2.39.5

[PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Sakari Ailus]

Obtain the link frequency from the sub-device instead of a control
handler. This allows obtaining it using the get_mbus_config() sub-device
pad op that which is only supported by the IVSC driver.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
index 051898ce53f4..da8581a37e22 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
@@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = {
 s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
 {
 	struct media_pad *src_pad;
-	struct v4l2_subdev *ext_sd;
-	struct device *dev;
 
 	if (!csi2)
 		return -EINVAL;
 
-	dev = &csi2->isys->adev->auxdev.dev;
 	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);
 	if (IS_ERR(src_pad)) {
-		dev_err(dev, "can't get source pad of %s (%ld)\n",
+		dev_err(&csi2->isys->adev->auxdev.dev,
+			"can't get source pad of %s (%ld)\n",
 			csi2->asd.sd.name, PTR_ERR(src_pad));
 		return PTR_ERR(src_pad);
 	}
 
-	ext_sd = media_entity_to_v4l2_subdev(src_pad->entity);
-	if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name))
-		return -ENODEV;
-
-	return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0);
+	return v4l2_get_link_freq(src_pad, 0, 0);
 }
 
 static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
-- 
2.39.5

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Jacopo Mondi]

Hi Sakari

On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> Add the get_mbus_config() as the means for conveying the link frequency
> towards the receiver drivers.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/driver-api/media/tx-rx.rst | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> index dd09484df1d3..1430c330fd52 100644
> --- a/Documentation/driver-api/media/tx-rx.rst
> +++ b/Documentation/driver-api/media/tx-rx.rst
> @@ -49,6 +49,10 @@ Link frequency
>  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
>  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
>
> +On devices where the link frequency isn't configurable, the link_freq field of
> +struct v4l2_mbus_config is recommended over controls for conveying the link
> +frequency to the downstream driver in the pipeline.

When read in its entirety, the paragraphs

 Link frequency
 ^^^^^^^^^^^^^^

 The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
 receiver the frequency of the bus (i.e. it is not the same as the symbol rate).

 On devices where the link frequency isn't configurable, the link_freq field of
 struct v4l2_mbus_config is recommended over controls for conveying the link
 frequency.

Sounds simpler without the last 7 words.

A nit and just tastes maybe

I like where this is going!

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


> +
>  ``.enable_streams()`` and ``.disable_streams()`` callbacks
>  ^^^^^^^^^^^^^^^^^^^^^^^^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> --
> 2.39.5
>
>

Re: [PATCH v7 1/5] media: v4l: Support passing media pad argument to v4l2_get_link_freq()

[Jacopo Mondi]

Hi Sakari

On Tue, Dec 10, 2024 at 09:59:02AM +0200, Sakari Ailus wrote:
> v4l2_get_link_freq() accepts a V4L2 control handler for now, but it needs
> to take struct media_pad argument in order to obtain the link frequency
> using get_mbus_config() pad op. Prepare for this by allowing struct
> media_pad as well.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
   j
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 21 ++++++++++++++++++---
>  include/media/v4l2-common.h           | 19 ++++++++++++++++---
>  2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 0a2f4f0d0a07..9fe74c7e064f 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -466,8 +466,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
>
> -s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> -		       unsigned int div)
> +s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> +			      unsigned int mul, unsigned int div)
>  {
>  	struct v4l2_ctrl *ctrl;
>  	s64 freq;
> @@ -502,7 +502,22 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
>
>  	return freq > 0 ? freq : -EINVAL;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
> +EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +			     unsigned int div)
> +{
> +	struct v4l2_subdev *sd;
> +
> +	sd = media_entity_to_v4l2_subdev(pad->entity);
> +	if (!sd)
> +		return -ENODEV;
> +
> +	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> +}
> +EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> +#endif /* CONFIG_MEDIA_CONTROLLER */
>
>  /*
>   * Simplify a fraction using a simple continued fraction decomposition. The
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 63ad36f04f72..fda903bb3674 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -525,7 +525,8 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>  /**
>   * v4l2_get_link_freq - Get link rate from transmitter
>   *
> - * @handler: The transmitter's control handler
> + * @pad: The transmitter's media pad (or control handler for non-MC users or
> + *	 compatibility reasons, don't use in new code)
>   * @mul: The multiplier between pixel rate and link frequency. Bits per pixel on
>   *	 D-PHY, samples per clock on parallel. 0 otherwise.
>   * @div: The divisor between pixel rate and link frequency. Number of data lanes
> @@ -541,8 +542,20 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>   * * %-ENOENT: Link frequency or pixel rate control not found
>   * * %-EINVAL: Invalid link frequency value
>   */
> -s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> -		       unsigned int div);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +#define v4l2_get_link_freq(pad, mul, div)				\
> +	_Generic(pad,							\
> +		 struct media_pad *: __v4l2_get_link_freq_pad,		\
> +		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
> +	(pad, mul, div)
> +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +			     unsigned int div);
> +#else
> +#define v4l2_get_link_freq(handler, mul, div)		\
> +	__v4l2_get_link_freq_ctrl(handler, mul, div)
> +#endif
> +s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> +			      unsigned int mul, unsigned int div);
>
>  void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
>  		unsigned int n_terms, unsigned int threshold);
> --
> 2.39.5
>
>

Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Jacopo Mondi]

Hi Sakari

On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> Add link_freq field to struct v4l2_mbus_config in order to pass the link
> frequency to the receiving sub-device.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
>  include/media/v4l2-mediabus.h         |  2 ++
>  2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 9fe74c7e064f..88fbd5608f00 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
>  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
>  			     unsigned int div)
>  {
> +	struct v4l2_mbus_config mbus_config = {};
>  	struct v4l2_subdev *sd;
> +	int ret;
>
>  	sd = media_entity_to_v4l2_subdev(pad->entity);
> -	if (!sd)
> -		return -ENODEV;
> +	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> +			       &mbus_config);
> +	if (ret < 0 && ret != -ENOIOCTLCMD)
> +		return ret;
>
> -	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> +	return mbus_config.link_freq ?:
> +		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
>  }
>  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 5bce6e423e94..cc5f776dc662 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -148,6 +148,7 @@ enum v4l2_mbus_type {
>  /**
>   * struct v4l2_mbus_config - media bus configuration
>   * @type: interface type
> + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
>   * @bus: bus configuration data structure
>   * @bus.parallel: embedded &struct v4l2_mbus_config_parallel.
>   *		  Used if the bus is parallel or BT.656.
> @@ -162,6 +163,7 @@ enum v4l2_mbus_type {
>   */
>  struct v4l2_mbus_config {
>  	enum v4l2_mbus_type type;
> +	u64 link_freq;
>  	union {
>  		struct v4l2_mbus_config_parallel parallel;
>  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> --
> 2.39.5
>
>

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Sakari Ailus]

Hi Jacopo,

On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > Add the get_mbus_config() as the means for conveying the link frequency
> > towards the receiver drivers.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > index dd09484df1d3..1430c330fd52 100644
> > --- a/Documentation/driver-api/media/tx-rx.rst
> > +++ b/Documentation/driver-api/media/tx-rx.rst
> > @@ -49,6 +49,10 @@ Link frequency
> >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> >
> > +On devices where the link frequency isn't configurable, the link_freq field of
> > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > +frequency to the downstream driver in the pipeline.
> 
> When read in its entirety, the paragraphs
> 
>  Link frequency
>  ^^^^^^^^^^^^^^
> 
>  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
>  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> 
>  On devices where the link frequency isn't configurable, the link_freq field of
>  struct v4l2_mbus_config is recommended over controls for conveying the link
>  frequency.
> 
> Sounds simpler without the last 7 words.
> 
> A nit and just tastes maybe

I used:

On devices where the link frequency isn't configurable, using the ``link_freq``
field of struct v4l2_mbus_config is recommended over controls.

> 
> I like where this is going!
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thank you!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v7 1/5] media: v4l: Support passing media pad argument to v4l2_get_link_freq()

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Dec 10, 2024 at 09:59:02AM +0200, Sakari Ailus wrote:
> v4l2_get_link_freq() accepts a V4L2 control handler for now, but it needs
> to take struct media_pad argument in order to obtain the link frequency
> using get_mbus_config() pad op. Prepare for this by allowing struct
> media_pad as well.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 21 ++++++++++++++++++---
>  include/media/v4l2-common.h           | 19 ++++++++++++++++---
>  2 files changed, 34 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 0a2f4f0d0a07..9fe74c7e064f 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -466,8 +466,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
>  
> -s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> -		       unsigned int div)
> +s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> +			      unsigned int mul, unsigned int div)
>  {
>  	struct v4l2_ctrl *ctrl;
>  	s64 freq;
> @@ -502,7 +502,22 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
>  
>  	return freq > 0 ? freq : -EINVAL;
>  }
> -EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
> +EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +			     unsigned int div)
> +{
> +	struct v4l2_subdev *sd;
> +
> +	sd = media_entity_to_v4l2_subdev(pad->entity);
> +	if (!sd)
> +		return -ENODEV;
> +
> +	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> +}
> +EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> +#endif /* CONFIG_MEDIA_CONTROLLER */
>  
>  /*
>   * Simplify a fraction using a simple continued fraction decomposition. The
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index 63ad36f04f72..fda903bb3674 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -525,7 +525,8 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>  /**
>   * v4l2_get_link_freq - Get link rate from transmitter
>   *
> - * @handler: The transmitter's control handler
> + * @pad: The transmitter's media pad (or control handler for non-MC users or
> + *	 compatibility reasons, don't use in new code)
>   * @mul: The multiplier between pixel rate and link frequency. Bits per pixel on
>   *	 D-PHY, samples per clock on parallel. 0 otherwise.
>   * @div: The divisor between pixel rate and link frequency. Number of data lanes
> @@ -541,8 +542,20 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
>   * * %-ENOENT: Link frequency or pixel rate control not found
>   * * %-EINVAL: Invalid link frequency value
>   */
> -s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> -		       unsigned int div);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +#define v4l2_get_link_freq(pad, mul, div)				\
> +	_Generic(pad,							\
> +		 struct media_pad *: __v4l2_get_link_freq_pad,		\
> +		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
> +	(pad, mul, div)
> +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +			     unsigned int div);
> +#else
> +#define v4l2_get_link_freq(handler, mul, div)		\
> +	__v4l2_get_link_freq_ctrl(handler, mul, div)
> +#endif
> +s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> +			      unsigned int mul, unsigned int div);

Let's avoid this complexity by patching callers. I'm OK with this patch
as a temporary measure, but the series should end with a patch that
removes the ability to pass a control handler.

>  
>  void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
>  		unsigned int n_terms, unsigned int threshold);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> Add link_freq field to struct v4l2_mbus_config in order to pass the link
> frequency to the receiving sub-device.

The documentation of v4l2_get_link_freq() should be expanded to explain
the new mode of operation. It should document which of the supported
methods are recommended for new drivers.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
>  include/media/v4l2-mediabus.h         |  2 ++
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 9fe74c7e064f..88fbd5608f00 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
>  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
>  			     unsigned int div)
>  {
> +	struct v4l2_mbus_config mbus_config = {};

Isn't mbus_config fully populated by the .get_mbus_config() operation ?
That should be documented in the .get_mbus_config() operation
documentation.

>  	struct v4l2_subdev *sd;
> +	int ret;
>  
>  	sd = media_entity_to_v4l2_subdev(pad->entity);
> -	if (!sd)
> -		return -ENODEV;
> +	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> +			       &mbus_config);
> +	if (ret < 0 && ret != -ENOIOCTLCMD)
> +		return ret;
>  
> -	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> +	return mbus_config.link_freq ?:
> +		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);

	if (mbus_config.link_freq)
		return mbus_config.link_freq;

	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);

I wonder if we should also add a message in case link_freq is 0, to get
drivers to convert to reporting the link frequency through
.get_mbus_config() if they already implement the operation.

>  }
>  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 5bce6e423e94..cc5f776dc662 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -148,6 +148,7 @@ enum v4l2_mbus_type {
>  /**
>   * struct v4l2_mbus_config - media bus configuration
>   * @type: interface type
> + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.

Not a candidate for this series, but I'd like to simplify drivers by
implementing the LINK_FREQ control automatically.

>   * @bus: bus configuration data structure
>   * @bus.parallel: embedded &struct v4l2_mbus_config_parallel.
>   *		  Used if the bus is parallel or BT.656.
> @@ -162,6 +163,7 @@ enum v4l2_mbus_type {
>   */
>  struct v4l2_mbus_config {
>  	enum v4l2_mbus_type type;
> +	u64 link_freq;
>  	union {
>  		struct v4l2_mbus_config_parallel parallel;
>  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Laurent Pinchart]

On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > Add the get_mbus_config() as the means for conveying the link frequency
> > > towards the receiver drivers.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > index dd09484df1d3..1430c330fd52 100644
> > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > @@ -49,6 +49,10 @@ Link frequency
> > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > >
> > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > +frequency to the downstream driver in the pipeline.
> > 
> > When read in its entirety, the paragraphs
> > 
> >  Link frequency
> >  ^^^^^^^^^^^^^^
> > 
> >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > 
> >  On devices where the link frequency isn't configurable, the link_freq field of
> >  struct v4l2_mbus_config is recommended over controls for conveying the link
> >  frequency.
> > 
> > Sounds simpler without the last 7 words.
> > 
> > A nit and just tastes maybe
> 
> I used:
> 
> On devices where the link frequency isn't configurable, using the ``link_freq``
> field of struct v4l2_mbus_config is recommended over controls.

Is it a recommendation for the transmitter driver or the receiver driver
? If it's a recommendation for the transmitter driver, does that mean it
should not implement V4L2_CID_LINK_FREQ in that case ? How about the
need to expose the link frequency to userspace when it's fixed ?

I think the documentation needs further clarification to clearly explain
what transmitters should do and what receivers should do.

> > 
> > I like where this is going!
> > 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

I think this should come before 4/5.

On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> Obtain the link frequency from the sub-device instead of a control
> handler. This allows obtaining it using the get_mbus_config() sub-device
> pad op that which is only supported by the IVSC driver.

"which is the only method supported by the IVSC driver"

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> index 051898ce53f4..da8581a37e22 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = {
>  s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
>  {
>  	struct media_pad *src_pad;
> -	struct v4l2_subdev *ext_sd;
> -	struct device *dev;
>  
>  	if (!csi2)
>  		return -EINVAL;
>  
> -	dev = &csi2->isys->adev->auxdev.dev;
>  	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);

Not a candidate for this patch, but can the source change, or can it be
cached at probe time (or notifier bound time) ?

>  	if (IS_ERR(src_pad)) {
> -		dev_err(dev, "can't get source pad of %s (%ld)\n",
> +		dev_err(&csi2->isys->adev->auxdev.dev,
> +			"can't get source pad of %s (%ld)\n",
>  			csi2->asd.sd.name, PTR_ERR(src_pad));
>  		return PTR_ERR(src_pad);
>  	}
>  
> -	ext_sd = media_entity_to_v4l2_subdev(src_pad->entity);
> -	if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name))
> -		return -ENODEV;
> -
> -	return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0);
> +	return v4l2_get_link_freq(src_pad, 0, 0);
>  }
>  
>  static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Sakari Ailus]

Hi Laurent,

On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.

Thank you for the review. I asked you to review a set but it wasn't this
one:
<URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>.
:-)

> 
> I think this should come before 4/5.
> 
> On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> > Obtain the link frequency from the sub-device instead of a control
> > handler. This allows obtaining it using the get_mbus_config() sub-device
> > pad op that which is only supported by the IVSC driver.
> 
> "which is the only method supported by the IVSC driver"
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++---------
> >  1 file changed, 3 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > index 051898ce53f4..da8581a37e22 100644
> > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = {
> >  s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
> >  {
> >  	struct media_pad *src_pad;
> > -	struct v4l2_subdev *ext_sd;
> > -	struct device *dev;
> >  
> >  	if (!csi2)
> >  		return -EINVAL;
> >  
> > -	dev = &csi2->isys->adev->auxdev.dev;
> >  	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);
> 
> Not a candidate for this patch, but can the source change, or can it be
> cached at probe time (or notifier bound time) ?

It could be, but why would you do that?

This would also prevent connecting multiple sensors to a single CSI-2
receiver.

> 
> >  	if (IS_ERR(src_pad)) {
> > -		dev_err(dev, "can't get source pad of %s (%ld)\n",
> > +		dev_err(&csi2->isys->adev->auxdev.dev,
> > +			"can't get source pad of %s (%ld)\n",
> >  			csi2->asd.sd.name, PTR_ERR(src_pad));
> >  		return PTR_ERR(src_pad);
> >  	}
> >  
> > -	ext_sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > -	if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name))
> > -		return -ENODEV;
> > -
> > -	return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0);
> > +	return v4l2_get_link_freq(src_pad, 0, 0);
> >  }
> >  
> >  static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Sakari Ailus]

On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> I think this should come before 4/5.

Because of...? Both are needed for this to work.

> 
> On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> > Obtain the link frequency from the sub-device instead of a control
> > handler. This allows obtaining it using the get_mbus_config() sub-device
> > pad op that which is only supported by the IVSC driver.
> 
> "which is the only method supported by the IVSC driver"

Yes.

-- 
Sakari Ailus

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Sakari Ailus]

Hi Laurent,

On Sun, Dec 15, 2024 at 07:02:40PM +0200, Laurent Pinchart wrote:
> On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> > On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > > Add the get_mbus_config() as the means for conveying the link frequency
> > > > towards the receiver drivers.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > > index dd09484df1d3..1430c330fd52 100644
> > > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > > @@ -49,6 +49,10 @@ Link frequency
> > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > >
> > > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > +frequency to the downstream driver in the pipeline.
> > > 
> > > When read in its entirety, the paragraphs
> > > 
> > >  Link frequency
> > >  ^^^^^^^^^^^^^^
> > > 
> > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > 
> > >  On devices where the link frequency isn't configurable, the link_freq field of
> > >  struct v4l2_mbus_config is recommended over controls for conveying the link
> > >  frequency.
> > > 
> > > Sounds simpler without the last 7 words.
> > > 
> > > A nit and just tastes maybe
> > 
> > I used:
> > 
> > On devices where the link frequency isn't configurable, using the ``link_freq``
> > field of struct v4l2_mbus_config is recommended over controls.
> 
> Is it a recommendation for the transmitter driver or the receiver driver
> ? If it's a recommendation for the transmitter driver, does that mean it
> should not implement V4L2_CID_LINK_FREQ in that case ? How about the
> need to expose the link frequency to userspace when it's fixed ?

For the transmitter. Receivers do not implement get_mbus_config op. I can
add a note on this.

The only reason this has been exposed to the user space is because it's
been a control that has been modifiable. Also HDMI to CSI-2 bridges work
this way.

The LINK_FREQ control is also an integer menu so the same control couldn't
be used as-is. These were the reasons why the earlier review concluded with
impelmenting this via get_mbus_config().

> 
> I think the documentation needs further clarification to clearly explain
> what transmitters should do and what receivers should do.

I can add this.

> 
> > > 
> > > I like where this is going!
> > > 
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Sakari Ailus]

On Mon, Dec 16, 2024 at 08:07:26AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Sun, Dec 15, 2024 at 07:02:40PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> > > On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > > > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > > > Add the get_mbus_config() as the means for conveying the link frequency
> > > > > towards the receiver drivers.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > > > index dd09484df1d3..1430c330fd52 100644
> > > > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > > > @@ -49,6 +49,10 @@ Link frequency
> > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > >
> > > > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > > +frequency to the downstream driver in the pipeline.
> > > > 
> > > > When read in its entirety, the paragraphs
> > > > 
> > > >  Link frequency
> > > >  ^^^^^^^^^^^^^^
> > > > 
> > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > 
> > > >  On devices where the link frequency isn't configurable, the link_freq field of
> > > >  struct v4l2_mbus_config is recommended over controls for conveying the link
> > > >  frequency.
> > > > 
> > > > Sounds simpler without the last 7 words.
> > > > 
> > > > A nit and just tastes maybe
> > > 
> > > I used:
> > > 
> > > On devices where the link frequency isn't configurable, using the ``link_freq``
> > > field of struct v4l2_mbus_config is recommended over controls.
> > 
> > Is it a recommendation for the transmitter driver or the receiver driver
> > ? If it's a recommendation for the transmitter driver, does that mean it
> > should not implement V4L2_CID_LINK_FREQ in that case ? How about the
> > need to expose the link frequency to userspace when it's fixed ?
> 
> For the transmitter. Receivers do not implement get_mbus_config op. I can
> add a note on this.

This text is already under "Transmitter drivers".

-- 
Sakari Ailus

Re: [PATCH v7 1/5] media: v4l: Support passing media pad argument to v4l2_get_link_freq()

[Sakari Ailus]

Hi Laurent,

On Sun, Dec 15, 2024 at 06:45:23PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tue, Dec 10, 2024 at 09:59:02AM +0200, Sakari Ailus wrote:
> > v4l2_get_link_freq() accepts a V4L2 control handler for now, but it needs
> > to take struct media_pad argument in order to obtain the link frequency
> > using get_mbus_config() pad op. Prepare for this by allowing struct
> > media_pad as well.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 21 ++++++++++++++++++---
> >  include/media/v4l2-common.h           | 19 ++++++++++++++++---
> >  2 files changed, 34 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 0a2f4f0d0a07..9fe74c7e064f 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -466,8 +466,8 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> >  
> > -s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> > -		       unsigned int div)
> > +s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> > +			      unsigned int mul, unsigned int div)
> >  {
> >  	struct v4l2_ctrl *ctrl;
> >  	s64 freq;
> > @@ -502,7 +502,22 @@ s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> >  
> >  	return freq > 0 ? freq : -EINVAL;
> >  }
> > -EXPORT_SYMBOL_GPL(v4l2_get_link_freq);
> > +EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> > +
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > +			     unsigned int div)
> > +{
> > +	struct v4l2_subdev *sd;
> > +
> > +	sd = media_entity_to_v4l2_subdev(pad->entity);
> > +	if (!sd)
> > +		return -ENODEV;
> > +
> > +	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > +}
> > +EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> > +#endif /* CONFIG_MEDIA_CONTROLLER */
> >  
> >  /*
> >   * Simplify a fraction using a simple continued fraction decomposition. The
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index 63ad36f04f72..fda903bb3674 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -525,7 +525,8 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
> >  /**
> >   * v4l2_get_link_freq - Get link rate from transmitter
> >   *
> > - * @handler: The transmitter's control handler
> > + * @pad: The transmitter's media pad (or control handler for non-MC users or
> > + *	 compatibility reasons, don't use in new code)
> >   * @mul: The multiplier between pixel rate and link frequency. Bits per pixel on
> >   *	 D-PHY, samples per clock on parallel. 0 otherwise.
> >   * @div: The divisor between pixel rate and link frequency. Number of data lanes
> > @@ -541,8 +542,20 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt, u32 pixelformat,
> >   * * %-ENOENT: Link frequency or pixel rate control not found
> >   * * %-EINVAL: Invalid link frequency value
> >   */
> > -s64 v4l2_get_link_freq(struct v4l2_ctrl_handler *handler, unsigned int mul,
> > -		       unsigned int div);
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +#define v4l2_get_link_freq(pad, mul, div)				\
> > +	_Generic(pad,							\
> > +		 struct media_pad *: __v4l2_get_link_freq_pad,		\
> > +		 struct v4l2_ctrl_handler *: __v4l2_get_link_freq_ctrl)	\
> > +	(pad, mul, div)
> > +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > +			     unsigned int div);
> > +#else
> > +#define v4l2_get_link_freq(handler, mul, div)		\
> > +	__v4l2_get_link_freq_ctrl(handler, mul, div)
> > +#endif
> > +s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> > +			      unsigned int mul, unsigned int div);
> 
> Let's avoid this complexity by patching callers. I'm OK with this patch
> as a temporary measure, but the series should end with a patch that
> removes the ability to pass a control handler.

I intend to do that. However new drivers are being merged and the set has
been around since the spring and it fixes an issue in the IVSC driver (not
being able to convey the link frequency).

Therefore I prefer to get this merged now and then convert the rest of the
users.

> 
> >  
> >  void v4l2_simplify_fraction(u32 *numerator, u32 *denominator,
> >  		unsigned int n_terms, unsigned int threshold);
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Sakari Ailus]

Hi Laurent,

On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > frequency to the receiving sub-device.
> 
> The documentation of v4l2_get_link_freq() should be expanded to explain
> the new mode of operation. It should document which of the supported
> methods are recommended for new drivers.
> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
> >  include/media/v4l2-mediabus.h         |  2 ++
> >  2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 9fe74c7e064f..88fbd5608f00 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> >  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> >  			     unsigned int div)
> >  {
> > +	struct v4l2_mbus_config mbus_config = {};
> 
> Isn't mbus_config fully populated by the .get_mbus_config() operation ?
> That should be documented in the .get_mbus_config() operation
> documentation.

It's a good practice to zero the memory before drivers get to work on it. I
presume drivers will set the fields that are relevant for them and ignore
the rest.

I can add a note on get_mbus_config() the drivers should set all struct
fields to known values.

> 
> >  	struct v4l2_subdev *sd;
> > +	int ret;
> >  
> >  	sd = media_entity_to_v4l2_subdev(pad->entity);
> > -	if (!sd)
> > -		return -ENODEV;
> > +	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> > +			       &mbus_config);
> > +	if (ret < 0 && ret != -ENOIOCTLCMD)
> > +		return ret;
> >  
> > -	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > +	return mbus_config.link_freq ?:
> > +		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> 
> 	if (mbus_config.link_freq)
> 		return mbus_config.link_freq;
> 
> 	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);

Whether this would be cleaner is debatable at least. I can switch if you
insist though.

> 
> I wonder if we should also add a message in case link_freq is 0, to get
> drivers to convert to reporting the link frequency through
> .get_mbus_config() if they already implement the operation.

I'm not sure this will be useful: in most cases the LINK_FREQ control is
used and for a reason: it's user-configurable. Drivers should only populate
the field if the link frequency is determined by the driver. For the
receiver drivers it does not matter: they use v4l2_get_link_freq().

> 
> >  }
> >  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> >  #endif /* CONFIG_MEDIA_CONTROLLER */
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 5bce6e423e94..cc5f776dc662 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -148,6 +148,7 @@ enum v4l2_mbus_type {
> >  /**
> >   * struct v4l2_mbus_config - media bus configuration
> >   * @type: interface type
> > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
> 
> Not a candidate for this series, but I'd like to simplify drivers by
> implementing the LINK_FREQ control automatically.
> 
> >   * @bus: bus configuration data structure
> >   * @bus.parallel: embedded &struct v4l2_mbus_config_parallel.
> >   *		  Used if the bus is parallel or BT.656.
> > @@ -162,6 +163,7 @@ enum v4l2_mbus_type {
> >   */
> >  struct v4l2_mbus_config {
> >  	enum v4l2_mbus_type type;
> > +	u64 link_freq;
> >  	union {
> >  		struct v4l2_mbus_config_parallel parallel;
> >  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote:
> On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> 
> Thank you for the review. I asked you to review a set but it wasn't this
> one:
> <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>.
> :-)

Are you complaining that I review too many patches ? :-)

> > I think this should come before 4/5.
> > 
> > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> > > Obtain the link frequency from the sub-device instead of a control
> > > handler. This allows obtaining it using the get_mbus_config() sub-device
> > > pad op that which is only supported by the IVSC driver.
> > 
> > "which is the only method supported by the IVSC driver"
> > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++---------
> > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > index 051898ce53f4..da8581a37e22 100644
> > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = {
> > >  s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
> > >  {
> > >  	struct media_pad *src_pad;
> > > -	struct v4l2_subdev *ext_sd;
> > > -	struct device *dev;
> > >  
> > >  	if (!csi2)
> > >  		return -EINVAL;
> > >  
> > > -	dev = &csi2->isys->adev->auxdev.dev;
> > >  	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);
> > 
> > Not a candidate for this patch, but can the source change, or can it be
> > cached at probe time (or notifier bound time) ?
> 
> It could be, but why would you do that?
> 
> This would also prevent connecting multiple sensors to a single CSI-2
> receiver.

Precisely because people shouldn't do this :-)

It would be more efficient to get the pad at probe time and cache it,
and would remove an error path at runtime. Until we have a use case
where we need to support more than one sensor on the same CSI-2 receiver
for this driver, I think that would be best.

> > >  	if (IS_ERR(src_pad)) {
> > > -		dev_err(dev, "can't get source pad of %s (%ld)\n",
> > > +		dev_err(&csi2->isys->adev->auxdev.dev,
> > > +			"can't get source pad of %s (%ld)\n",
> > >  			csi2->asd.sd.name, PTR_ERR(src_pad));
> > >  		return PTR_ERR(src_pad);
> > >  	}
> > >  
> > > -	ext_sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > > -	if (WARN(!ext_sd, "Failed to get subdev for %s\n", csi2->asd.sd.name))
> > > -		return -ENODEV;
> > > -
> > > -	return v4l2_get_link_freq(ext_sd->ctrl_handler, 0, 0);
> > > +	return v4l2_get_link_freq(src_pad, 0, 0);
> > >  }
> > >  
> > >  static int csi2_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Sakari Ailus]

Hi Laurent,

On Mon, Dec 16, 2024 at 11:07:36AM +0200, Laurent Pinchart wrote:
> On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote:
> > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > > 
> > > Thank you for the patch.
> > 
> > Thank you for the review. I asked you to review a set but it wasn't this
> > one:
> > <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>.
> > :-)
> 
> Are you complaining that I review too many patches ? :-)

No, I'm complaining your selection of patches to review. ;-)

> 
> > > I think this should come before 4/5.
> > > 
> > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> > > > Obtain the link frequency from the sub-device instead of a control
> > > > handler. This allows obtaining it using the get_mbus_config() sub-device
> > > > pad op that which is only supported by the IVSC driver.
> > > 
> > > "which is the only method supported by the IVSC driver"
> > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++---------
> > > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > index 051898ce53f4..da8581a37e22 100644
> > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = {
> > > >  s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
> > > >  {
> > > >  	struct media_pad *src_pad;
> > > > -	struct v4l2_subdev *ext_sd;
> > > > -	struct device *dev;
> > > >  
> > > >  	if (!csi2)
> > > >  		return -EINVAL;
> > > >  
> > > > -	dev = &csi2->isys->adev->auxdev.dev;
> > > >  	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);
> > > 
> > > Not a candidate for this patch, but can the source change, or can it be
> > > cached at probe time (or notifier bound time) ?
> > 
> > It could be, but why would you do that?
> > 
> > This would also prevent connecting multiple sensors to a single CSI-2
> > receiver.
> 
> Precisely because people shouldn't do this :-)
> 
> It would be more efficient to get the pad at probe time and cache it,
> and would remove an error path at runtime. Until we have a use case

I presume it'd take a bug somewhere for that to fail. I don't think a
relatively small number of instructions would make measurable a difference
in performance. If we thought this was a problem, there would be a lot to
work on elsewhere in the framework, starting with the control framework and
IOCTL handling. The problem is just that nearly all that code is there for
sound and important reasons.

> where we need to support more than one sensor on the same CSI-2 receiver
> for this driver, I think that would be best.

I don't disagree as such but we can't affect hardware design here. Nothing
currently prevents that and adding a driver bug that will cause whatever
ills when hit is not a solution either, even if the former were true. Well,
if this were Windows, the situation could be different.

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 09:18:31AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Mon, Dec 16, 2024 at 11:07:36AM +0200, Laurent Pinchart wrote:
> > On Mon, Dec 16, 2024 at 07:47:12AM +0000, Sakari Ailus wrote:
> > > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> > > > Hi Sakari,
> > > > 
> > > > Thank you for the patch.
> > > 
> > > Thank you for the review. I asked you to review a set but it wasn't this
> > > one:
> > > <URL:https://lore.kernel.org/linux-media/20241129095142.87196-1-sakari.ailus@linux.intel.com/T/#t>.
> > > :-)
> > 
> > Are you complaining that I review too many patches ? :-)
> 
> No, I'm complaining your selection of patches to review. ;-)
> 
> > 
> > > > I think this should come before 4/5.
> > > > 
> > > > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> > > > > Obtain the link frequency from the sub-device instead of a control
> > > > > handler. This allows obtaining it using the get_mbus_config() sub-device
> > > > > pad op that which is only supported by the IVSC driver.
> > > > 
> > > > "which is the only method supported by the IVSC driver"
> > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 12 +++---------
> > > > >  1 file changed, 3 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > index 051898ce53f4..da8581a37e22 100644
> > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > @@ -80,25 +80,19 @@ static const struct ipu6_csi2_error dphy_rx_errors[] = {
> > > > >  s64 ipu6_isys_csi2_get_link_freq(struct ipu6_isys_csi2 *csi2)
> > > > >  {
> > > > >  	struct media_pad *src_pad;
> > > > > -	struct v4l2_subdev *ext_sd;
> > > > > -	struct device *dev;
> > > > >  
> > > > >  	if (!csi2)
> > > > >  		return -EINVAL;
> > > > >  
> > > > > -	dev = &csi2->isys->adev->auxdev.dev;
> > > > >  	src_pad = media_entity_remote_source_pad_unique(&csi2->asd.sd.entity);
> > > > 
> > > > Not a candidate for this patch, but can the source change, or can it be
> > > > cached at probe time (or notifier bound time) ?
> > > 
> > > It could be, but why would you do that?
> > > 
> > > This would also prevent connecting multiple sensors to a single CSI-2
> > > receiver.
> > 
> > Precisely because people shouldn't do this :-)
> > 
> > It would be more efficient to get the pad at probe time and cache it,
> > and would remove an error path at runtime. Until we have a use case
> 
> I presume it'd take a bug somewhere for that to fail. I don't think a
> relatively small number of instructions would make measurable a difference
> in performance. If we thought this was a problem, there would be a lot to
> work on elsewhere in the framework, starting with the control framework and
> IOCTL handling. The problem is just that nearly all that code is there for
> sound and important reasons.
> 
> > where we need to support more than one sensor on the same CSI-2 receiver
> > for this driver, I think that would be best.
> 
> I don't disagree as such but we can't affect hardware design here. Nothing
> currently prevents that and adding a driver bug that will cause whatever

It wouldn't be a bug, the driver would just ignore all bug the first
source. If that doesn't work for someone, we would hear about it.

> ills when hit is not a solution either, even if the former were true. Well,
> if this were Windows, the situation could be different.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 08:03:08AM +0000, Sakari Ailus wrote:
> On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > Thank you for the patch.
> > 
> > I think this should come before 4/5.
> 
> Because of...? Both are needed for this to work.

Because, unless I'm mistaken, it would otherwise introduce a bisection
breakage. 4/5 drops the LINK_FREQ control from the IVSC driver, and
obtaining the link frequency through .get_mbus_config() is only
supported when using the v4l2_get_link_freq() variant that takes a pad
argument.

> > On Tue, Dec 10, 2024 at 09:59:06AM +0200, Sakari Ailus wrote:
> > > Obtain the link frequency from the sub-device instead of a control
> > > handler. This allows obtaining it using the get_mbus_config() sub-device
> > > pad op that which is only supported by the IVSC driver.
> > 
> > "which is the only method supported by the IVSC driver"
> 
> Yes.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote:
> On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote:
> > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> > > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > > frequency to the receiving sub-device.
> > 
> > The documentation of v4l2_get_link_freq() should be expanded to explain
> > the new mode of operation. It should document which of the supported
> > methods are recommended for new drivers.
> > 
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
> > >  include/media/v4l2-mediabus.h         |  2 ++
> > >  2 files changed, 10 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > index 9fe74c7e064f..88fbd5608f00 100644
> > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> > >  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > >  			     unsigned int div)
> > >  {
> > > +	struct v4l2_mbus_config mbus_config = {};
> > 
> > Isn't mbus_config fully populated by the .get_mbus_config() operation ?
> > That should be documented in the .get_mbus_config() operation
> > documentation.
> 
> It's a good practice to zero the memory before drivers get to work on it. I
> presume drivers will set the fields that are relevant for them and ignore
> the rest.
> 
> I can add a note on get_mbus_config() the drivers should set all struct
> fields to known values.

Thanks.

> > >  	struct v4l2_subdev *sd;
> > > +	int ret;
> > >  
> > >  	sd = media_entity_to_v4l2_subdev(pad->entity);
> > > -	if (!sd)
> > > -		return -ENODEV;
> > > +	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> > > +			       &mbus_config);
> > > +	if (ret < 0 && ret != -ENOIOCTLCMD)
> > > +		return ret;
> > >  
> > > -	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > > +	return mbus_config.link_freq ?:
> > > +		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > 
> > 	if (mbus_config.link_freq)
> > 		return mbus_config.link_freq;
> > 
> > 	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> 
> Whether this would be cleaner is debatable at least. I can switch if you
> insist though.

I think it's nicer. You could even write

 	if (mbus_config.link_freq)
 		return mbus_config.link_freq;

	/*
	 * Fall back to using the link frequency control if the media bus config
	 * doesn't provide a link frequency.
	 */
 	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);

> > I wonder if we should also add a message in case link_freq is 0, to get
> > drivers to convert to reporting the link frequency through
> > .get_mbus_config() if they already implement the operation.
> 
> I'm not sure this will be useful: in most cases the LINK_FREQ control is
> used and for a reason: it's user-configurable. Drivers should only populate
> the field if the link frequency is determined by the driver. For the
> receiver drivers it does not matter: they use v4l2_get_link_freq().

I think this depends on whether or not driver that have a configurable
link frequency should report the current value through
.get_mbus_config(). I think that drivers that implement
.get_mbus_config() should, for consistency.

> > >  }
> > >  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_pad);
> > >  #endif /* CONFIG_MEDIA_CONTROLLER */
> > > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > > index 5bce6e423e94..cc5f776dc662 100644
> > > --- a/include/media/v4l2-mediabus.h
> > > +++ b/include/media/v4l2-mediabus.h
> > > @@ -148,6 +148,7 @@ enum v4l2_mbus_type {
> > >  /**
> > >   * struct v4l2_mbus_config - media bus configuration
> > >   * @type: interface type
> > > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
> > 
> > Not a candidate for this series, but I'd like to simplify drivers by
> > implementing the LINK_FREQ control automatically.
> > 
> > >   * @bus: bus configuration data structure
> > >   * @bus.parallel: embedded &struct v4l2_mbus_config_parallel.
> > >   *		  Used if the bus is parallel or BT.656.
> > > @@ -162,6 +163,7 @@ enum v4l2_mbus_type {
> > >   */
> > >  struct v4l2_mbus_config {
> > >  	enum v4l2_mbus_type type;
> > > +	u64 link_freq;
> > >  	union {
> > >  		struct v4l2_mbus_config_parallel parallel;
> > >  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 08:07:26AM +0000, Sakari Ailus wrote:
> On Sun, Dec 15, 2024 at 07:02:40PM +0200, Laurent Pinchart wrote:
> > On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> > > On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > > > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > > > Add the get_mbus_config() as the means for conveying the link frequency
> > > > > towards the receiver drivers.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > > > index dd09484df1d3..1430c330fd52 100644
> > > > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > > > @@ -49,6 +49,10 @@ Link frequency
> > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > >
> > > > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > > +frequency to the downstream driver in the pipeline.
> > > > 
> > > > When read in its entirety, the paragraphs
> > > > 
> > > >  Link frequency
> > > >  ^^^^^^^^^^^^^^
> > > > 
> > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > 
> > > >  On devices where the link frequency isn't configurable, the link_freq field of
> > > >  struct v4l2_mbus_config is recommended over controls for conveying the link
> > > >  frequency.
> > > > 
> > > > Sounds simpler without the last 7 words.
> > > > 
> > > > A nit and just tastes maybe
> > > 
> > > I used:
> > > 
> > > On devices where the link frequency isn't configurable, using the ``link_freq``
> > > field of struct v4l2_mbus_config is recommended over controls.
> > 
> > Is it a recommendation for the transmitter driver or the receiver driver
> > ? If it's a recommendation for the transmitter driver, does that mean it
> > should not implement V4L2_CID_LINK_FREQ in that case ? How about the
> > need to expose the link frequency to userspace when it's fixed ?
> 
> For the transmitter. Receivers do not implement get_mbus_config op. I can
> add a note on this.

They don't implement it, but they call it. I wasn't sure if this
documentation was from the point of view of the caller or callee. As you
mentioned separately that it's located in the transmitter section, that
makes it clearer, but we can also improve the test:

Drivers that have a fixed link frequency should report it through the
``.get_mbus_config()`` subdev operation, in the``link_freq`` field of
struct v4l2_mbus_config, instead of through controls.

(I'll let you fix the markup for the mention of the .get_mbus_config()
operation.)

> The only reason this has been exposed to the user space is because it's
> been a control that has been modifiable. Also HDMI to CSI-2 bridges work
> this way.
> 
> The LINK_FREQ control is also an integer menu so the same control couldn't
> be used as-is. These were the reasons why the earlier review concluded with
> impelmenting this via get_mbus_config().
> 
> > I think the documentation needs further clarification to clearly explain
> > what transmitters should do and what receivers should do.
> 
> I can add this.
> 
> > > > 
> > > > I like where this is going!
> > > > 
> > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 5/5] media: intel/ipu6: Obtain link frequency from a sub-device

[Sakari Ailus]

On Mon, Dec 16, 2024 at 01:13:26PM +0200, Laurent Pinchart wrote:
> On Mon, Dec 16, 2024 at 08:03:08AM +0000, Sakari Ailus wrote:
> > On Sun, Dec 15, 2024 at 07:08:32PM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > > 
> > > Thank you for the patch.
> > > 
> > > I think this should come before 4/5.
> > 
> > Because of...? Both are needed for this to work.
> 
> Because, unless I'm mistaken, it would otherwise introduce a bisection
> breakage. 4/5 drops the LINK_FREQ control from the IVSC driver, and
> obtaining the link frequency through .get_mbus_config() is only
> supported when using the v4l2_get_link_freq() variant that takes a pad
> argument.

That's a fair point, I can change the order.

-- 
Sakari Ailus

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Sakari Ailus]

Hi Laurent,

On Mon, Dec 16, 2024 at 01:20:26PM +0200, Laurent Pinchart wrote:
> On Mon, Dec 16, 2024 at 08:07:26AM +0000, Sakari Ailus wrote:
> > On Sun, Dec 15, 2024 at 07:02:40PM +0200, Laurent Pinchart wrote:
> > > On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> > > > On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > > > > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > > > > Add the get_mbus_config() as the means for conveying the link frequency
> > > > > > towards the receiver drivers.
> > > > > >
> > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > ---
> > > > > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > > > > >  1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > > > > index dd09484df1d3..1430c330fd52 100644
> > > > > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > > > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > > > > @@ -49,6 +49,10 @@ Link frequency
> > > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > > >
> > > > > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > > > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > > > +frequency to the downstream driver in the pipeline.
> > > > > 
> > > > > When read in its entirety, the paragraphs
> > > > > 
> > > > >  Link frequency
> > > > >  ^^^^^^^^^^^^^^
> > > > > 
> > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > > 
> > > > >  On devices where the link frequency isn't configurable, the link_freq field of
> > > > >  struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > >  frequency.
> > > > > 
> > > > > Sounds simpler without the last 7 words.
> > > > > 
> > > > > A nit and just tastes maybe
> > > > 
> > > > I used:
> > > > 
> > > > On devices where the link frequency isn't configurable, using the ``link_freq``
> > > > field of struct v4l2_mbus_config is recommended over controls.
> > > 
> > > Is it a recommendation for the transmitter driver or the receiver driver
> > > ? If it's a recommendation for the transmitter driver, does that mean it
> > > should not implement V4L2_CID_LINK_FREQ in that case ? How about the
> > > need to expose the link frequency to userspace when it's fixed ?
> > 
> > For the transmitter. Receivers do not implement get_mbus_config op. I can
> > add a note on this.
> 
> They don't implement it, but they call it. I wasn't sure if this
> documentation was from the point of view of the caller or callee. As you
> mentioned separately that it's located in the transmitter section, that
> makes it clearer, but we can also improve the test:
> 
> Drivers that have a fixed link frequency should report it through the
> ``.get_mbus_config()`` subdev operation, in the``link_freq`` field of
> struct v4l2_mbus_config, instead of through controls.

This isn't correct: the link frequency isn't fixed, it's just not
user-configurable. How about:

Drivers that do not have user-configurable link frequency should report it
through the ``.get_mbus_config()`` subdev pad operation, in the ``link_freq``
field of struct v4l2_mbus_config, instead of through controls.

The ReST seems to be in line with the rest.

-- 
Sakari Ailus

Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Sakari Ailus]

Hi Laurent,

On Mon, Dec 16, 2024 at 01:16:45PM +0200, Laurent Pinchart wrote:
> On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote:
> > On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote:
> > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > > > frequency to the receiving sub-device.
> > > 
> > > The documentation of v4l2_get_link_freq() should be expanded to explain
> > > the new mode of operation. It should document which of the supported
> > > methods are recommended for new drivers.
> > > 
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
> > > >  include/media/v4l2-mediabus.h         |  2 ++
> > > >  2 files changed, 10 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > > index 9fe74c7e064f..88fbd5608f00 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> > > >  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > > >  			     unsigned int div)
> > > >  {
> > > > +	struct v4l2_mbus_config mbus_config = {};
> > > 
> > > Isn't mbus_config fully populated by the .get_mbus_config() operation ?
> > > That should be documented in the .get_mbus_config() operation
> > > documentation.
> > 
> > It's a good practice to zero the memory before drivers get to work on it. I
> > presume drivers will set the fields that are relevant for them and ignore
> > the rest.
> > 
> > I can add a note on get_mbus_config() the drivers should set all struct
> > fields to known values.
> 
> Thanks.
> 
> > > >  	struct v4l2_subdev *sd;
> > > > +	int ret;
> > > >  
> > > >  	sd = media_entity_to_v4l2_subdev(pad->entity);
> > > > -	if (!sd)
> > > > -		return -ENODEV;
> > > > +	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> > > > +			       &mbus_config);
> > > > +	if (ret < 0 && ret != -ENOIOCTLCMD)
> > > > +		return ret;
> > > >  
> > > > -	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > > > +	return mbus_config.link_freq ?:
> > > > +		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > > 
> > > 	if (mbus_config.link_freq)
> > > 		return mbus_config.link_freq;
> > > 
> > > 	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > 
> > Whether this would be cleaner is debatable at least. I can switch if you
> > insist though.
> 
> I think it's nicer. You could even write
> 
>  	if (mbus_config.link_freq)
>  		return mbus_config.link_freq;
> 
> 	/*
> 	 * Fall back to using the link frequency control if the media bus config
> 	 * doesn't provide a link frequency.
> 	 */
>  	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);

I can use this.

> 
> > > I wonder if we should also add a message in case link_freq is 0, to get
> > > drivers to convert to reporting the link frequency through
> > > .get_mbus_config() if they already implement the operation.
> > 
> > I'm not sure this will be useful: in most cases the LINK_FREQ control is
> > used and for a reason: it's user-configurable. Drivers should only populate
> > the field if the link frequency is determined by the driver. For the
> > receiver drivers it does not matter: they use v4l2_get_link_freq().
> 
> I think this depends on whether or not driver that have a configurable
> link frequency should report the current value through
> .get_mbus_config(). I think that drivers that implement
> .get_mbus_config() should, for consistency.

We should have a helper that obtains information using get_mbus_config() as
well as the fwnode endpoint. I've proposed that a few times over the years.
I'm hoping for someone who needs dynamic configuration e.g. for lane
numbers to implement it. :-)

I wouldn't try to add more burden for drivers on this. Beyond that, it's
common that if you have multiple implementations of something, one of them
(the unused one) eventually breaks. What we really need is to obtain the
information from the sub-device API, using the method the driver uses to
report it.

-- 
Regards,

Sakari Ailus

Re: [PATCH v7 2/5] media: v4l: Support obtaining link frequency via get_mbus_config

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 12:15:31PM +0000, Sakari Ailus wrote:
> On Mon, Dec 16, 2024 at 01:16:45PM +0200, Laurent Pinchart wrote:
> > On Mon, Dec 16, 2024 at 08:46:43AM +0000, Sakari Ailus wrote:
> > > On Sun, Dec 15, 2024 at 06:59:35PM +0200, Laurent Pinchart wrote:
> > > > On Tue, Dec 10, 2024 at 09:59:03AM +0200, Sakari Ailus wrote:
> > > > > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > > > > frequency to the receiving sub-device.
> > > > 
> > > > The documentation of v4l2_get_link_freq() should be expanded to explain
> > > > the new mode of operation. It should document which of the supported
> > > > methods are recommended for new drivers.
> > > > 
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/v4l2-core/v4l2-common.c | 11 ++++++++---
> > > > >  include/media/v4l2-mediabus.h         |  2 ++
> > > > >  2 files changed, 10 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > > > > index 9fe74c7e064f..88fbd5608f00 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-common.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > > > > @@ -508,13 +508,18 @@ EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> > > > >  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > > > >  			     unsigned int div)
> > > > >  {
> > > > > +	struct v4l2_mbus_config mbus_config = {};
> > > > 
> > > > Isn't mbus_config fully populated by the .get_mbus_config() operation ?
> > > > That should be documented in the .get_mbus_config() operation
> > > > documentation.
> > > 
> > > It's a good practice to zero the memory before drivers get to work on it. I
> > > presume drivers will set the fields that are relevant for them and ignore
> > > the rest.
> > > 
> > > I can add a note on get_mbus_config() the drivers should set all struct
> > > fields to known values.
> > 
> > Thanks.
> > 
> > > > >  	struct v4l2_subdev *sd;
> > > > > +	int ret;
> > > > >  
> > > > >  	sd = media_entity_to_v4l2_subdev(pad->entity);
> > > > > -	if (!sd)
> > > > > -		return -ENODEV;
> > > > > +	ret = v4l2_subdev_call(sd, pad, get_mbus_config, pad->index,
> > > > > +			       &mbus_config);
> > > > > +	if (ret < 0 && ret != -ENOIOCTLCMD)
> > > > > +		return ret;
> > > > >  
> > > > > -	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > > > > +	return mbus_config.link_freq ?:
> > > > > +		__v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > > > 
> > > > 	if (mbus_config.link_freq)
> > > > 		return mbus_config.link_freq;
> > > > 
> > > > 	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> > > 
> > > Whether this would be cleaner is debatable at least. I can switch if you
> > > insist though.
> > 
> > I think it's nicer. You could even write
> > 
> >  	if (mbus_config.link_freq)
> >  		return mbus_config.link_freq;
> > 
> > 	/*
> > 	 * Fall back to using the link frequency control if the media bus config
> > 	 * doesn't provide a link frequency.
> > 	 */
> >  	return __v4l2_get_link_freq_ctrl(sd->ctrl_handler, mul, div);
> 
> I can use this.
> 
> > 
> > > > I wonder if we should also add a message in case link_freq is 0, to get
> > > > drivers to convert to reporting the link frequency through
> > > > .get_mbus_config() if they already implement the operation.
> > > 
> > > I'm not sure this will be useful: in most cases the LINK_FREQ control is
> > > used and for a reason: it's user-configurable. Drivers should only populate
> > > the field if the link frequency is determined by the driver. For the
> > > receiver drivers it does not matter: they use v4l2_get_link_freq().
> > 
> > I think this depends on whether or not driver that have a configurable
> > link frequency should report the current value through
> > .get_mbus_config(). I think that drivers that implement
> > .get_mbus_config() should, for consistency.
> 
> We should have a helper that obtains information using get_mbus_config() as
> well as the fwnode endpoint. I've proposed that a few times over the years.
> I'm hoping for someone who needs dynamic configuration e.g. for lane
> numbers to implement it. :-)

I'm not sure to follow you here, I don't really see how it's related, or
exactly what that helper would do.

> I wouldn't try to add more burden for drivers on this. Beyond that, it's
> common that if you have multiple implementations of something, one of them
> (the unused one) eventually breaks. What we really need is to obtain the
> information from the sub-device API, using the method the driver uses to
> report it.

We should have a single way to report a given piece of information, that
would be the simplest for drivers and the least error prone. I think
drivers should implement the LINK_FREQ control for userspace, and the
.get_mbus_config() operation for kernel space.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v7 3/5] media: Documentation: Update link frequency driver documentation

[Laurent Pinchart]

On Mon, Dec 16, 2024 at 12:05:19PM +0000, Sakari Ailus wrote:
> On Mon, Dec 16, 2024 at 01:20:26PM +0200, Laurent Pinchart wrote:
> > On Mon, Dec 16, 2024 at 08:07:26AM +0000, Sakari Ailus wrote:
> > > On Sun, Dec 15, 2024 at 07:02:40PM +0200, Laurent Pinchart wrote:
> > > > On Fri, Dec 13, 2024 at 07:15:55AM +0000, Sakari Ailus wrote:
> > > > > On Thu, Dec 12, 2024 at 05:53:53PM +0100, Jacopo Mondi wrote:
> > > > > > On Tue, Dec 10, 2024 at 09:59:04AM +0200, Sakari Ailus wrote:
> > > > > > > Add the get_mbus_config() as the means for conveying the link frequency
> > > > > > > towards the receiver drivers.
> > > > > > >
> > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > > > ---
> > > > > > >  Documentation/driver-api/media/tx-rx.rst | 4 ++++
> > > > > > >  1 file changed, 4 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/driver-api/media/tx-rx.rst b/Documentation/driver-api/media/tx-rx.rst
> > > > > > > index dd09484df1d3..1430c330fd52 100644
> > > > > > > --- a/Documentation/driver-api/media/tx-rx.rst
> > > > > > > +++ b/Documentation/driver-api/media/tx-rx.rst
> > > > > > > @@ -49,6 +49,10 @@ Link frequency
> > > > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > > > >
> > > > > > > +On devices where the link frequency isn't configurable, the link_freq field of
> > > > > > > +struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > > > > +frequency to the downstream driver in the pipeline.
> > > > > > 
> > > > > > When read in its entirety, the paragraphs
> > > > > > 
> > > > > >  Link frequency
> > > > > >  ^^^^^^^^^^^^^^
> > > > > > 
> > > > > >  The :ref:`V4L2_CID_LINK_FREQ <v4l2-cid-link-freq>` control is used to tell the
> > > > > >  receiver the frequency of the bus (i.e. it is not the same as the symbol rate).
> > > > > > 
> > > > > >  On devices where the link frequency isn't configurable, the link_freq field of
> > > > > >  struct v4l2_mbus_config is recommended over controls for conveying the link
> > > > > >  frequency.
> > > > > > 
> > > > > > Sounds simpler without the last 7 words.
> > > > > > 
> > > > > > A nit and just tastes maybe
> > > > > 
> > > > > I used:
> > > > > 
> > > > > On devices where the link frequency isn't configurable, using the ``link_freq``
> > > > > field of struct v4l2_mbus_config is recommended over controls.
> > > > 
> > > > Is it a recommendation for the transmitter driver or the receiver driver
> > > > ? If it's a recommendation for the transmitter driver, does that mean it
> > > > should not implement V4L2_CID_LINK_FREQ in that case ? How about the
> > > > need to expose the link frequency to userspace when it's fixed ?
> > > 
> > > For the transmitter. Receivers do not implement get_mbus_config op. I can
> > > add a note on this.
> > 
> > They don't implement it, but they call it. I wasn't sure if this
> > documentation was from the point of view of the caller or callee. As you
> > mentioned separately that it's located in the transmitter section, that
> > makes it clearer, but we can also improve the test:
> > 
> > Drivers that have a fixed link frequency should report it through the
> > ``.get_mbus_config()`` subdev operation, in the``link_freq`` field of
> > struct v4l2_mbus_config, instead of through controls.
> 
> This isn't correct: the link frequency isn't fixed, it's just not
> user-configurable. How about:
> 
> Drivers that do not have user-configurable link frequency should report it
> through the ``.get_mbus_config()`` subdev pad operation, in the ``link_freq``
> field of struct v4l2_mbus_config, instead of through controls.

I'm OK with that.

> The ReST seems to be in line with the rest.

:-)

-- 
Regards,

Laurent Pinchart