[PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The documentation for the ctm matrix suggests a two's complement
format, but at least the i915 implementation is using sign-magnitude
instead. And looks like malidp is doing the same. Change the docs
to match the current implementation, and change the type from __s64
to __u64 to drive the point home.

Cc: dri-devel@lists.freedesktop.org
Cc: Mihail Atanassov <mihail.atanassov@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Brian Starkey <brian.starkey@arm.com>
Cc: Mali DP Maintainers <malidp@foss.arm.com>
Cc: Johnson Lin <johnson.lin@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 include/uapi/drm/drm_mode.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 2c575794fb52..b5d7d9e0eff5 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
 };
 
 struct drm_color_ctm {
-	/* Conversion matrix in S31.32 format. */
-	__s64 matrix[9];
+	/*
+	 * Conversion matrix in S31.32 sign-magnitude
+	 * (not two's complement!) format.
+	 */
+	__u64 matrix[9];
 };
 
 struct drm_color_lut {
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Remove the pointless 1:1 matrix copy

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If we don't have to frob with the user provided ctm matrix there's
no point in copying it over. Just point at the user ctm directly.

Also the matrix gets fully populated by ctm_mult_by_limited() so
no need to zero initialize it.

Cc: Johnson Lin <johnson.lin@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index a383d993b844..c9af260be113 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -86,7 +86,7 @@ static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
  * When using limited range, multiply the matrix given by userspace by
  * the matrix that we would use for the limited range.
  */
-static void ctm_mult_by_limited(u64 *result, const u64 *input)
+static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
 {
 	int i;
 
@@ -104,6 +104,8 @@ static void ctm_mult_by_limited(u64 *result, const u64 *input)
 		result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
 		result[i] |= user_coeff & CTM_COEFF_SIGN;
 	}
+
+	return result;
 }
 
 static void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
@@ -146,14 +148,13 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	} else if (crtc_state->ctm) {
 		struct drm_color_ctm *ctm =
 			(struct drm_color_ctm *)crtc_state->ctm->data;
-		uint64_t input[9] = { 0, };
+		const u64 *input;
+		u64 temp[9];
 
-		if (intel_crtc_state->limited_color_range) {
-			ctm_mult_by_limited(input, ctm->matrix);
-		} else {
-			for (i = 0; i < ARRAY_SIZE(input); i++)
-				input[i] = ctm->matrix[i];
-		}
+		if (intel_crtc_state->limited_color_range)
+			input = ctm_mult_by_limited(temp, ctm->matrix);
+		else
+			input = ctm->matrix;
 
 		/*
 		 * Convert fixed point S31.32 input to format supported by the
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: Remove the pointless 1:1 matrix copy

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 23, 2018 3:13 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
>Subject: [PATCH 2/4] drm/i915: Remove the pointless 1:1 matrix copy
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>If we don't have to frob with the user provided ctm matrix there's no point in
>copying it over. Just point at the user ctm directly.
>
>Also the matrix gets fully populated by ctm_mult_by_limited() so no need to zero
>initialize it.
>

Looks ok to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

>Cc: Johnson Lin <johnson.lin@intel.com>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Shashank Sharma <shashank.sharma@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_color.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c
>b/drivers/gpu/drm/i915/intel_color.c
>index a383d993b844..c9af260be113 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -86,7 +86,7 @@ static bool crtc_state_is_legacy_gamma(struct
>drm_crtc_state *state)
>  * When using limited range, multiply the matrix given by userspace by
>  * the matrix that we would use for the limited range.
>  */
>-static void ctm_mult_by_limited(u64 *result, const u64 *input)
>+static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
> {
> 	int i;
>
>@@ -104,6 +104,8 @@ static void ctm_mult_by_limited(u64 *result, const u64
>*input)
> 		result[i] = mul_u32_u32(limited_coeff, abs_coeff) >> 30;
> 		result[i] |= user_coeff & CTM_COEFF_SIGN;
> 	}
>+
>+	return result;
> }
>
> static void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc) @@
>-146,14 +148,13 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
> 	} else if (crtc_state->ctm) {
> 		struct drm_color_ctm *ctm =
> 			(struct drm_color_ctm *)crtc_state->ctm->data;
>-		uint64_t input[9] = { 0, };
>+		const u64 *input;
>+		u64 temp[9];
>
>-		if (intel_crtc_state->limited_color_range) {
>-			ctm_mult_by_limited(input, ctm->matrix);
>-		} else {
>-			for (i = 0; i < ARRAY_SIZE(input); i++)
>-				input[i] = ctm->matrix[i];
>-		}
>+		if (intel_crtc_state->limited_color_range)
>+			input = ctm_mult_by_limited(temp, ctm->matrix);
>+		else
>+			input = ctm->matrix;
>
> 		/*
> 		 * Convert fixed point S31.32 input to format supported by the
>--
>2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Rename pipe CSC to use ilk_ prefix

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The pipe CSC was introduced by ILK, so change everything related
to use ilk_ as the prefix.

Cc: Johnson Lin <johnson.lin@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 39 +++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index c9af260be113..af1e61d3bacd 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -66,13 +66,13 @@
  * of the CTM coefficient and we write the value from bit 3. We also round the
  * value.
  */
-#define I9XX_CSC_COEFF_FP(coeff, fbits)	\
+#define ILK_CSC_COEFF_FP(coeff, fbits)	\
 	(clamp_val(((coeff) >> (32 - (fbits) - 3)) + 4, 0, 0xfff) & 0xff8)
 
-#define I9XX_CSC_COEFF_LIMITED_RANGE	\
-	I9XX_CSC_COEFF_FP(CTM_COEFF_LIMITED_RANGE, 9)
-#define I9XX_CSC_COEFF_1_0		\
-	((7 << 12) | I9XX_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
+#define ILK_CSC_COEFF_LIMITED_RANGE	\
+	ILK_CSC_COEFF_FP(CTM_COEFF_LIMITED_RANGE, 9)
+#define ILK_CSC_COEFF_1_0		\
+	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
 
 static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)
 {
@@ -108,7 +108,7 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
 	return result;
 }
 
-static void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
+static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
 {
 	int pipe = intel_crtc->pipe;
 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
@@ -132,8 +132,7 @@ static void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
 	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
 }
 
-/* Set up the pipe CSC unit. */
-static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
+static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 {
 	struct drm_crtc *crtc = crtc_state->crtc;
 	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
@@ -143,7 +142,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
 
 	if (intel_crtc_state->ycbcr420) {
-		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
+		ilk_load_ycbcr_conversion_matrix(intel_crtc);
 		return;
 	} else if (crtc_state->ctm) {
 		struct drm_color_ctm *ctm =
@@ -175,21 +174,21 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 
 			if (abs_coeff < CTM_COEFF_0_125)
 				coeffs[i] |= (3 << 12) |
-					I9XX_CSC_COEFF_FP(abs_coeff, 12);
+					ILK_CSC_COEFF_FP(abs_coeff, 12);
 			else if (abs_coeff < CTM_COEFF_0_25)
 				coeffs[i] |= (2 << 12) |
-					I9XX_CSC_COEFF_FP(abs_coeff, 11);
+					ILK_CSC_COEFF_FP(abs_coeff, 11);
 			else if (abs_coeff < CTM_COEFF_0_5)
 				coeffs[i] |= (1 << 12) |
-					I9XX_CSC_COEFF_FP(abs_coeff, 10);
+					ILK_CSC_COEFF_FP(abs_coeff, 10);
 			else if (abs_coeff < CTM_COEFF_1_0)
-				coeffs[i] |= I9XX_CSC_COEFF_FP(abs_coeff, 9);
+				coeffs[i] |= ILK_CSC_COEFF_FP(abs_coeff, 9);
 			else if (abs_coeff < CTM_COEFF_2_0)
 				coeffs[i] |= (7 << 12) |
-					I9XX_CSC_COEFF_FP(abs_coeff, 8);
+					ILK_CSC_COEFF_FP(abs_coeff, 8);
 			else
 				coeffs[i] |= (6 << 12) |
-					I9XX_CSC_COEFF_FP(abs_coeff, 7);
+					ILK_CSC_COEFF_FP(abs_coeff, 7);
 		}
 	} else {
 		/*
@@ -203,9 +202,9 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
 		for (i = 0; i < 3; i++) {
 			if (intel_crtc_state->limited_color_range)
 				coeffs[i * 3 + i] =
-					I9XX_CSC_COEFF_LIMITED_RANGE;
+					ILK_CSC_COEFF_LIMITED_RANGE;
 			else
-				coeffs[i * 3 + i] = I9XX_CSC_COEFF_1_0;
+				coeffs[i * 3 + i] = ILK_CSC_COEFF_1_0;
 		}
 	}
 
@@ -651,14 +650,14 @@ void intel_color_init(struct drm_crtc *crtc)
 		dev_priv->display.load_csc_matrix = cherryview_load_csc_matrix;
 		dev_priv->display.load_luts = cherryview_load_luts;
 	} else if (IS_HASWELL(dev_priv)) {
-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = haswell_load_luts;
 	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
 		   IS_BROXTON(dev_priv)) {
-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = broadwell_load_luts;
 	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = glk_load_luts;
 	} else {
 		dev_priv->display.load_luts = i9xx_load_luts;
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Rename pipe CSC to use ilk_ prefix

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 23, 2018 3:13 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
>Subject: [PATCH 3/4] drm/i915: Rename pipe CSC to use ilk_ prefix
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>The pipe CSC was introduced by ILK, so change everything related to use ilk_ as
>the prefix.
>

Looks ok to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

>Cc: Johnson Lin <johnson.lin@intel.com>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Shashank Sharma <shashank.sharma@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_color.c | 39 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c
>b/drivers/gpu/drm/i915/intel_color.c
>index c9af260be113..af1e61d3bacd 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -66,13 +66,13 @@
>  * of the CTM coefficient and we write the value from bit 3. We also round the
>  * value.
>  */
>-#define I9XX_CSC_COEFF_FP(coeff, fbits)	\
>+#define ILK_CSC_COEFF_FP(coeff, fbits)	\
> 	(clamp_val(((coeff) >> (32 - (fbits) - 3)) + 4, 0, 0xfff) & 0xff8)
>
>-#define I9XX_CSC_COEFF_LIMITED_RANGE	\
>-	I9XX_CSC_COEFF_FP(CTM_COEFF_LIMITED_RANGE, 9)
>-#define I9XX_CSC_COEFF_1_0		\
>-	((7 << 12) | I9XX_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
>+#define ILK_CSC_COEFF_LIMITED_RANGE	\
>+	ILK_CSC_COEFF_FP(CTM_COEFF_LIMITED_RANGE, 9)
>+#define ILK_CSC_COEFF_1_0		\
>+	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
>
> static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)  { @@ -
>108,7 +108,7 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
> 	return result;
> }
>
>-static void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
>+static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc
>+*intel_crtc)
> {
> 	int pipe = intel_crtc->pipe;
> 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); @@ -
>132,8 +132,7 @@ static void i9xx_load_ycbcr_conversion_matrix(struct
>intel_crtc *intel_crtc)
> 	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> }
>
>-/* Set up the pipe CSC unit. */
>-static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
>+static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> {
> 	struct drm_crtc *crtc = crtc_state->crtc;
> 	struct drm_i915_private *dev_priv = to_i915(crtc->dev); @@ -143,7
>+142,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
> 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>
> 	if (intel_crtc_state->ycbcr420) {
>-		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
>+		ilk_load_ycbcr_conversion_matrix(intel_crtc);
> 		return;
> 	} else if (crtc_state->ctm) {
> 		struct drm_color_ctm *ctm =
>@@ -175,21 +174,21 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
>
> 			if (abs_coeff < CTM_COEFF_0_125)
> 				coeffs[i] |= (3 << 12) |
>-					I9XX_CSC_COEFF_FP(abs_coeff, 12);
>+					ILK_CSC_COEFF_FP(abs_coeff, 12);
> 			else if (abs_coeff < CTM_COEFF_0_25)
> 				coeffs[i] |= (2 << 12) |
>-					I9XX_CSC_COEFF_FP(abs_coeff, 11);
>+					ILK_CSC_COEFF_FP(abs_coeff, 11);
> 			else if (abs_coeff < CTM_COEFF_0_5)
> 				coeffs[i] |= (1 << 12) |
>-					I9XX_CSC_COEFF_FP(abs_coeff, 10);
>+					ILK_CSC_COEFF_FP(abs_coeff, 10);
> 			else if (abs_coeff < CTM_COEFF_1_0)
>-				coeffs[i] |= I9XX_CSC_COEFF_FP(abs_coeff, 9);
>+				coeffs[i] |= ILK_CSC_COEFF_FP(abs_coeff, 9);
> 			else if (abs_coeff < CTM_COEFF_2_0)
> 				coeffs[i] |= (7 << 12) |
>-					I9XX_CSC_COEFF_FP(abs_coeff, 8);
>+					ILK_CSC_COEFF_FP(abs_coeff, 8);
> 			else
> 				coeffs[i] |= (6 << 12) |
>-					I9XX_CSC_COEFF_FP(abs_coeff, 7);
>+					ILK_CSC_COEFF_FP(abs_coeff, 7);
> 		}
> 	} else {
> 		/*
>@@ -203,9 +202,9 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
> 		for (i = 0; i < 3; i++) {
> 			if (intel_crtc_state->limited_color_range)
> 				coeffs[i * 3 + i] =
>-					I9XX_CSC_COEFF_LIMITED_RANGE;
>+					ILK_CSC_COEFF_LIMITED_RANGE;
> 			else
>-				coeffs[i * 3 + i] = I9XX_CSC_COEFF_1_0;
>+				coeffs[i * 3 + i] = ILK_CSC_COEFF_1_0;
> 		}
> 	}
>
>@@ -651,14 +650,14 @@ void intel_color_init(struct drm_crtc *crtc)
> 		dev_priv->display.load_csc_matrix =
>cherryview_load_csc_matrix;
> 		dev_priv->display.load_luts = cherryview_load_luts;
> 	} else if (IS_HASWELL(dev_priv)) {
>-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
>+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> 		dev_priv->display.load_luts = haswell_load_luts;
> 	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> 		   IS_BROXTON(dev_priv)) {
>-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
>+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> 		dev_priv->display.load_luts = broadwell_load_luts;
> 	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
>-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
>+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> 		dev_priv->display.load_luts = glk_load_luts;
> 	} else {
> 		dev_priv->display.load_luts = i9xx_load_luts;
>--
>2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Rename pipe CSC to use ilk_ prefix

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 01:05:25PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, February 23, 2018 3:13 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
> ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
> >Subject: [PATCH 3/4] drm/i915: Rename pipe CSC to use ilk_ prefix
> >
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >The pipe CSC was introduced by ILK, so change everything related to use ilk_ as
> >the prefix.
> >
> 
> Looks ok to me.
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>

Patches 2 and 3 pushed to dinq. Thanks for the review.

Patch 4 still needs an r-b...

> 
> >Cc: Johnson Lin <johnson.lin@intel.com>
> >Cc: Uma Shankar <uma.shankar@intel.com>
> >Cc: Shashank Sharma <shashank.sharma@intel.com>
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/intel_color.c | 39 +++++++++++++++++++-------------------
> > 1 file changed, 19 insertions(+), 20 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_color.c
> >b/drivers/gpu/drm/i915/intel_color.c
> >index c9af260be113..af1e61d3bacd 100644
> >--- a/drivers/gpu/drm/i915/intel_color.c
> >+++ b/drivers/gpu/drm/i915/intel_color.c
> >@@ -66,13 +66,13 @@
> >  * of the CTM coefficient and we write the value from bit 3. We also round the
> >  * value.
> >  */
> >-#define I9XX_CSC_COEFF_FP(coeff, fbits)	\
> >+#define ILK_CSC_COEFF_FP(coeff, fbits)	\
> > 	(clamp_val(((coeff) >> (32 - (fbits) - 3)) + 4, 0, 0xfff) & 0xff8)
> >
> >-#define I9XX_CSC_COEFF_LIMITED_RANGE	\
> >-	I9XX_CSC_COEFF_FP(CTM_COEFF_LIMITED_RANGE, 9)
> >-#define I9XX_CSC_COEFF_1_0		\
> >-	((7 << 12) | I9XX_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
> >+#define ILK_CSC_COEFF_LIMITED_RANGE	\
> >+	ILK_CSC_COEFF_FP(CTM_COEFF_LIMITED_RANGE, 9)
> >+#define ILK_CSC_COEFF_1_0		\
> >+	((7 << 12) | ILK_CSC_COEFF_FP(CTM_COEFF_1_0, 8))
> >
> > static bool crtc_state_is_legacy_gamma(struct drm_crtc_state *state)  { @@ -
> >108,7 +108,7 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
> > 	return result;
> > }
> >
> >-static void i9xx_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
> >+static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc
> >+*intel_crtc)
> > {
> > 	int pipe = intel_crtc->pipe;
> > 	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev); @@ -
> >132,8 +132,7 @@ static void i9xx_load_ycbcr_conversion_matrix(struct
> >intel_crtc *intel_crtc)
> > 	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> > }
> >
> >-/* Set up the pipe CSC unit. */
> >-static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
> >+static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> > {
> > 	struct drm_crtc *crtc = crtc_state->crtc;
> > 	struct drm_i915_private *dev_priv = to_i915(crtc->dev); @@ -143,7
> >+142,7 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state *crtc_state)
> > 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
> >
> > 	if (intel_crtc_state->ycbcr420) {
> >-		i9xx_load_ycbcr_conversion_matrix(intel_crtc);
> >+		ilk_load_ycbcr_conversion_matrix(intel_crtc);
> > 		return;
> > 	} else if (crtc_state->ctm) {
> > 		struct drm_color_ctm *ctm =
> >@@ -175,21 +174,21 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state
> >*crtc_state)
> >
> > 			if (abs_coeff < CTM_COEFF_0_125)
> > 				coeffs[i] |= (3 << 12) |
> >-					I9XX_CSC_COEFF_FP(abs_coeff, 12);
> >+					ILK_CSC_COEFF_FP(abs_coeff, 12);
> > 			else if (abs_coeff < CTM_COEFF_0_25)
> > 				coeffs[i] |= (2 << 12) |
> >-					I9XX_CSC_COEFF_FP(abs_coeff, 11);
> >+					ILK_CSC_COEFF_FP(abs_coeff, 11);
> > 			else if (abs_coeff < CTM_COEFF_0_5)
> > 				coeffs[i] |= (1 << 12) |
> >-					I9XX_CSC_COEFF_FP(abs_coeff, 10);
> >+					ILK_CSC_COEFF_FP(abs_coeff, 10);
> > 			else if (abs_coeff < CTM_COEFF_1_0)
> >-				coeffs[i] |= I9XX_CSC_COEFF_FP(abs_coeff, 9);
> >+				coeffs[i] |= ILK_CSC_COEFF_FP(abs_coeff, 9);
> > 			else if (abs_coeff < CTM_COEFF_2_0)
> > 				coeffs[i] |= (7 << 12) |
> >-					I9XX_CSC_COEFF_FP(abs_coeff, 8);
> >+					ILK_CSC_COEFF_FP(abs_coeff, 8);
> > 			else
> > 				coeffs[i] |= (6 << 12) |
> >-					I9XX_CSC_COEFF_FP(abs_coeff, 7);
> >+					ILK_CSC_COEFF_FP(abs_coeff, 7);
> > 		}
> > 	} else {
> > 		/*
> >@@ -203,9 +202,9 @@ static void i9xx_load_csc_matrix(struct drm_crtc_state
> >*crtc_state)
> > 		for (i = 0; i < 3; i++) {
> > 			if (intel_crtc_state->limited_color_range)
> > 				coeffs[i * 3 + i] =
> >-					I9XX_CSC_COEFF_LIMITED_RANGE;
> >+					ILK_CSC_COEFF_LIMITED_RANGE;
> > 			else
> >-				coeffs[i * 3 + i] = I9XX_CSC_COEFF_1_0;
> >+				coeffs[i * 3 + i] = ILK_CSC_COEFF_1_0;
> > 		}
> > 	}
> >
> >@@ -651,14 +650,14 @@ void intel_color_init(struct drm_crtc *crtc)
> > 		dev_priv->display.load_csc_matrix =
> >cherryview_load_csc_matrix;
> > 		dev_priv->display.load_luts = cherryview_load_luts;
> > 	} else if (IS_HASWELL(dev_priv)) {
> >-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> >+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> > 		dev_priv->display.load_luts = haswell_load_luts;
> > 	} else if (IS_BROADWELL(dev_priv) || IS_GEN9_BC(dev_priv) ||
> > 		   IS_BROXTON(dev_priv)) {
> >-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> >+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> > 		dev_priv->display.load_luts = broadwell_load_luts;
> > 	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> >-		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> >+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> > 		dev_priv->display.load_luts = glk_load_luts;
> > 	} else {
> > 		dev_priv->display.load_luts = i9xx_load_luts;
> >--
> >2.16.1
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On pre-HSW we have dedicated hardware for the RGB limited range
handling, and so we don't want to compress with the CSC matrix.

Toss in a FIXME about gamma LUT vs. limited range using the CSC.

Cc: Johnson Lin <johnson.lin@intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index af1e61d3bacd..89ab0f70aa22 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	int i, pipe = intel_crtc->pipe;
 	uint16_t coeffs[9] = { 0, };
 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
+	bool limited_color_range = false;
+
+	/*
+	 * FIXME if there's a gamma LUT after the CSC, we should
+	 * do the range compression using the gamma LUT instead.
+	 */
+	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
+		limited_color_range = intel_crtc_state->limited_color_range;
 
 	if (intel_crtc_state->ycbcr420) {
 		ilk_load_ycbcr_conversion_matrix(intel_crtc);
@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 		const u64 *input;
 		u64 temp[9];
 
-		if (intel_crtc_state->limited_color_range)
+		if (limited_color_range)
 			input = ctm_mult_by_limited(temp, ctm->matrix);
 		else
 			input = ctm->matrix;
@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 		 * into consideration.
 		 */
 		for (i = 0; i < 3; i++) {
-			if (intel_crtc_state->limited_color_range)
+			if (limited_color_range)
 				coeffs[i * 3 + i] =
 					ILK_CSC_COEFF_LIMITED_RANGE;
 			else
@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	if (INTEL_GEN(dev_priv) > 6) {
 		uint16_t postoff = 0;
 
-		if (intel_crtc_state->limited_color_range)
+		if (limited_color_range)
 			postoff = (16 * (1 << 12) / 255) & 0x1fff;
 
 		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff);
@@ -235,7 +243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 	} else {
 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
 
-		if (intel_crtc_state->limited_color_range)
+		if (limited_color_range)
 			mode |= CSC_BLACK_SCREEN_OFFSET;
 
 		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 23, 2018 3:13 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
>Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>On pre-HSW we have dedicated hardware for the RGB limited range handling, and
>so we don't want to compress with the CSC matrix.
>
>Toss in a FIXME about gamma LUT vs. limited range using the CSC.
>
>Cc: Johnson Lin <johnson.lin@intel.com>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Shashank Sharma <shashank.sharma@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_color.c
>b/drivers/gpu/drm/i915/intel_color.c
>index af1e61d3bacd..89ab0f70aa22 100644
>--- a/drivers/gpu/drm/i915/intel_color.c
>+++ b/drivers/gpu/drm/i915/intel_color.c
>@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
> 	int i, pipe = intel_crtc->pipe;
> 	uint16_t coeffs[9] = { 0, };
> 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
>+	bool limited_color_range = false;
>+
>+	/*
>+	 * FIXME if there's a gamma LUT after the CSC, we should
>+	 * do the range compression using the gamma LUT instead.
>+	 */
>+	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>+		limited_color_range = intel_crtc_state->limited_color_range;
>

Hi Ville,
For VLV or similar platforms (having dedicated color range h/w) for limited_range this matrix
would have been getting used. Though they have a dedicated h/w but I don't think it's getting
programmed currently. Not sure, with removing this CSC scaling logic, it may break the
limited_color scenarios on those platforms. I believe using the dedicated h/w or this scaled_down
CSC should be giving similar results making the things work currently. Not sure if we are using
any limited_range combinations on those platforms though :)

Regards,
Uma Shankar

> 	if (intel_crtc_state->ycbcr420) {
> 		ilk_load_ycbcr_conversion_matrix(intel_crtc);
>@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
> 		const u64 *input;
> 		u64 temp[9];
>
>-		if (intel_crtc_state->limited_color_range)
>+		if (limited_color_range)
> 			input = ctm_mult_by_limited(temp, ctm->matrix);
> 		else
> 			input = ctm->matrix;
>@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
> 		 * into consideration.
> 		 */
> 		for (i = 0; i < 3; i++) {
>-			if (intel_crtc_state->limited_color_range)
>+			if (limited_color_range)
> 				coeffs[i * 3 + i] =
> 					ILK_CSC_COEFF_LIMITED_RANGE;
> 			else
>@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
> 	if (INTEL_GEN(dev_priv) > 6) {
> 		uint16_t postoff = 0;
>
>-		if (intel_crtc_state->limited_color_range)
>+		if (limited_color_range)
> 			postoff = (16 * (1 << 12) / 255) & 0x1fff;
>
> 		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7
>+243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> 	} else {
> 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>
>-		if (intel_crtc_state->limited_color_range)
>+		if (limited_color_range)
> 			mode |= CSC_BLACK_SCREEN_OFFSET;
>
> 		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
>--
>2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 01:33:42PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, February 23, 2018 3:13 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
> ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
> >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW
> >
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >On pre-HSW we have dedicated hardware for the RGB limited range handling, and
> >so we don't want to compress with the CSC matrix.
> >
> >Toss in a FIXME about gamma LUT vs. limited range using the CSC.
> >
> >Cc: Johnson Lin <johnson.lin@intel.com>
> >Cc: Uma Shankar <uma.shankar@intel.com>
> >Cc: Shashank Sharma <shashank.sharma@intel.com>
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++----
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_color.c
> >b/drivers/gpu/drm/i915/intel_color.c
> >index af1e61d3bacd..89ab0f70aa22 100644
> >--- a/drivers/gpu/drm/i915/intel_color.c
> >+++ b/drivers/gpu/drm/i915/intel_color.c
> >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
> >*crtc_state)
> > 	int i, pipe = intel_crtc->pipe;
> > 	uint16_t coeffs[9] = { 0, };
> > 	struct intel_crtc_state *intel_crtc_state = to_intel_crtc_state(crtc_state);
> >+	bool limited_color_range = false;
> >+
> >+	/*
> >+	 * FIXME if there's a gamma LUT after the CSC, we should
> >+	 * do the range compression using the gamma LUT instead.
> >+	 */
> >+	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> >+		limited_color_range = intel_crtc_state->limited_color_range;
> >
> 
> Hi Ville,
> For VLV or similar platforms (having dedicated color range h/w) for limited_range this matrix
> would have been getting used. Though they have a dedicated h/w but I don't think it's getting
> programmed currently. Not sure, with removing this CSC scaling logic, it may break the
> limited_color scenarios on those platforms. I believe using the dedicated h/w or this scaled_down
> CSC should be giving similar results making the things work currently. Not sure if we are using
> any limited_range combinations on those platforms though :)

All pre-HSW platforms that have the pipe CSC (ILK,SNB,IVB) are using
the dedicated hardware for the limited range RGB output. We don't use
the CSC on those platforms for anything at the moment so it doesn't
actually matter what we program into it. But we want to start using
the CSC for ctm and ycbcr444 output which means we have to start
setting it up correctly.

> 
> Regards,
> Uma Shankar
> 
> > 	if (intel_crtc_state->ycbcr420) {
> > 		ilk_load_ycbcr_conversion_matrix(intel_crtc);
> >@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
> >*crtc_state)
> > 		const u64 *input;
> > 		u64 temp[9];
> >
> >-		if (intel_crtc_state->limited_color_range)
> >+		if (limited_color_range)
> > 			input = ctm_mult_by_limited(temp, ctm->matrix);
> > 		else
> > 			input = ctm->matrix;
> >@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
> >*crtc_state)
> > 		 * into consideration.
> > 		 */
> > 		for (i = 0; i < 3; i++) {
> >-			if (intel_crtc_state->limited_color_range)
> >+			if (limited_color_range)
> > 				coeffs[i * 3 + i] =
> > 					ILK_CSC_COEFF_LIMITED_RANGE;
> > 			else
> >@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
> >*crtc_state)
> > 	if (INTEL_GEN(dev_priv) > 6) {
> > 		uint16_t postoff = 0;
> >
> >-		if (intel_crtc_state->limited_color_range)
> >+		if (limited_color_range)
> > 			postoff = (16 * (1 << 12) / 255) & 0x1fff;
> >
> > 		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7
> >+243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> > 	} else {
> > 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
> >
> >-		if (intel_crtc_state->limited_color_range)
> >+		if (limited_color_range)
> > 			mode |= CSC_BLACK_SCREEN_OFFSET;
> >
> > 		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
> >--
> >2.16.1
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 23, 2018 7:23 PM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson.lin@intel.com>;
>Sharma, Shashank <shashank.sharma@intel.com>
>Subject: Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW
>
>On Fri, Feb 23, 2018 at 01:33:42PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Friday, February 23, 2018 3:13 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
>> ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
>> >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW
>> >
>> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> >On pre-HSW we have dedicated hardware for the RGB limited range
>> >handling, and so we don't want to compress with the CSC matrix.
>> >
>> >Toss in a FIXME about gamma LUT vs. limited range using the CSC.
>> >
>> >Cc: Johnson Lin <johnson.lin@intel.com>
>> >Cc: Uma Shankar <uma.shankar@intel.com>
>> >Cc: Shashank Sharma <shashank.sharma@intel.com>
>> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >---
>> > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++----
>> > 1 file changed, 12 insertions(+), 4 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/intel_color.c
>> >b/drivers/gpu/drm/i915/intel_color.c
>> >index af1e61d3bacd..89ab0f70aa22 100644
>> >--- a/drivers/gpu/drm/i915/intel_color.c
>> >+++ b/drivers/gpu/drm/i915/intel_color.c
>> >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct
>> >drm_crtc_state
>> >*crtc_state)
>> > 	int i, pipe = intel_crtc->pipe;
>> > 	uint16_t coeffs[9] = { 0, };
>> > 	struct intel_crtc_state *intel_crtc_state =
>> >to_intel_crtc_state(crtc_state);
>> >+	bool limited_color_range = false;
>> >+
>> >+	/*
>> >+	 * FIXME if there's a gamma LUT after the CSC, we should
>> >+	 * do the range compression using the gamma LUT instead.
>> >+	 */
>> >+	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
>> >+		limited_color_range = intel_crtc_state->limited_color_range;
>> >
>>
>> Hi Ville,
>> For VLV or similar platforms (having dedicated color range h/w) for
>> limited_range this matrix would have been getting used. Though they
>> have a dedicated h/w but I don't think it's getting programmed
>> currently. Not sure, with removing this CSC scaling logic, it may
>> break the limited_color scenarios on those platforms. I believe using
>> the dedicated h/w or this scaled_down CSC should be giving similar
>> results making the things work currently. Not sure if we are using any
>> limited_range combinations on those platforms though :)
>
>All pre-HSW platforms that have the pipe CSC (ILK,SNB,IVB) are using the
>dedicated hardware for the limited range RGB output. We don't use the CSC on
>those platforms for anything at the moment so it doesn't actually matter what
>we program into it. But we want to start using the CSC for ctm and ycbcr444
>output which means we have to start setting it up correctly.
>

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

>>
>> Regards,
>> Uma Shankar
>>
>> > 	if (intel_crtc_state->ycbcr420) {
>> > 		ilk_load_ycbcr_conversion_matrix(intel_crtc);
>> >@@ -150,7 +158,7 @@ static void ilk_load_csc_matrix(struct
>> >drm_crtc_state
>> >*crtc_state)
>> > 		const u64 *input;
>> > 		u64 temp[9];
>> >
>> >-		if (intel_crtc_state->limited_color_range)
>> >+		if (limited_color_range)
>> > 			input = ctm_mult_by_limited(temp, ctm->matrix);
>> > 		else
>> > 			input = ctm->matrix;
>> >@@ -200,7 +208,7 @@ static void ilk_load_csc_matrix(struct
>> >drm_crtc_state
>> >*crtc_state)
>> > 		 * into consideration.
>> > 		 */
>> > 		for (i = 0; i < 3; i++) {
>> >-			if (intel_crtc_state->limited_color_range)
>> >+			if (limited_color_range)
>> > 				coeffs[i * 3 + i] =
>> > 					ILK_CSC_COEFF_LIMITED_RANGE;
>> > 			else
>> >@@ -224,7 +232,7 @@ static void ilk_load_csc_matrix(struct
>> >drm_crtc_state
>> >*crtc_state)
>> > 	if (INTEL_GEN(dev_priv) > 6) {
>> > 		uint16_t postoff = 0;
>> >
>> >-		if (intel_crtc_state->limited_color_range)
>> >+		if (limited_color_range)
>> > 			postoff = (16 * (1 << 12) / 255) & 0x1fff;
>> >
>> > 		I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), postoff); @@ -235,7
>> >+243,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
>> >+*crtc_state)
>> > 	} else {
>> > 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>> >
>> >-		if (intel_crtc_state->limited_color_range)
>> >+		if (limited_color_range)
>> > 			mode |= CSC_BLACK_SCREEN_OFFSET;
>> >
>> > 		I915_WRITE(PIPE_CSC_MODE(pipe), mode);
>> >--
>> >2.16.1
>>
>
>--
>Ville Syrjälä
>Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW

[Ville Syrjälä]

On Wed, Feb 28, 2018 at 01:11:12PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, February 23, 2018 7:23 PM
> >To: Shankar, Uma <uma.shankar@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Lin, Johnson <johnson.lin@intel.com>;
> >Sharma, Shashank <shashank.sharma@intel.com>
> >Subject: Re: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW
> >
> >On Fri, Feb 23, 2018 at 01:33:42PM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
> >> >Sent: Friday, February 23, 2018 3:13 AM
> >> >To: intel-gfx@lists.freedesktop.org
> >> >Cc: Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
> >> ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
> >> >Subject: [PATCH 4/4] drm/i915: Don't mangle the CTM on pre-HSW
> >> >
> >> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> >On pre-HSW we have dedicated hardware for the RGB limited range
> >> >handling, and so we don't want to compress with the CSC matrix.
> >> >
> >> >Toss in a FIXME about gamma LUT vs. limited range using the CSC.
> >> >
> >> >Cc: Johnson Lin <johnson.lin@intel.com>
> >> >Cc: Uma Shankar <uma.shankar@intel.com>
> >> >Cc: Shashank Sharma <shashank.sharma@intel.com>
> >> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >---
> >> > drivers/gpu/drm/i915/intel_color.c | 16 ++++++++++++----
> >> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/intel_color.c
> >> >b/drivers/gpu/drm/i915/intel_color.c
> >> >index af1e61d3bacd..89ab0f70aa22 100644
> >> >--- a/drivers/gpu/drm/i915/intel_color.c
> >> >+++ b/drivers/gpu/drm/i915/intel_color.c
> >> >@@ -140,6 +140,14 @@ static void ilk_load_csc_matrix(struct
> >> >drm_crtc_state
> >> >*crtc_state)
> >> > 	int i, pipe = intel_crtc->pipe;
> >> > 	uint16_t coeffs[9] = { 0, };
> >> > 	struct intel_crtc_state *intel_crtc_state =
> >> >to_intel_crtc_state(crtc_state);
> >> >+	bool limited_color_range = false;
> >> >+
> >> >+	/*
> >> >+	 * FIXME if there's a gamma LUT after the CSC, we should
> >> >+	 * do the range compression using the gamma LUT instead.
> >> >+	 */
> >> >+	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv))
> >> >+		limited_color_range = intel_crtc_state->limited_color_range;
> >> >
> >>
> >> Hi Ville,
> >> For VLV or similar platforms (having dedicated color range h/w) for
> >> limited_range this matrix would have been getting used. Though they
> >> have a dedicated h/w but I don't think it's getting programmed
> >> currently. Not sure, with removing this CSC scaling logic, it may
> >> break the limited_color scenarios on those platforms. I believe using
> >> the dedicated h/w or this scaled_down CSC should be giving similar
> >> results making the things work currently. Not sure if we are using any
> >> limited_range combinations on those platforms though :)
> >
> >All pre-HSW platforms that have the pipe CSC (ILK,SNB,IVB) are using the
> >dedicated hardware for the limited range RGB output. We don't use the CSC on
> >those platforms for anything at the moment so it doesn't actually matter what
> >we program into it. But we want to start using the CSC for ctm and ycbcr444
> >output which means we have to start setting it up correctly.
> >
> 
> Sounds good. 
> Reviewed-by: Uma Shankar <uma.shankar@intel.com>

Thanks. Pushed to dinq.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for series starting with [1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/uapi: The ctm matrix uses sign-magnitude representation
URL   : https://patchwork.freedesktop.org/series/38811/
State : success

== Summary ==

Series 38811v1 series starting with [1/4] drm/uapi: The ctm matrix uses sign-magnitude representation
https://patchwork.freedesktop.org/api/1.0/series/38811/revisions/1/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                fail       -> PASS       (fi-skl-6700k2)

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:418s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:425s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:373s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:482s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:283s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:483s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:484s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:463s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:463s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:569s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:180  dwarn:0   dfail:0   fail:0   skip:108 time:284s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:511s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:384s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:406s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:456s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:412s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:454s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:491s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:493s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:583s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:422s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:515s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:469s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:405s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:426s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:530s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:391s
Blacklisted hosts:
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:394s

b9f1df86d4379ec76dddc07cf6e1cc85ffb26ffd drm-tip: 2018y-02m-22d-18h-25m-51s UTC integration manifest
c8d05ca32b33 drm/i915: Don't mangle the CTM on pre-HSW
a149f7b2df96 drm/i915: Rename pipe CSC to use ilk_ prefix
cad5dc84ad78 drm/i915: Remove the pointless 1:1 matrix copy
1212ff10f716 drm/uapi: The ctm matrix uses sign-magnitude representation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8139/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for series starting with [1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/uapi: The ctm matrix uses sign-magnitude representation
URL   : https://patchwork.freedesktop.org/series/38811/
State : warning

== Summary ==

Test kms_flip:
        Subgroup flip-vs-wf_vblank-interruptible:
                pass       -> FAIL       (shard-apl) fdo#100368 +2
        Subgroup dpms-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#103060
Test kms_flip_tiling:
        Subgroup flip-to-yf-tiled:
                fail       -> PASS       (shard-apl) fdo#103822
Test kms_vblank:
        Subgroup pipe-b-ts-continuation-suspend:
                pass       -> SKIP       (shard-snb)
        Subgroup pipe-a-accuracy-idle:
                pass       -> FAIL       (shard-hsw) fdo#102583
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-indfb-msflip-blt:
                fail       -> PASS       (shard-apl) fdo#103167

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#102583 https://bugs.freedesktop.org/show_bug.cgi?id=102583
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167

shard-apl        total:3465 pass:1820 dwarn:1   dfail:0   fail:13  skip:1631 time:12342s
shard-hsw        total:3465 pass:1766 dwarn:1   dfail:0   fail:4   skip:1693 time:11640s
shard-snb        total:3465 pass:1357 dwarn:1   dfail:0   fail:2   skip:2105 time:6576s
Blacklisted hosts:
shard-kbl        total:3428 pass:1926 dwarn:13  dfail:0   fail:15  skip:1473 time:9296s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8139/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Shankar, Uma]

>-----Original Message-----
>From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, February 23, 2018 3:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: dri-devel@lists.freedesktop.org; Mihail Atanassov
><mihail.atanassov@arm.com>; Liviu Dudau <liviu.dudau@arm.com>; Brian
>Starkey <brian.starkey@arm.com>; Mali DP Maintainers
><malidp@foss.arm.com>; Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma
><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com>
>Subject: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude
>representation
>
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>The documentation for the ctm matrix suggests a two's complement format, but
>at least the i915 implementation is using sign-magnitude instead. And looks like
>malidp is doing the same. Change the docs to match the current implementation,
>and change the type from __s64 to __u64 to drive the point home.
>

Looks ok to me.

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

>Cc: dri-devel@lists.freedesktop.org
>Cc: Mihail Atanassov <mihail.atanassov@arm.com>
>Cc: Liviu Dudau <liviu.dudau@arm.com>
>Cc: Brian Starkey <brian.starkey@arm.com>
>Cc: Mali DP Maintainers <malidp@foss.arm.com>
>Cc: Johnson Lin <johnson.lin@intel.com>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Shashank Sharma <shashank.sharma@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> include/uapi/drm/drm_mode.h | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>index 2c575794fb52..b5d7d9e0eff5 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {  };
>
> struct drm_color_ctm {
>-	/* Conversion matrix in S31.32 format. */
>-	__s64 matrix[9];
>+	/*
>+	 * Conversion matrix in S31.32 sign-magnitude
>+	 * (not two's complement!) format.
>+	 */
>+	__u64 matrix[9];
> };
>
> struct drm_color_lut {
>--
>2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Brian Starkey]

Hi Ville,

On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote:
>From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>The documentation for the ctm matrix suggests a two's complement
>format, but at least the i915 implementation is using sign-magnitude
>instead. And looks like malidp is doing the same. Change the docs
>to match the current implementation, and change the type from __s64
>to __u64 to drive the point home.

I totally agree that this is a good idea, but...

>
>Cc: dri-devel@lists.freedesktop.org
>Cc: Mihail Atanassov <mihail.atanassov@arm.com>
>Cc: Liviu Dudau <liviu.dudau@arm.com>
>Cc: Brian Starkey <brian.starkey@arm.com>
>Cc: Mali DP Maintainers <malidp@foss.arm.com>
>Cc: Johnson Lin <johnson.lin@intel.com>
>Cc: Uma Shankar <uma.shankar@intel.com>
>Cc: Shashank Sharma <shashank.sharma@intel.com>
>Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>---
> include/uapi/drm/drm_mode.h | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>index 2c575794fb52..b5d7d9e0eff5 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
> };
>
> struct drm_color_ctm {
>-	/* Conversion matrix in S31.32 format. */
>-	__s64 matrix[9];
>+	/*
>+	 * Conversion matrix in S31.32 sign-magnitude
>+	 * (not two's complement!) format.
>+	 */
>+	__u64 matrix[9];

Isn't changing the type liable to break something for someone?

Thanks,
-Brian

> };
>
> struct drm_color_lut {
>-- 
>2.16.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 01:52:22PM +0000, Brian Starkey wrote:
> Hi Ville,
> 
> On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >The documentation for the ctm matrix suggests a two's complement
> >format, but at least the i915 implementation is using sign-magnitude
> >instead. And looks like malidp is doing the same. Change the docs
> >to match the current implementation, and change the type from __s64
> >to __u64 to drive the point home.
> 
> I totally agree that this is a good idea, but...
> 
> >
> >Cc: dri-devel@lists.freedesktop.org
> >Cc: Mihail Atanassov <mihail.atanassov@arm.com>
> >Cc: Liviu Dudau <liviu.dudau@arm.com>
> >Cc: Brian Starkey <brian.starkey@arm.com>
> >Cc: Mali DP Maintainers <malidp@foss.arm.com>
> >Cc: Johnson Lin <johnson.lin@intel.com>
> >Cc: Uma Shankar <uma.shankar@intel.com>
> >Cc: Shashank Sharma <shashank.sharma@intel.com>
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > include/uapi/drm/drm_mode.h | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >index 2c575794fb52..b5d7d9e0eff5 100644
> >--- a/include/uapi/drm/drm_mode.h
> >+++ b/include/uapi/drm/drm_mode.h
> >@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
> > };
> >
> > struct drm_color_ctm {
> >-	/* Conversion matrix in S31.32 format. */
> >-	__s64 matrix[9];
> >+	/*
> >+	 * Conversion matrix in S31.32 sign-magnitude
> >+	 * (not two's complement!) format.
> >+	 */
> >+	__u64 matrix[9];
> 
> Isn't changing the type liable to break something for someone?

I hope not. Renaming the member would be a no no, but just changing the
type should be mostly safe I think. I suppose if someone is building
something with very strict compiler -W flags and -Werror it might cause
a build failure, so I guess one might label it as a minor api break but
not an abi break.

If people think that's a serious concern I guess we can keep the
__s64, but I'd rather not give people that much rope to hang
themselves by interpreting it as 2's complement.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 04:04:10PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 23, 2018 at 01:52:22PM +0000, Brian Starkey wrote:
> > Hi Ville,
> > 
> > On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote:
> > >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > >The documentation for the ctm matrix suggests a two's complement
> > >format, but at least the i915 implementation is using sign-magnitude
> > >instead. And looks like malidp is doing the same. Change the docs
> > >to match the current implementation, and change the type from __s64
> > >to __u64 to drive the point home.
> > 
> > I totally agree that this is a good idea, but...
> > 
> > >
> > >Cc: dri-devel@lists.freedesktop.org
> > >Cc: Mihail Atanassov <mihail.atanassov@arm.com>
> > >Cc: Liviu Dudau <liviu.dudau@arm.com>
> > >Cc: Brian Starkey <brian.starkey@arm.com>
> > >Cc: Mali DP Maintainers <malidp@foss.arm.com>
> > >Cc: Johnson Lin <johnson.lin@intel.com>
> > >Cc: Uma Shankar <uma.shankar@intel.com>
> > >Cc: Shashank Sharma <shashank.sharma@intel.com>
> > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >---
> > > include/uapi/drm/drm_mode.h | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > >index 2c575794fb52..b5d7d9e0eff5 100644
> > >--- a/include/uapi/drm/drm_mode.h
> > >+++ b/include/uapi/drm/drm_mode.h
> > >@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
> > > };
> > >
> > > struct drm_color_ctm {
> > >-	/* Conversion matrix in S31.32 format. */
> > >-	__s64 matrix[9];
> > >+	/*
> > >+	 * Conversion matrix in S31.32 sign-magnitude
> > >+	 * (not two's complement!) format.
> > >+	 */
> > >+	__u64 matrix[9];
> > 
> > Isn't changing the type liable to break something for someone?
> 
> I hope not. Renaming the member would be a no no, but just changing the
> type should be mostly safe I think. I suppose if someone is building
> something with very strict compiler -W flags and -Werror it might cause
> a build failure, so I guess one might label it as a minor api break but
> not an abi break.
> 
> If people think that's a serious concern I guess we can keep the
> __s64, but I'd rather not give people that much rope to hang
> themselves by interpreting it as 2's complement.

OK, no one complained loudly so I've gone and pushed this to
drm-misc-next. Now we wait and see whether I can dodge the egg...

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Harry Wentland]

On 2018-02-22 04:42 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The documentation for the ctm matrix suggests a two's complement
> format, but at least the i915 implementation is using sign-magnitude
> instead. And looks like malidp is doing the same. Change the docs
> to match the current implementation, and change the type from __s64
> to __u64 to drive the point home.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Mihail Atanassov <mihail.atanassov@arm.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Brian Starkey <brian.starkey@arm.com>
> Cc: Mali DP Maintainers <malidp@foss.arm.com>
> Cc: Johnson Lin <johnson.lin@intel.com>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Good clarification. Our new CTM implementation (1) actually assumed two's complement but nobody's using it yet, so we'll patch it to convert.

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

(1) https://patchwork.freedesktop.org/patch/204005/

Harry

> ---
>  include/uapi/drm/drm_mode.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 2c575794fb52..b5d7d9e0eff5 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
>  };
>  
>  struct drm_color_ctm {
> -	/* Conversion matrix in S31.32 format. */
> -	__s64 matrix[9];
> +	/*
> +	 * Conversion matrix in S31.32 sign-magnitude
> +	 * (not two's complement!) format.
> +	 */
> +	__u64 matrix[9];
>  };
>  
>  struct drm_color_lut {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 11:26:41AM -0500, Harry Wentland wrote:
> On 2018-02-22 04:42 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The documentation for the ctm matrix suggests a two's complement
> > format, but at least the i915 implementation is using sign-magnitude
> > instead. And looks like malidp is doing the same. Change the docs
> > to match the current implementation, and change the type from __s64
> > to __u64 to drive the point home.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Mihail Atanassov <mihail.atanassov@arm.com>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Brian Starkey <brian.starkey@arm.com>
> > Cc: Mali DP Maintainers <malidp@foss.arm.com>
> > Cc: Johnson Lin <johnson.lin@intel.com>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Good clarification. Our new CTM implementation (1) actually assumed two's complement but nobody's using it yet, so we'll patch it to convert.

Nice. Looks like we caught this just in time.

> 
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> (1) https://patchwork.freedesktop.org/patch/204005/
> 
> Harry
> 
> > ---
> >  include/uapi/drm/drm_mode.h | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 2c575794fb52..b5d7d9e0eff5 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
> >  };
> >  
> >  struct drm_color_ctm {
> > -	/* Conversion matrix in S31.32 format. */
> > -	__s64 matrix[9];
> > +	/*
> > +	 * Conversion matrix in S31.32 sign-magnitude
> > +	 * (not two's complement!) format.
> > +	 */
> > +	__u64 matrix[9];
> >  };
> >  
> >  struct drm_color_lut {
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Daniel Vetter]

On Fri, Feb 23, 2018 at 11:26:41AM -0500, Harry Wentland wrote:
> On 2018-02-22 04:42 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The documentation for the ctm matrix suggests a two's complement
> > format, but at least the i915 implementation is using sign-magnitude
> > instead. And looks like malidp is doing the same. Change the docs
> > to match the current implementation, and change the type from __s64
> > to __u64 to drive the point home.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Mihail Atanassov <mihail.atanassov@arm.com>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Brian Starkey <brian.starkey@arm.com>
> > Cc: Mali DP Maintainers <malidp@foss.arm.com>
> > Cc: Johnson Lin <johnson.lin@intel.com>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Good clarification. Our new CTM implementation (1) actually assumed
> two's complement but nobody's using it yet, so we'll patch it to
> convert.

Another reason to start looking into igt and the tests there? That would
have caught this too ...
-Daniel

> 
> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
> 
> (1) https://patchwork.freedesktop.org/patch/204005/
> 
> Harry
> 
> > ---
> >  include/uapi/drm/drm_mode.h | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > index 2c575794fb52..b5d7d9e0eff5 100644
> > --- a/include/uapi/drm/drm_mode.h
> > +++ b/include/uapi/drm/drm_mode.h
> > @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
> >  };
> >  
> >  struct drm_color_ctm {
> > -	/* Conversion matrix in S31.32 format. */
> > -	__s64 matrix[9];
> > +	/*
> > +	 * Conversion matrix in S31.32 sign-magnitude
> > +	 * (not two's complement!) format.
> > +	 */
> > +	__u64 matrix[9];
> >  };
> >  
> >  struct drm_color_lut {
> > 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

[Harry Wentland]

On 2018-03-06 02:51 AM, Daniel Vetter wrote:
> On Fri, Feb 23, 2018 at 11:26:41AM -0500, Harry Wentland wrote:
>> On 2018-02-22 04:42 PM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> The documentation for the ctm matrix suggests a two's complement
>>> format, but at least the i915 implementation is using sign-magnitude
>>> instead. And looks like malidp is doing the same. Change the docs
>>> to match the current implementation, and change the type from __s64
>>> to __u64 to drive the point home.
>>>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: Mihail Atanassov <mihail.atanassov@arm.com>
>>> Cc: Liviu Dudau <liviu.dudau@arm.com>
>>> Cc: Brian Starkey <brian.starkey@arm.com>
>>> Cc: Mali DP Maintainers <malidp@foss.arm.com>
>>> Cc: Johnson Lin <johnson.lin@intel.com>
>>> Cc: Uma Shankar <uma.shankar@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Good clarification. Our new CTM implementation (1) actually assumed
>> two's complement but nobody's using it yet, so we'll patch it to
>> convert.
> 
> Another reason to start looking into igt and the tests there? That would
> have caught this too ...

There need to be new IGT tests that can actually test for signed-magnitude vs two's compliment differences, which would have to be tests of the CTM in some non-RGB colorspace where negative numbers are actually meaningful. The existing tests will test a negative matrix but in RGB colorspace any negative number will simply map to zero/black, no matter the magnitude.

I'll put such a test on our team's backlog but not sure when we'll actually get to it. We'd have to check if we could have a meaningful test for this with the current capabilities of DC.

Harry

> -Daniel
> 
>>
>> Reviewed-by: Harry Wentland <harry.wentland@amd.com>
>>
>> (1) https://patchwork.freedesktop.org/patch/204005/
>>
>> Harry
>>
>>> ---
>>>  include/uapi/drm/drm_mode.h | 7 +++++--
>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>> index 2c575794fb52..b5d7d9e0eff5 100644
>>> --- a/include/uapi/drm/drm_mode.h
>>> +++ b/include/uapi/drm/drm_mode.h
>>> @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut {
>>>  };
>>>  
>>>  struct drm_color_ctm {
>>> -	/* Conversion matrix in S31.32 format. */
>>> -	__s64 matrix[9];
>>> +	/*
>>> +	 * Conversion matrix in S31.32 sign-magnitude
>>> +	 * (not two's complement!) format.
>>> +	 */
>>> +	__u64 matrix[9];
>>>  };
>>>  
>>>  struct drm_color_lut {
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx