Re: [PATCH v8 03/38] media: uapi: Add generic serial metadata mbus formats

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

Hi Sakari,

On Wed, Mar 20, 2024 at 04:23:01PM +0000, Sakari Ailus wrote:
> On Wed, Mar 20, 2024 at 12:59:48AM +0200, Laurent Pinchart wrote:
> > On Wed, Mar 13, 2024 at 09:24:41AM +0200, 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.
> > > 
> > > Also add a definition for "Data unit" to cover what is essentially a pixel
> > > but is not image data.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  .../userspace-api/media/glossary.rst          |   9 +
> > >  .../media/v4l/subdev-formats.rst              | 258 ++++++++++++++++++
> > >  include/uapi/linux/media-bus-format.h         |   9 +
> > >  3 files changed, 276 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/glossary.rst b/Documentation/userspace-api/media/glossary.rst
> > > index ef0ab601b5bf..7078141894c5 100644
> > > --- a/Documentation/userspace-api/media/glossary.rst
> > > +++ b/Documentation/userspace-api/media/glossary.rst
> > > @@ -25,6 +25,15 @@ Glossary
> > >  
> > >  	See :ref:`cec`.
> > >  
> > > +.. _media-glossary-data-unit:
> > > +
> > > +    Data unit
> > > +
> > > +	Unit of data transported by a bus. On parallel buses, the data unit
> > > +	consists of one or more related samples while on serial buses the data
> > > +	unit is logical. If the data unit is image data, it may also be called a
> > > +	pixel.
> > 
> > I'm pretty sure nobody will be able to understand what this means, but I
> > don't have a better proposal at the moment.
> > 
> > > +
> > >      Device Driver
> > >  	Part of the Linux Kernel that implements support for a hardware
> > >  	component.
> > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > index eb3cd20b0cf2..cbd475f7cae9 100644
> > > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > @@ -8306,3 +8306,261 @@ 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 buses where the actual data
> > > +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.
> > 
> > Maybe this paragraph would be best added in the patch that adds the
> > specific metadata formats, you could then mention one of them as an
> > example:
> > 
> > The exact format of the data generated by the device is reported on the
> > internal source pad of the originating sub-device, using one of the more
> > specific metadata formats such as MEDIA_BUS_FMT_CCS_EMBEDDED.
> 
> I'd really like to get rid of the "internal source" pads as the naming is
> really confusing (it's present still in this version but not in many
> locations). They're sink pads after all, so I'd call them such. In a few
> locations there's text that explains they do represent sources of data
> within the sub-device itself.

I thought we had agreed to use the term "internal sink pad", but I saw a
patch in this series that consistently makes use of "internal source
pad". Both are confusing for different reasons, but regardless of which
one we pick (that discussion belongs to the other patch that documents
the internal source pads), please make sure to use the same terms
consistently through the series in v9.

> > > +
> > > +"b" in an array cell signifies a byte of data, followed by the number of the bit
> > 
> > s/bit$/byte/
> 
> Uh, yes.
> 
> > > +and finally the bit number in subscript. "X" indicates a padding bit.
> > 
> > We use a lower-case x in pixfmt-rgb.rst, I would do the same here. We
> > also use single quotes there, turning "b" and "x" into 'b' and 'x'.
> 
> This is documentation, not C source code where we'd want to denote a single
> character. Double quotes should thus be used instead.

We seem to use single quotes in format documentations, I'd continue
doing so for consistency.

> I'll switch to lower case X.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thank you!

-- 
Regards,

Laurent Pinchart