Re: [Intel-gfx] [PATCH v2 2/4] drm/dsc: Define Display Stream Compression PPS infoframe

[Manasi Navare]

On Wed, May 16, 2018 at 02:14:56PM -0400, Harry Wentland wrote:
> On 2018-05-14 10:05 PM, Manasi Navare wrote:
> > This patch defines a new header file for all the DSC 1.2 structures
> > and creates a structure for PPS infoframe which will be used to send
> > picture parameter set secondary data packet for display stream compression.
> > All the PPS infoframe syntax elements are taken from DSC 1.2 specification
> > from VESA.
> > 
> > v2:
> > * Fix the comments for kernel-doc
> > 
> > Cc: dri-de...@lists.freedesktop.org
> > Cc: Jani Nikula 
> > Cc: Ville Syrjala 
> > Cc: Anusha Srivatsa 
> > Signed-off-by: Manasi Navare 
> > ---
> >  include/drm/drm_dsc.h | 437 
> > ++
> >  1 file changed, 437 insertions(+)
> >  create mode 100644 include/drm/drm_dsc.h
> > 
> > diff --git a/include/drm/drm_dsc.h b/include/drm/drm_dsc.h
> > new file mode 100644
> > index 000..5ee72e8
> > --- /dev/null
> > +++ b/include/drm/drm_dsc.h
> > @@ -0,0 +1,437 @@
> > +/*
> > + * Copyright (C) 2018 Intel Corp.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the 
> > "Software"),
> > + * to deal in the Software without restriction, including without 
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + *
> > + * Authors:
> > + * Manasi Navare 
> > + */
> > +
> > +#ifndef DRM_DSC_H_
> > +#define DRM_DSC_H_
> > +
> > +#include 
> > +
> > +#define DSC_NUM_BUF_RANGES 15
> > +
> > +/**
> > + * struct picture_parameter_set - Represents 128 bytes of Picture 
> > Parameter Set
> > + *
> > + * The VESA DSC standard defines picture parameter set (PPS) which display 
> > stream
> > + * compression encoders must communicate to decoders. The PPS is 
> > encapsulated
> > + * in 128 bytes (PPS 0 through PPS 127). The fields in this structure are 
> > as per
> > + * Table 4.1 in Vesa DSC specification v1.1/v1.2.
> > + * The PPS fields that span over more than a byte should be stored in Big 
> > Endian
> > + * format.
> > + */
> > +struct picture_parameter_set {
> > +   /**
> > +* @dsc_version_minor:
> > +* PPS0[3:0] - Contains Minor version of DSC
> > +*/
> > +   u8 dsc_version_minor:4;
> > +   /**
> > +* @dsc_version_major:
> > +* PPS0[7:4] - Contains major version of DSC
> > +*/
> > +   u8 dsc_version_major:4;
> > +   /**
> > +* @pps_identifier:
> > +* PPS1[7:0] - Application specific identifier that can be
> > +* used to differentiate between different PPS tables.
> > +*/
> > +   u8 pps_identifier;
> > +   /**
> > +* @pps2_reserved:
> > +* PPS2[7:0]- RESERVED Byte
> > +*/
> > +   u8 pps2_reserved;
> > +   /**
> > +* @linebuf_depth:
> > +* PPS3[3:0] - Contains linebuffer bit depth used to generate
> > +* the bitstream. (0x0 - 16 bits for DSc 1.2, 0x8 - 8 bits,
> > +* 0xA - 10 bits, 0xB - 11 bits, 0xC - 12 bits, 0xD - 13 bits,
> > +* 0xE - 14 bits for DSC1.2, 0xF - 14 bits for DSC 1.2.
> > +*/
> > +   u8 linebuf_depth:4;
> > +   /**
> > +* @bits_per_component:
> > +* PPS3[7:4] - Bits per component fo rthe original pixels
> 
> typo: "for the"

Yes wll fix that.

> 
> > +* of the encoded picture.
> > +*/
> 
> Would it make sense to indicate in the comments what the values mean?
> 
> From the spec:
> 0x0 = 16bpc (allowed only when dsc_version_minor = 0x2).
> 0x8 = 8bpc.
> 0xA = 10bpc.
> 0xC = 12bpc.
> 0xE = 14bpc (allowed only when dsc_version_minor = 0x2).
> All other encodings are RESERVED.
> 
> Harry

This is definitely a good point. I can add these in the block comment for
bits_per_component like I have added for linebuf_depth.

Regards
Manasi

> 
> > +   u8 bits_per_component:4;
> > +   /**
> > +* @bpp_high:
> > +* PPS4[1:0] - These are the most significant 2 bits of
> > +* compressed BPP bits_per_pixel[9:0] syntax element.
> > +*/
> > +   u8 bpp_high:2;
> > +   /**
> > 

Re: [Intel-gfx] [PATCH v2 2/4] drm/dsc: Define Display Stream Compression PPS infoframe

[Harry Wentland]

On 2018-05-14 10:05 PM, Manasi Navare wrote:
> This patch defines a new header file for all the DSC 1.2 structures
> and creates a structure for PPS infoframe which will be used to send
> picture parameter set secondary data packet for display stream compression.
> All the PPS infoframe syntax elements are taken from DSC 1.2 specification
> from VESA.
> 
> v2:
> * Fix the comments for kernel-doc
> 
> Cc: dri-de...@lists.freedesktop.org
> Cc: Jani Nikula 
> Cc: Ville Syrjala 
> Cc: Anusha Srivatsa 
> Signed-off-by: Manasi Navare 
> ---
>  include/drm/drm_dsc.h | 437 
> ++
>  1 file changed, 437 insertions(+)
>  create mode 100644 include/drm/drm_dsc.h
> 
> diff --git a/include/drm/drm_dsc.h b/include/drm/drm_dsc.h
> new file mode 100644
> index 000..5ee72e8
> --- /dev/null
> +++ b/include/drm/drm_dsc.h
> @@ -0,0 +1,437 @@
> +/*
> + * Copyright (C) 2018 Intel Corp.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + * OTHER DEALINGS IN THE SOFTWARE.
> + *
> + * Authors:
> + * Manasi Navare 
> + */
> +
> +#ifndef DRM_DSC_H_
> +#define DRM_DSC_H_
> +
> +#include 
> +
> +#define DSC_NUM_BUF_RANGES   15
> +
> +/**
> + * struct picture_parameter_set - Represents 128 bytes of Picture Parameter 
> Set
> + *
> + * The VESA DSC standard defines picture parameter set (PPS) which display 
> stream
> + * compression encoders must communicate to decoders. The PPS is encapsulated
> + * in 128 bytes (PPS 0 through PPS 127). The fields in this structure are as 
> per
> + * Table 4.1 in Vesa DSC specification v1.1/v1.2.
> + * The PPS fields that span over more than a byte should be stored in Big 
> Endian
> + * format.
> + */
> +struct picture_parameter_set {
> + /**
> +  * @dsc_version_minor:
> +  * PPS0[3:0] - Contains Minor version of DSC
> +  */
> + u8 dsc_version_minor:4;
> + /**
> +  * @dsc_version_major:
> +  * PPS0[7:4] - Contains major version of DSC
> +  */
> + u8 dsc_version_major:4;
> + /**
> +  * @pps_identifier:
> +  * PPS1[7:0] - Application specific identifier that can be
> +  * used to differentiate between different PPS tables.
> +  */
> + u8 pps_identifier;
> + /**
> +  * @pps2_reserved:
> +  * PPS2[7:0]- RESERVED Byte
> +  */
> + u8 pps2_reserved;
> + /**
> +  * @linebuf_depth:
> +  * PPS3[3:0] - Contains linebuffer bit depth used to generate
> +  * the bitstream. (0x0 - 16 bits for DSc 1.2, 0x8 - 8 bits,
> +  * 0xA - 10 bits, 0xB - 11 bits, 0xC - 12 bits, 0xD - 13 bits,
> +  * 0xE - 14 bits for DSC1.2, 0xF - 14 bits for DSC 1.2.
> +  */
> + u8 linebuf_depth:4;
> + /**
> +  * @bits_per_component:
> +  * PPS3[7:4] - Bits per component fo rthe original pixels

typo: "for the"

> +  * of the encoded picture.
> +  */

Would it make sense to indicate in the comments what the values mean?

From the spec:
0x0 = 16bpc (allowed only when dsc_version_minor = 0x2).
0x8 = 8bpc.
0xA = 10bpc.
0xC = 12bpc.
0xE = 14bpc (allowed only when dsc_version_minor = 0x2).
All other encodings are RESERVED.

Harry

> + u8 bits_per_component:4;
> + /**
> +  * @bpp_high:
> +  * PPS4[1:0] - These are the most significant 2 bits of
> +  * compressed BPP bits_per_pixel[9:0] syntax element.
> +  */
> + u8 bpp_high:2;
> + /**
> +  * @vbr_enable:
> +  * PPS4[2] - 0 = VBR disabled, 1 = VBR enabled
> +  */
> + u8 vbr_enable:1;
> + /**
> +  * @simple_422:
> +  * PPS4[3] - Indicates if decoder drops samples to
> +  * reconstruct the 4:2:2 picture.
> +  */
> + u8 simple_422:1;
> + /**
> +  * @convert_rgb:
> +  * PPS4[4] - Indicates if DSC color space conversion is active
> +  */
> + u8 convert_rgb:1;
> 

[Intel-gfx] [PATCH v2 2/4] drm/dsc: Define Display Stream Compression PPS infoframe

[Manasi Navare]

This patch defines a new header file for all the DSC 1.2 structures
and creates a structure for PPS infoframe which will be used to send
picture parameter set secondary data packet for display stream compression.
All the PPS infoframe syntax elements are taken from DSC 1.2 specification
from VESA.

v2:
* Fix the comments for kernel-doc

Cc: dri-de...@lists.freedesktop.org
Cc: Jani Nikula 
Cc: Ville Syrjala 
Cc: Anusha Srivatsa 
Signed-off-by: Manasi Navare 
---
 include/drm/drm_dsc.h | 437 ++
 1 file changed, 437 insertions(+)
 create mode 100644 include/drm/drm_dsc.h

diff --git a/include/drm/drm_dsc.h b/include/drm/drm_dsc.h
new file mode 100644
index 000..5ee72e8
--- /dev/null
+++ b/include/drm/drm_dsc.h
@@ -0,0 +1,437 @@
+/*
+ * Copyright (C) 2018 Intel Corp.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ * Manasi Navare 
+ */
+
+#ifndef DRM_DSC_H_
+#define DRM_DSC_H_
+
+#include 
+
+#define DSC_NUM_BUF_RANGES 15
+
+/**
+ * struct picture_parameter_set - Represents 128 bytes of Picture Parameter Set
+ *
+ * The VESA DSC standard defines picture parameter set (PPS) which display 
stream
+ * compression encoders must communicate to decoders. The PPS is encapsulated
+ * in 128 bytes (PPS 0 through PPS 127). The fields in this structure are as 
per
+ * Table 4.1 in Vesa DSC specification v1.1/v1.2.
+ * The PPS fields that span over more than a byte should be stored in Big 
Endian
+ * format.
+ */
+struct picture_parameter_set {
+   /**
+* @dsc_version_minor:
+* PPS0[3:0] - Contains Minor version of DSC
+*/
+   u8 dsc_version_minor:4;
+   /**
+* @dsc_version_major:
+* PPS0[7:4] - Contains major version of DSC
+*/
+   u8 dsc_version_major:4;
+   /**
+* @pps_identifier:
+* PPS1[7:0] - Application specific identifier that can be
+* used to differentiate between different PPS tables.
+*/
+   u8 pps_identifier;
+   /**
+* @pps2_reserved:
+* PPS2[7:0]- RESERVED Byte
+*/
+   u8 pps2_reserved;
+   /**
+* @linebuf_depth:
+* PPS3[3:0] - Contains linebuffer bit depth used to generate
+* the bitstream. (0x0 - 16 bits for DSc 1.2, 0x8 - 8 bits,
+* 0xA - 10 bits, 0xB - 11 bits, 0xC - 12 bits, 0xD - 13 bits,
+* 0xE - 14 bits for DSC1.2, 0xF - 14 bits for DSC 1.2.
+*/
+   u8 linebuf_depth:4;
+   /**
+* @bits_per_component:
+* PPS3[7:4] - Bits per component fo rthe original pixels
+* of the encoded picture.
+*/
+   u8 bits_per_component:4;
+   /**
+* @bpp_high:
+* PPS4[1:0] - These are the most significant 2 bits of
+* compressed BPP bits_per_pixel[9:0] syntax element.
+*/
+   u8 bpp_high:2;
+   /**
+* @vbr_enable:
+* PPS4[2] - 0 = VBR disabled, 1 = VBR enabled
+*/
+   u8 vbr_enable:1;
+   /**
+* @simple_422:
+* PPS4[3] - Indicates if decoder drops samples to
+* reconstruct the 4:2:2 picture.
+*/
+   u8 simple_422:1;
+   /**
+* @convert_rgb:
+* PPS4[4] - Indicates if DSC color space conversion is active
+*/
+   u8 convert_rgb:1;
+   /**
+* @block_pred_enable:
+* PPS4[5] - Indicates if BP is used to code any groups in picture
+*/
+   u8 block_pred_enable:1;
+   /**
+* @pps4_reserved:
+* PPS4[7:6] - Reseved bits
+*/
+   u8 pps4_reserved:2;
+   /**
+* @bpp_low:
+* PPS5[7:0] - This indicates the lower significant 8 bits of
+* the compressed BPP bits_per_pixel[9:0] element.
+*/
+   u8 bpp_low;
+   /**
+