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

[Hans Verkuil]

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

Regards,

	Hans