Re: [PATCH v11 24/66] media: uapi: Add V4L2_CID_METADATA_LAYOUT control

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

Hi Jacopo,

On Mon, Sep 01, 2025 at 05:07:55PM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Aug 25, 2025 at 12:50:25PM +0300, Sakari Ailus wrote:
> > The metadata layout control defines the layout of the metadata on the bus.
> > It is used on sub-devices that use generic metadata mbus codes on at least
> > on one of the pads.
> 
> Ideally:
> 1) we have per pad controls
> 2) s/V4L2_CID_COLOR_PATTERN/V4L2_CID_DATA_PATTERN
> 3) we use V4L2_CID_DATA_PATTERN for both color components ordering and
>    metadata layouts. A sub-device will expose the same control, but on
>    different pads
> 
> I know we don't want to block this series to wait for per-pad
> controls, but I'm a bit afraid V4L2_CID_METADATA_LAYOUT will be made
> redundant before it gets used for real (which maybe it's not a big
> deal as if no one uses it, we will be able to easily replace it).

I wouldn't do that, for the reason that the mbus codes in the two cases
will be different in any case so both will have separate namespaces.

Metadata is also unaffected by flipping, unlike the colour patterns.

I wonder what others think.

> 
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  .../userspace-api/media/v4l/ext-ctrls-image-source.rst      | 6 ++++++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c                   | 1 +
> >  include/uapi/linux/v4l2-controls.h                          | 2 ++
> >  3 files changed, 9 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> > index 3cb7ee1b0aed..64c0f9ff5b1b 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-image-source.rst
> > @@ -145,3 +145,9 @@ Image Source Control IDs
> >      of the reversed readout. ``V4L2_COLOR_PATTERN_FLIP_BOTH`` for setting both
> >      ``V4L2_COLOR_PATTERN_FLIP_HORIZONTAL`` and
> >      ``V4L2_COLOR_PATTERN_FLIP_VERTICAL`` is provided as well.
> > +
> > +``V4L2_CID_METADATA_LAYOUT (integer)``
> > +    The metadata layout control defines the on-bus metadata layout for metadata
> > +    streams. The control is used in conjunction with :ref:`generic metadata
> > +    formats <media-bus-format-generic-meta>` formats to specify the layout of the
> > +    data.
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index 9ec65998a8f7..a7ea380de5ee 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1157,6 +1157,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_NOTIFY_GAINS:		return "Notify Gains";
> >  	case V4L2_CID_COLOR_PATTERN:		return "Color Pattern";
> >  	case V4L2_CID_COLOR_PATTERN_FLIP:	return "Color Pattern Flip";
> > +	case V4L2_CID_METADATA_LAYOUT:		return "Metadata Layout";
> >
> >  	/* Image processing controls */
> >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index eb9ffdd74d32..74e22a930be4 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1239,6 +1239,8 @@ enum v4l2_jpeg_chroma_subsampling {
> >  #define V4L2_COLOR_PATTERN_FLIP_BOTH \
> >  	(V4L2_COLOR_PATTERN_FLIP_HORIZONTAL | V4L2_COLOR_PATTERN_FLIP_VERTICAL)
> >
> > +#define V4L2_CID_METADATA_LAYOUT		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 12)
> > +
> 
> Intentional empty line ?

I'll drop it.

> 
> For the time being
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thank you.

-- 
Kind regards,

Sakari Ailus