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

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

[Dafna Hirschfeld]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 2/2] media: staging: rkisp1: isp: set metadata pads to MEDIA_BUS_FMT_METADATA_FIXED

[Dafna Hirschfeld]

Set the code of the metadata pads of the isp entity to
MEDIA_BUS_FMT_METADATA_FIXED and set the width and
height of their formats to 0. This solves the TODO
item:
"Fix pad format size for statistics and parameters entities."

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
no changes since v1 and v2

 drivers/staging/media/rkisp1/TODO         | 1 -
 drivers/staging/media/rkisp1/rkisp1-isp.c | 8 ++++----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
index e7c8398fc2ce..c30c83681452 100644
--- a/drivers/staging/media/rkisp1/TODO
+++ b/drivers/staging/media/rkisp1/TODO
@@ -1,4 +1,3 @@
-* Fix pad format size for statistics and parameters entities.
 * Fix checkpatch errors.
 * Add uapi docs. Remember to add documentation of how quantization is handled.
 * streaming paths (mainpath and selfpath) check if the other path is streaming
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index a9715b0b7264..48d08ff87da2 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -574,7 +574,7 @@ static int rkisp1_isp_enum_mbus_code(struct v4l2_subdev *sd,
 	} else {
 		if (code->index > 0)
 			return -EINVAL;
-		code->code = MEDIA_BUS_FMT_FIXED;
+		code->code = MEDIA_BUS_FMT_METADATA_FIXED;
 		return 0;
 	}
 
@@ -633,10 +633,10 @@ static int rkisp1_isp_init_config(struct v4l2_subdev *sd,
 					      RKISP1_ISP_PAD_SINK_PARAMS);
 	src_fmt = v4l2_subdev_get_try_format(sd, cfg,
 					     RKISP1_ISP_PAD_SOURCE_STATS);
-	sink_fmt->width = RKISP1_DEFAULT_WIDTH;
-	sink_fmt->height = RKISP1_DEFAULT_HEIGHT;
+	sink_fmt->width = 0;
+	sink_fmt->height = 0;
 	sink_fmt->field = V4L2_FIELD_NONE;
-	sink_fmt->code = MEDIA_BUS_FMT_FIXED;
+	sink_fmt->code = MEDIA_BUS_FMT_METADATA_FIXED;
 	*src_fmt = *sink_fmt;
 
 	return 0;
-- 
2.17.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Sakari Ailus]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Hans Verkuil]

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 */
> 


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [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@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.

-- 
Regards,

Sakari Ailus

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [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@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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [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 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.

-- 
Regards,

Sakari Ailus

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Hans Verkuil]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [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@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>

-- 
Sakari Ailus

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Dafna Hirschfeld]

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

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

[Sakari Ailus]

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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip