Re: [PATCH 1/6] media: Define MIPI CSI-2 data types in a shared header file

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

Hello,

On Wed, Jan 26, 2022 at 11:50:21AM +0000, Dave Stevenson wrote:
> On Mon, 24 Jan 2022 at 15:22, Pratyush Yadav <p.yadav@ti.com> wrote:
> > On 23/01/22 06:08PM, Laurent Pinchart wrote:
> > > There are many CSI-2-related drivers in the media subsystem that come
> > > with their own macros to handle the CSI-2 data types (or just hardcode
> > > the numerical values). Provide a shared header with definitions for
> > > those data types that driver can use.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > >  include/media/mipi-csi2.h | 45 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 45 insertions(+)
> > >  create mode 100644 include/media/mipi-csi2.h
> > >
> > > diff --git a/include/media/mipi-csi2.h b/include/media/mipi-csi2.h
> > > new file mode 100644
> > > index 000000000000..392794e5badd
> > > --- /dev/null
> > > +++ b/include/media/mipi-csi2.h
> > > @@ -0,0 +1,45 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > +/*
> > > + * MIPI CSI-2 Data Types
> > > + *
> > > + * Copyright (C) 2022 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > + */
> > > +
> > > +#ifndef _MEDIA_MIPI_CSI2_H
> > > +#define _MEDIA_MIPI_CSI2_H
> > > +
> > > +/* Short packet data types */
> > > +#define MIPI_CSI2_DT_FS                      0x00
> > > +#define MIPI_CSI2_DT_FE                      0x01
> > > +#define MIPI_CSI2_DT_LS                      0x02
> > > +#define MIPI_CSI2_DT_LE                      0x03
> > > +#define MIPI_CSI2_DT_GENERIC_SHORT(n)        (0x08 + (n))    /* 0..7 */
> >
> > IIUC there is currently no way to actually capture packets with these
> > data types, and these are added here for completeness's sake, right?
> 
> They aren't generally captured, but are of use.
> For Unicam we have a packet compare & capture trigger generally used
> for debug. However it can also be used for capturing the 16bit frame
> number attached to FS and FE events.
> It's been of use for devices such as Analog Devices ADV728[0|2]M which
> use the frame number to identify the field when sending interlaced
> content.
> 
> > > +
> > > +/* Long packet data types */
> > > +#define MIPI_CSI2_DT_NULL            0x10
> > > +#define MIPI_CSI2_DT_BLANKING                0x11
> > > +#define MIPI_CSI2_DT_EMBEDDED_8B     0x12
> > > +#define MIPI_CSI2_DT_YUV420_8B               0x18
> > > +#define MIPI_CSI2_DT_YUV420_10B              0x19
> > > +#define MIPI_CSI2_DT_YUV420_8B_LEGACY        0x1a
> > > +#define MIPI_CSI2_DT_YUV420_8B_CS    0x1c
> > > +#define MIPI_CSI2_DT_YUV420_10B_CS   0x1d
> > > +#define MIPI_CSI2_DT_YUV422_8B               0x1e
> > > +#define MIPI_CSI2_DT_YUV422_10B              0x1f
> > > +#define MIPI_CSI2_DT_RGB444          0x20
> > > +#define MIPI_CSI2_DT_RGB555          0x21
> > > +#define MIPI_CSI2_DT_RGB565          0x22
> > > +#define MIPI_CSI2_DT_RGB666          0x23
> > > +#define MIPI_CSI2_DT_RGB888          0x24
> > > +#define MIPI_CSI2_DT_RAW24           0x27
> >
> > I have the CSI-2 spec v1.3, and it lists 0x27 as reserved under RGB
> > Image data, and I don't see a data type value for RAW24. Where did you
> > get this value from?
> >
> > > +#define MIPI_CSI2_DT_RAW6            0x28
> > > +#define MIPI_CSI2_DT_RAW7            0x29
> > > +#define MIPI_CSI2_DT_RAW8            0x2a
> > > +#define MIPI_CSI2_DT_RAW10           0x2b
> > > +#define MIPI_CSI2_DT_RAW12           0x2c
> > > +#define MIPI_CSI2_DT_RAW14           0x2d
> > > +#define MIPI_CSI2_DT_RAW16           0x2e
> > > +#define MIPI_CSI2_DT_RAW20           0x2f
> >
> > These two are also listed as reserved in the spec I have. Rest of the
> > values look good to me.
> 
> I'm also only on v1.3, but otherwise agree that all the other values match.
> I see that MIPI are now up to v4.0, and their performance
> highlights[1] include RAW16 and RAW24 support, so I assume they have
> been added in a later revision.

I don't have access to more a more recent version of the CSI-2
specification, but I've gathered RAW16, RAW20 and RAW24 from out-of-tree
code. Sakari, could you confirm those values ?

> [1] https://www.mipi.org/specifications/csi-2
> 
> > > +#define MIPI_CSI2_DT_USER_DEFINED(n) (0x30 + (n))    /* 0..7 */
> > > +
> > > +#endif /* _MEDIA_MIPI_CSI2_H */
> >
> > I think this patch is a good idea in general, and it should remove a lot
> > of repetition in the drivers.
> >
> > BTW, I also see lots of drivers adding tables having mapping between
> > MBUS formats, FOURCC formats, bpp, data type, etc. It would be useful to
> > have those in a central place IMO.

I agree. Patches are welcome :-)

-- 
Regards,

Laurent Pinchart