Re: [PATCH v4 24/29] media: v4l2-subdev: Introduce v4l2_subdev_get_frame_desc()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Mon, Apr 13, 2026 at 11:07:07AM +0300, Sakari Ailus wrote:
> On Fri, Apr 10, 2026 at 12:53:42PM +0200, Jacopo Mondi wrote:
> > On Wed, Apr 08, 2026 at 06:39:33PM +0300, Sakari Ailus wrote:
> > > Introduce v4l2_subdev_get_frame_desc() in order to facilitate implementing
> > > drivers that need frame descriptors. 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

s/the device for which/whose device/

I think all sensor drivers should implement .get_frame_desc(), possibly
with a helper.

> > > single stream, or managing the situation on the caller side.

That part I agree with, we currently need to manage the situation on the
caller side because not all subdevs implement the operation. That will
still be the case for a while, so this helper is useful.

Can you also replace manual calls to .get_frame_desc() with the helper ?
Some of the existing callers implement fallbacks, I fonud them at least
in

drivers/media/platform/broadcom/bcm2835-unicam.c
drivers/media/platform/raspberrypi/rp1-cfe/cfe.c
drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
drivers/media/platform/ti/cal/cal.c

(directly in the function that calls .get_frame_desc(), or in its
caller)

Replacing those will ensure the helper works as expected.

> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-subdev.c | 96 +++++++++++++++++++++++++++
> > >  include/media/v4l2-subdev.h           | 20 ++++++
> > >  2 files changed, 116 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 647587c0499a..40b28e070726 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>
> > > @@ -2758,3 +2759,98 @@ void v4l2_subdev_put_privacy_led(struct v4l2_subdev *sd)
> > >  #endif
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_subdev_put_privacy_led);
> > > +
> > > +static int get_mipi_dt_for_mbus(u32 code)

v4l2_subdev_get_mipi_dt_for_mbus()

Or move the function to v4l2-common.c, name it mipi_csi2_dt_for_mbus()
and declare it in include/media/mipi-csi2.h.

> > > +{
> > > +	switch (code) {
> > > +	case MEDIA_BUS_FMT_BGR888_1X24:
> > > +		return MIPI_CSI2_DT_RGB888;
> > > +	case MEDIA_BUS_FMT_Y8_1X8:
> > > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > > +		return MIPI_CSI2_DT_RAW8;
> > > +	case MEDIA_BUS_FMT_Y10_1X10:
> > > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > > +		return MIPI_CSI2_DT_RAW10;
> > > +	case MEDIA_BUS_FMT_Y12_1X12:
> > > +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> > > +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> > > +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> > > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > > +		return MIPI_CSI2_DT_RAW12;
> > > +	case MEDIA_BUS_FMT_Y14_1X14:
> > > +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> > > +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> > > +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> > > +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> > > +		return MIPI_CSI2_DT_RAW14;
> > > +	case MEDIA_BUS_FMT_Y16_1X16:
> > > +	case MEDIA_BUS_FMT_SBGGR16_1X16:
> > > +	case MEDIA_BUS_FMT_SGBRG16_1X16:
> > > +	case MEDIA_BUS_FMT_SGRBG16_1X16:
> > > +	case MEDIA_BUS_FMT_SRGGB16_1X16:
> > > +		return MIPI_CSI2_DT_RAW16;
> > > +	case MEDIA_BUS_FMT_SBGGR20_1X20:
> > > +	case MEDIA_BUS_FMT_SGBRG20_1X20:
> > > +	case MEDIA_BUS_FMT_SGRBG20_1X20:
> > > +	case MEDIA_BUS_FMT_SRGGB20_1X20:
> > > +		return MIPI_CSI2_DT_RAW20;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +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;
> > > +
> > > +	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 (desc->type != type)
> > > +			return -EINVAL;

I'd add a dev_err() here. There are .get_frame_desc() callers that check
if the returned type matches what they expect and log an error
otherwise. When using this helper the check can't be performed in the
callera any more, leading to possibly hard to debug issues if no message
is printed.

> > > +
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (desc->type != V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL &&
> > > +	    desc->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
> > > +		return -EINVAL;
> > > +
> > > +	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, NULL,
> > > +			       &subdev_fmt);
> > > +	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 = get_mipi_dt_for_mbus(subdev_fmt.format.code);
> > > +		if (dt < 0)
> > > +			return dt;
> > > +
> > > +		entry.bus.csi2.dt = dt;
> > > +	}
> > > +
> > > +	desc->entry[0] = entry;
> > > +	desc->num_entries = 1;
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_subdev_get_frame_desc);
> > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > > index 4588992b4417..93b672edd08e 100644
> > > --- a/include/media/v4l2-subdev.h
> > > +++ b/include/media/v4l2-subdev.h
> > > @@ -2058,4 +2058,24 @@ void v4l2_subdev_notify_event(struct v4l2_subdev *sd,
> > >   */
> > >  bool v4l2_subdev_is_streaming(struct v4l2_subdev *sd);
> > >
> > > +/**
> > > + * v4l2_subdev_get_frame_desc() - Get a pad's frame descriptor
> > > + * @sd: The 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 one containing a single entry is created using the information from

s/one /a frame descriptor/

> > > + * the sub-device active state, which this function locks for the duration of
> > > + * the call to obtain it.
> > 
> > This doesn't seem to apply anymore...
> 
> I'll rephrase it.
> 
> > > + *
> > > + * The caller is required to set @desc->type to the expected bus type.
> > 
> > Is it worth mentioning that only V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL
> > and V4L2_MBUS_FRAME_DESC_TYPE_CSI2 are supported ?
> 
> I'll add that.
> 
> > > + *
> > > + * 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);
> > 
> > With the documentation addressed
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

-- 
Regards,

Laurent Pinchart