[PATCH v4 0/4] Use V4L2 mbus config for conveying MEI CSI link frequency

[PATCH v4 0/4] 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 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 (4):
  media: v4l: Support passing sub-device 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

 Documentation/driver-api/media/tx-rx.rst |  4 ++
 drivers/media/pci/intel/ivsc/mei_csi.c   | 72 ++++++++----------------
 drivers/media/v4l2-core/v4l2-common.c    | 24 +++++++-
 include/media/v4l2-common.h              | 14 ++++-
 include/media/v4l2-mediabus.h            |  2 +
 5 files changed, 63 insertions(+), 53 deletions(-)


base-commit: 12e3384b874e9f67c3e98ce501561fe5a54a0add
-- 
2.39.2

[PATCH v4 1/4] media: v4l: Support passing sub-device 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 v4l2_subdev argument in order to obtain the link frequency
using get_mbus_config() pad op. Prepare for this by allowing struct
v4l2_subdev as well.

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

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 4165c815faef..7f69b5a025fa 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -464,8 +464,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;
@@ -500,7 +500,20 @@ 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);
+
+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);
 
 /*
  * 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..d7115cd61a38 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 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,15 @@ 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);
+#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);
+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.2

[PATCH v4 2/4] 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 reciving sub-device.

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

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 7f69b5a025fa..09b26ce612e9 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -503,15 +503,20 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
 EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
 
 s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
-			     unsigned int div)
+			      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);
 
diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 5bce6e423e94..2f39b52bb4d4 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -159,6 +159,7 @@ enum v4l2_mbus_type {
  * @bus.mipi_csi2: embedded &struct v4l2_mbus_config_mipi_csi2.
  *		   Used if the bus is MIPI Alliance's Camera Serial
  *		   Interface version 2 (MIPI CSI2).
+ * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
  */
 struct v4l2_mbus_config {
 	enum v4l2_mbus_type type;
@@ -167,6 +168,7 @@ struct v4l2_mbus_config {
 		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
 		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
 	} bus;
+	u64 link_freq;
 };
 
 /**
-- 
2.39.2

[PATCH v4 3/4] 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 29d66a47b56e..2f22a1534da9 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).
 
+For devices where the link frequency is read-only, 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.
+
 ``.s_stream()`` callback
 ^^^^^^^^^^^^^^^^^^^^^^^^
 
-- 
2.39.2

[PATCH v4 4/4] 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 89b582a221ab..621a6ee32e01 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,12 +119,11 @@ 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;
-	unsigned int remote_pad;
 	/* start streaming or not */
 	int streaming;
 
@@ -148,10 +145,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);
@@ -284,11 +277,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);
@@ -307,11 +302,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);
@@ -468,34 +463,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,
@@ -504,6 +494,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 = {
@@ -531,8 +522,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,
@@ -556,26 +546,14 @@ 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;
 
-	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->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.2

Re: [PATCH v4 1/4] media: v4l: Support passing sub-device argument to v4l2_get_link_freq()

[Jacopo Mondi]

Hi Sakari

On Mon, Apr 29, 2024 at 10:08:49PM +0300, Sakari Ailus wrote:
> v4l2_get_link_freq() accepts a V4L2 control handler for now, but it needs
> to take struct v4l2_subdev argument in order to obtain the link frequency
> using get_mbus_config() pad op. Prepare for this by allowing struct
> v4l2_subdev as well.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Nice! I need exactly something like this to avoid pre-computing a lot
a frequencies in a driver I'm working on.

> ---
>  drivers/media/v4l2-core/v4l2-common.c | 19 ++++++++++++++++---
>  include/media/v4l2-common.h           | 14 +++++++++++---
>  2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 4165c815faef..7f69b5a025fa 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -464,8 +464,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;
> @@ -500,7 +500,20 @@ 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);
> +
> +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);
>
>  /*
>   * 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..d7115cd61a38 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 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,15 @@ 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);
> +#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)

That's nice and elegant, but given that I count 10 drivers in mainline
using this function, wouldn't it be better to make them use a pad and
remove the ctrl_handler version completely to avoid it being used in
future ?

> +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> +			     unsigned int div);
> +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.2
>
>

Re: [PATCH v4 2/4] media: v4l: Support obtaining link frequency via get_mbus_config

[Jacopo Mondi]

Hi Sakari

On Mon, Apr 29, 2024 at 10:08:50PM +0300, Sakari Ailus wrote:
> Add link_freq field to struct v4l2_mbus_config in order to pass the link
> frequency to the reciving sub-device.

'receiving'

>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-common.c | 13 +++++++++----
>  include/media/v4l2-mediabus.h         |  2 ++
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 7f69b5a025fa..09b26ce612e9 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -503,15 +503,20 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
>  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
>
>  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> -			     unsigned int div)
> +			      unsigned int div)

Goes in the previous patch that introduces the function ?

>  {
> +	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);
>
> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> index 5bce6e423e94..2f39b52bb4d4 100644
> --- a/include/media/v4l2-mediabus.h
> +++ b/include/media/v4l2-mediabus.h
> @@ -159,6 +159,7 @@ enum v4l2_mbus_type {
>   * @bus.mipi_csi2: embedded &struct v4l2_mbus_config_mipi_csi2.
>   *		   Used if the bus is MIPI Alliance's Camera Serial
>   *		   Interface version 2 (MIPI CSI2).
> + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
>   */
>  struct v4l2_mbus_config {
>  	enum v4l2_mbus_type type;
> @@ -167,6 +168,7 @@ struct v4l2_mbus_config {
>  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
>  		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
>  	} bus;
> +	u64 link_freq;

Does this apply to all the supported busses ?

I count:

        struct v4l2_mbus_config_parallel parallel;
        struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
        struct v4l2_mbus_config_mipi_csi2 mipi_csi2;

not sure about csi1, but I would say "yes" ?

However it would feel more natural to fetch the 'link_freq' from the
bus-specific structure like in 'bus.mipi_csi2.link_freq' (and 'bus' is
an union, so we're not consuming any additional space if we move it to
the per-bus structure).


>  };
>
>  /**
> --
> 2.39.2
>
>

Re: [PATCH v4 3/4] media: Documentation: Update link frequency driver documentation

[Jacopo Mondi]

Hi Sakari

On Mon, Apr 29, 2024 at 10:08:51PM +0300, 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 29d66a47b56e..2f22a1534da9 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).
>
> +For devices where the link frequency is read-only, the link_freq field of struct

A control can be 'read-only' as well.

What about something along the lines of:

For devices where the link frequency doesn't need to be exposed to userspace,
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.
> +
>  ``.s_stream()`` callback
>  ^^^^^^^^^^^^^^^^^^^^^^^^
>
> --
> 2.39.2
>
>

Re: [PATCH v4 1/4] media: v4l: Support passing sub-device argument to v4l2_get_link_freq()

[Sakari Ailus]

Hi Jacopo,

Thanks for the review.

On Tue, Apr 30, 2024 at 09:01:09AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Apr 29, 2024 at 10:08:49PM +0300, Sakari Ailus wrote:
> > v4l2_get_link_freq() accepts a V4L2 control handler for now, but it needs
> > to take struct v4l2_subdev argument in order to obtain the link frequency

This should have been media_pad actually. I'll update the text for v5 (same
in the subject).

> > using get_mbus_config() pad op. Prepare for this by allowing struct
> > v4l2_subdev as well.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Nice! I need exactly something like this to avoid pre-computing a lot
> a frequencies in a driver I'm working on.
> 
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 19 ++++++++++++++++---
> >  include/media/v4l2-common.h           | 14 +++++++++++---
> >  2 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 4165c815faef..7f69b5a025fa 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -464,8 +464,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;
> > @@ -500,7 +500,20 @@ 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);
> > +
> > +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);
> >
> >  /*
> >   * 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..d7115cd61a38 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 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,15 @@ 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);
> > +#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)
> 
> That's nice and elegant, but given that I count 10 drivers in mainline
> using this function, wouldn't it be better to make them use a pad and
> remove the ctrl_handler version completely to avoid it being used in
> future ?

It'd be nice to add a function that takes the CSI-2 configuration from the
endpoint as well, doing this manually in drivers is awkward and a lot of
extra code. I'm all for this but I'd do this separately, after which we can
remove the above macro.

Note that for the IVSC driver this is a bugfix.

> 
> > +s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > +			     unsigned int div);
> > +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);

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 2/4] media: v4l: Support obtaining link frequency via get_mbus_config

[Sakari Ailus]

Hi Jacopo,

On Tue, Apr 30, 2024 at 09:19:32AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Apr 29, 2024 at 10:08:50PM +0300, Sakari Ailus wrote:
> > Add link_freq field to struct v4l2_mbus_config in order to pass the link
> > frequency to the reciving sub-device.
> 
> 'receiving'
> 
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-common.c | 13 +++++++++----
> >  include/media/v4l2-mediabus.h         |  2 ++
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 7f69b5a025fa..09b26ce612e9 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -503,15 +503,20 @@ s64 __v4l2_get_link_freq_ctrl(struct v4l2_ctrl_handler *handler,
> >  EXPORT_SYMBOL_GPL(__v4l2_get_link_freq_ctrl);
> >
> >  s64 __v4l2_get_link_freq_pad(struct media_pad *pad, unsigned int mul,
> > -			     unsigned int div)
> > +			      unsigned int div)
> 
> Goes in the previous patch that introduces the function ?

Yes.

> 
> >  {
> > +	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);
> >
> > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
> > index 5bce6e423e94..2f39b52bb4d4 100644
> > --- a/include/media/v4l2-mediabus.h
> > +++ b/include/media/v4l2-mediabus.h
> > @@ -159,6 +159,7 @@ enum v4l2_mbus_type {
> >   * @bus.mipi_csi2: embedded &struct v4l2_mbus_config_mipi_csi2.
> >   *		   Used if the bus is MIPI Alliance's Camera Serial
> >   *		   Interface version 2 (MIPI CSI2).
> > + * @link_freq: The link frequency. See also V4L2_CID_LINK_FREQ control.
> >   */
> >  struct v4l2_mbus_config {
> >  	enum v4l2_mbus_type type;
> > @@ -167,6 +168,7 @@ struct v4l2_mbus_config {
> >  		struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
> >  		struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> >  	} bus;
> > +	u64 link_freq;
> 
> Does this apply to all the supported busses ?

Just like the LINK_FREQ control, it does, albeit the current user (IVSC
driver) uses CSI-2 only.

I'm fine moving it after "type" though, that may be a better location for
it.

> 
> I count:
> 
>         struct v4l2_mbus_config_parallel parallel;
>         struct v4l2_mbus_config_mipi_csi1 mipi_csi1;
>         struct v4l2_mbus_config_mipi_csi2 mipi_csi2;
> 
> not sure about csi1, but I would say "yes" ?
> 
> However it would feel more natural to fetch the 'link_freq' from the
> bus-specific structure like in 'bus.mipi_csi2.link_freq' (and 'bus' is
> an union, so we're not consuming any additional space if we move it to
> the per-bus structure).
> 
> 
> >  };
> >
> >  /**

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 3/4] media: Documentation: Update link frequency driver documentation

[Sakari Ailus]

Hi Jacopo,

On Tue, Apr 30, 2024 at 09:23:52AM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Apr 29, 2024 at 10:08:51PM +0300, 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 29d66a47b56e..2f22a1534da9 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).
> >
> > +For devices where the link frequency is read-only, the link_freq field of struct
> 
> A control can be 'read-only' as well.
> 
> What about something along the lines of:
> 
> For devices where the link frequency doesn't need to be exposed to userspace,
> the link_freq field of struct

I think we could use something like that. The LINK_FREQ control indeed
allows choosing this from the user space but other than that it has little
use AFAIU.

> 
> > +v4l2_mbus_config is recommended over controls for conveying the link frequency
> > +to the downstream driver in the pipeline.
> > +
> >  ``.s_stream()`` callback
> >  ^^^^^^^^^^^^^^^^^^^^^^^^
> >

-- 
Kind regards,

Sakari Ailus