Re: [PATCH 03/17] media: v4l2-subdev: Prepare for changes in getting frame descriptors

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari

On Wed, May 13, 2026 at 01:43:44PM +0300, Sakari Ailus wrote:
> Introduce v4l2_subdev_alloc_frame_desc() and v4l2_subdev_free_frame_desc()
> to both facilitate implementing drivers that need frame descriptors as
> well as prepare for having a larger number of frame descriptors.

As noticed by Frank, this line doesn't seem to match the patch content

> If the remote sub-device does not support frame descriptors,
> v4l2_subdev_get_frame_desc() creates one (with a single entry)
> opportunistically, thus avoiding the need to add frame descriptor support
> to sensor drivers the device for which only generates a single stream, or
> managing the situation on the caller side.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 59 +++++++++++++++++++++++++++
>  include/media/v4l2-subdev.h           | 20 +++++++++
>  2 files changed, 79 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index d93ed50255ed..b8acce8f9c33 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -20,6 +20,7 @@
>  #include <linux/version.h>
>  #include <linux/videodev2.h>
>
> +#include <media/mipi-csi2.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -2671,6 +2672,64 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
>  }
>  EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_desc_passthrough);
>
> +int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_frame_desc *desc)
> +{
> +	struct v4l2_subdev_format subdev_fmt = {
> +		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> +		.pad = pad,
> +	};
> +	int ret;

is it worth being extra paranoid and add an

        if (WARN_ON(!desc))
                return -EINVAL;

here ?

And make immediately sure that desc->type is populated by checking for

        if (desc->type == V4L2_MBUS_FRAME_DESC_TYPE_UNDEFINED)
                return -EINVAL;

or is passing UNDEFINED in supported ?

> +
> +	if (v4l2_subdev_has_op(sd, pad, get_frame_desc)) {
> +		unsigned int type = desc->type;
> +
> +		ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, desc);
> +		if (ret)
> +			return ret;
> +
> +		if (desc->type != type) {
> +			dev_dbg(sd->dev,
> +				"wrong type of frame descriptor for pad %d (got %u, expected %u)\n",
> +				pad, desc->type, type);
> +			return -EINVAL;
> +		}
> +
> +		return ret;

You can return 0;

> +	}
> +
> +	if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL &&
> +	    desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> +		return -EINVAL;
> +
> +	struct v4l2_subdev_state *state =
> +		v4l2_subdev_lock_and_get_active_state(sd);
> +	ret = v4l2_subdev_call(sd, pad, get_fmt, state, &subdev_fmt);
> +	v4l2_subdev_unlock_state(state);
> +	if (ret)
> +		return ret;
> +
> +	struct v4l2_mbus_frame_desc_entry entry = {
> +		.pixelcode = subdev_fmt.format.code,
> +	};
> +
> +	if (desc->type == V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> +		int dt;
> +
> +		dt = mipi_csi2_dt_for_mbus(subdev_fmt.format.code);
> +		if (dt < 0)
> +			return dt;
> +
> +		entry.bus.csi2.dt = dt;
> +	}
> +
> +	desc->entry[0] = entry;

Can't you populate desc->entry[0] directly and avoid a copy ?

> +	desc->num_entries = 1;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_desc);
> +
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d256b7ec8f84..c9e74566c85a 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -1778,6 +1778,26 @@ int v4l2_subdev_get_frame_desc_passthrough(struct v4l2_subdev *sd,
>  					   unsigned int pad,
>  					   struct v4l2_mbus_frame_desc *fd);
>
> +/**
> + * v4l2_subdev_get_frame_desc() - Get a frame descriptor for a pad
> + * @sd: The sub-device
           The -remote- sub-device ?

> + * @pad: The number of the pad in @sd from which to obtain the frame descriptor
> + * @desc: A pointer to a frame descriptor, with its type field set
> + *
> + * Obtain a frame descriptor from a sub-device. If the sub-device supports the
> + * get_frame_desc pad operation, its result is returned, just like calling it
> + * directly using v4l2_subdev_call(). If the sub-device driver does not support
> + * it, then a frame descriptor containing a single entry is created using the
> + * information from the sub-device format for types
> + * V4L2_MBUS_FRAME_DESC_TYPE_CSI2 and V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL.
> + *
> + * The caller is required to set @desc->type to the expected bus type.
> + *
> + * Return: %0 on success or negative error code on failure.
> + */
> +int v4l2_subdev_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> +			       struct v4l2_mbus_frame_desc *desc);
> +
>  #endif /* CONFIG_VIDEO_V4L2_SUBDEV_API */
>
>  #endif /* CONFIG_MEDIA_CONTROLLER */
> --
> 2.47.3
>
>