On Wed, Sep 06, 2023 at 01:25:53PM +0000, Sakari Ailus wrote: > On Wed, Sep 06, 2023 at 03:36:58PM +0300, Laurent Pinchart wrote: > > On Wed, Sep 06, 2023 at 11:36:45AM +0000, Sakari Ailus wrote: > > > On Tue, Sep 05, 2023 at 07:47:20PM +0300, Laurent Pinchart wrote: > > > > On Fri, Aug 11, 2023 at 09:11:39AM +0000, Sakari Ailus wrote: > > > > > On Fri, Aug 11, 2023 at 08:31:16AM +0200, Jacopo Mondi wrote: > > > > > > > > +V4L2_META_FMT_GENERIC_CSI2_10 > > > > > > > > +----------------------------- > > > > > > > > + > > > > > > > > +V4L2_META_FMT_GENERIC_CSI2_10 contains packed 8-bit generic metadata, 10 bits > > > > > > > > +for each 8 bits of data. Every four bytes of metadata is followed by a single > > > > > > > > +byte of padding. The way the data is stored follows the CSI-2 specification. > > > > > > > > + > > > > > > > > +This format is also used on CSI-2 on 20 bits per sample format that packs two > > > > > > > > +bytes of metadata into one sample. > > > > > > > > + > > > > > > > > +This format is little endian. > > > > > > > > + > > > > > > > > +**Byte Order Of V4L2_META_FMT_GENERIC_CSI2_10.** > > > > > > > > +Each cell is one byte. "M" denotes a byte of metadata and "p" a byte of padding. > > > > > > > > > > > > > > I think you should document whether the padding is always 0 or can be any value. > > > > > > > Perhaps 'X' is a better 'name' for the padding byte in the latter case. > > > > > > > > > > > > Did I get this right that this format is supposed to work as the RAW10 > > > > > > CSI-2 packed image format, where 4 bytes contain the higher 8 bits of > > > > > > the 10 bits sample and the 5th byte every 4 contains the lower 2 bits of > > > > > > the previous 4 sample ? > > > > > > > > > > > > If that's the case, is 'padding' the correct term here ? > > > > > > > > > > What else would you call it? It'll be zeros that exist just due to the bit > > > > > depth used and as such not interesting at all. > > > > > > > > It's actually not 0, CCS requires the padding bytes to be 0x55. > > > > > > > > I wonder if the conformance test suite tests the contents of the padding > > > > bytes. > > > > > > I don't know. I could add the value is unspecified but as it has not been > > > specified, there's no change in meaning (just size). > > > > I started writing that I don't see how it could help applications to > > know that the padding byte is 0x55, but the SMIA++ embedded data parser > > in libcamera actually checks for it, and considers the embedded data to > > be erroneous if it has a different value. > > I think it's fine to check for it if you know it's CCS/SMIA++/SMIA embedded > data. But documenting it here isn't a great idea as then other uses of this > format definition would be excluded. I'm OK with that, but note that you've mentioned in a different patch in the same series that you wouldn't use the CCS media bus code for sensors that are compliant with the CCS packing and encoding but not the CCS register set. That's not very consistent :-) -- Regards, Laurent Pinchart
