[10/12] drm/i915: Document ILK+ pipe csc matrix better

Submitted by Ville Syrjälä on July 18, 2019, 2:50 p.m.

Details

Message ID 20190718145053.25808-11-ville.syrjala@linux.intel.com
State Accepted
Headers show
Series "drm/i915: YCbCr output fixes and prep work for YCbCr 4:4:4 output" ( rev: 2 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

Ville Syrjälä July 18, 2019, 2:50 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add comments to explain the ilk pipe csc operation a bit better.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 26 +++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 23a84dd7989f..736c42720daf 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -42,6 +42,21 @@ 
 
 #define LEGACY_LUT_LENGTH		256
 
+/*
+ * ILK+ csc matrix:
+ *
+ * |R/Cr|   | c0 c1 c2 |   ( |R/Cr|   |preoff0| )   |postoff0|
+ * |G/Y | = | c3 c4 c5 | x ( |G/Y | + |preoff1| ) + |postoff1|
+ * |B/Cb|   | c6 c7 c8 |   ( |B/Cb|   |preoff2| )   |postoff2|
+ *
+ * ILK/SNB don't have explicit post offsets, and instead
+ * CSC_MODE_YUV_TO_RGB and CSC_BLACK_SCREEN_OFFSET are used:
+ *  CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=0 -> 1/2, 0, 1/2
+ *  CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/2, 1/16, 1/2
+ *  CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=0 -> 0, 0, 0
+ *  CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/16, 1/16, 1/16
+ */
+
 /*
  * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
  * format). This macro takes the coefficient we want transformed and the
@@ -59,37 +74,38 @@ 
 
 #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
 
+/* Nop pre/post offsets */
 static const u16 ilk_csc_off_zero[3] = {};
 
+/* Identity matrix */
 static const u16 ilk_csc_coeff_identity[9] = {
 	ILK_CSC_COEFF_1_0, 0, 0,
 	0, ILK_CSC_COEFF_1_0, 0,
 	0, 0, ILK_CSC_COEFF_1_0,
 };
 
+/* Limited range RGB post offsets */
 static const u16 ilk_csc_postoff_limited_range[3] = {
 	ILK_CSC_POSTOFF_LIMITED_RANGE,
 	ILK_CSC_POSTOFF_LIMITED_RANGE,
 	ILK_CSC_POSTOFF_LIMITED_RANGE,
 };
 
+/* Full range RGB -> limited range RGB matrix */
 static const u16 ilk_csc_coeff_limited_range[9] = {
 	ILK_CSC_COEFF_LIMITED_RANGE, 0, 0,
 	0, ILK_CSC_COEFF_LIMITED_RANGE, 0,
 	0, 0, ILK_CSC_COEFF_LIMITED_RANGE,
 };
 
-/*
- * These values are direct register values specified in the Bspec,
- * for RGB->YUV conversion matrix (colorspace BT709)
- */
+/* BT.709 full range RGB -> limited range YCbCr matrix */
 static const u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
 	0x1e08, 0x9cc0, 0xb528,
 	0x2ba8, 0x09d8, 0x37e8,
 	0xbce8, 0x9ad8, 0x1e08,
 };
 
-/* Post offset values for RGB->YCBCR conversion */
+/* Limited range YCbCr post offsets */
 static const u16 ilk_csc_postoff_rgb_to_ycbcr[3] = {
 	0x0800, 0x0100, 0x0800,
 };

Comments

On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 

> Add comments to explain the ilk pipe csc operation a bit better.

> 

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/display/intel_color.c | 26 +++++++++++++++++---

> --

>  1 file changed, 21 insertions(+), 5 deletions(-)

> 

> diff --git a/drivers/gpu/drm/i915/display/intel_color.c

> b/drivers/gpu/drm/i915/display/intel_color.c

> index 23a84dd7989f..736c42720daf 100644

> --- a/drivers/gpu/drm/i915/display/intel_color.c

> +++ b/drivers/gpu/drm/i915/display/intel_color.c

> @@ -42,6 +42,21 @@

>  

>  #define LEGACY_LUT_LENGTH		256

>  

> +/*

> + * ILK+ csc matrix:

> + *

> + * |R/Cr|   | c0 c1 c2 |   ( |R/Cr|   |preoff0| )   |postoff0|

> + * |G/Y | = | c3 c4 c5 | x ( |G/Y | + |preoff1| ) + |postoff1|

> + * |B/Cb|   | c6 c7 c8 |   ( |B/Cb|   |preoff2| )   |postoff2|

> + *

> + * ILK/SNB don't have explicit post offsets, and instead

> + * CSC_MODE_YUV_TO_RGB and CSC_BLACK_SCREEN_OFFSET are used:

> + *  CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=0 -> 1/2, 0, 1/2

> + *  CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/2, 1/16,

> 1/2

It seems that the calculated values are assumes ITU-R BT.709 spec,
if you don't mind, can we add some comments which is based on ITU-R
BT.709?
> + *  CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=0 -> 0, 0, 0

> + *  CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/16, 1/16,

> 1/16

> + */

> +

>  /*

>   * Extract the CSC coefficient from a CTM coefficient (in U32.32

> fixed point

>   * format). This macro takes the coefficient we want transformed and

> the

> @@ -59,37 +74,38 @@

>  

>  #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)

>  

> +/* Nop pre/post offsets */

>  static const u16 ilk_csc_off_zero[3] = {};

>  

> +/* Identity matrix */

>  static const u16 ilk_csc_coeff_identity[9] = {

>  	ILK_CSC_COEFF_1_0, 0, 0,

>  	0, ILK_CSC_COEFF_1_0, 0,

>  	0, 0, ILK_CSC_COEFF_1_0,

>  };

>  

> +/* Limited range RGB post offsets */

>  static const u16 ilk_csc_postoff_limited_range[3] = {

>  	ILK_CSC_POSTOFF_LIMITED_RANGE,

>  	ILK_CSC_POSTOFF_LIMITED_RANGE,

>  	ILK_CSC_POSTOFF_LIMITED_RANGE,

>  };

>  

> +/* Full range RGB -> limited range RGB matrix */

>  static const u16 ilk_csc_coeff_limited_range[9] = {

>  	ILK_CSC_COEFF_LIMITED_RANGE, 0, 0,

>  	0, ILK_CSC_COEFF_LIMITED_RANGE, 0,

>  	0, 0, ILK_CSC_COEFF_LIMITED_RANGE,

>  };

>  

> -/*

> - * These values are direct register values specified in the Bspec,

> - * for RGB->YUV conversion matrix (colorspace BT709)

> - */

> +/* BT.709 full range RGB -> limited range YCbCr matrix */

>  static const u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {

>  	0x1e08, 0x9cc0, 0xb528,

>  	0x2ba8, 0x09d8, 0x37e8,

>  	0xbce8, 0x9ad8, 0x1e08,

>  };

>  

> -/* Post offset values for RGB->YCBCR conversion */

> +/* Limited range YCbCr post offsets */

>  static const u16 ilk_csc_postoff_rgb_to_ycbcr[3] = {

>  	0x0800, 0x0100, 0x0800,

>  };

The changes look good to me.
Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
On Fri, Sep 20, 2019 at 02:24:32PM +0000, Mun, Gwan-gyeong wrote:
> On Thu, 2019-07-18 at 17:50 +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add comments to explain the ilk pipe csc operation a bit better.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_color.c | 26 +++++++++++++++++---
> > --
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 23a84dd7989f..736c42720daf 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -42,6 +42,21 @@
> >  
> >  #define LEGACY_LUT_LENGTH		256
> >  
> > +/*
> > + * ILK+ csc matrix:
> > + *
> > + * |R/Cr|   | c0 c1 c2 |   ( |R/Cr|   |preoff0| )   |postoff0|
> > + * |G/Y | = | c3 c4 c5 | x ( |G/Y | + |preoff1| ) + |postoff1|
> > + * |B/Cb|   | c6 c7 c8 |   ( |B/Cb|   |preoff2| )   |postoff2|
> > + *
> > + * ILK/SNB don't have explicit post offsets, and instead
> > + * CSC_MODE_YUV_TO_RGB and CSC_BLACK_SCREEN_OFFSET are used:
> > + *  CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=0 -> 1/2, 0, 1/2
> > + *  CSC_MODE_YUV_TO_RGB=0 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/2, 1/16,
> > 1/2
> It seems that the calculated values are assumes ITU-R BT.709 spec,
> if you don't mind, can we add some comments which is based on ITU-R
> BT.709?
> > + *  CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=0 -> 0, 0, 0
> > + *  CSC_MODE_YUV_TO_RGB=1 + CSC_BLACK_SCREEN_OFFSET=1 -> 1/16, 1/16,
> > 1/16
> > + */
> > +
> >  /*
> >   * Extract the CSC coefficient from a CTM coefficient (in U32.32
> > fixed point
> >   * format). This macro takes the coefficient we want transformed and
> > the
> > @@ -59,37 +74,38 @@
> >  
> >  #define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
> >  
> > +/* Nop pre/post offsets */
> >  static const u16 ilk_csc_off_zero[3] = {};
> >  
> > +/* Identity matrix */
> >  static const u16 ilk_csc_coeff_identity[9] = {
> >  	ILK_CSC_COEFF_1_0, 0, 0,
> >  	0, ILK_CSC_COEFF_1_0, 0,
> >  	0, 0, ILK_CSC_COEFF_1_0,
> >  };
> >  
> > +/* Limited range RGB post offsets */
> >  static const u16 ilk_csc_postoff_limited_range[3] = {
> >  	ILK_CSC_POSTOFF_LIMITED_RANGE,
> >  	ILK_CSC_POSTOFF_LIMITED_RANGE,
> >  	ILK_CSC_POSTOFF_LIMITED_RANGE,
> >  };
> >  
> > +/* Full range RGB -> limited range RGB matrix */
> >  static const u16 ilk_csc_coeff_limited_range[9] = {
> >  	ILK_CSC_COEFF_LIMITED_RANGE, 0, 0,
> >  	0, ILK_CSC_COEFF_LIMITED_RANGE, 0,
> >  	0, 0, ILK_CSC_COEFF_LIMITED_RANGE,
> >  };
> >  
> > -/*
> > - * These values are direct register values specified in the Bspec,
> > - * for RGB->YUV conversion matrix (colorspace BT709)
> > - */
> > +/* BT.709 full range RGB -> limited range YCbCr matrix */

The comment is here ^

> >  static const u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
> >  	0x1e08, 0x9cc0, 0xb528,
> >  	0x2ba8, 0x09d8, 0x37e8,
> >  	0xbce8, 0x9ad8, 0x1e08,
> >  };
> >  
> > -/* Post offset values for RGB->YCBCR conversion */
> > +/* Limited range YCbCr post offsets */
> >  static const u16 ilk_csc_postoff_rgb_to_ycbcr[3] = {
> >  	0x0800, 0x0100, 0x0800,
> >  };
> The changes look good to me.
> Reviewed-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>