diff mbox series

[4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming

Message ID 20190218193137.22914-5-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Clean up ilk+ csc stuff | expand

Commit Message

Ville Syrjälä Feb. 18, 2019, 7:31 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have far too much messy duplicated code in the
pipe/output CSC programming. Simply provide two functions
(ilk_update_pipe_csc() and icl_update_output_csc()) to
program the relevant CSC registers. The desired offsets
and coefficients are passed in as parameters.

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

Comments

Shankar, Uma March 13, 2019, 4:38 p.m. UTC | #1 
>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Tuesday, February 19, 2019 1:02 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Shankar, Uma <uma.shankar@intel.com>; Roper, Matthew D
><matthew.d.roper@intel.com>
>Subject: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>We have far too much messy duplicated code in the pipe/output CSC programming.
>Simply provide two functions
>(ilk_update_pipe_csc() and icl_update_output_csc()) to program the relevant CSC
>registers. The desired offsets and coefficients are passed in as parameters.
>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_color.c | 168 ++++++++++++++---------------
> 1 file changed, 82 insertions(+), 86 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>index ddc48c0d45ac..61cb69058b35 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -40,23 +40,6 @@
> #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>
> #define LEGACY_LUT_LENGTH		256
>-
>-/* Post offset values for RGB->YCBCR conversion */ -#define
>POSTOFF_RGB_TO_YUV_HI 0x800 -#define POSTOFF_RGB_TO_YUV_ME 0x100 -
>#define POSTOFF_RGB_TO_YUV_LO 0x800
>-
>-/*
>- * These values are direct register values specified in the Bspec,
>- * for RGB->YUV conversion matrix (colorspace BT709)
>- */
>-#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 -#define CSC_RGB_TO_YUV_BU
>0x37e80000 -#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 -#define
>CSC_RGB_TO_YUV_BY 0xb5280000 -#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 -
>#define CSC_RGB_TO_YUV_BV 0x1e080000
>-
> /*
>  * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>  * format). This macro takes the coefficient we want transformed and the @@ -74,6
>+57,31 @@
> #define ILK_CSC_COEFF_1_0		\
> 	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
>
>+#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
>+
>+static const u16 ilk_csc_off_zero[3] = {};
>+
>+static const u16 ilk_csc_postoff_limited_range[3] = {
>+	ILK_CSC_POSTOFF_LIMITED_RANGE,
>+	ILK_CSC_POSTOFF_LIMITED_RANGE,
>+	ILK_CSC_POSTOFF_LIMITED_RANGE,
>+};
>+
>+/*
>+ * These values are direct register values specified in the Bspec,
>+ * for RGB->YUV conversion matrix (colorspace BT709)  */ static const
>+u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
>+	0x1e08, 0x9cc0, 0xb528,
>+	0x2ba8, 0x09d8, 0x37e8,
>+	0xbce8, 0x9ad8, 0x1e08,
>+};

I am not sure if the matrix coefficients are correct. Can you please cross check, if I am
missing something. Spec has these as values (hoping table doesn’t get distorted while sending :))
	Bt.601			Bt.709
	Value	Program	Value		Program
RU	0.2990	0x1990		0.21260		0x2D98
GU	0.5870	0x0968		0.71520		0x0B70
BU	0.1140	0x3E98		0.07220		0x3940
RV	-0.1687	0xAAC8		-0.11460	0xBEA8
GV	-0.3313	0x9A98		-0.38540	0x9C58
BV	0.5000	0x0800		0.50000		0x0800
RY	0.5000	0x0800		0.50000		0x0800
GY	-0.4187	0x9D68		-0.45420	0x9E88
BY	-0.0813	0xBA68		-0.04580	0xB5E0


>+
>+/* Post offset values for RGB->YCBCR conversion */ static const u16
>+ilk_csc_postoff_rgb_to_ycbcr[3] = {
>+	0x0800, 0x0100, 0x0800,
>+};
>+
> static bool lut_is_legacy(const struct drm_property_blob *lut)  {
> 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -113,54
>+121,60 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
> 	return result;
> }
>
>-static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
>+static void ilk_update_pipe_csc(struct intel_crtc *crtc,
>+				const u16 preoff[3],
>+				const u16 coeff[9],
>+				const u16 postoff[3])
> {
> 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> 	enum pipe pipe = crtc->pipe;
>
>-	if (INTEL_GEN(dev_priv) < 11) {
>-		I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>-		I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>-		I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>+	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), preoff[0]);
>+	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), preoff[1]);
>+	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), preoff[2]);
>
>-		I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe),
>CSC_RGB_TO_YUV_RU_GU);
>-		I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
>+	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeff[0] << 16 | coeff[1]);
>+	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeff[2] << 16);
>
>-		I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe),
>CSC_RGB_TO_YUV_RY_GY);
>-		I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
>+	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeff[3] << 16 | coeff[4]);
>+	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeff[5] << 16);
>
>-		I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe),
>CSC_RGB_TO_YUV_RV_GV);
>-		I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
>+	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeff[6] << 16 | coeff[7]);
>+	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeff[8] << 16);
>
>-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe),
>POSTOFF_RGB_TO_YUV_HI);
>-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe),
>POSTOFF_RGB_TO_YUV_ME);
>-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe),
>POSTOFF_RGB_TO_YUV_LO);
>-	} else {
>-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), 0);
>-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), 0);
>-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), 0);
>-
>-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe),
>-			   CSC_RGB_TO_YUV_RU_GU);
>-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe),
>CSC_RGB_TO_YUV_BU);
>-
>-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe),
>-			   CSC_RGB_TO_YUV_RY_GY);
>-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe),
>CSC_RGB_TO_YUV_BY);
>-
>-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe),
>-			   CSC_RGB_TO_YUV_RV_GV);
>-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe),
>CSC_RGB_TO_YUV_BV);
>-
>-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe),
>-			   POSTOFF_RGB_TO_YUV_HI);
>-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe),
>-			   POSTOFF_RGB_TO_YUV_ME);
>-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe),
>-			   POSTOFF_RGB_TO_YUV_LO);
>+	if (INTEL_GEN(dev_priv) >= 7) {
>+		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff[0]);
>+		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff[1]);
>+		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff[2]);
> 	}
> }
>
>+static void icl_update_output_csc(struct intel_crtc *crtc,
>+				  const u16 preoff[3],
>+				  const u16 coeff[9],
>+				  const u16 postoff[3])
>+{
>+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>+	enum pipe pipe = crtc->pipe;
>+
>+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), preoff[0]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), preoff[1]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), preoff[2]);
>+
>+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe), coeff[0] << 16 |
>coeff[1]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe), coeff[2]);
>+
>+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe), coeff[3] << 16 |
>coeff[4]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe), coeff[5]);
>+
>+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe), coeff[6] << 16 |
>coeff[7]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe), coeff[8]);
>+
>+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe), postoff[0]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe), postoff[1]);
>+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe), postoff[2]); }
>+
> static bool ilk_csc_limited_range(const struct intel_crtc_state *crtc_state)  {
> 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>@@ -185,7 +199,15 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state
>*crtc_state)
>
> 	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> 	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
>-		ilk_load_ycbcr_conversion_matrix(crtc);
>+		if (INTEL_GEN(dev_priv) >= 11)
>+			icl_update_output_csc(crtc, ilk_csc_off_zero,
>+					      ilk_csc_coeff_rgb_to_ycbcr,
>+					      ilk_csc_postoff_rgb_to_ycbcr);
>+		else
>+			ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
>+					    ilk_csc_coeff_rgb_to_ycbcr,
>+					    ilk_csc_postoff_rgb_to_ycbcr);
>+
> 		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> 		/*
> 		 * On pre GEN11 output CSC is not there, so with 1 pipe CSC @@ -
>258,38 +280,12 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state
>*crtc_state)
> 		}
> 	}
>
>-	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeffs[0] << 16 | coeffs[1]);
>-	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeffs[2] << 16);
>-
>-	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeffs[3] << 16 | coeffs[4]);
>-	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeffs[5] << 16);
>-
>-	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeffs[6] << 16 | coeffs[7]);
>-	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeffs[8] << 16);
>+	ilk_update_pipe_csc(crtc, ilk_csc_off_zero, coeffs,
>+			    limited_color_range ?
>+			    ilk_csc_postoff_limited_range :
>+			    ilk_csc_off_zero);
>
>-	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>-	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>-	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>-
>-	if (INTEL_GEN(dev_priv) > 6) {
>-		u16 postoff = 0;
>-
>-		if (limited_color_range)
>-			postoff = (16 * (1 << 12) / 255) & 0x1fff;
>-
>-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
>-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>-
>-		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
>-	} else {
>-		u32 mode = CSC_MODE_YUV_TO_RGB;
>-
>-		if (limited_color_range)
>-			mode |= CSC_BLACK_SCREEN_OFFSET;
>-
>-		I915_WRITE(PIPE_CSC_MODE(pipe), mode);

Looks like this is not handled and got dropped. Pre Gen7 stuff.

>-	}
>+	I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> }
>
> /*
>--
>2.19.2
Ville Syrjälä March 13, 2019, 7:51 p.m. UTC | #2 
On Wed, Mar 13, 2019 at 04:38:01PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, February 19, 2019 1:02 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Shankar, Uma <uma.shankar@intel.com>; Roper, Matthew D
> ><matthew.d.roper@intel.com>
> >Subject: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
> >
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >We have far too much messy duplicated code in the pipe/output CSC programming.
> >Simply provide two functions
> >(ilk_update_pipe_csc() and icl_update_output_csc()) to program the relevant CSC
> >registers. The desired offsets and coefficients are passed in as parameters.
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/intel_color.c | 168 ++++++++++++++---------------
> > 1 file changed, 82 insertions(+), 86 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >index ddc48c0d45ac..61cb69058b35 100644
> >--- a/drivers/gpu/drm/i915/intel_color.c
> >+++ b/drivers/gpu/drm/i915/intel_color.c
> >@@ -40,23 +40,6 @@
> > #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
> >
> > #define LEGACY_LUT_LENGTH		256
> >-
> >-/* Post offset values for RGB->YCBCR conversion */ -#define
> >POSTOFF_RGB_TO_YUV_HI 0x800 -#define POSTOFF_RGB_TO_YUV_ME 0x100 -
> >#define POSTOFF_RGB_TO_YUV_LO 0x800
> >-
> >-/*
> >- * These values are direct register values specified in the Bspec,
> >- * for RGB->YUV conversion matrix (colorspace BT709)
> >- */
> >-#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 -#define CSC_RGB_TO_YUV_BU
> >0x37e80000 -#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 -#define
> >CSC_RGB_TO_YUV_BY 0xb5280000 -#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8 -
> >#define CSC_RGB_TO_YUV_BV 0x1e080000
> >-
> > /*
> >  * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> >  * format). This macro takes the coefficient we want transformed and the @@ -74,6
> >+57,31 @@
> > #define ILK_CSC_COEFF_1_0		\
> > 	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
> >
> >+#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
> >+
> >+static const u16 ilk_csc_off_zero[3] = {};
> >+
> >+static const u16 ilk_csc_postoff_limited_range[3] = {
> >+	ILK_CSC_POSTOFF_LIMITED_RANGE,
> >+	ILK_CSC_POSTOFF_LIMITED_RANGE,
> >+	ILK_CSC_POSTOFF_LIMITED_RANGE,
> >+};
> >+
> >+/*
> >+ * These values are direct register values specified in the Bspec,
> >+ * for RGB->YUV conversion matrix (colorspace BT709)  */ static const
> >+u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
> >+	0x1e08, 0x9cc0, 0xb528,
> >+	0x2ba8, 0x09d8, 0x37e8,
> >+	0xbce8, 0x9ad8, 0x1e08,
> >+};
> 
> I am not sure if the matrix coefficients are correct. Can you please cross check, if I am
> missing something. Spec has these as values (hoping table doesn’t get distorted while sending :))
> 	Bt.601			Bt.709
> 	Value	Program	Value		Program
> RU	0.2990	0x1990		0.21260		0x2D98
> GU	0.5870	0x0968		0.71520		0x0B70
> BU	0.1140	0x3E98		0.07220		0x3940
> RV	-0.1687	0xAAC8		-0.11460	0xBEA8
> GV	-0.3313	0x9A98		-0.38540	0x9C58
> BV	0.5000	0x0800		0.50000		0x0800
> RY	0.5000	0x0800		0.50000		0x0800
> GY	-0.4187	0x9D68		-0.45420	0x9E88
> BY	-0.0813	0xBA68		-0.04580	0xB5E0

My calculations are giving me this:
	0x1e10, 0x9cc8, 0xb528,
	0x2bb0, 0x09d0, 0x37f0,
	0xbce0, 0x9ad8, 0x1e10,

The difference between my numbers and the ones in the code seem
to be more or less just due to rounding.

The numbers in the spec would appear to be for full range output,
but we want limited range.

> 
> 
> >+
> >+/* Post offset values for RGB->YCBCR conversion */ static const u16
> >+ilk_csc_postoff_rgb_to_ycbcr[3] = {
> >+	0x0800, 0x0100, 0x0800,
> >+};
> >+
> > static bool lut_is_legacy(const struct drm_property_blob *lut)  {
> > 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -113,54
> >+121,60 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
> > 	return result;
> > }
> >
> >-static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
> >+static void ilk_update_pipe_csc(struct intel_crtc *crtc,
> >+				const u16 preoff[3],
> >+				const u16 coeff[9],
> >+				const u16 postoff[3])
> > {
> > 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > 	enum pipe pipe = crtc->pipe;
> >
> >-	if (INTEL_GEN(dev_priv) < 11) {
> >-		I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> >-		I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> >-		I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> >+	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), preoff[0]);
> >+	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), preoff[1]);
> >+	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), preoff[2]);
> >
> >-		I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe),
> >CSC_RGB_TO_YUV_RU_GU);
> >-		I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
> >+	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeff[0] << 16 | coeff[1]);
> >+	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeff[2] << 16);
> >
> >-		I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe),
> >CSC_RGB_TO_YUV_RY_GY);
> >-		I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
> >+	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeff[3] << 16 | coeff[4]);
> >+	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeff[5] << 16);
> >
> >-		I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe),
> >CSC_RGB_TO_YUV_RV_GV);
> >-		I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
> >+	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeff[6] << 16 | coeff[7]);
> >+	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeff[8] << 16);
> >
> >-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe),
> >POSTOFF_RGB_TO_YUV_HI);
> >-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe),
> >POSTOFF_RGB_TO_YUV_ME);
> >-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe),
> >POSTOFF_RGB_TO_YUV_LO);
> >-	} else {
> >-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), 0);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), 0);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), 0);
> >-
> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe),
> >-			   CSC_RGB_TO_YUV_RU_GU);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe),
> >CSC_RGB_TO_YUV_BU);
> >-
> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe),
> >-			   CSC_RGB_TO_YUV_RY_GY);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe),
> >CSC_RGB_TO_YUV_BY);
> >-
> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe),
> >-			   CSC_RGB_TO_YUV_RV_GV);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe),
> >CSC_RGB_TO_YUV_BV);
> >-
> >-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe),
> >-			   POSTOFF_RGB_TO_YUV_HI);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe),
> >-			   POSTOFF_RGB_TO_YUV_ME);
> >-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe),
> >-			   POSTOFF_RGB_TO_YUV_LO);
> >+	if (INTEL_GEN(dev_priv) >= 7) {
> >+		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff[0]);
> >+		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff[1]);
> >+		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff[2]);
> > 	}
> > }
> >
> >+static void icl_update_output_csc(struct intel_crtc *crtc,
> >+				  const u16 preoff[3],
> >+				  const u16 coeff[9],
> >+				  const u16 postoff[3])
> >+{
> >+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >+	enum pipe pipe = crtc->pipe;
> >+
> >+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), preoff[0]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), preoff[1]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), preoff[2]);
> >+
> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe), coeff[0] << 16 |
> >coeff[1]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe), coeff[2]);
> >+
> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe), coeff[3] << 16 |
> >coeff[4]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe), coeff[5]);
> >+
> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe), coeff[6] << 16 |
> >coeff[7]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe), coeff[8]);
> >+
> >+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe), postoff[0]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe), postoff[1]);
> >+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe), postoff[2]); }
> >+
> > static bool ilk_csc_limited_range(const struct intel_crtc_state *crtc_state)  {
> > 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >@@ -185,7 +199,15 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state
> >*crtc_state)
> >
> > 	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> > 	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
> >-		ilk_load_ycbcr_conversion_matrix(crtc);
> >+		if (INTEL_GEN(dev_priv) >= 11)
> >+			icl_update_output_csc(crtc, ilk_csc_off_zero,
> >+					      ilk_csc_coeff_rgb_to_ycbcr,
> >+					      ilk_csc_postoff_rgb_to_ycbcr);
> >+		else
> >+			ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
> >+					    ilk_csc_coeff_rgb_to_ycbcr,
> >+					    ilk_csc_postoff_rgb_to_ycbcr);
> >+
> > 		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> > 		/*
> > 		 * On pre GEN11 output CSC is not there, so with 1 pipe CSC @@ -
> >258,38 +280,12 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state
> >*crtc_state)
> > 		}
> > 	}
> >
> >-	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeffs[0] << 16 | coeffs[1]);
> >-	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeffs[2] << 16);
> >-
> >-	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeffs[3] << 16 | coeffs[4]);
> >-	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeffs[5] << 16);
> >-
> >-	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeffs[6] << 16 | coeffs[7]);
> >-	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeffs[8] << 16);
> >+	ilk_update_pipe_csc(crtc, ilk_csc_off_zero, coeffs,
> >+			    limited_color_range ?
> >+			    ilk_csc_postoff_limited_range :
> >+			    ilk_csc_off_zero);
> >
> >-	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
> >-	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> >-	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
> >-
> >-	if (INTEL_GEN(dev_priv) > 6) {
> >-		u16 postoff = 0;
> >-
> >-		if (limited_color_range)
> >-			postoff = (16 * (1 << 12) / 255) & 0x1fff;
> >-
> >-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
> >-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
> >-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
> >-
> >-		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> >-	} else {
> >-		u32 mode = CSC_MODE_YUV_TO_RGB;
> >-
> >-		if (limited_color_range)
> >-			mode |= CSC_BLACK_SCREEN_OFFSET;
> >-
> >-		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
> 
> Looks like this is not handled and got dropped. Pre Gen7 stuff.

Yeah, we don't use the CSC (yet) on pre-HSW. Even if we start to
use it we'll not be using it for RGB full->limited conversion.
So we won't actually program it like this in the end.

> 
> >-	}
> >+	I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
> > }
> >
> > /*
> >--
> >2.19.2
>
Shankar, Uma March 14, 2019, 1:23 p.m. UTC | #3 
>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Thursday, March 14, 2019 1:21 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Roper, Matthew D
><matthew.d.roper@intel.com>
>Subject: Re: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC programming
>
>On Wed, Mar 13, 2019 at 04:38:01PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Tuesday, February 19, 2019 1:02 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: Shankar, Uma <uma.shankar@intel.com>; Roper, Matthew D
>> ><matthew.d.roper@intel.com>
>> >Subject: [PATCH 4/7] drm/i915: Clean up ilk/icl pipe/output CSC
>> >programming
>> >
>> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> >We have far too much messy duplicated code in the pipe/output CSC
>programming.
>> >Simply provide two functions
>> >(ilk_update_pipe_csc() and icl_update_output_csc()) to program the
>> >relevant CSC registers. The desired offsets and coefficients are passed in as
>parameters.
>> >
>> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >---
>> > drivers/gpu/drm/i915/intel_color.c | 168
>> >++++++++++++++---------------
>> > 1 file changed, 82 insertions(+), 86 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/intel_color.c
>> >b/drivers/gpu/drm/i915/intel_color.c
>> >index ddc48c0d45ac..61cb69058b35 100644
>> >--- a/drivers/gpu/drm/i915/intel_color.c
>> >+++ b/drivers/gpu/drm/i915/intel_color.c
>> >@@ -40,23 +40,6 @@
>> > #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>> >
>> > #define LEGACY_LUT_LENGTH		256
>> >-
>> >-/* Post offset values for RGB->YCBCR conversion */ -#define
>> >POSTOFF_RGB_TO_YUV_HI 0x800 -#define POSTOFF_RGB_TO_YUV_ME 0x100 -
>> >#define POSTOFF_RGB_TO_YUV_LO 0x800
>> >-
>> >-/*
>> >- * These values are direct register values specified in the Bspec,
>> >- * for RGB->YUV conversion matrix (colorspace BT709)
>> >- */
>> >-#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8 -#define CSC_RGB_TO_YUV_BU
>> >0x37e80000 -#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0 -#define
>> >CSC_RGB_TO_YUV_BY 0xb5280000 -#define CSC_RGB_TO_YUV_RV_GV
>0xbce89ad8
>> >- #define CSC_RGB_TO_YUV_BV 0x1e080000
>> >-
>> > /*
>> >  * Extract the CSC coefficient from a CTM coefficient (in U32.32
>> >fixed point
>> >  * format). This macro takes the coefficient we want transformed and
>> >the @@ -74,6
>> >+57,31 @@
>> > #define ILK_CSC_COEFF_1_0		\
>> > 	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
>> >
>> >+#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
>> >+
>> >+static const u16 ilk_csc_off_zero[3] = {};
>> >+
>> >+static const u16 ilk_csc_postoff_limited_range[3] = {
>> >+	ILK_CSC_POSTOFF_LIMITED_RANGE,
>> >+	ILK_CSC_POSTOFF_LIMITED_RANGE,
>> >+	ILK_CSC_POSTOFF_LIMITED_RANGE,
>> >+};
>> >+
>> >+/*
>> >+ * These values are direct register values specified in the Bspec,
>> >+ * for RGB->YUV conversion matrix (colorspace BT709)  */ static
>> >+const
>> >+u16 ilk_csc_coeff_rgb_to_ycbcr[9] = {
>> >+	0x1e08, 0x9cc0, 0xb528,
>> >+	0x2ba8, 0x09d8, 0x37e8,
>> >+	0xbce8, 0x9ad8, 0x1e08,
>> >+};
>>
>> I am not sure if the matrix coefficients are correct. Can you please
>> cross check, if I am missing something. Spec has these as values (hoping table
>doesn’t get distorted while sending :))
>> 	Bt.601			Bt.709
>> 	Value	Program	Value		Program
>> RU	0.2990	0x1990		0.21260		0x2D98
>> GU	0.5870	0x0968		0.71520		0x0B70
>> BU	0.1140	0x3E98		0.07220		0x3940
>> RV	-0.1687	0xAAC8		-0.11460	0xBEA8
>> GV	-0.3313	0x9A98		-0.38540	0x9C58
>> BV	0.5000	0x0800		0.50000		0x0800
>> RY	0.5000	0x0800		0.50000		0x0800
>> GY	-0.4187	0x9D68		-0.45420	0x9E88
>> BY	-0.0813	0xBA68		-0.04580	0xB5E0
>
>My calculations are giving me this:
>	0x1e10, 0x9cc8, 0xb528,
>	0x2bb0, 0x09d0, 0x37f0,
>	0xbce0, 0x9ad8, 0x1e10,
>
>The difference between my numbers and the ones in the code seem to be more or
>less just due to rounding.
>
>The numbers in the spec would appear to be for full range output, but we want
>limited range.

Hmm yes indeed the spec is putting full range there.  So this is fine then.

Reviewed-by: Uma Shankar <uma.shankar@intel.com>

>>
>>
>> >+
>> >+/* Post offset values for RGB->YCBCR conversion */ static const u16
>> >+ilk_csc_postoff_rgb_to_ycbcr[3] = {
>> >+	0x0800, 0x0100, 0x0800,
>> >+};
>> >+
>> > static bool lut_is_legacy(const struct drm_property_blob *lut)  {
>> > 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH; @@ -113,54
>> >+121,60 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64
>> >+*input)
>> > 	return result;
>> > }
>> >
>> >-static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc
>> >*crtc)
>> >+static void ilk_update_pipe_csc(struct intel_crtc *crtc,
>> >+				const u16 preoff[3],
>> >+				const u16 coeff[9],
>> >+				const u16 postoff[3])
>> > {
>> > 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> > 	enum pipe pipe = crtc->pipe;
>> >
>> >-	if (INTEL_GEN(dev_priv) < 11) {
>> >-		I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>> >-		I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>> >-		I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>> >+	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), preoff[0]);
>> >+	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), preoff[1]);
>> >+	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), preoff[2]);
>> >
>> >-		I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe),
>> >CSC_RGB_TO_YUV_RU_GU);
>> >-		I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
>> >+	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeff[0] << 16 | coeff[1]);
>> >+	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeff[2] << 16);
>> >
>> >-		I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe),
>> >CSC_RGB_TO_YUV_RY_GY);
>> >-		I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
>> >+	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeff[3] << 16 | coeff[4]);
>> >+	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeff[5] << 16);
>> >
>> >-		I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe),
>> >CSC_RGB_TO_YUV_RV_GV);
>> >-		I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
>> >+	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeff[6] << 16 | coeff[7]);
>> >+	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeff[8] << 16);
>> >
>> >-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe),
>> >POSTOFF_RGB_TO_YUV_HI);
>> >-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe),
>> >POSTOFF_RGB_TO_YUV_ME);
>> >-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe),
>> >POSTOFF_RGB_TO_YUV_LO);
>> >-	} else {
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), 0);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), 0);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), 0);
>> >-
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe),
>> >-			   CSC_RGB_TO_YUV_RU_GU);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe),
>> >CSC_RGB_TO_YUV_BU);
>> >-
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe),
>> >-			   CSC_RGB_TO_YUV_RY_GY);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe),
>> >CSC_RGB_TO_YUV_BY);
>> >-
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe),
>> >-			   CSC_RGB_TO_YUV_RV_GV);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe),
>> >CSC_RGB_TO_YUV_BV);
>> >-
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe),
>> >-			   POSTOFF_RGB_TO_YUV_HI);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe),
>> >-			   POSTOFF_RGB_TO_YUV_ME);
>> >-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe),
>> >-			   POSTOFF_RGB_TO_YUV_LO);
>> >+	if (INTEL_GEN(dev_priv) >= 7) {
>> >+		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff[0]);
>> >+		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff[1]);
>> >+		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff[2]);
>> > 	}
>> > }
>> >
>> >+static void icl_update_output_csc(struct intel_crtc *crtc,
>> >+				  const u16 preoff[3],
>> >+				  const u16 coeff[9],
>> >+				  const u16 postoff[3])
>> >+{
>> >+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> >+	enum pipe pipe = crtc->pipe;
>> >+
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), preoff[0]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), preoff[1]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), preoff[2]);
>> >+
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe), coeff[0] << 16 |
>> >coeff[1]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe), coeff[2]);
>> >+
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe), coeff[3] << 16 |
>> >coeff[4]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe), coeff[5]);
>> >+
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe), coeff[6] << 16 |
>> >coeff[7]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe), coeff[8]);
>> >+
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe), postoff[0]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe), postoff[1]);
>> >+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe), postoff[2]); }
>> >+
>> > static bool ilk_csc_limited_range(const struct intel_crtc_state *crtc_state)  {
>> > 	struct drm_i915_private *dev_priv =
>> >to_i915(crtc_state->base.crtc->dev);
>> >@@ -185,7 +199,15 @@ static void ilk_load_csc_matrix(const struct
>> >intel_crtc_state
>> >*crtc_state)
>> >
>> > 	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>> > 	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
>> >-		ilk_load_ycbcr_conversion_matrix(crtc);
>> >+		if (INTEL_GEN(dev_priv) >= 11)
>> >+			icl_update_output_csc(crtc, ilk_csc_off_zero,
>> >+					      ilk_csc_coeff_rgb_to_ycbcr,
>> >+					      ilk_csc_postoff_rgb_to_ycbcr);
>> >+		else
>> >+			ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
>> >+					    ilk_csc_coeff_rgb_to_ycbcr,
>> >+					    ilk_csc_postoff_rgb_to_ycbcr);
>> >+
>> > 		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
>> > 		/*
>> > 		 * On pre GEN11 output CSC is not there, so with 1 pipe CSC @@ -
>> >258,38 +280,12 @@ static void ilk_load_csc_matrix(const struct
>> >intel_crtc_state
>> >*crtc_state)
>> > 		}
>> > 	}
>> >
>> >-	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeffs[0] << 16 | coeffs[1]);
>> >-	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeffs[2] << 16);
>> >-
>> >-	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeffs[3] << 16 | coeffs[4]);
>> >-	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeffs[5] << 16);
>> >-
>> >-	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeffs[6] << 16 | coeffs[7]);
>> >-	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeffs[8] << 16);
>> >+	ilk_update_pipe_csc(crtc, ilk_csc_off_zero, coeffs,
>> >+			    limited_color_range ?
>> >+			    ilk_csc_postoff_limited_range :
>> >+			    ilk_csc_off_zero);
>> >
>> >-	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>> >-	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
>> >-	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
>> >-
>> >-	if (INTEL_GEN(dev_priv) > 6) {
>> >-		u16 postoff = 0;
>> >-
>> >-		if (limited_color_range)
>> >-			postoff = (16 * (1 << 12) / 255) & 0x1fff;
>> >-
>> >-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
>> >-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>> >-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>> >-
>> >-		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
>> >-	} else {
>> >-		u32 mode = CSC_MODE_YUV_TO_RGB;
>> >-
>> >-		if (limited_color_range)
>> >-			mode |= CSC_BLACK_SCREEN_OFFSET;
>> >-
>> >-		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
>>
>> Looks like this is not handled and got dropped. Pre Gen7 stuff.
>
>Yeah, we don't use the CSC (yet) on pre-HSW. Even if we start to use it we'll not be
>using it for RGB full->limited conversion.
>So we won't actually program it like this in the end.

Hmm ok, got it. In that case, its ok to drop it. 

>>
>> >-	}
>> >+	I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
>> > }
>> >
>> > /*
>> >--
>> >2.19.2
>>
>
>--
>Ville Syrjälä
>Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index ddc48c0d45ac..61cb69058b35 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -40,23 +40,6 @@ 
 #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
 
 #define LEGACY_LUT_LENGTH		256
-
-/* Post offset values for RGB->YCBCR conversion */
-#define POSTOFF_RGB_TO_YUV_HI 0x800
-#define POSTOFF_RGB_TO_YUV_ME 0x100
-#define POSTOFF_RGB_TO_YUV_LO 0x800
-
-/*
- * These values are direct register values specified in the Bspec,
- * for RGB->YUV conversion matrix (colorspace BT709)
- */
-#define CSC_RGB_TO_YUV_RU_GU 0x2ba809d8
-#define CSC_RGB_TO_YUV_BU 0x37e80000
-#define CSC_RGB_TO_YUV_RY_GY 0x1e089cc0
-#define CSC_RGB_TO_YUV_BY 0xb5280000
-#define CSC_RGB_TO_YUV_RV_GV 0xbce89ad8
-#define CSC_RGB_TO_YUV_BV 0x1e080000
-
 /*
  * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
  * format). This macro takes the coefficient we want transformed and the
@@ -74,6 +57,31 @@ 
 #define ILK_CSC_COEFF_1_0		\
 	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
 
+#define ILK_CSC_POSTOFF_LIMITED_RANGE (16 * (1 << 12) / 255)
+
+static const u16 ilk_csc_off_zero[3] = {};
+
+static const u16 ilk_csc_postoff_limited_range[3] = {
+	ILK_CSC_POSTOFF_LIMITED_RANGE,
+	ILK_CSC_POSTOFF_LIMITED_RANGE,
+	ILK_CSC_POSTOFF_LIMITED_RANGE,
+};
+
+/*
+ * These values are direct register values specified in the Bspec,
+ * for RGB->YUV conversion matrix (colorspace BT709)
+ */
+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 */
+static const u16 ilk_csc_postoff_rgb_to_ycbcr[3] = {
+	0x0800, 0x0100, 0x0800,
+};
+
 static bool lut_is_legacy(const struct drm_property_blob *lut)
 {
 	return drm_color_lut_size(lut) == LEGACY_LUT_LENGTH;
@@ -113,54 +121,60 @@  static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
 	return result;
 }
 
-static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *crtc)
+static void ilk_update_pipe_csc(struct intel_crtc *crtc,
+				const u16 preoff[3],
+				const u16 coeff[9],
+				const u16 postoff[3])
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 
-	if (INTEL_GEN(dev_priv) < 11) {
-		I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
-		I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
-		I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
+	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), preoff[0]);
+	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), preoff[1]);
+	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), preoff[2]);
 
-		I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), CSC_RGB_TO_YUV_RU_GU);
-		I915_WRITE(PIPE_CSC_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
+	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeff[0] << 16 | coeff[1]);
+	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeff[2] << 16);
 
-		I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), CSC_RGB_TO_YUV_RY_GY);
-		I915_WRITE(PIPE_CSC_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
+	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeff[3] << 16 | coeff[4]);
+	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeff[5] << 16);
 
-		I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), CSC_RGB_TO_YUV_RV_GV);
-		I915_WRITE(PIPE_CSC_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
+	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeff[6] << 16 | coeff[7]);
+	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeff[8] << 16);
 
-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
-	} else {
-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), 0);
-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), 0);
-		I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), 0);
-
-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe),
-			   CSC_RGB_TO_YUV_RU_GU);
-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe), CSC_RGB_TO_YUV_BU);
-
-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe),
-			   CSC_RGB_TO_YUV_RY_GY);
-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe), CSC_RGB_TO_YUV_BY);
-
-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe),
-			   CSC_RGB_TO_YUV_RV_GV);
-		I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe), CSC_RGB_TO_YUV_BV);
-
-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe),
-			   POSTOFF_RGB_TO_YUV_HI);
-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe),
-			   POSTOFF_RGB_TO_YUV_ME);
-		I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe),
-			   POSTOFF_RGB_TO_YUV_LO);
+	if (INTEL_GEN(dev_priv) >= 7) {
+		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff[0]);
+		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff[1]);
+		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff[2]);
 	}
 }
 
+static void icl_update_output_csc(struct intel_crtc *crtc,
+				  const u16 preoff[3],
+				  const u16 coeff[9],
+				  const u16 postoff[3])
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+
+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_HI(pipe), preoff[0]);
+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_ME(pipe), preoff[1]);
+	I915_WRITE(PIPE_CSC_OUTPUT_PREOFF_LO(pipe), preoff[2]);
+
+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RY_GY(pipe), coeff[0] << 16 | coeff[1]);
+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BY(pipe), coeff[2]);
+
+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RU_GU(pipe), coeff[3] << 16 | coeff[4]);
+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BU(pipe), coeff[5]);
+
+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_RV_GV(pipe), coeff[6] << 16 | coeff[7]);
+	I915_WRITE(PIPE_CSC_OUTPUT_COEFF_BV(pipe), coeff[8]);
+
+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_HI(pipe), postoff[0]);
+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_ME(pipe), postoff[1]);
+	I915_WRITE(PIPE_CSC_OUTPUT_POSTOFF_LO(pipe), postoff[2]);
+}
+
 static bool ilk_csc_limited_range(const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -185,7 +199,15 @@  static void ilk_load_csc_matrix(const struct intel_crtc_state *crtc_state)
 
 	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
 	    crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
-		ilk_load_ycbcr_conversion_matrix(crtc);
+		if (INTEL_GEN(dev_priv) >= 11)
+			icl_update_output_csc(crtc, ilk_csc_off_zero,
+					      ilk_csc_coeff_rgb_to_ycbcr,
+					      ilk_csc_postoff_rgb_to_ycbcr);
+		else
+			ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
+					    ilk_csc_coeff_rgb_to_ycbcr,
+					    ilk_csc_postoff_rgb_to_ycbcr);
+
 		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
 		/*
 		 * On pre GEN11 output CSC is not there, so with 1 pipe CSC
@@ -258,38 +280,12 @@  static void ilk_load_csc_matrix(const struct intel_crtc_state *crtc_state)
 		}
 	}
 
-	I915_WRITE(PIPE_CSC_COEFF_RY_GY(pipe), coeffs[0] << 16 | coeffs[1]);
-	I915_WRITE(PIPE_CSC_COEFF_BY(pipe), coeffs[2] << 16);
-
-	I915_WRITE(PIPE_CSC_COEFF_RU_GU(pipe), coeffs[3] << 16 | coeffs[4]);
-	I915_WRITE(PIPE_CSC_COEFF_BU(pipe), coeffs[5] << 16);
-
-	I915_WRITE(PIPE_CSC_COEFF_RV_GV(pipe), coeffs[6] << 16 | coeffs[7]);
-	I915_WRITE(PIPE_CSC_COEFF_BV(pipe), coeffs[8] << 16);
+	ilk_update_pipe_csc(crtc, ilk_csc_off_zero, coeffs,
+			    limited_color_range ?
+			    ilk_csc_postoff_limited_range :
+			    ilk_csc_off_zero);
 
-	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
-	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
-	I915_WRITE(PIPE_CSC_PREOFF_LO(pipe), 0);
-
-	if (INTEL_GEN(dev_priv) > 6) {
-		u16 postoff = 0;
-
-		if (limited_color_range)
-			postoff = (16 * (1 << 12) / 255) & 0x1fff;
-
-		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
-		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
-		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
-
-		I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
-	} else {
-		u32 mode = CSC_MODE_YUV_TO_RGB;
-
-		if (limited_color_range)
-			mode |= CSC_BLACK_SCREEN_OFFSET;
-
-		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
-	}
+	I915_WRITE(PIPE_CSC_MODE(pipe), crtc_state->csc_mode);
 }
 
 /*