diff mbox series

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

Message ID 20210217112122.424236-9-robert.foss@linaro.org
State New
Headers show
Series Add support for the SDM845 Camera Subsystem | expand

Commit Message

Robert Foss Feb. 17, 2021, 11:21 a.m. UTC
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(+)

-- 
2.27.0

Comments

Andrey Konovalov Feb. 22, 2021, 4:26 p.m. UTC | #1
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.

> +

> +#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?

[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.

> +

> +#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).

Thanks,
Andrey

> +

> +

>   enum csid_payload_mode {

>   	CSID_PAYLOAD_MODE_INCREMENTING = 0,

>   	CSID_PAYLOAD_MODE_ALTERNATING_55_AA = 1,

>
Robert Foss Feb. 23, 2021, 5:25 p.m. UTC | #2
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,
> >
diff mbox series

Patch

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
+
+#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
+
+#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
+
+#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 */
+
+
 enum csid_payload_mode {
 	CSID_PAYLOAD_MODE_INCREMENTING = 0,
 	CSID_PAYLOAD_MODE_ALTERNATING_55_AA = 1,