Re: [PATCH v5 08/22] media: camss: Add missing format identifiers

[Robert Foss <robert.foss@linaro.org>]

On Mon, 22 Feb 2021 at 17:26, Andrey Konovalov
<andrey.konovalov@linaro.org> wrote:
>
> Hi Robert,
>
> Thank you for your patch!
>
> On 17.02.2021 14:21, Robert Foss wrote:
> > The CSI-2 spec defines the following types:
> >   - Data Type - Often abbreviated DT
> >   - Decode Format - Often abbreviated as DF
> >   - Encode Format
> >
> > These definitions are as far as I can tell complete for CSI-2.
> >
> > Additionally the Qualcomm internal type describing Plain Formats
> > has been added. Plain formats describe the size of the pixels
> > written by the RDI units to memory. PLAIN8 for example has the size
> > 8 bits, and PLAIN32 32 bits. The appropriate Plain Format is
> > determined by the Decode Format used. The smallest Plain Format
> > that is able to contain a pixel of the used Decode Format is the
> > appropriate one to use.
> >
> > Signed-off-by: Robert Foss <robert.foss@linaro.org>
> > ---
> >   .../media/platform/qcom/camss/camss-csid.h    | 50 +++++++++++++++++++
> >   1 file changed, 50 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss-csid.h b/drivers/media/platform/qcom/camss/camss-csid.h
> > index 1824b3745e10..02fc34ee8a41 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csid.h
> > +++ b/drivers/media/platform/qcom/camss/camss-csid.h
> > @@ -21,6 +21,56 @@
> >   #define MSM_CSID_PAD_SRC 1
> >   #define MSM_CSID_PADS_NUM 2
> >
> > +#define DATA_TYPE_EMBEDDED_DATA_8BIT 0x12
> > +#define DATA_TYPE_YUV420_8BIT                0x18
> > +#define DATA_TYPE_YUV420_10BIT               0x19
> > +#define DATA_TYPE_YUV420_8BIT_LEGACY 0x1a
> > +#define DATA_TYPE_YUV420_8BIT_SHIFTED        0x1c /* Chroma Shifted Pixel Sampling */
> > +#define DATA_TYPE_YUV420_10BIT_SHIFTED       0x1d /* Chroma Shifted Pixel Sampling */
> > +#define DATA_TYPE_YUV422_8BIT                0x1e
> > +#define DATA_TYPE_YUV422_10BIT               0x1f
> > +#define DATA_TYPE_RGB444             0x20
> > +#define DATA_TYPE_RGB555             0x21
> > +#define DATA_TYPE_RGB565             0x22
> > +#define DATA_TYPE_RGB666             0x23
> > +#define DATA_TYPE_RGB888             0x24
> > +#define DATA_TYPE_RAW_24BIT          0x27
> > +#define DATA_TYPE_RAW_6BIT           0x28
> > +#define DATA_TYPE_RAW_7BIT           0x29
> > +#define DATA_TYPE_RAW_8BIT           0x2a
> > +#define DATA_TYPE_RAW_10BIT          0x2b
> > +#define DATA_TYPE_RAW_12BIT          0x2c
> > +#define DATA_TYPE_RAW_14BIT          0x2d
> > +#define DATA_TYPE_RAW_16BIT          0x2e
> > +#define DATA_TYPE_RAW_20BIT          0x2f
>
> - these look OK for me (the old MIPI spec draft I have doesn't have
>    some of the data types listed above).
>
>    As these are generic values from the MIPI standard, it could probably make
>    sense to create a common header file for that someday.
>    E.g. the very similar defines (same values, different names) are present in
>    drivers/staging/media/atomisp/pci/isp_capture_defs.h
>    But it looks like most of the current drivers don't need the MIPI data type
>    defines, so not a problem at the moment.

Both for the DTs and the DF/EF/PFs I figured it would be nice to have
as much of these variables listed somewhere as they aren't always so
easy to find a reference for.

>
> > +
> > +#define DECODE_FORMAT_UNCOMPRESSED_6_BIT     0x0
> > +#define DECODE_FORMAT_UNCOMPRESSED_8_BIT     0x1
> > +#define DECODE_FORMAT_UNCOMPRESSED_10_BIT    0x2
> > +#define DECODE_FORMAT_UNCOMPRESSED_12_BIT    0x3
> > +#define DECODE_FORMAT_UNCOMPRESSED_14_BIT    0x4
> > +#define DECODE_FORMAT_UNCOMPRESSED_16_BIT    0x5
> > +#define DECODE_FORMAT_UNCOMPRESSED_20_BIT    0x6
> > +#define DECODE_FORMAT_DPCM_10_6_10           0x7
> > +#define DECODE_FORMAT_DPCM_10_8_10           0x8
> > +#define DECODE_FORMAT_DPCM_12_6_12           0x9
> > +#define DECODE_FORMAT_DPCM_12_8_12           0xA
> > +#define DECODE_FORMAT_DPCM_14_8_14           0xB
> > +#define DECODE_FORMAT_DPCM_14_10_14          0xC
> > +#define DECODE_FORMAT_USER_DEFINED           0xE
> > +#define DECODE_FORMAT_PAYLOAD_ONLY           0xF
>
> - interesting that the subset of the DECODE_FORMAT's used in
>    camss-csid-4-1.c (the first four formats above - UNCOMPRESSED_6_BIT
>    to UNCOMPRESSED_12_BIT ones) has the same values as the corresponding
>    field in the CSID_CID_n_CFG registers - according to the public
>    "APQ8016E Technical Reference Manual" [1]. So these exact DECODE_FORMAT_*
>    values are written into the bits 7:4 of the hw register.
>    But in [1] the values of DPCM_10_6_10 to DPCM_12_8_12 are 0x4 to 0x7
>    (as the camss-csid-4-1.c doesn't support DPCM this is not an issue).
>    Are the DECODE_FORMAT_* values above defined in the MIPI standard, or did
>    they come from the datasheet for a particular SOC?

DF & EFs are a part of the MIPI spec, but the identifier is SOC/vendor specific.
PFs are not a part of the MIPI spec at all, but rather a Qcom specific term.

Having looked over the DFs again, there are some incompatible
differences between Gen1 & Gen2. I think the way forward is moving
DF/EF/PFs to Gen 1/2 specific headers.

>
> [1] https://developer.qualcomm.com/download/sd410/snapdragon-410e-technical-reference-manual.pdf
>      page 990
> > +
> > +#define ENCODE_FORMAT_RAW_8_BIT              0x1
> > +#define ENCODE_FORMAT_RAW_10_BIT     0x2
> > +#define ENCODE_FORMAT_RAW_12_BIT     0x3
> > +#define ENCODE_FORMAT_RAW_14_BIT     0x4
> > +#define ENCODE_FORMAT_RAW_16_BIT     0x5
>
> - the ENCODE_FORMAT_* defines are not used in the driver.

That's intentional, collecting this information somewhere for future
use seemed like a good idea. But I'm happy to remove it if that's a
bad idea.

>
> > +
> > +#define PLAIN_FORMAT_PLAIN8  0x0 /* supports DPCM, UNCOMPRESSED_6/8_BIT */
> > +#define PLAIN_FORMAT_PLAIN16 0x1 /* supports DPCM, UNCOMPRESSED_10/16_BIT */
> > +#define PLAIN_FORMAT_PLAIN32 0x2 /* supports UNCOMPRESSED_20_BIT */
>
> - the PLAIN_FORMAT_* defines are not used in the driver, but
>    camss-csid-4-1.c and camss-csid-4-7.c do define there own
>    CAMSS_CSID_CID_n_CFG_PLAIN_FORMAT_8 and CAMSS_CSID_CID_n_CFG_PLAIN_FORMAT_16
>    (without relying on PLAIN_FORMAT_PLAIN8 or PLAIN_FORMAT_PLAIN16).

I missed this duplication, I'll fix this in the next version.

>
> Thanks,
> Andrey
>
> > +
> > +
> >   enum csid_payload_mode {
> >       CSID_PAYLOAD_MODE_INCREMENTING = 0,
> >       CSID_PAYLOAD_MODE_ALTERNATING_55_AA = 1,
> >