diff mbox series

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

Message ID 20201030134609.30020-1-dafna.hirschfeld@collabora.com
State Accepted
Commit 6ad253cc3436269fc6bcff03d704c672f368da0a
Headers show
Series [v3,1/2] media: uapi: add MEDIA_BUS_FMT_METADATA_FIXED media bus format. | expand

Commit Message

Dafna Hirschfeld Oct. 30, 2020, 1:46 p.m. UTC
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@collabora.com>
Acked-by: Helen Koike <helen.koike@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(+)

Comments

Sakari Ailus Oct. 30, 2020, 2:02 p.m. UTC | #1
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@collabora.com>

> Acked-by: Helen Koike <helen.koike@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.

> +#define MEDIA_BUS_FMT_METADATA_FIXED		0x7001

> +

>  #endif /* __LINUX_MEDIA_BUS_FORMAT_H */


-- 
Kind regards,

Sakari Ailus
Hans Verkuil Nov. 4, 2020, 12:16 p.m. UTC | #2
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@collabora.com>
>> Acked-by: Helen Koike <helen.koike@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.

Regards,

	Hans

> 
>> +#define MEDIA_BUS_FMT_METADATA_FIXED		0x7001
>> +
>>  #endif /* __LINUX_MEDIA_BUS_FORMAT_H */
>
Sakari Ailus Nov. 4, 2020, 12:32 p.m. UTC | #3
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@collabora.com>
> >> Acked-by: Helen Koike <helen.koike@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.
Hans Verkuil Nov. 4, 2020, 1:46 p.m. UTC | #4
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@collabora.com>

>>>> Acked-by: Helen Koike <helen.koike@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
Sakari Ailus Nov. 4, 2020, 2:54 p.m. UTC | #5
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@collabora.com>
> >>>> Acked-by: Helen Koike <helen.koike@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.
Hans Verkuil Nov. 5, 2020, 12:35 p.m. UTC | #6
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@collabora.com>
>>>>>> Acked-by: Helen Koike <helen.koike@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.

Regards,

	Hans
Sakari Ailus Nov. 5, 2020, 12:45 p.m. UTC | #7
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@collabora.com>
> >>>>>> Acked-by: Helen Koike <helen.koike@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@linux.intel.com>
Dafna Hirschfeld Nov. 5, 2020, 2:21 p.m. UTC | #8
Hi

Am 05.11.20 um 13:45 schrieb 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@collabora.com>

>>>>>>>> Acked-by: Helen Koike <helen.koike@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@linux.intel.com>

> 


Hi,
There is another patch sent by jmondi that adds csi-2 embedded data format:

https://patchwork.kernel.org/project/linux-media/patch/20201102165258.408049-3-jacopo@jmondi.org/

Maybe it worth thinking already how those two patches should fit together.
Currently they both try to acquire 0x7001 code.

Thanks,
Dafna
Sakari Ailus Nov. 5, 2020, 2:44 p.m. UTC | #9
On Thu, Nov 05, 2020 at 03:21:45PM +0100, Dafna Hirschfeld wrote:
> Hi

> 

> Am 05.11.20 um 13:45 schrieb 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@collabora.com>

> > > > > > > > > Acked-by: Helen Koike <helen.koike@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@linux.intel.com>

> > 

> 

> Hi,

> There is another patch sent by jmondi that adds csi-2 embedded data format:

> 

> https://patchwork.kernel.org/project/linux-media/patch/20201102165258.408049-3-jacopo@jmondi.org/

> 

> Maybe it worth thinking already how those two patches should fit together.

> Currently they both try to acquire 0x7001 code.


I'll reply to Jacopo. This will at least require discussion, as not all
metadata is created equal.

-- 
Sakari Ailus
diff mbox series

Patch

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.
+ */
+#define MEDIA_BUS_FMT_METADATA_FIXED		0x7001
+
 #endif /* __LINUX_MEDIA_BUS_FORMAT_H */