Re: [RFC 5/7] media: uapi: Add generic serial metadata mbus formats

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thank you for the patch.

On Sat, May 06, 2023 at 12:52:55AM +0300, Sakari Ailus wrote:
> Add generic serial metadata mbus formats. These formats describe data
> width and packing but not the content itself. The reason for specifying
> such formats is that the formats as such are fairly device specific but
> they are still handled by CSI-2 receiver drivers that should not be aware
> of device specific formats. What makes generic metadata formats possible
> is that these formats are parsed by software only, after capturing the
> data to system memory.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  .../media/v4l/subdev-formats.rst              | 257 ++++++++++++++++++
>  include/uapi/linux/media-bus-format.h         |   9 +
>  2 files changed, 266 insertions(+)
> 
> diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> index a3a35eeed708..1492fff58426 100644
> --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> @@ -8234,3 +8234,260 @@ The following table lists the existing metadata formats.
>  	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.
> +
> +Generic Serial Metadata Formats
> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> +Generic serial metadata formats are used on serial busses where the actual data

s/busses/buses/

> +content is more or less device specific but the data is transmitted and received
> +by multiple devices that do not process the data in any way, simply writing
> +it to system memory for processing in software at the end of the pipeline.
> +
> +The more specific variant describing the actual data is used on the internal
> +source pad of the originating sub-device.

What do you mean by "more specific variant" here ? Please include an
example in the documentation.

I'm not sure I like mentioning internal source pads here, are we
guaranteed that metadata will always originate from an internal source
pad ?

> +
> +"b" in an array cell signifies a byte of data, followed by the number of byte
> +and finally the bit number in subscript. "p" indicates a padding bit.
> +
> +.. _media-bus-format-generic-meta:
> +
> +.. cssclass: longtable
> +
> +.. flat-table:: Generic Serial Metadata Formats
> +    :header-rows:  2
> +    :stub-columns: 0
> +
> +    * - Identifier
> +      - Code
> +      -
> +      - :cspan:`23` Data organization
> +    * -
> +      -
> +      - Bit
> +      - 23
> +      - 22
> +      - 21
> +      - 20
> +      - 19
> +      - 18
> +      - 17
> +      - 16
> +      - 15
> +      - 14
> +      - 13
> +      - 12
> +      - 11
> +      - 10
> +      - 9
> +      - 8
> +      - 7
> +      - 6
> +      - 5
> +      - 4
> +      - 3
> +      - 2
> +      - 1
> +      - 0
> +    * .. _MEDIA-BUS-FMT-META-1X8-8:
> +
> +      - MEDIA_BUS_FMT_META_1X8_8
> +      - 0x8001
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +    * .. _MEDIA-BUS-FMT-META-1X8-10:
> +
> +      - MEDIA_BUS_FMT_META_1X8_10
> +      - 0x8002
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +      - p
> +      - p
> +    * .. _MEDIA-BUS-FMT-META-1X8-12:
> +
> +      - MEDIA_BUS_FMT_META_1X8_12
> +      - 0x8003
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +      - p
> +      - p
> +      - p
> +      - p
> +    * .. _MEDIA-BUS-FMT-META-1X8-14:
> +
> +      - MEDIA_BUS_FMT_META_1X8_14
> +      - 0x8004
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +    * .. _MEDIA-BUS-FMT-META-1X8-16:
> +
> +      - MEDIA_BUS_FMT_META_1X8_16
> +      - 0x8005
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +    * .. _MEDIA-BUS-FMT-META-1X8-20:
> +
> +      - MEDIA_BUS_FMT_META_1X8_20
> +      - 0x8007
> +      -
> +      -
> +      -
> +      -
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +    * .. _MEDIA-BUS-FMT-META-1X8-24:
> +
> +      - MEDIA_BUS_FMT_META_1X8_24
> +      - 0x8009
> +      -
> +      - b0\ :sub:`7`
> +      - b0\ :sub:`6`
> +      - b0\ :sub:`5`
> +      - b0\ :sub:`4`
> +      - b0\ :sub:`3`
> +      - b0\ :sub:`2`
> +      - b0\ :sub:`1`
> +      - b0\ :sub:`0`
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> +      - p
> diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
> index a03c543cb072..722463523bbd 100644
> --- a/include/uapi/linux/media-bus-format.h
> +++ b/include/uapi/linux/media-bus-format.h
> @@ -173,4 +173,13 @@
>   */
>  #define MEDIA_BUS_FMT_METADATA_FIXED		0x7001
>  
> +/* Generic line based metadata formats for serial buses. Next is 0x800b. */
> +#define MEDIA_BUS_FMT_META_1X8_8		0x8001
> +#define MEDIA_BUS_FMT_META_1X8_10		0x8002
> +#define MEDIA_BUS_FMT_META_1X8_12		0x8003
> +#define MEDIA_BUS_FMT_META_1X8_14		0x8004
> +#define MEDIA_BUS_FMT_META_1X8_16		0x8005
> +#define MEDIA_BUS_FMT_META_1X8_20		0x8007
> +#define MEDIA_BUS_FMT_META_1X8_24		0x8009

We've discussed this before privately, it's time to come to a conclusion
:-)

My preference would be 

#define MEDIA_BUS_FMT_META_RAW8			0x8001
#define MEDIA_BUS_FMT_META_RAW10		0x8002
#define MEDIA_BUS_FMT_META_RAW12		0x8003
#define MEDIA_BUS_FMT_META_RAW14		0x8004
#define MEDIA_BUS_FMT_META_RAW16		0x8005
#define MEDIA_BUS_FMT_META_RAW20		0x8007
#define MEDIA_BUS_FMT_META_RAW24		0x8009

without defining the contents of the data (that is, no mention of
padding bits).

> +
>  #endif /* __LINUX_MEDIA_BUS_FORMAT_H */

-- 
Regards,

Laurent Pinchart