Re: [PATCH v3 04/10] media: uapi: Add generic serial metadata mbus formats

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Wed, Sep 06, 2023 at 03:50:09PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Sep 06, 2023 at 12:34:10PM +0000, Sakari Ailus wrote:
> > On Wed, Sep 06, 2023 at 02:31:17PM +0300, Laurent Pinchart wrote:
> > > On Wed, Sep 06, 2023 at 11:28:37AM +0300, Tomi Valkeinen wrote:
> > > > On 05/09/2023 19:38, Laurent Pinchart wrote:
> > > > > On Thu, Aug 24, 2023 at 11:26:56AM +0300, Tomi Valkeinen wrote:
> > > > >> On 24/08/2023 10:24, Sakari Ailus wrote:
> > > > >>> On Wed, Aug 23, 2023 at 04:16:13PM +0300, Tomi Valkeinen wrote:
> > > > >>>> On 08/08/2023 10:55, 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
> > > > > 
> > > > > Do we want these formats to be CSI-2-specific ?
> > > > >
> > > > >>>>> 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.
> > > > >>>>
> > > > >>>> If I'm not mistaken, the CSI-2 spec doesn't say much about embedded data,
> > > > >>>> except that it may exist. Afaics, in CSI-2, the embedded data is split into
> > > > >>>> "lines", although the amount of data can be different than in the video
> > > > >>>> lines.
> > > > >>>>
> > > > >>>> The CCS specs talks more about embedded data. Some of it is quite odd, like:
> > > > >>>> "The length of the embedded data line shall not exceed the length of the
> > > > >>>> image data line. The embedded data line should have the same length as the
> > > > >>>> image data line.". I think it means the embedded line can be shorter than
> > > > >>>> image line, but not longer.
> > > > >>>
> > > > >>> That's what it means, yes. The CCS also has means to obtain the actual
> > > > >>> length --- frame format descriptors.
> > > > >>>
> > > > >>>> CCS also says that an embedded line should use 0x07 as padding at the end of
> > > > >>>> the line, if there's less data than the image line.
> > > > >>>>
> > > > >>>> CCS also talks about how the embedded data would be packed, and in some
> > > > >>>> cases that packing would be the same as for pixel data.
> > > > >>>
> > > > >>> In fact the packing is the same as for pixel data: the CSI-2 does not
> > > > >>> really differentiate there.
> > > > >>
> > > > >> If I understand the CCS spec right, the packing is not the same as for
> > > > >> the pixel data. You can have RAW12 pixel data but 8-byte embedded data.
> > > > >> But if you meant that the different packing style options are the same
> > > > >> for pixel and embedded data, yes.
> > > > > 
> > > > > The idea is to allow CSI-2 receivers to unpack embedded data the same
> > > > > way as pixel data at the hardware level, and guarantee that no embedded
> > > > 
> > > > Yes, but my comment was to Sakari's "the packing is the same as for 
> > > > pixel data" comment. Afaics, there's nothing in CSI-2 or CCS that says 
> > > > the packing is the same for pixel data and embedded data.
> > > > 
> > > > In fact, the CSS says "By default, top-embedded data shall use the same 
> > > > data packing as the Visible pixel data with one embedded data byte per 
> > > > pixel", implying that it would not always be the case. It continues that 
> > > > for RAW16/RAW20/RAW24 pixels the embedded data could be sent with 
> > > > RAW8/RAW10/RAW12.
> > > 
> > > The spec if a bit ambiguous :-S "by default" implies that something else
> > > could be done, but it's not clear what the other options are.
> > 
> > The options for packing are detailed in the spec. For higher bit depths
> > there are options (as detailed by Tomi above).
> 
> The "by default" wording could be construed as allowing any packing. I
> think the intent was to only allow an alternative packing for RAW16 and
> higher, but it's not very clear.

The optimised packing convention is only available for RAW16 and higher ---
otherwise you won't have enough bits needed to carry two bytes of data. The
packing used is configured using emb_data_ctrl register.

Maybe annex A could be merged to section 7.15 in order to make things
clearer.

> 
> > > > That was just for top-embedded data, and I couldn't right away see 
> > > > mention of other embedded data.
> > > 
> > > The specification also mentions PDAF data for instance.
> > 
> > The bottom PDAF data is vendor defined (as it's processed) so the vendor is
> > free to do whatever there. Note that the better way to support PDAF is via
> > embedded PDAF pixels, not via separate bottom embedded (PDAF) data. The
> > bottom embedded data could also be different kinds of statistics.
> 
> Lots of ISPs don't support hardware processing of embedded PDAF pixels,
> so sensor-side processing is often nice to have. I wouldn't consider the
> embedded PDAF lines to be always worse than embedded PDAF pixels.

Not necessarily, indeed, but these formats tend not to be documented. :-(

-- 
Regards,

Sakari Ailus