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

[Sakari Ailus]

On Thu, Nov 05, 2020 at 01:35:00PM +0100, Hans Verkuil wrote:
> On 04/11/2020 15:54, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Nov 04, 2020 at 02:46:39PM +0100, Hans Verkuil wrote:
> >> On 04/11/2020 13:32, Sakari Ailus wrote:
> >>> 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?
> >>
> >> Yes. It's still metadata, but it has the same 'format' as video data.
> >> We have similar situations such as with v4l-touch devices: the data
> >> is formatted like video, but it is actually pressure values from a
> >> touch pad. But it is 'video-like' in its behavior.
> >>
> >>>
> >>> 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.
> >>>
> >>
> >> So are you OK with setting width and height to 0 for MEDIA_BUS_FMT_METADATA_*?
> > 
> > One more question.
> > 
> > What do you do if a link can carry both metadata (as in
> > V4L2_BUF_TYPE_METADATA_CAPTURE) as well as pixel data, but with a fixed
> > format?
> > 
> > I'm not sure we have any such case at the moment but it's not
> > inconceivable.
> > 
> 
> This should be reflected in the mediabus format. So METADATA_FIXED if it carries
> metadata, or a video format if it carries video. Userspace could configure this
> with VIDIOC_SUBDEV_S_FMT if it is something that userspace can actually change.

Works for me.

Acked-by: Sakari Ailus <sakari.ailus at linux.intel.com>

-- 
Sakari Ailus