[PATCH v3 1/2] media: uapi: add MEDIA_BUS_FMT_METADATA_FIXED media bus format.

[Sakari Ailus]

Hi Hans,

On Wed, Nov 04, 2020 at 01:16:03PM +0100, Hans Verkuil wrote:
> On 30/10/2020 15:02, Sakari Ailus wrote:
> > Hi Dafna,
> > 
> > On Fri, Oct 30, 2020 at 02:46:08PM +0100, Dafna Hirschfeld wrote:
> >> MEDIA_BUS_FMT_METADATA_FIXED should be used when
> >> the same driver handles both sides of the link and
> >> the bus format is a fixed metadata format that is
> >> not configurable from userspace.
> >> The width and height will be set to 0 for this format.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld at collabora.com>
> >> Acked-by: Helen Koike <helen.koike at collabora.com>
> >> ---
> >> changes since v2:
> >> added documentation in subdev-formats.rst
> >> changes since v1:
> >> 1. replace "This format may have 0 height and width."
> >> with "Width and height will be set to 0 for this format."
> >> and add it also to the commit log
> >> 2. s/meida:/media:/ in the patch subject line
> >>
> >>  .../media/v4l/subdev-formats.rst              | 27 +++++++++++++++++++
> >>  include/uapi/linux/media-bus-format.h         |  8 ++++++
> >>  2 files changed, 35 insertions(+)
> >>
> >> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> index c9b7bb3ca089..7f16cbe46e5c 100644
> >> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> >> @@ -7899,3 +7899,30 @@ formats.
> >>        - 0x5001
> >>        - Interleaved raw UYVY and JPEG image format with embedded meta-data
> >>  	used by Samsung S3C73MX camera sensors.
> >> +
> >> +.. _v4l2-mbus-metadata-fmts:
> >> +
> >> +Metadata Formats
> >> +^^^^^^^^^^^^^^^^
> >> +
> >> +This section lists all metadata formats.
> >> +
> >> +The following table lists the existing metadata formats.
> >> +
> >> +.. tabularcolumns:: |p{8.0cm}|p{1.4cm}|p{7.7cm}|
> >> +
> >> +.. flat-table:: Metadata formats
> >> +    :header-rows:  1
> >> +    :stub-columns: 0
> >> +
> >> +    * - Identifier
> >> +      - Code
> >> +      - Comments
> >> +    * .. _MEDIA-BUS-FMT-METADATA-FIXED:
> >> +
> >> +      - MEDIA_BUS_FMT_METADATA_FIXED
> >> +      - 0x7001
> >> +      - This format should be used when the same driver handles
> >> +	both sides of the link and the bus format is a fixed
> >> +	metadata format that is not configurable from userspace.
> >> +	Width and height will be set to 0 for this format.
> >> diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
> >> index 84fa53ffb13f..2ce3d891d344 100644
> >> --- a/include/uapi/linux/media-bus-format.h
> >> +++ b/include/uapi/linux/media-bus-format.h
> >> @@ -156,4 +156,12 @@
> >>  /* HSV - next is	0x6002 */
> >>  #define MEDIA_BUS_FMT_AHSV8888_1X32		0x6001
> >>  
> >> +/*
> >> + * This format should be used when the same driver handles
> >> + * both sides of the link and the bus format is a fixed
> >> + * metadata format that is not configurable from userspace.
> >> + * Width and height will be set to 0 for this format.
> >> + */
> > 
> > Does this mean that metadata with dimensions should not use
> > MEDIA_BUS_FMT_METADATA_FIXED? I guess that's not the intention? For some
> > formats the dimensions would be relevant but for others not. I'd thus
> > replace "will" by "may". Same for the documentation.
> 
> struct v4l2_meta_format as used with VIDIOC_G/S/TRY_FMT doesn't have
> a width or height either. Supporting width and height for metadata
> doesn't really make sense for me for metadata.
> 
> Explicitly specifying the width and height here indicates that the
> data is basically an array of width x height of some sort which makes
> sense for video devices.
> 
> Metadata is more complex data that cannot be represented like that.
> If metadata is actually an array, then I'm not sure I would call it
> metadata, I would probably see it as video with its own pixelformat
> that contains non-video data.

Let's say the metadata is laid out in a similar way than an image; you have
lines of data, followed by some padding at the end, and a line has length
and a buffer has a number of lines. Sensor metadata falls into this
description.

Would you then use struct v4l2_pix_format for it, and use
V4L2_BUF_TYPE_VIDEO_CAPTURE for it?

That would make a few things easier but this is still metadata, not video
data. Albeit I guess it's not important to be so strict about that
interface-wise, indeed this is not a bad fit for such metadata. Still some
fields such as colourspace and quantisation are not relevant, but that
holds also for some pixel formats.

-- 
Regards,

Sakari Ailus