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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

On Thu, May 14, 2026 at 12:02:50PM +0200, Jacopo Mondi wrote:
> 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

Indeed. I'll fix this for v2.

> 
> > 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;

In v2 the frame descriptor will be returned to the caller.

> 
> 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 ?

In fact for v4l2_subdev_get_frame_desc() we should require either of the
supported types. I'll move the descriptor type check here.

> 
> > +
> > +	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;

Yes.

> 
> > +	}
> > +
> > +	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 ?

That'd work, yes. I'll change this for v2.

> 
> > +	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 ?

The API is meant for all sub-devices, in most cases it's remote but not
always. Getting a frame descriptor from the local sub-device likely has no
use cases but I wouldn't document that here.

> 
> > + * @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 */

-- 
Kind regards,

Sakari Ailus