[PATCH 00/10] drm/i915: Clean up intel_color_check()

[PATCH 00/10] drm/i915: Clean up intel_color_check()

[Ville Syrjala]

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

intel_color_check() is mess. Blow it up into clean platform sized
chunks. Also eliminate some redundant linear LUT loads as a result.

Ville Syrjälä (10):
  drm/i915: Extract check_luts()
  drm/i915: Turn intel_color_check() into a vfunc
  drm/i915: Extract i9xx_color_check()
  drm/i915: Extract chv_color_check()
  drm/i915: Extract icl_color_check()
  drm/i915: Extract glk_color_check()
  drm/i915: Extract bdw_color_check()
  drm/i915: Extract ilk_color_check()
  drm/i915: Drop the pointless linear legacy LUT load on CHV
  drm/i915: Skip the linear degamma LUT load on ICL+

 drivers/gpu/drm/i915/i915_drv.h    |   1 +
 drivers/gpu/drm/i915/intel_color.c | 384 +++++++++++++++++++++--------
 2 files changed, 286 insertions(+), 99 deletions(-)

-- 
2.19.2

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

[PATCH 01/10] drm/i915: Extract check_luts()

[Ville Syrjala]

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

In prepartion for per-platform color_check() functions extract the
common code into a separate function.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 467fd1a1630c..b6a992a5b14f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
 	return 0;
 }
 
+static int check_luts(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
+	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
+	int gamma_length, degamma_length;
+	u32 gamma_tests, degamma_tests;
+
+	/* Always allow legacy gamma LUT with no further checking. */
+	if (crtc_state_is_legacy_gamma(crtc_state))
+		return 0;
+
+	/* C8 needs the legacy LUT all to itself */
+	if (crtc_state->c8_planes)
+		return -EINVAL;
+
+	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
+	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
+	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
+
+	if (check_lut_size(degamma_lut, degamma_length) ||
+	    check_lut_size(gamma_lut, gamma_length))
+		return -EINVAL;
+
+	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
+	    drm_color_lut_check(gamma_lut, gamma_tests))
+		return -EINVAL;
+
+	return 0;
+}
+
 static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
 {
 	u32 cgm_mode = 0;
@@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
 	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
 	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
 	bool limited_color_range = false;
-	int gamma_length, degamma_length;
-	u32 gamma_tests, degamma_tests;
 	int ret;
 
-	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
-	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
-	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
-	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
-
-	/* C8 needs the legacy LUT all to itself */
-	if (crtc_state->c8_planes &&
-	    !crtc_state_is_legacy_gamma(crtc_state))
-		return -EINVAL;
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
 
 	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
 		!crtc_state->c8_planes;
@@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
 	if (IS_CHERRYVIEW(dev_priv))
 		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
 
-	/* Always allow legacy gamma LUT with no further checking. */
 	if (!crtc_state->gamma_enable ||
-	    crtc_state_is_legacy_gamma(crtc_state)) {
+	    crtc_state_is_legacy_gamma(crtc_state))
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
-		if (INTEL_GEN(dev_priv) >= 11 &&
-		    crtc_state->gamma_enable)
-			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
-		return 0;
-	}
-
-	if (check_lut_size(degamma_lut, degamma_length) ||
-	    check_lut_size(gamma_lut, gamma_length))
-		return -EINVAL;
-
-	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
-	    drm_color_lut_check(gamma_lut, gamma_tests))
-		return -EINVAL;
-
-	if (INTEL_GEN(dev_priv) >= 11)
+	else if (INTEL_GEN(dev_priv) >= 11)
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
 					 PRE_CSC_GAMMA_ENABLE |
 					 POST_CSC_GAMMA_ENABLE;
@@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
 
 	if (INTEL_GEN(dev_priv) >= 11) {
+		if (crtc_state->gamma_enable)
+			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
+
 		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
 		    crtc_state->limited_color_range)
 			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
-- 
2.19.2

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

[PATCH 02/10] drm/i915: Turn intel_color_check() into a vfunc

[Ville Syrjala]

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

The current intel_color_check() is a mess, and worse yet it is
in fact incorrect for several platforms. The hardware has
evolved quite a bit over the years, so let's just go for a clean
split between the platforms by turning this into a vfunc.
The actual work to split it up will follow.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_color.c | 12 +++++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c65c2e6649df..38dfeb110dd4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -320,6 +320,7 @@ struct drm_i915_display_funcs {
 	/* display clock increase/decrease */
 	/* pll clock increase/decrease */
 
+	int (*color_check)(struct intel_crtc_state *crtc_state);
 	/*
 	 * Program double buffered color management registers during
 	 * vblank evasion. The registers should then latch during the
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index b6a992a5b14f..e68c936c784b 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -704,6 +704,13 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
 	dev_priv->display.color_commit(crtc_state);
 }
 
+int intel_color_check(struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+
+	return dev_priv->display.color_check(crtc_state);
+}
+
 static bool need_plane_update(struct intel_plane *plane,
 			      const struct intel_crtc_state *crtc_state)
 {
@@ -820,7 +827,7 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
 	return cgm_mode;
 }
 
-int intel_color_check(struct intel_crtc_state *crtc_state)
+static int _intel_color_check(struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
 	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
@@ -894,6 +901,7 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.load_luts = i9xx_load_luts;
 
 		dev_priv->display.color_commit = i9xx_color_commit;
+		dev_priv->display.color_check = _intel_color_check;
 	} else {
 		if (INTEL_GEN(dev_priv) >= 11)
 			dev_priv->display.load_luts = icl_load_luts;
@@ -910,6 +918,8 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_commit = hsw_color_commit;
 		else
 			dev_priv->display.color_commit = ilk_color_commit;
+
+		dev_priv->display.color_check = _intel_color_check;
 	}
 
 	/* Enable color management support when we have degamma & gamma LUTs. */
-- 
2.19.2

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

[PATCH 03/10] drm/i915: Extract i9xx_color_check()

[Ville Syrjala]

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

Apart from CHV the other gmch platforms don't currently
require much work in .color_check(). So let's start by
extracting i9xx_color_check().

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index e68c936c784b..3d19b1a7ee95 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -810,6 +810,27 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static int i9xx_color_check(struct intel_crtc_state *crtc_state)
+{
+	int ret;
+
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
+
+	crtc_state->gamma_enable =
+		crtc_state->base.gamma_lut &&
+		!crtc_state->c8_planes;
+
+	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
+
+	ret = intel_color_add_affected_planes(crtc_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
 {
 	u32 cgm_mode = 0;
@@ -895,13 +916,15 @@ void intel_color_init(struct intel_crtc *crtc)
 	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
 
 	if (HAS_GMCH(dev_priv)) {
-		if (IS_CHERRYVIEW(dev_priv))
+		if (IS_CHERRYVIEW(dev_priv)) {
+			dev_priv->display.color_check = _intel_color_check;
+			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = cherryview_load_luts;
-		else
+		} else {
+			dev_priv->display.color_check = i9xx_color_check;
+			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = i9xx_load_luts;
-
-		dev_priv->display.color_commit = i9xx_color_commit;
-		dev_priv->display.color_check = _intel_color_check;
+		}
 	} else {
 		if (INTEL_GEN(dev_priv) >= 11)
 			dev_priv->display.load_luts = icl_load_luts;
-- 
2.19.2

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

[PATCH 04/10] drm/i915: Extract chv_color_check()

[Ville Syrjala]

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

Since CHV has the CGM unit we require a custom implementation
of .color_check().

This fixes the computation of gamma_enable as previously we
left it enabled even when were using the CGM gamma instead.
Now we turn off the legacy LUT unless it's actually required.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 3d19b1a7ee95..cb88651f1da4 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -848,6 +848,29 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
 	return cgm_mode;
 }
 
+static int chv_color_check(struct intel_crtc_state *crtc_state)
+{
+	int ret;
+
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
+
+	crtc_state->gamma_enable =
+		crtc_state_is_legacy_gamma(crtc_state) &&
+		!crtc_state->c8_planes;
+
+	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
+
+	crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
+
+	ret = intel_color_add_affected_planes(crtc_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int _intel_color_check(struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -877,9 +900,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state)
 
 	crtc_state->csc_mode = 0;
 
-	if (IS_CHERRYVIEW(dev_priv))
-		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
-
 	if (!crtc_state->gamma_enable ||
 	    crtc_state_is_legacy_gamma(crtc_state))
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
@@ -917,7 +937,7 @@ void intel_color_init(struct intel_crtc *crtc)
 
 	if (HAS_GMCH(dev_priv)) {
 		if (IS_CHERRYVIEW(dev_priv)) {
-			dev_priv->display.color_check = _intel_color_check;
+			dev_priv->display.color_check = chv_color_check;
 			dev_priv->display.color_commit = i9xx_color_commit;
 			dev_priv->display.load_luts = cherryview_load_luts;
 		} else {
-- 
2.19.2

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

[PATCH 05/10] drm/i915: Extract icl_color_check()

[Ville Syrjala]

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

ICL is rather easy when it comes to .color_check() as it
finally provides us with a full color pipeline with
individual knobs for each stage.

We'll also start bypassing each LUT individually when
it is not needed.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index cb88651f1da4..86580541b0da 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -871,6 +871,55 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
+{
+	u32 gamma_mode = 0;
+
+	if (crtc_state->base.degamma_lut)
+		gamma_mode |= PRE_CSC_GAMMA_ENABLE;
+
+	if (crtc_state->base.gamma_lut &&
+	    !crtc_state->c8_planes)
+		gamma_mode |= POST_CSC_GAMMA_ENABLE;
+
+	if (!crtc_state->base.gamma_lut ||
+	    crtc_state_is_legacy_gamma(crtc_state))
+		gamma_mode |= GAMMA_MODE_MODE_8BIT;
+	else
+		gamma_mode |= GAMMA_MODE_MODE_10BIT;
+
+	return gamma_mode;
+}
+
+static u32 icl_csc_mode(const struct intel_crtc_state *crtc_state)
+{
+	u32 csc_mode = 0;
+
+	if (crtc_state->base.ctm)
+		csc_mode |= ICL_CSC_ENABLE;
+
+	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+	    crtc_state->limited_color_range)
+		csc_mode |= ICL_OUTPUT_CSC_ENABLE;
+
+	return csc_mode;
+}
+
+static int icl_color_check(struct intel_crtc_state *crtc_state)
+{
+	int ret;
+
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
+
+	crtc_state->gamma_mode = icl_gamma_mode(crtc_state);
+
+	crtc_state->csc_mode = icl_csc_mode(crtc_state);
+
+	return 0;
+}
+
 static int _intel_color_check(struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
@@ -903,10 +952,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state)
 	if (!crtc_state->gamma_enable ||
 	    crtc_state_is_legacy_gamma(crtc_state))
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
-	else if (INTEL_GEN(dev_priv) >= 11)
-		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
-					 PRE_CSC_GAMMA_ENABLE |
-					 POST_CSC_GAMMA_ENABLE;
 	else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
 	else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
@@ -914,18 +959,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state)
 	else
 		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
 
-	if (INTEL_GEN(dev_priv) >= 11) {
-		if (crtc_state->gamma_enable)
-			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
-
-		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
-		    crtc_state->limited_color_range)
-			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
-
-		if (crtc_state->base.ctm)
-			crtc_state->csc_mode |= ICL_CSC_ENABLE;
-	}
-
 	return 0;
 }
 
@@ -962,7 +995,10 @@ void intel_color_init(struct intel_crtc *crtc)
 		else
 			dev_priv->display.color_commit = ilk_color_commit;
 
-		dev_priv->display.color_check = _intel_color_check;
+		if (INTEL_GEN(dev_priv) >= 11)
+			dev_priv->display.color_check = icl_color_check;
+		else
+			dev_priv->display.color_check = _intel_color_check;
 	}
 
 	/* Enable color management support when we have degamma & gamma LUTs. */
-- 
2.19.2

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

[PATCH 06/10] drm/i915: Extract glk_color_check()

[Ville Syrjala]

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

Unlike the earlier platforms GLK has dedicated degamma and gamma
LUTs. And quite curiously the degamma LUT is actually controlled
via the PLANE_COLOR_CTL CSC enable bit. Hence we must compute
gamma_enable and csc_enable differently to pre-GLK platforms.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 86580541b0da..e2a1c823650b 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -871,6 +871,44 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static u32 glk_gamma_mode(const struct intel_crtc_state *crtc_state)
+{
+	if (!crtc_state->gamma_enable ||
+	    crtc_state_is_legacy_gamma(crtc_state))
+		return GAMMA_MODE_MODE_8BIT;
+	else
+		return GAMMA_MODE_MODE_10BIT;
+}
+
+static int glk_color_check(struct intel_crtc_state *crtc_state)
+{
+	int ret;
+
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
+
+	crtc_state->gamma_enable =
+		crtc_state->base.gamma_lut &&
+		!crtc_state->c8_planes;
+
+	/* On GLK+ degamma LUT is controlled by csc_enable */
+	crtc_state->csc_enable =
+		crtc_state->base.degamma_lut ||
+		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+		crtc_state->base.ctm || crtc_state->limited_color_range;
+
+	crtc_state->gamma_mode = glk_gamma_mode(crtc_state);
+
+	crtc_state->csc_mode = 0;
+
+	ret = intel_color_add_affected_planes(crtc_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
 {
 	u32 gamma_mode = 0;
@@ -997,6 +1035,8 @@ void intel_color_init(struct intel_crtc *crtc)
 
 		if (INTEL_GEN(dev_priv) >= 11)
 			dev_priv->display.color_check = icl_color_check;
+		else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+			dev_priv->display.color_check = glk_color_check;
 		else
 			dev_priv->display.color_check = _intel_color_check;
 	}
-- 
2.19.2

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

[PATCH 07/10] drm/i915: Extract bdw_color_check()

[Ville Syrjala]

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

Provide a separate .color_check() for BDW+ where we currently
provide the split gamma mode etc.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index e2a1c823650b..6507e5f79c06 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -871,6 +871,43 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static u32 bdw_gamma_mode(const struct intel_crtc_state *crtc_state)
+{
+	if (!crtc_state->gamma_enable ||
+	    crtc_state_is_legacy_gamma(crtc_state))
+		return GAMMA_MODE_MODE_8BIT;
+	else
+		return GAMMA_MODE_MODE_SPLIT;
+}
+
+static int bdw_color_check(struct intel_crtc_state *crtc_state)
+{
+	int ret;
+
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
+
+	crtc_state->gamma_enable =
+		(crtc_state->base.gamma_lut ||
+		 crtc_state->base.degamma_lut) &&
+		!crtc_state->c8_planes;
+
+	crtc_state->csc_enable =
+		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
+		crtc_state->base.ctm || crtc_state->limited_color_range;
+
+	crtc_state->gamma_mode = bdw_gamma_mode(crtc_state);
+
+	crtc_state->csc_mode = 0;
+
+	ret = intel_color_add_affected_planes(crtc_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u32 glk_gamma_mode(const struct intel_crtc_state *crtc_state)
 {
 	if (!crtc_state->gamma_enable ||
@@ -1037,6 +1074,8 @@ void intel_color_init(struct intel_crtc *crtc)
 			dev_priv->display.color_check = icl_color_check;
 		else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 			dev_priv->display.color_check = glk_color_check;
+		else if (INTEL_GEN(dev_priv) >= 8)
+			dev_priv->display.color_check = bdw_color_check;
 		else
 			dev_priv->display.color_check = _intel_color_check;
 	}
-- 
2.19.2

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

[PATCH 08/10] drm/i915: Extract ilk_color_check()

[Ville Syrjala]

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

With everything else moved out of the way only ilk+
remains using _intel_color_check(). Streamline the logic
into ilk_color_check().

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 6507e5f79c06..8c5c66f5d100 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -871,6 +871,32 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
+static int ilk_color_check(struct intel_crtc_state *crtc_state)
+{
+	int ret;
+
+	ret = check_luts(crtc_state);
+	if (ret)
+		return ret;
+
+	crtc_state->gamma_enable =
+		crtc_state->base.gamma_lut &&
+		!crtc_state->c8_planes;
+
+	crtc_state->csc_enable =
+		ilk_csc_limited_range(crtc_state);
+
+	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
+
+	crtc_state->csc_mode = 0;
+
+	ret = intel_color_add_affected_planes(crtc_state);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static u32 bdw_gamma_mode(const struct intel_crtc_state *crtc_state)
 {
 	if (!crtc_state->gamma_enable ||
@@ -995,48 +1021,6 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
 	return 0;
 }
 
-static int _intel_color_check(struct intel_crtc_state *crtc_state)
-{
-	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
-	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
-	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
-	bool limited_color_range = false;
-	int ret;
-
-	ret = check_luts(crtc_state);
-	if (ret)
-		return ret;
-
-	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
-		!crtc_state->c8_planes;
-
-	if (INTEL_GEN(dev_priv) >= 9 ||
-	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
-		limited_color_range = crtc_state->limited_color_range;
-
-	crtc_state->csc_enable =
-		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
-		crtc_state->base.ctm || limited_color_range;
-
-	ret = intel_color_add_affected_planes(crtc_state);
-	if (ret)
-		return ret;
-
-	crtc_state->csc_mode = 0;
-
-	if (!crtc_state->gamma_enable ||
-	    crtc_state_is_legacy_gamma(crtc_state))
-		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
-	else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
-	else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
-		crtc_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
-	else
-		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
-
-	return 0;
-}
-
 void intel_color_init(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
@@ -1077,7 +1061,7 @@ void intel_color_init(struct intel_crtc *crtc)
 		else if (INTEL_GEN(dev_priv) >= 8)
 			dev_priv->display.color_check = bdw_color_check;
 		else
-			dev_priv->display.color_check = _intel_color_check;
+			dev_priv->display.color_check = ilk_color_check;
 	}
 
 	/* Enable color management support when we have degamma & gamma LUTs. */
-- 
2.19.2

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

[PATCH 09/10] drm/i915: Drop the pointless linear legacy LUT load on CHV

[Ville Syrjala]

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

We now bypass the legacy LUT when it's not needed, so
no point in filling it up with a linear LUT.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 8c5c66f5d100..b84a2a3f5844 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -376,15 +376,6 @@ static void i9xx_load_luts_internal(const struct intel_crtc_state *crtc_state,
 				(drm_color_lut_extract(lut[i].green, 8) << 8) |
 				drm_color_lut_extract(lut[i].blue, 8);
 
-			if (HAS_GMCH(dev_priv))
-				I915_WRITE(PALETTE(pipe, i), word);
-			else
-				I915_WRITE(LGC_PALETTE(pipe, i), word);
-		}
-	} else {
-		for (i = 0; i < 256; i++) {
-			u32 word = (i << 16) | (i << 8) | i;
-
 			if (HAS_GMCH(dev_priv))
 				I915_WRITE(PALETTE(pipe, i), word);
 			else
@@ -643,7 +634,7 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
 	cherryview_load_csc_matrix(crtc_state);
 
 	if (crtc_state_is_legacy_gamma(crtc_state)) {
-		i9xx_load_luts_internal(crtc_state, gamma_lut);
+		i9xx_load_luts(crtc_state);
 		return;
 	}
 
@@ -682,12 +673,6 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
 			I915_WRITE(CGM_PIPE_GAMMA(pipe, i, 1), word1);
 		}
 	}
-
-	/*
-	 * Also program a linear LUT in the legacy block (behind the
-	 * CGM block).
-	 */
-	i9xx_load_luts_internal(crtc_state, NULL);
 }
 
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
-- 
2.19.2

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

[PATCH 10/10] drm/i915: Skip the linear degamma LUT load on ICL+

[Ville Syrjala]

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

Don't load the linear degamma LUT on ICL. The hardware no longer
has any silly linkages between the CSC enable and degamma LUT
enable so the degamma LUT is only needed when it's actually
enabled.

Also add comments to explain the situation on GLK.

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

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index b84a2a3f5844..95e44f80bd8a 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -273,6 +273,14 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state *crtc_state)
 				    ilk_csc_coeff_limited_range,
 				    ilk_csc_postoff_limited_range);
 	} else if (crtc_state->csc_enable) {
+		/*
+		 * On GLK+ both pipe CSC and degamma LUT are controlled
+		 * by csc_enable. Hence for the cases where the degama
+		 * LUT is needed but CSC is not we need to load an
+		 * identity matrix.
+		 */
+		WARN_ON(!IS_CANNONLAKE(dev_priv) && !IS_GEMINILAKE(dev_priv));
+
 		ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
 				    ilk_csc_coeff_identity,
 				    ilk_csc_off_zero);
@@ -559,6 +567,7 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 	const u32 lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
+	struct drm_color_lut *lut = crtc_state->base.degamma_lut->data;
 	u32 i;
 
 	/*
@@ -569,32 +578,48 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
 	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
 	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
 
-	if (crtc_state->base.degamma_lut) {
-		struct drm_color_lut *lut = crtc_state->base.degamma_lut->data;
+	for (i = 0; i < lut_size; i++) {
+		/*
+		 * First 33 entries represent range from 0 to 1.0
+		 * 34th and 35th entry will represent extended range
+		 * inputs 3.0 and 7.0 respectively, currently clamped
+		 * at 1.0. Since the precision is 16bit, the user
+		 * value can be directly filled to register.
+		 * The pipe degamma table in GLK+ onwards doesn't
+		 * support different values per channel, so this just
+		 * programs green value which will be equal to Red and
+		 * Blue into the lut registers.
+		 * ToDo: Extend to max 7.0. Enable 32 bit input value
+		 * as compared to just 16 to achieve this.
+		 */
+		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].green);
+	}
 
-		for (i = 0; i < lut_size; i++) {
-			/*
-			 * First 33 entries represent range from 0 to 1.0
-			 * 34th and 35th entry will represent extended range
-			 * inputs 3.0 and 7.0 respectively, currently clamped
-			 * at 1.0. Since the precision is 16bit, the user
-			 * value can be directly filled to register.
-			 * The pipe degamma table in GLK+ onwards doesn't
-			 * support different values per channel, so this just
-			 * programs green value which will be equal to Red and
-			 * Blue into the lut registers.
-			 * ToDo: Extend to max 7.0. Enable 32 bit input value
-			 * as compared to just 16 to achieve this.
-			 */
-			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].green);
-		}
-	} else {
-		/* load a linear table. */
-		for (i = 0; i < lut_size; i++) {
-			u32 v = (i * (1 << 16)) / (lut_size - 1);
+	/* Clamp values > 1.0. */
+	while (i++ < 35)
+		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
+}
 
-			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
-		}
+static void glk_load_degamma_lut_linear(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+	const u32 lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
+	u32 i;
+
+	/*
+	 * When setting the auto-increment bit, the hardware seems to
+	 * ignore the index bits, so we need to reset it to index 0
+	 * separately.
+	 */
+	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
+	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
+
+	for (i = 0; i < lut_size; i++) {
+		u32 v = (i  << 16) / (lut_size - 1);
+
+		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
 	}
 
 	/* Clamp values > 1.0. */
@@ -604,7 +629,18 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
 
 static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 {
-	glk_load_degamma_lut(crtc_state);
+	/*
+	 * On GLK+ both pipe CSC and degamma LUT are controlled
+	 * by csc_enable. Hence for the cases where the CSC is
+	 * needed but degamma LUT is not we need to load a
+	 * linear degamma LUT. In fact we'll just always load
+	 * the degama LUT so that we don't have to reload
+	 * it every time the pipe CSC is being enabled.
+	 */
+	if (crtc_state->base.degamma_lut)
+		glk_load_degamma_lut(crtc_state);
+	else
+		glk_load_degamma_lut_linear(crtc_state);
 
 	if (crtc_state_is_legacy_gamma(crtc_state))
 		i9xx_load_luts(crtc_state);
@@ -614,7 +650,8 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 
 static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 {
-	glk_load_degamma_lut(crtc_state);
+	if (crtc_state->base.degamma_lut)
+		glk_load_degamma_lut(crtc_state);
 
 	if (crtc_state_is_legacy_gamma(crtc_state))
 		i9xx_load_luts(crtc_state);
-- 
2.19.2

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

✗ Fi.CI.SPARSE: warning for drm/i915: Clean up intel_color_check()

[Patchwork]

== Series Details ==

Series: drm/i915: Clean up intel_color_check()
URL   : https://patchwork.freedesktop.org/series/58137/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Extract check_luts()
Okay!

Commit: drm/i915: Turn intel_color_check() into a vfunc
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3558:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3559:16: warning: expression using sizeof(void)

Commit: drm/i915: Extract i9xx_color_check()
Okay!

Commit: drm/i915: Extract chv_color_check()
Okay!

Commit: drm/i915: Extract icl_color_check()
Okay!

Commit: drm/i915: Extract glk_color_check()
Okay!

Commit: drm/i915: Extract bdw_color_check()
Okay!

Commit: drm/i915: Extract ilk_color_check()
Okay!

Commit: drm/i915: Drop the pointless linear legacy LUT load on CHV
Okay!

Commit: drm/i915: Skip the linear degamma LUT load on ICL+
Okay!

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

✓ Fi.CI.BAT: success for drm/i915: Clean up intel_color_check()

[Patchwork]

== Series Details ==

Series: drm/i915: Clean up intel_color_check()
URL   : https://patchwork.freedesktop.org/series/58137/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5768 -> Patchwork_12502
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58137/revisions/1/mbox/

Known issues
------------

  Here are the changes found in Patchwork_12502 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109315] +17

  * igt@gem_exec_basic@basic-vebox:
    - fi-ivb-3770:        NOTRUN -> SKIP [fdo#109271] +48

  * igt@gem_exec_basic@gtt-bsd1:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_basic@gtt-bsd2:
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] +57

  * igt@gem_exec_parse@basic-rejected:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109289] +1

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-byt-j1900:       PASS -> SKIP [fdo#109271]

  * igt@i915_pm_rpm@basic-rte:
    - fi-byt-j1900:       PASS -> FAIL [fdo#108800]

  * igt@i915_selftest@live_contexts:
    - fi-icl-u3:          NOTRUN -> INCOMPLETE [fdo#108569]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927] / [fdo#109720]

  * igt@kms_busy@basic-flip-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]
    - fi-byt-clapper:     NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109284] +8

  * igt@kms_force_connector_basic@prune-stale-modes:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          NOTRUN -> FAIL [fdo#103167]
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +48

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-byt-clapper:     NOTRUN -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
#### Warnings ####

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-kbl-7567u:       DMESG-FAIL [fdo#105079] -> DMESG-WARN [fdo#103558] / [fdo#105079] / [fdo#105602]

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105079]: https://bugs.freedesktop.org/show_bug.cgi?id=105079
  [fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108800]: https://bugs.freedesktop.org/show_bug.cgi?id=108800
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720


Participating hosts (44 -> 40)
------------------------------

  Additional (3): fi-byt-clapper fi-ivb-3770 fi-icl-u3 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-gdg-551 fi-icl-y fi-bdw-samus 


Build changes
-------------

    * Linux: CI_DRM_5768 -> Patchwork_12502

  CI_DRM_5768: e9ff09b836a2473adacf7eb108af0348d91cd136 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4888: 71ad19eb8fe4f0eecae3bf063e107293b90b9abc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12502: 08d701b4cdf5e5d9231607b4d8d6188cbbe7110d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

08d701b4cdf5 drm/i915: Skip the linear degamma LUT load on ICL+
b1b50e6311a9 drm/i915: Drop the pointless linear legacy LUT load on CHV
907213c3a267 drm/i915: Extract ilk_color_check()
ad915e3a39cd drm/i915: Extract bdw_color_check()
86417f30d6cd drm/i915: Extract glk_color_check()
807ee50dc8d0 drm/i915: Extract icl_color_check()
623ba8036501 drm/i915: Extract chv_color_check()
b006bc4da413 drm/i915: Extract i9xx_color_check()
89aad101ba23 drm/i915: Turn intel_color_check() into a vfunc
ec05d6616fb3 drm/i915: Extract check_luts()

== Logs ==

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

✗ Fi.CI.IGT: failure for drm/i915: Clean up intel_color_check()

[Patchwork]

== Series Details ==

Series: drm/i915: Clean up intel_color_check()
URL   : https://patchwork.freedesktop.org/series/58137/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5768_full -> Patchwork_12502_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12502_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12502_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12502_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_create@create-clear:
    - shard-hsw:          PASS -> DMESG-WARN

  * igt@runner@aborted:
    - shard-hsw:          NOTRUN -> FAIL

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * {igt@kms_plane@pixel-format-pipe-b-planes}:
    - shard-iclb:         PASS -> FAIL

  
Known issues
------------

  Here are the changes found in Patchwork_12502_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@fifo-bsd2:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +74

  * igt@gem_ppgtt@blt-vs-render-ctx0:
    - shard-iclb:         PASS -> INCOMPLETE [fdo#109766] / [fdo#109801]

  * igt@gem_pread@stolen-uncached:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +21

  * igt@gem_tiled_pread_pwrite:
    - shard-iclb:         PASS -> TIMEOUT [fdo#109673]

  * igt@gem_wait@write-busy-bsd2:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109276]

  * igt@i915_pm_rpm@debugfs-read:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  * igt@i915_pm_rpm@fences:
    - shard-skl:          NOTRUN -> INCOMPLETE [fdo#107807]

  * igt@i915_pm_rpm@system-suspend-devices:
    - shard-iclb:         PASS -> DMESG-WARN [fdo#109638]

  * igt@kms_atomic_transition@6x-modeset-transitions-nonblocking:
    - shard-hsw:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-hsw:          PASS -> FAIL [fdo#106641]
    - shard-snb:          PASS -> FAIL [fdo#106641]

  * igt@kms_busy@extended-modeset-hang-oldfb-render-d:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-c:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +9

  * igt@kms_busy@extended-pageflip-hang-newfb-render-f:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109278]

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_chamelium@hdmi-crc-abgr8888:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109284] +1

  * igt@kms_color@pipe-a-degamma:
    - shard-apl:          PASS -> FAIL [fdo#104782] / [fdo#108145]

  * igt@kms_color@pipe-c-degamma:
    - shard-iclb:         NOTRUN -> FAIL [fdo#104782]

  * igt@kms_content_protection@legacy:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108597] / [fdo#108739]

  * igt@kms_cursor_crc@cursor-128x42-offscreen:
    - shard-skl:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x256-sliding:
    - shard-apl:          PASS -> FAIL [fdo#103232]

  * igt@kms_cursor_legacy@cursor-vs-flip-varying-size:
    - shard-iclb:         PASS -> FAIL [fdo#103355] +1

  * igt@kms_flip@2x-modeset-vs-vblank-race:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109274]

  * igt@kms_flip_tiling@flip-x-tiled:
    - shard-iclb:         PASS -> FAIL [fdo#108303]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
    - shard-apl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-iclb:         PASS -> FAIL [fdo#103167] +10

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-mmap-gtt:
    - shard-iclb:         PASS -> FAIL [fdo#109247] +13

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-blt:
    - shard-skl:          NOTRUN -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-primscrn-pri-shrfb-draw-mmap-gtt:
    - shard-iclb:         NOTRUN -> SKIP [fdo#109280] +1

  * igt@kms_frontbuffer_tracking@fbcpsr-rgb101010-draw-mmap-wc:
    - shard-iclb:         PASS -> FAIL [fdo#105682] / [fdo#109247]

  * igt@kms_frontbuffer_tracking@psr-2p-scndscrn-cur-indfb-draw-blt:
    - shard-hsw:          NOTRUN -> SKIP [fdo#109271] +9

  * igt@kms_frontbuffer_tracking@psr-rgb565-draw-mmap-cpu:
    - shard-skl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          PASS -> FAIL [fdo#107815] / [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-max:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          PASS -> FAIL [fdo#108145]

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         PASS -> SKIP [fdo#109642]

  * igt@kms_psr@psr2_cursor_plane_onoff:
    - shard-iclb:         PASS -> SKIP [fdo#109441] +1

  * igt@kms_psr@sprite_mmap_cpu:
    - shard-iclb:         PASS -> FAIL [fdo#107383] +1

  * igt@kms_rotation_crc@multiplane-rotation-cropping-bottom:
    - shard-kbl:          PASS -> DMESG-FAIL [fdo#105763]

  * igt@kms_universal_plane@disable-primary-vs-flip-pipe-f:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +3

  * igt@kms_vblank@pipe-c-ts-continuation-modeset:
    - shard-kbl:          NOTRUN -> FAIL [fdo#104894]

  * igt@prime_vgem@sync-bsd2:
    - shard-skl:          NOTRUN -> SKIP [fdo#109271] +41

  * igt@runner@aborted:
    - shard-apl:          NOTRUN -> ( 3 FAIL ) [fdo#109373]

  
#### Possible fixes ####

  * igt@gem_media_vme:
    - shard-glk:          FAIL -> PASS

  * igt@gem_partial_pwrite_pread@write-uncached:
    - shard-iclb:         TIMEOUT [fdo#109673] -> PASS

  * igt@gem_ppgtt@blt-vs-render-ctxn:
    - shard-iclb:         DMESG-WARN [fdo#109801] -> PASS

  * igt@i915_pm_rpm@sysfs-read:
    - shard-skl:          INCOMPLETE [fdo#107807] -> PASS +1

  * igt@i915_selftest@live_workarounds:
    - shard-iclb:         DMESG-FAIL [fdo#108954] -> PASS

  * igt@kms_color@pipe-a-ctm-max:
    - shard-skl:          FAIL [fdo#108147] -> PASS

  * igt@kms_color@pipe-a-degamma:
    - shard-glk:          FAIL [fdo#104782] / [fdo#108145] -> PASS

  * igt@kms_color@pipe-c-degamma:
    - shard-glk:          FAIL [fdo#104782] -> PASS +1

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +1

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy:
    - shard-hsw:          FAIL [fdo#105767] -> PASS

  * igt@kms_cursor_legacy@cursor-vs-flip-legacy:
    - shard-iclb:         FAIL [fdo#103355] -> PASS

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          FAIL [fdo#105363] -> PASS

  * igt@kms_flip@2x-plain-flip-fb-recreate:
    - shard-glk:          FAIL [fdo#100368] -> PASS

  * igt@kms_flip@flip-vs-blocking-wf-vblank:
    - shard-skl:          FAIL [fdo#100368] -> PASS

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          FAIL [fdo#105363] -> PASS

  * igt@kms_flip@flip-vs-suspend:
    - shard-skl:          INCOMPLETE [fdo#107773] / [fdo#109507] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-move:
    - shard-glk:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-iclb:         FAIL [fdo#103167] -> PASS +6

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-indfb-draw-pwrite:
    - shard-iclb:         FAIL [fdo#109247] -> PASS +19

  * {igt@kms_plane@pixel-format-pipe-a-planes}:
    - shard-iclb:         FAIL [fdo#110190] -> PASS

  * {igt@kms_plane@pixel-format-pipe-b-planes}:
    - shard-glk:          SKIP [fdo#109271] -> PASS

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-skl:          INCOMPLETE [fdo#104108] -> PASS

  * {igt@kms_plane_multiple@atomic-pipe-c-tiling-x}:
    - shard-glk:          FAIL [fdo#110037] -> PASS

  * {igt@kms_plane_multiple@atomic-pipe-c-tiling-yf}:
    - shard-apl:          FAIL [fdo#110037] -> PASS +2
    - shard-iclb:         FAIL [fdo#110037] -> PASS +1

  * igt@kms_psr@no_drrs:
    - shard-iclb:         FAIL [fdo#108341] -> PASS

  * igt@kms_psr@psr2_sprite_render:
    - shard-iclb:         SKIP [fdo#109441] -> PASS +1

  * igt@kms_psr@sprite_blt:
    - shard-iclb:         FAIL [fdo#107383] -> PASS +3

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-hsw:          INCOMPLETE [fdo#103540] -> PASS

  
#### Warnings ####

  * igt@kms_plane_scaling@pipe-c-scaler-with-pixel-format:
    - shard-glk:          SKIP [fdo#109271] / [fdo#109278] -> FAIL [fdo#110098]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105763]: https://bugs.freedesktop.org/show_bug.cgi?id=105763
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#107383]: https://bugs.freedesktop.org/show_bug.cgi?id=107383
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107815]: https://bugs.freedesktop.org/show_bug.cgi?id=107815
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108303]: https://bugs.freedesktop.org/show_bug.cgi?id=108303
  [fdo#108341]: https://bugs.freedesktop.org/show_bug.cgi?id=108341
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#108597]: https://bugs.freedesktop.org/show_bug.cgi?id=108597
  [fdo#108739]: https://bugs.freedesktop.org/show_bug.cgi?id=108739
  [fdo#108954]: https://bugs.freedesktop.org/show_bug.cgi?id=108954
  [fdo#109247]: https://bugs.freedesktop.org/show_bug.cgi?id=109247
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109373]: https://bugs.freedesktop.org/show_bug.cgi?id=109373
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109507]: https://bugs.freedesktop.org/show_bug.cgi?id=109507
  [fdo#109638]: https://bugs.freedesktop.org/show_bug.cgi?id=109638
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#109766]: https://bugs.freedesktop.org/show_bug.cgi?id=109766
  [fdo#109801]: https://bugs.freedesktop.org/show_bug.cgi?id=109801
  [fdo#110037]: https://bugs.freedesktop.org/show_bug.cgi?id=110037
  [fdo#110098]: https://bugs.freedesktop.org/show_bug.cgi?id=110098
  [fdo#110190]: https://bugs.freedesktop.org/show_bug.cgi?id=110190


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

    * Linux: CI_DRM_5768 -> Patchwork_12502

  CI_DRM_5768: e9ff09b836a2473adacf7eb108af0348d91cd136 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4888: 71ad19eb8fe4f0eecae3bf063e107293b90b9abc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12502: 08d701b4cdf5e5d9231607b4d8d6188cbbe7110d @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Jani Nikula]

On Mon, 18 Mar 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> In prepartion for per-platform color_check() functions extract the
> common code into a separate function.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 467fd1a1630c..b6a992a5b14f 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
>  	return 0;
>  }
>  
> +static int check_luts(const struct intel_crtc_state *crtc_state)

luts_check() to be more in line with other _commit and _check functions?

BR,
Jani.

> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> +	int gamma_length, degamma_length;
> +	u32 gamma_tests, degamma_tests;
> +
> +	/* Always allow legacy gamma LUT with no further checking. */
> +	if (crtc_state_is_legacy_gamma(crtc_state))
> +		return 0;
> +
> +	/* C8 needs the legacy LUT all to itself */
> +	if (crtc_state->c8_planes)
> +		return -EINVAL;
> +
> +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> +
> +	if (check_lut_size(degamma_lut, degamma_length) ||
> +	    check_lut_size(gamma_lut, gamma_length))
> +		return -EINVAL;
> +
> +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> +	    drm_color_lut_check(gamma_lut, gamma_tests))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 cgm_mode = 0;
> @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
>  	bool limited_color_range = false;
> -	int gamma_length, degamma_length;
> -	u32 gamma_tests, degamma_tests;
>  	int ret;
>  
> -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> -
> -	/* C8 needs the legacy LUT all to itself */
> -	if (crtc_state->c8_planes &&
> -	    !crtc_state_is_legacy_gamma(crtc_state))
> -		return -EINVAL;
> +	ret = check_luts(crtc_state);
> +	if (ret)
> +		return ret;
>  
>  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
>  		!crtc_state->c8_planes;
> @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	if (IS_CHERRYVIEW(dev_priv))
>  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
>  
> -	/* Always allow legacy gamma LUT with no further checking. */
>  	if (!crtc_state->gamma_enable ||
> -	    crtc_state_is_legacy_gamma(crtc_state)) {
> +	    crtc_state_is_legacy_gamma(crtc_state))
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> -		if (INTEL_GEN(dev_priv) >= 11 &&
> -		    crtc_state->gamma_enable)
> -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> -		return 0;
> -	}
> -
> -	if (check_lut_size(degamma_lut, degamma_length) ||
> -	    check_lut_size(gamma_lut, gamma_length))
> -		return -EINVAL;
> -
> -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> -	    drm_color_lut_check(gamma_lut, gamma_tests))
> -		return -EINVAL;
> -
> -	if (INTEL_GEN(dev_priv) >= 11)
> +	else if (INTEL_GEN(dev_priv) >= 11)
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
>  					 PRE_CSC_GAMMA_ENABLE |
>  					 POST_CSC_GAMMA_ENABLE;
> @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> +		if (crtc_state->gamma_enable)
> +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> +
>  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
>  		    crtc_state->limited_color_range)
>  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/10] drm/i915: Clean up intel_color_check()

[Jani Nikula]

On Mon, 18 Mar 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> intel_color_check() is mess. Blow it up into clean platform sized
> chunks. Also eliminate some redundant linear LUT loads as a result.

How about a "load lut" -> "write lut" rename while at it? I think
"loading" is confusing.

BR,
Jani.


>
> Ville Syrjälä (10):
>   drm/i915: Extract check_luts()
>   drm/i915: Turn intel_color_check() into a vfunc
>   drm/i915: Extract i9xx_color_check()
>   drm/i915: Extract chv_color_check()
>   drm/i915: Extract icl_color_check()
>   drm/i915: Extract glk_color_check()
>   drm/i915: Extract bdw_color_check()
>   drm/i915: Extract ilk_color_check()
>   drm/i915: Drop the pointless linear legacy LUT load on CHV
>   drm/i915: Skip the linear degamma LUT load on ICL+
>
>  drivers/gpu/drm/i915/i915_drv.h    |   1 +
>  drivers/gpu/drm/i915/intel_color.c | 384 +++++++++++++++++++++--------
>  2 files changed, 286 insertions(+), 99 deletions(-)

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/10] drm/i915: Clean up intel_color_check()

[Ville Syrjälä]

On Tue, Mar 19, 2019 at 06:26:43PM +0200, Jani Nikula wrote:
> On Mon, 18 Mar 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > intel_color_check() is mess. Blow it up into clean platform sized
> > chunks. Also eliminate some redundant linear LUT loads as a result.
> 
> How about a "load lut" -> "write lut" rename while at it? I think
> "loading" is confusing.

Should probably rename it something a bit more generic since vlv/chv
load the csc there too. .color_update_postvbl() or something, though
I'm not sure that shade really makes me happy either.

> 
> BR,
> Jani.
> 
> 
> >
> > Ville Syrjälä (10):
> >   drm/i915: Extract check_luts()
> >   drm/i915: Turn intel_color_check() into a vfunc
> >   drm/i915: Extract i9xx_color_check()
> >   drm/i915: Extract chv_color_check()
> >   drm/i915: Extract icl_color_check()
> >   drm/i915: Extract glk_color_check()
> >   drm/i915: Extract bdw_color_check()
> >   drm/i915: Extract ilk_color_check()
> >   drm/i915: Drop the pointless linear legacy LUT load on CHV
> >   drm/i915: Skip the linear degamma LUT load on ICL+
> >
> >  drivers/gpu/drm/i915/i915_drv.h    |   1 +
> >  drivers/gpu/drm/i915/intel_color.c | 384 +++++++++++++++++++++--------
> >  2 files changed, 286 insertions(+), 99 deletions(-)
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> In prepartion for per-platform color_check() functions extract the
> common code into a separate function.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
>  1 file changed, 40 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 467fd1a1630c..b6a992a5b14f 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
>  	return 0;
>  }
>  
> +static int check_luts(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> +	int gamma_length, degamma_length;
> +	u32 gamma_tests, degamma_tests;
> +
> +	/* Always allow legacy gamma LUT with no further checking. */
> +	if (crtc_state_is_legacy_gamma(crtc_state))
> +		return 0;
> +
> +	/* C8 needs the legacy LUT all to itself */
> +	if (crtc_state->c8_planes)
> +		return -EINVAL;

Do we need to test for non-NULL gamma_lut here?

The logic is the same as before your patch, so either we already had a
pre-existing bug or, more likely, I'm overlooking something obvious.

> +
> +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> +
> +	if (check_lut_size(degamma_lut, degamma_length) ||
> +	    check_lut_size(gamma_lut, gamma_length))
> +		return -EINVAL;
> +
> +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> +	    drm_color_lut_check(gamma_lut, gamma_tests))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 cgm_mode = 0;
> @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
>  	bool limited_color_range = false;
> -	int gamma_length, degamma_length;
> -	u32 gamma_tests, degamma_tests;
>  	int ret;
>  
> -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> -
> -	/* C8 needs the legacy LUT all to itself */
> -	if (crtc_state->c8_planes &&
> -	    !crtc_state_is_legacy_gamma(crtc_state))
> -		return -EINVAL;
> +	ret = check_luts(crtc_state);
> +	if (ret)
> +		return ret;
>  
>  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
>  		!crtc_state->c8_planes;
> @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  	if (IS_CHERRYVIEW(dev_priv))
>  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
>  
> -	/* Always allow legacy gamma LUT with no further checking. */
>  	if (!crtc_state->gamma_enable ||
> -	    crtc_state_is_legacy_gamma(crtc_state)) {
> +	    crtc_state_is_legacy_gamma(crtc_state))
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> -		if (INTEL_GEN(dev_priv) >= 11 &&
> -		    crtc_state->gamma_enable)
> -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> -		return 0;
> -	}
> -
> -	if (check_lut_size(degamma_lut, degamma_length) ||
> -	    check_lut_size(gamma_lut, gamma_length))
> -		return -EINVAL;
> -
> -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> -	    drm_color_lut_check(gamma_lut, gamma_tests))
> -		return -EINVAL;
> -
> -	if (INTEL_GEN(dev_priv) >= 11)
> +	else if (INTEL_GEN(dev_priv) >= 11)
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
>  					 PRE_CSC_GAMMA_ENABLE |
>  					 POST_CSC_GAMMA_ENABLE;

Should we drop the GAMMA_ENABLE here and just rely on the new check
you're adding below to handle it?


Matt

> @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
>  
>  	if (INTEL_GEN(dev_priv) >= 11) {
> +		if (crtc_state->gamma_enable)
> +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> +
>  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
>  		    crtc_state->limited_color_range)
>  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/10] drm/i915: Turn intel_color_check() into a vfunc

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:09PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The current intel_color_check() is a mess, and worse yet it is
> in fact incorrect for several platforms. The hardware has
> evolved quite a bit over the years, so let's just go for a clean
> split between the platforms by turning this into a vfunc.
> The actual work to split it up will follow.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_color.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c65c2e6649df..38dfeb110dd4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -320,6 +320,7 @@ struct drm_i915_display_funcs {
>  	/* display clock increase/decrease */
>  	/* pll clock increase/decrease */
>  
> +	int (*color_check)(struct intel_crtc_state *crtc_state);
>  	/*
>  	 * Program double buffered color management registers during
>  	 * vblank evasion. The registers should then latch during the
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index b6a992a5b14f..e68c936c784b 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -704,6 +704,13 @@ void intel_color_commit(const struct intel_crtc_state *crtc_state)
>  	dev_priv->display.color_commit(crtc_state);
>  }
>  
> +int intel_color_check(struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +
> +	return dev_priv->display.color_check(crtc_state);
> +}
> +
>  static bool need_plane_update(struct intel_plane *plane,
>  			      const struct intel_crtc_state *crtc_state)
>  {
> @@ -820,7 +827,7 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
>  	return cgm_mode;
>  }
>  
> -int intel_color_check(struct intel_crtc_state *crtc_state)
> +static int _intel_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
>  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> @@ -894,6 +901,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  			dev_priv->display.load_luts = i9xx_load_luts;
>  
>  		dev_priv->display.color_commit = i9xx_color_commit;
> +		dev_priv->display.color_check = _intel_color_check;

It might look a bit nicer to assign these in the same order they show up
in the vfunc structure (check first, commit second).  I think your later
patches wind up eventually reversing the order anyway.

But either way,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

>  	} else {
>  		if (INTEL_GEN(dev_priv) >= 11)
>  			dev_priv->display.load_luts = icl_load_luts;
> @@ -910,6 +918,8 @@ void intel_color_init(struct intel_crtc *crtc)
>  			dev_priv->display.color_commit = hsw_color_commit;
>  		else
>  			dev_priv->display.color_commit = ilk_color_commit;
> +
> +		dev_priv->display.color_check = _intel_color_check;
>  	}
>  
>  	/* Enable color management support when we have degamma & gamma LUTs. */
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/10] drm/i915: Extract i9xx_color_check()

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:10PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apart from CHV the other gmch platforms don't currently
> require much work in .color_check(). So let's start by
> extracting i9xx_color_check().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_color.c | 33 +++++++++++++++++++++++++-----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index e68c936c784b..3d19b1a7ee95 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -810,6 +810,27 @@ static int check_luts(const struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int i9xx_color_check(struct intel_crtc_state *crtc_state)
> +{
> +	int ret;
> +
> +	ret = check_luts(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state->gamma_enable =
> +		crtc_state->base.gamma_lut &&
> +		!crtc_state->c8_planes;
> +
> +	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> +
> +	ret = intel_color_add_affected_planes(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
>  {
>  	u32 cgm_mode = 0;
> @@ -895,13 +916,15 @@ void intel_color_init(struct intel_crtc *crtc)
>  	drm_mode_crtc_set_gamma_size(&crtc->base, 256);
>  
>  	if (HAS_GMCH(dev_priv)) {
> -		if (IS_CHERRYVIEW(dev_priv))
> +		if (IS_CHERRYVIEW(dev_priv)) {
> +			dev_priv->display.color_check = _intel_color_check;
> +			dev_priv->display.color_commit = i9xx_color_commit;
>  			dev_priv->display.load_luts = cherryview_load_luts;
> -		else
> +		} else {
> +			dev_priv->display.color_check = i9xx_color_check;
> +			dev_priv->display.color_commit = i9xx_color_commit;
>  			dev_priv->display.load_luts = i9xx_load_luts;
> -
> -		dev_priv->display.color_commit = i9xx_color_commit;
> -		dev_priv->display.color_check = _intel_color_check;
> +		}
>  	} else {
>  		if (INTEL_GEN(dev_priv) >= 11)
>  			dev_priv->display.load_luts = icl_load_luts;
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/10] drm/i915: Extract chv_color_check()

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:11PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since CHV has the CGM unit we require a custom implementation
> of .color_check().
> 
> This fixes the computation of gamma_enable as previously we
> left it enabled even when were using the CGM gamma instead.
> Now we turn off the legacy LUT unless it's actually required.

The CGM stuff is a bit non-obvious to anyone who hasn't focused on the
CHV platform specifically, so it might be worth adding this
justification in a code comment.

In fact, a general overview comment somewhere (maybe on the CHV_COLORS
definition?) about the difference between legacy gamma and CGM might be
helpful.  It looks like our public PRM's for CHV don't really cover
anything display-wise.

Anyway, the code change looks correct, so

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 3d19b1a7ee95..cb88651f1da4 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -848,6 +848,29 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
>  	return cgm_mode;
>  }
>  
> +static int chv_color_check(struct intel_crtc_state *crtc_state)
> +{
> +	int ret;
> +
> +	ret = check_luts(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state->gamma_enable =
> +		crtc_state_is_legacy_gamma(crtc_state) &&
> +		!crtc_state->c8_planes;
> +
> +	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> +
> +	crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> +
> +	ret = intel_color_add_affected_planes(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int _intel_color_check(struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> @@ -877,9 +900,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state)
>  
>  	crtc_state->csc_mode = 0;
>  
> -	if (IS_CHERRYVIEW(dev_priv))
> -		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> -
>  	if (!crtc_state->gamma_enable ||
>  	    crtc_state_is_legacy_gamma(crtc_state))
>  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> @@ -917,7 +937,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  
>  	if (HAS_GMCH(dev_priv)) {
>  		if (IS_CHERRYVIEW(dev_priv)) {
> -			dev_priv->display.color_check = _intel_color_check;
> +			dev_priv->display.color_check = chv_color_check;
>  			dev_priv->display.color_commit = i9xx_color_commit;
>  			dev_priv->display.load_luts = cherryview_load_luts;
>  		} else {
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Ville Syrjälä]

On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote:
> On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > In prepartion for per-platform color_check() functions extract the
> > common code into a separate function.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
> >  1 file changed, 40 insertions(+), 28 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 467fd1a1630c..b6a992a5b14f 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
> >  	return 0;
> >  }
> >  
> > +static int check_luts(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > +	int gamma_length, degamma_length;
> > +	u32 gamma_tests, degamma_tests;
> > +
> > +	/* Always allow legacy gamma LUT with no further checking. */
> > +	if (crtc_state_is_legacy_gamma(crtc_state))
> > +		return 0;
> > +
> > +	/* C8 needs the legacy LUT all to itself */
> > +	if (crtc_state->c8_planes)
> > +		return -EINVAL;
> 
> Do we need to test for non-NULL gamma_lut here?

For c8? We already checked crtc_state_is_legacy_gamma() above.

> 
> The logic is the same as before your patch, so either we already had a
> pre-existing bug or, more likely, I'm overlooking something obvious.
> 
> > +
> > +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > +
> > +	if (check_lut_size(degamma_lut, degamma_length) ||
> > +	    check_lut_size(gamma_lut, gamma_length))
> > +		return -EINVAL;
> > +
> > +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > +	    drm_color_lut_check(gamma_lut, gamma_tests))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
> >  {
> >  	u32 cgm_mode = 0;
> > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> >  	bool limited_color_range = false;
> > -	int gamma_length, degamma_length;
> > -	u32 gamma_tests, degamma_tests;
> >  	int ret;
> >  
> > -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > -
> > -	/* C8 needs the legacy LUT all to itself */
> > -	if (crtc_state->c8_planes &&
> > -	    !crtc_state_is_legacy_gamma(crtc_state))
> > -		return -EINVAL;
> > +	ret = check_luts(crtc_state);
> > +	if (ret)
> > +		return ret;
> >  
> >  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> >  		!crtc_state->c8_planes;
> > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  	if (IS_CHERRYVIEW(dev_priv))
> >  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> >  
> > -	/* Always allow legacy gamma LUT with no further checking. */
> >  	if (!crtc_state->gamma_enable ||
> > -	    crtc_state_is_legacy_gamma(crtc_state)) {
> > +	    crtc_state_is_legacy_gamma(crtc_state))
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > -		if (INTEL_GEN(dev_priv) >= 11 &&
> > -		    crtc_state->gamma_enable)
> > -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > -		return 0;
> > -	}
> > -
> > -	if (check_lut_size(degamma_lut, degamma_length) ||
> > -	    check_lut_size(gamma_lut, gamma_length))
> > -		return -EINVAL;
> > -
> > -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > -	    drm_color_lut_check(gamma_lut, gamma_tests))
> > -		return -EINVAL;
> > -
> > -	if (INTEL_GEN(dev_priv) >= 11)
> > +	else if (INTEL_GEN(dev_priv) >= 11)
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
> >  					 PRE_CSC_GAMMA_ENABLE |
> >  					 POST_CSC_GAMMA_ENABLE;
> 
> Should we drop the GAMMA_ENABLE here and just rely on the new check
> you're adding below to handle it?

This will get rewritten entirely in following patches, so I figured I'd
just keep this patch minimal.

> 
> 
> Matt
> 
> > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 11) {
> > +		if (crtc_state->gamma_enable)
> > +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > +
> >  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> >  		    crtc_state->limited_color_range)
> >  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 04/10] drm/i915: Extract chv_color_check()

[Ville Syrjälä]

On Tue, Mar 19, 2019 at 03:00:26PM -0700, Matt Roper wrote:
> On Mon, Mar 18, 2019 at 06:13:11PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Since CHV has the CGM unit we require a custom implementation
> > of .color_check().
> > 
> > This fixes the computation of gamma_enable as previously we
> > left it enabled even when were using the CGM gamma instead.
> > Now we turn off the legacy LUT unless it's actually required.
> 
> The CGM stuff is a bit non-obvious to anyone who hasn't focused on the
> CHV platform specifically, so it might be worth adding this
> justification in a code comment.
> 
> In fact, a general overview comment somewhere (maybe on the CHV_COLORS
> definition?) about the difference between legacy gamma and CGM might be
> helpful.  It looks like our public PRM's for CHV don't really cover
> anything display-wise.

Yeah. I think I had a patch somewhere where I document the full 
pipeline for VLV/CHV. I'll see if I can dig it out.

> 
> Anyway, the code change looks correct, so
> 
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> 
> Matt
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c | 28 ++++++++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 3d19b1a7ee95..cb88651f1da4 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -848,6 +848,29 @@ static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
> >  	return cgm_mode;
> >  }
> >  
> > +static int chv_color_check(struct intel_crtc_state *crtc_state)
> > +{
> > +	int ret;
> > +
> > +	ret = check_luts(crtc_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	crtc_state->gamma_enable =
> > +		crtc_state_is_legacy_gamma(crtc_state) &&
> > +		!crtc_state->c8_planes;
> > +
> > +	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > +
> > +	crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> > +
> > +	ret = intel_color_add_affected_planes(crtc_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> >  static int _intel_color_check(struct intel_crtc_state *crtc_state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > @@ -877,9 +900,6 @@ static int _intel_color_check(struct intel_crtc_state *crtc_state)
> >  
> >  	crtc_state->csc_mode = 0;
> >  
> > -	if (IS_CHERRYVIEW(dev_priv))
> > -		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> > -
> >  	if (!crtc_state->gamma_enable ||
> >  	    crtc_state_is_legacy_gamma(crtc_state))
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > @@ -917,7 +937,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >  
> >  	if (HAS_GMCH(dev_priv)) {
> >  		if (IS_CHERRYVIEW(dev_priv)) {
> > -			dev_priv->display.color_check = _intel_color_check;
> > +			dev_priv->display.color_check = chv_color_check;
> >  			dev_priv->display.color_commit = i9xx_color_commit;
> >  			dev_priv->display.load_luts = cherryview_load_luts;
> >  		} else {
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Ville Syrjälä]

On Tue, Mar 19, 2019 at 06:24:47PM +0200, Jani Nikula wrote:
> On Mon, 18 Mar 2019, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > In prepartion for per-platform color_check() functions extract the
> > common code into a separate function.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
> >  1 file changed, 40 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 467fd1a1630c..b6a992a5b14f 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
> >  	return 0;
> >  }
> >  
> > +static int check_luts(const struct intel_crtc_state *crtc_state)
> 
> luts_check() to be more in line with other _commit and _check functions?

Shrug. For some reason check_luts() feels more natural to me.

> 
> BR,
> Jani.
> 
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > +	int gamma_length, degamma_length;
> > +	u32 gamma_tests, degamma_tests;
> > +
> > +	/* Always allow legacy gamma LUT with no further checking. */
> > +	if (crtc_state_is_legacy_gamma(crtc_state))
> > +		return 0;
> > +
> > +	/* C8 needs the legacy LUT all to itself */
> > +	if (crtc_state->c8_planes)
> > +		return -EINVAL;
> > +
> > +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > +
> > +	if (check_lut_size(degamma_lut, degamma_length) ||
> > +	    check_lut_size(gamma_lut, gamma_length))
> > +		return -EINVAL;
> > +
> > +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > +	    drm_color_lut_check(gamma_lut, gamma_tests))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
> >  {
> >  	u32 cgm_mode = 0;
> > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> >  	bool limited_color_range = false;
> > -	int gamma_length, degamma_length;
> > -	u32 gamma_tests, degamma_tests;
> >  	int ret;
> >  
> > -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > -
> > -	/* C8 needs the legacy LUT all to itself */
> > -	if (crtc_state->c8_planes &&
> > -	    !crtc_state_is_legacy_gamma(crtc_state))
> > -		return -EINVAL;
> > +	ret = check_luts(crtc_state);
> > +	if (ret)
> > +		return ret;
> >  
> >  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> >  		!crtc_state->c8_planes;
> > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  	if (IS_CHERRYVIEW(dev_priv))
> >  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> >  
> > -	/* Always allow legacy gamma LUT with no further checking. */
> >  	if (!crtc_state->gamma_enable ||
> > -	    crtc_state_is_legacy_gamma(crtc_state)) {
> > +	    crtc_state_is_legacy_gamma(crtc_state))
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > -		if (INTEL_GEN(dev_priv) >= 11 &&
> > -		    crtc_state->gamma_enable)
> > -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > -		return 0;
> > -	}
> > -
> > -	if (check_lut_size(degamma_lut, degamma_length) ||
> > -	    check_lut_size(gamma_lut, gamma_length))
> > -		return -EINVAL;
> > -
> > -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > -	    drm_color_lut_check(gamma_lut, gamma_tests))
> > -		return -EINVAL;
> > -
> > -	if (INTEL_GEN(dev_priv) >= 11)
> > +	else if (INTEL_GEN(dev_priv) >= 11)
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
> >  					 PRE_CSC_GAMMA_ENABLE |
> >  					 POST_CSC_GAMMA_ENABLE;
> > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> >  
> >  	if (INTEL_GEN(dev_priv) >= 11) {
> > +		if (crtc_state->gamma_enable)
> > +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > +
> >  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> >  		    crtc_state->limited_color_range)
> >  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

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

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Matt Roper]

On Wed, Mar 20, 2019 at 04:41:31PM +0200, Ville Syrjälä wrote:
> On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote:
> > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > In prepartion for per-platform color_check() functions extract the
> > > common code into a separate function.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
> > >  1 file changed, 40 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > index 467fd1a1630c..b6a992a5b14f 100644
> > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
> > >  	return 0;
> > >  }
> > >  
> > > +static int check_luts(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > > +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > > +	int gamma_length, degamma_length;
> > > +	u32 gamma_tests, degamma_tests;
> > > +
> > > +	/* Always allow legacy gamma LUT with no further checking. */
> > > +	if (crtc_state_is_legacy_gamma(crtc_state))
> > > +		return 0;
> > > +
> > > +	/* C8 needs the legacy LUT all to itself */
> > > +	if (crtc_state->c8_planes)
> > > +		return -EINVAL;
> > 
> > Do we need to test for non-NULL gamma_lut here?
> 
> For c8? We already checked crtc_state_is_legacy_gamma() above.
> 

If gamma_lut == NULL, crtc_state_is_legacy_gamma() will return false, so
we'll wind up rejecting any setup involving c8 planes with -EINVAL,
which I don't think is what we wanted.


Matt


> > 
> > The logic is the same as before your patch, so either we already had a
> > pre-existing bug or, more likely, I'm overlooking something obvious.
> > 
> > > +
> > > +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > +
> > > +	if (check_lut_size(degamma_lut, degamma_length) ||
> > > +	    check_lut_size(gamma_lut, gamma_length))
> > > +		return -EINVAL;
> > > +
> > > +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > +	    drm_color_lut_check(gamma_lut, gamma_tests))
> > > +		return -EINVAL;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
> > >  {
> > >  	u32 cgm_mode = 0;
> > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > >  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > >  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > >  	bool limited_color_range = false;
> > > -	int gamma_length, degamma_length;
> > > -	u32 gamma_tests, degamma_tests;
> > >  	int ret;
> > >  
> > > -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > -
> > > -	/* C8 needs the legacy LUT all to itself */
> > > -	if (crtc_state->c8_planes &&
> > > -	    !crtc_state_is_legacy_gamma(crtc_state))
> > > -		return -EINVAL;
> > > +	ret = check_luts(crtc_state);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> > >  		!crtc_state->c8_planes;
> > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > >  	if (IS_CHERRYVIEW(dev_priv))
> > >  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> > >  
> > > -	/* Always allow legacy gamma LUT with no further checking. */
> > >  	if (!crtc_state->gamma_enable ||
> > > -	    crtc_state_is_legacy_gamma(crtc_state)) {
> > > +	    crtc_state_is_legacy_gamma(crtc_state))
> > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > > -		if (INTEL_GEN(dev_priv) >= 11 &&
> > > -		    crtc_state->gamma_enable)
> > > -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > > -		return 0;
> > > -	}
> > > -
> > > -	if (check_lut_size(degamma_lut, degamma_length) ||
> > > -	    check_lut_size(gamma_lut, gamma_length))
> > > -		return -EINVAL;
> > > -
> > > -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > -	    drm_color_lut_check(gamma_lut, gamma_tests))
> > > -		return -EINVAL;
> > > -
> > > -	if (INTEL_GEN(dev_priv) >= 11)
> > > +	else if (INTEL_GEN(dev_priv) >= 11)
> > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
> > >  					 PRE_CSC_GAMMA_ENABLE |
> > >  					 POST_CSC_GAMMA_ENABLE;
> > 
> > Should we drop the GAMMA_ENABLE here and just rely on the new check
> > you're adding below to handle it?
> 
> This will get rewritten entirely in following patches, so I figured I'd
> just keep this patch minimal.
> 
> > 
> > 
> > Matt
> > 
> > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 11) {
> > > +		if (crtc_state->gamma_enable)
> > > +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > > +
> > >  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> > >  		    crtc_state->limited_color_range)
> > >  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
> > > -- 
> > > 2.19.2
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Ville Syrjälä]

On Wed, Mar 20, 2019 at 08:14:56AM -0700, Matt Roper wrote:
> On Wed, Mar 20, 2019 at 04:41:31PM +0200, Ville Syrjälä wrote:
> > On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote:
> > > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > In prepartion for per-platform color_check() functions extract the
> > > > common code into a separate function.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
> > > >  1 file changed, 40 insertions(+), 28 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > > index 467fd1a1630c..b6a992a5b14f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +static int check_luts(const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > > +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > > > +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > > > +	int gamma_length, degamma_length;
> > > > +	u32 gamma_tests, degamma_tests;
> > > > +
> > > > +	/* Always allow legacy gamma LUT with no further checking. */
> > > > +	if (crtc_state_is_legacy_gamma(crtc_state))
> > > > +		return 0;
> > > > +
> > > > +	/* C8 needs the legacy LUT all to itself */
> > > > +	if (crtc_state->c8_planes)
> > > > +		return -EINVAL;
> > > 
> > > Do we need to test for non-NULL gamma_lut here?
> > 
> > For c8? We already checked crtc_state_is_legacy_gamma() above.
> > 
> 
> If gamma_lut == NULL, crtc_state_is_legacy_gamma() will return false, so
> we'll wind up rejecting any setup involving c8 planes with -EINVAL,
> which I don't think is what we wanted.

I think it is. c8 scanout always uses the legacy LUT, so it had 
better be populated with the correct data or else we'll risk
producing garbage on the display.

> 
> 
> Matt
> 
> 
> > > 
> > > The logic is the same as before your patch, so either we already had a
> > > pre-existing bug or, more likely, I'm overlooking something obvious.
> > > 
> > > > +
> > > > +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > > +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > > +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > > +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > > +
> > > > +	if (check_lut_size(degamma_lut, degamma_length) ||
> > > > +	    check_lut_size(gamma_lut, gamma_length))
> > > > +		return -EINVAL;
> > > > +
> > > > +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > > +	    drm_color_lut_check(gamma_lut, gamma_tests))
> > > > +		return -EINVAL;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
> > > >  {
> > > >  	u32 cgm_mode = 0;
> > > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > > >  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > > >  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > > >  	bool limited_color_range = false;
> > > > -	int gamma_length, degamma_length;
> > > > -	u32 gamma_tests, degamma_tests;
> > > >  	int ret;
> > > >  
> > > > -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > > -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > > -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > > -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > > -
> > > > -	/* C8 needs the legacy LUT all to itself */
> > > > -	if (crtc_state->c8_planes &&
> > > > -	    !crtc_state_is_legacy_gamma(crtc_state))
> > > > -		return -EINVAL;
> > > > +	ret = check_luts(crtc_state);
> > > > +	if (ret)
> > > > +		return ret;
> > > >  
> > > >  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> > > >  		!crtc_state->c8_planes;
> > > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > > >  	if (IS_CHERRYVIEW(dev_priv))
> > > >  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> > > >  
> > > > -	/* Always allow legacy gamma LUT with no further checking. */
> > > >  	if (!crtc_state->gamma_enable ||
> > > > -	    crtc_state_is_legacy_gamma(crtc_state)) {
> > > > +	    crtc_state_is_legacy_gamma(crtc_state))
> > > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > > > -		if (INTEL_GEN(dev_priv) >= 11 &&
> > > > -		    crtc_state->gamma_enable)
> > > > -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > > > -		return 0;
> > > > -	}
> > > > -
> > > > -	if (check_lut_size(degamma_lut, degamma_length) ||
> > > > -	    check_lut_size(gamma_lut, gamma_length))
> > > > -		return -EINVAL;
> > > > -
> > > > -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > > -	    drm_color_lut_check(gamma_lut, gamma_tests))
> > > > -		return -EINVAL;
> > > > -
> > > > -	if (INTEL_GEN(dev_priv) >= 11)
> > > > +	else if (INTEL_GEN(dev_priv) >= 11)
> > > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
> > > >  					 PRE_CSC_GAMMA_ENABLE |
> > > >  					 POST_CSC_GAMMA_ENABLE;
> > > 
> > > Should we drop the GAMMA_ENABLE here and just rely on the new check
> > > you're adding below to handle it?
> > 
> > This will get rewritten entirely in following patches, so I figured I'd
> > just keep this patch minimal.
> > 
> > > 
> > > 
> > > Matt
> > > 
> > > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > > >  
> > > >  	if (INTEL_GEN(dev_priv) >= 11) {
> > > > +		if (crtc_state->gamma_enable)
> > > > +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > > > +
> > > >  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> > > >  		    crtc_state->limited_color_range)
> > > >  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
> > > > -- 
> > > > 2.19.2
> > > > 
> > > 
> > > -- 
> > > Matt Roper
> > > Graphics Software Engineer
> > > IoTG Platform Enabling & Development
> > > Intel Corporation
> > > (916) 356-2795
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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

Re: [PATCH 01/10] drm/i915: Extract check_luts()

[Matt Roper]

On Wed, Mar 20, 2019 at 05:31:07PM +0200, Ville Syrjälä wrote:
> On Wed, Mar 20, 2019 at 08:14:56AM -0700, Matt Roper wrote:
> > On Wed, Mar 20, 2019 at 04:41:31PM +0200, Ville Syrjälä wrote:
> > > On Tue, Mar 19, 2019 at 02:59:38PM -0700, Matt Roper wrote:
> > > > On Mon, Mar 18, 2019 at 06:13:08PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > In prepartion for per-platform color_check() functions extract the
> > > > > common code into a separate function.
> > > > > 
> > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_color.c | 68 ++++++++++++++++++------------
> > > > >  1 file changed, 40 insertions(+), 28 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > > > > index 467fd1a1630c..b6a992a5b14f 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_color.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_color.c
> > > > > @@ -771,6 +771,38 @@ static int check_lut_size(const struct drm_property_blob *lut, int expected)
> > > > >  	return 0;
> > > > >  }
> > > > >  
> > > > > +static int check_luts(const struct intel_crtc_state *crtc_state)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > > > +	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > > > > +	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > > > > +	int gamma_length, degamma_length;
> > > > > +	u32 gamma_tests, degamma_tests;
> > > > > +
> > > > > +	/* Always allow legacy gamma LUT with no further checking. */
> > > > > +	if (crtc_state_is_legacy_gamma(crtc_state))
> > > > > +		return 0;
> > > > > +
> > > > > +	/* C8 needs the legacy LUT all to itself */
> > > > > +	if (crtc_state->c8_planes)
> > > > > +		return -EINVAL;
> > > > 
> > > > Do we need to test for non-NULL gamma_lut here?
> > > 
> > > For c8? We already checked crtc_state_is_legacy_gamma() above.
> > > 
> > 
> > If gamma_lut == NULL, crtc_state_is_legacy_gamma() will return false, so
> > we'll wind up rejecting any setup involving c8 planes with -EINVAL,
> > which I don't think is what we wanted.
> 
> I think it is. c8 scanout always uses the legacy LUT, so it had 
> better be populated with the correct data or else we'll risk
> producing garbage on the display.

Ah, makes sense now.  I think I misinterpreted the "all to itself" part
of the comment.  Maybe making this condition an 'else if' and rewording
the comment to something like "C8 relies on its palette being stored in
the legacy LUT" would make it more obvious.

Either way,

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> 
> > 
> > 
> > Matt
> > 
> > 
> > > > 
> > > > The logic is the same as before your patch, so either we already had a
> > > > pre-existing bug or, more likely, I'm overlooking something obvious.
> > > > 
> > > > > +
> > > > > +	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > > > +	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > > > +	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > > > +	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > > > +
> > > > > +	if (check_lut_size(degamma_lut, degamma_length) ||
> > > > > +	    check_lut_size(gamma_lut, gamma_length))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > > > +	    drm_color_lut_check(gamma_lut, gamma_tests))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static u32 chv_cgm_mode(const struct intel_crtc_state *crtc_state)
> > > > >  {
> > > > >  	u32 cgm_mode = 0;
> > > > > @@ -794,19 +826,11 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > > > >  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > > > >  	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > > > >  	bool limited_color_range = false;
> > > > > -	int gamma_length, degamma_length;
> > > > > -	u32 gamma_tests, degamma_tests;
> > > > >  	int ret;
> > > > >  
> > > > > -	degamma_length = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> > > > > -	gamma_length = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > > > > -	degamma_tests = INTEL_INFO(dev_priv)->color.degamma_lut_tests;
> > > > > -	gamma_tests = INTEL_INFO(dev_priv)->color.gamma_lut_tests;
> > > > > -
> > > > > -	/* C8 needs the legacy LUT all to itself */
> > > > > -	if (crtc_state->c8_planes &&
> > > > > -	    !crtc_state_is_legacy_gamma(crtc_state))
> > > > > -		return -EINVAL;
> > > > > +	ret = check_luts(crtc_state);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > >  
> > > > >  	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> > > > >  		!crtc_state->c8_planes;
> > > > > @@ -828,25 +852,10 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > > > >  	if (IS_CHERRYVIEW(dev_priv))
> > > > >  		crtc_state->cgm_mode = chv_cgm_mode(crtc_state);
> > > > >  
> > > > > -	/* Always allow legacy gamma LUT with no further checking. */
> > > > >  	if (!crtc_state->gamma_enable ||
> > > > > -	    crtc_state_is_legacy_gamma(crtc_state)) {
> > > > > +	    crtc_state_is_legacy_gamma(crtc_state))
> > > > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > > > > -		if (INTEL_GEN(dev_priv) >= 11 &&
> > > > > -		    crtc_state->gamma_enable)
> > > > > -			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > > > > -		return 0;
> > > > > -	}
> > > > > -
> > > > > -	if (check_lut_size(degamma_lut, degamma_length) ||
> > > > > -	    check_lut_size(gamma_lut, gamma_length))
> > > > > -		return -EINVAL;
> > > > > -
> > > > > -	if (drm_color_lut_check(degamma_lut, degamma_tests) ||
> > > > > -	    drm_color_lut_check(gamma_lut, gamma_tests))
> > > > > -		return -EINVAL;
> > > > > -
> > > > > -	if (INTEL_GEN(dev_priv) >= 11)
> > > > > +	else if (INTEL_GEN(dev_priv) >= 11)
> > > > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT |
> > > > >  					 PRE_CSC_GAMMA_ENABLE |
> > > > >  					 POST_CSC_GAMMA_ENABLE;
> > > > 
> > > > Should we drop the GAMMA_ENABLE here and just rely on the new check
> > > > you're adding below to handle it?
> > > 
> > > This will get rewritten entirely in following patches, so I figured I'd
> > > just keep this patch minimal.
> > > 
> > > > 
> > > > 
> > > > Matt
> > > > 
> > > > > @@ -858,6 +867,9 @@ int intel_color_check(struct intel_crtc_state *crtc_state)
> > > > >  		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > > > >  
> > > > >  	if (INTEL_GEN(dev_priv) >= 11) {
> > > > > +		if (crtc_state->gamma_enable)
> > > > > +			crtc_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
> > > > > +
> > > > >  		if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> > > > >  		    crtc_state->limited_color_range)
> > > > >  			crtc_state->csc_mode |= ICL_OUTPUT_CSC_ENABLE;
> > > > > -- 
> > > > > 2.19.2
> > > > > 
> > > > 
> > > > -- 
> > > > Matt Roper
> > > > Graphics Software Engineer
> > > > IoTG Platform Enabling & Development
> > > > Intel Corporation
> > > > (916) 356-2795
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/10] drm/i915: Extract bdw_color_check()

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:14PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Provide a separate .color_check() for BDW+ where we currently
> provide the split gamma mode etc.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Patches #5-7:

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_color.c | 39 ++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index e2a1c823650b..6507e5f79c06 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -871,6 +871,43 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static u32 bdw_gamma_mode(const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->gamma_enable ||
> +	    crtc_state_is_legacy_gamma(crtc_state))
> +		return GAMMA_MODE_MODE_8BIT;
> +	else
> +		return GAMMA_MODE_MODE_SPLIT;
> +}
> +
> +static int bdw_color_check(struct intel_crtc_state *crtc_state)
> +{
> +	int ret;
> +
> +	ret = check_luts(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	crtc_state->gamma_enable =
> +		(crtc_state->base.gamma_lut ||
> +		 crtc_state->base.degamma_lut) &&
> +		!crtc_state->c8_planes;
> +
> +	crtc_state->csc_enable =
> +		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> +		crtc_state->base.ctm || crtc_state->limited_color_range;
> +
> +	crtc_state->gamma_mode = bdw_gamma_mode(crtc_state);
> +
> +	crtc_state->csc_mode = 0;
> +
> +	ret = intel_color_add_affected_planes(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static u32 glk_gamma_mode(const struct intel_crtc_state *crtc_state)
>  {
>  	if (!crtc_state->gamma_enable ||
> @@ -1037,6 +1074,8 @@ void intel_color_init(struct intel_crtc *crtc)
>  			dev_priv->display.color_check = icl_color_check;
>  		else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  			dev_priv->display.color_check = glk_color_check;
> +		else if (INTEL_GEN(dev_priv) >= 8)
> +			dev_priv->display.color_check = bdw_color_check;
>  		else
>  			dev_priv->display.color_check = _intel_color_check;
>  	}
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/10] drm/i915: Extract ilk_color_check()

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:15PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> With everything else moved out of the way only ilk+
> remains using _intel_color_check(). Streamline the logic
> into ilk_color_check().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_color.c | 70 ++++++++++++------------------
>  1 file changed, 27 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 6507e5f79c06..8c5c66f5d100 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -871,6 +871,32 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> +static int ilk_color_check(struct intel_crtc_state *crtc_state)

Since it confused me at first, you might want to add a comment on this
function that we don't expose color management properties to userspace
on any platforms that use this function.  So the only gamma we can wind
up with is legacy (via ioctl) and the only CSC usage possible is our own
internal limited range conversion.

> +{
> +	int ret;
> +
> +	ret = check_luts(crtc_state);
> +	if (ret)
> +		return ret;

Do we need to call this?  There's never a degamma LUT for these
platforms, and I think the only gamma LUT we'll ever encounter comes
from the legacy gamma ioctl which has already checked that it's a valid
legacy table.


Matt

> +
> +	crtc_state->gamma_enable =
> +		crtc_state->base.gamma_lut &&
> +		!crtc_state->c8_planes;
> +
> +	crtc_state->csc_enable =
> +		ilk_csc_limited_range(crtc_state);
> +
> +	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> +
> +	crtc_state->csc_mode = 0;
> +
> +	ret = intel_color_add_affected_planes(crtc_state);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static u32 bdw_gamma_mode(const struct intel_crtc_state *crtc_state)
>  {
>  	if (!crtc_state->gamma_enable ||
> @@ -995,48 +1021,6 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
>  	return 0;
>  }
>  
> -static int _intel_color_check(struct intel_crtc_state *crtc_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> -	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> -	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> -	bool limited_color_range = false;
> -	int ret;
> -
> -	ret = check_luts(crtc_state);
> -	if (ret)
> -		return ret;
> -
> -	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> -		!crtc_state->c8_planes;
> -
> -	if (INTEL_GEN(dev_priv) >= 9 ||
> -	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> -		limited_color_range = crtc_state->limited_color_range;
> -
> -	crtc_state->csc_enable =
> -		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> -		crtc_state->base.ctm || limited_color_range;
> -
> -	ret = intel_color_add_affected_planes(crtc_state);
> -	if (ret)
> -		return ret;
> -
> -	crtc_state->csc_mode = 0;
> -
> -	if (!crtc_state->gamma_enable ||
> -	    crtc_state_is_legacy_gamma(crtc_state))
> -		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> -	else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> -		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> -	else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> -		crtc_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
> -	else
> -		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> -
> -	return 0;
> -}
> -
>  void intel_color_init(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -1077,7 +1061,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  		else if (INTEL_GEN(dev_priv) >= 8)
>  			dev_priv->display.color_check = bdw_color_check;
>  		else
> -			dev_priv->display.color_check = _intel_color_check;
> +			dev_priv->display.color_check = ilk_color_check;
>  	}
>  
>  	/* Enable color management support when we have degamma & gamma LUTs. */
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/10] drm/i915: Drop the pointless linear legacy LUT load on CHV

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:16PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We now bypass the legacy LUT when it's not needed, so
> no point in filling it up with a linear LUT.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_color.c | 17 +----------------
>  1 file changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 8c5c66f5d100..b84a2a3f5844 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -376,15 +376,6 @@ static void i9xx_load_luts_internal(const struct intel_crtc_state *crtc_state,
>  				(drm_color_lut_extract(lut[i].green, 8) << 8) |
>  				drm_color_lut_extract(lut[i].blue, 8);
>  
> -			if (HAS_GMCH(dev_priv))
> -				I915_WRITE(PALETTE(pipe, i), word);
> -			else
> -				I915_WRITE(LGC_PALETTE(pipe, i), word);
> -		}
> -	} else {
> -		for (i = 0; i < 256; i++) {
> -			u32 word = (i << 16) | (i << 8) | i;
> -
>  			if (HAS_GMCH(dev_priv))
>  				I915_WRITE(PALETTE(pipe, i), word);
>  			else
> @@ -643,7 +634,7 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
>  	cherryview_load_csc_matrix(crtc_state);
>  
>  	if (crtc_state_is_legacy_gamma(crtc_state)) {
> -		i9xx_load_luts_internal(crtc_state, gamma_lut);
> +		i9xx_load_luts(crtc_state);
>  		return;
>  	}
>  
> @@ -682,12 +673,6 @@ static void cherryview_load_luts(const struct intel_crtc_state *crtc_state)
>  			I915_WRITE(CGM_PIPE_GAMMA(pipe, i, 1), word1);
>  		}
>  	}
> -
> -	/*
> -	 * Also program a linear LUT in the legacy block (behind the
> -	 * CGM block).
> -	 */
> -	i9xx_load_luts_internal(crtc_state, NULL);
>  }
>  
>  void intel_color_load_luts(const struct intel_crtc_state *crtc_state)
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/10] drm/i915: Skip the linear degamma LUT load on ICL+

[Matt Roper]

On Mon, Mar 18, 2019 at 06:13:17PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't load the linear degamma LUT on ICL. The hardware no longer
> has any silly linkages between the CSC enable and degamma LUT
> enable so the degamma LUT is only needed when it's actually
> enabled.
> 
> Also add comments to explain the situation on GLK.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_color.c | 89 +++++++++++++++++++++---------
>  1 file changed, 63 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index b84a2a3f5844..95e44f80bd8a 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -273,6 +273,14 @@ static void ilk_load_csc_matrix(const struct intel_crtc_state *crtc_state)
>  				    ilk_csc_coeff_limited_range,
>  				    ilk_csc_postoff_limited_range);
>  	} else if (crtc_state->csc_enable) {
> +		/*
> +		 * On GLK+ both pipe CSC and degamma LUT are controlled
> +		 * by csc_enable. Hence for the cases where the degama
> +		 * LUT is needed but CSC is not we need to load an
> +		 * identity matrix.
> +		 */
> +		WARN_ON(!IS_CANNONLAKE(dev_priv) && !IS_GEMINILAKE(dev_priv));
> +
>  		ilk_update_pipe_csc(crtc, ilk_csc_off_zero,
>  				    ilk_csc_coeff_identity,
>  				    ilk_csc_off_zero);
> @@ -559,6 +567,7 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
>  	const u32 lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> +	struct drm_color_lut *lut = crtc_state->base.degamma_lut->data;
>  	u32 i;
>  
>  	/*
> @@ -569,32 +578,48 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
>  	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
>  	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
>  
> -	if (crtc_state->base.degamma_lut) {
> -		struct drm_color_lut *lut = crtc_state->base.degamma_lut->data;
> +	for (i = 0; i < lut_size; i++) {
> +		/*
> +		 * First 33 entries represent range from 0 to 1.0
> +		 * 34th and 35th entry will represent extended range
> +		 * inputs 3.0 and 7.0 respectively, currently clamped
> +		 * at 1.0. Since the precision is 16bit, the user
> +		 * value can be directly filled to register.
> +		 * The pipe degamma table in GLK+ onwards doesn't
> +		 * support different values per channel, so this just
> +		 * programs green value which will be equal to Red and
> +		 * Blue into the lut registers.
> +		 * ToDo: Extend to max 7.0. Enable 32 bit input value
> +		 * as compared to just 16 to achieve this.
> +		 */
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].green);
> +	}
>  
> -		for (i = 0; i < lut_size; i++) {
> -			/*
> -			 * First 33 entries represent range from 0 to 1.0
> -			 * 34th and 35th entry will represent extended range
> -			 * inputs 3.0 and 7.0 respectively, currently clamped
> -			 * at 1.0. Since the precision is 16bit, the user
> -			 * value can be directly filled to register.
> -			 * The pipe degamma table in GLK+ onwards doesn't
> -			 * support different values per channel, so this just
> -			 * programs green value which will be equal to Red and
> -			 * Blue into the lut registers.
> -			 * ToDo: Extend to max 7.0. Enable 32 bit input value
> -			 * as compared to just 16 to achieve this.
> -			 */
> -			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), lut[i].green);
> -		}
> -	} else {
> -		/* load a linear table. */
> -		for (i = 0; i < lut_size; i++) {
> -			u32 v = (i * (1 << 16)) / (lut_size - 1);
> +	/* Clamp values > 1.0. */
> +	while (i++ < 35)
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> +}
>  
> -			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> -		}
> +static void glk_load_degamma_lut_linear(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	const u32 lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> +	u32 i;
> +
> +	/*
> +	 * When setting the auto-increment bit, the hardware seems to
> +	 * ignore the index bits, so we need to reset it to index 0
> +	 * separately.
> +	 */
> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> +
> +	for (i = 0; i < lut_size; i++) {
> +		u32 v = (i  << 16) / (lut_size - 1);
> +
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
>  	}
>  
>  	/* Clamp values > 1.0. */
> @@ -604,7 +629,18 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state)
>  
>  static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>  {
> -	glk_load_degamma_lut(crtc_state);
> +	/*
> +	 * On GLK+ both pipe CSC and degamma LUT are controlled
> +	 * by csc_enable. Hence for the cases where the CSC is
> +	 * needed but degamma LUT is not we need to load a
> +	 * linear degamma LUT. In fact we'll just always load
> +	 * the degama LUT so that we don't have to reload
> +	 * it every time the pipe CSC is being enabled.
> +	 */
> +	if (crtc_state->base.degamma_lut)
> +		glk_load_degamma_lut(crtc_state);
> +	else
> +		glk_load_degamma_lut_linear(crtc_state);
>  
>  	if (crtc_state_is_legacy_gamma(crtc_state))
>  		i9xx_load_luts(crtc_state);
> @@ -614,7 +650,8 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>  
>  static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  {
> -	glk_load_degamma_lut(crtc_state);
> +	if (crtc_state->base.degamma_lut)
> +		glk_load_degamma_lut(crtc_state);
>  
>  	if (crtc_state_is_legacy_gamma(crtc_state))
>  		i9xx_load_luts(crtc_state);
> -- 
> 2.19.2
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/10] drm/i915: Extract ilk_color_check()

[Ville Syrjälä]

On Wed, Mar 20, 2019 at 03:49:31PM -0700, Matt Roper wrote:
> On Mon, Mar 18, 2019 at 06:13:15PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > With everything else moved out of the way only ilk+
> > remains using _intel_color_check(). Streamline the logic
> > into ilk_color_check().
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_color.c | 70 ++++++++++++------------------
> >  1 file changed, 27 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 6507e5f79c06..8c5c66f5d100 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -871,6 +871,32 @@ static int chv_color_check(struct intel_crtc_state *crtc_state)
> >  	return 0;
> >  }
> >  
> > +static int ilk_color_check(struct intel_crtc_state *crtc_state)
> 
> Since it confused me at first, you might want to add a comment on this
> function that we don't expose color management properties to userspace
> on any platforms that use this function.  So the only gamma we can wind
> up with is legacy (via ioctl) and the only CSC usage possible is our own
> internal limited range conversion.

Sure. I have patches already typed up to change that though ;)

> 
> > +{
> > +	int ret;
> > +
> > +	ret = check_luts(crtc_state);
> > +	if (ret)
> > +		return ret;
> 
> Do we need to call this?  There's never a degamma LUT for these
> platforms, and I think the only gamma LUT we'll ever encounter comes
> from the legacy gamma ioctl which has already checked that it's a valid
> legacy table.

We'll need to do the c8 vs. legacy LUT check.

> 
> 
> Matt
> 
> > +
> > +	crtc_state->gamma_enable =
> > +		crtc_state->base.gamma_lut &&
> > +		!crtc_state->c8_planes;
> > +
> > +	crtc_state->csc_enable =
> > +		ilk_csc_limited_range(crtc_state);
> > +
> > +	crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > +
> > +	crtc_state->csc_mode = 0;
> > +
> > +	ret = intel_color_add_affected_planes(crtc_state);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> >  static u32 bdw_gamma_mode(const struct intel_crtc_state *crtc_state)
> >  {
> >  	if (!crtc_state->gamma_enable ||
> > @@ -995,48 +1021,6 @@ static int icl_color_check(struct intel_crtc_state *crtc_state)
> >  	return 0;
> >  }
> >  
> > -static int _intel_color_check(struct intel_crtc_state *crtc_state)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > -	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> > -	const struct drm_property_blob *degamma_lut = crtc_state->base.degamma_lut;
> > -	bool limited_color_range = false;
> > -	int ret;
> > -
> > -	ret = check_luts(crtc_state);
> > -	if (ret)
> > -		return ret;
> > -
> > -	crtc_state->gamma_enable = (gamma_lut || degamma_lut) &&
> > -		!crtc_state->c8_planes;
> > -
> > -	if (INTEL_GEN(dev_priv) >= 9 ||
> > -	    IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > -		limited_color_range = crtc_state->limited_color_range;
> > -
> > -	crtc_state->csc_enable =
> > -		crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> > -		crtc_state->base.ctm || limited_color_range;
> > -
> > -	ret = intel_color_add_affected_planes(crtc_state);
> > -	if (ret)
> > -		return ret;
> > -
> > -	crtc_state->csc_mode = 0;
> > -
> > -	if (!crtc_state->gamma_enable ||
> > -	    crtc_state_is_legacy_gamma(crtc_state))
> > -		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > -	else if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > -		crtc_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> > -	else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> > -		crtc_state->gamma_mode = GAMMA_MODE_MODE_SPLIT;
> > -	else
> > -		crtc_state->gamma_mode = GAMMA_MODE_MODE_8BIT;
> > -
> > -	return 0;
> > -}
> > -
> >  void intel_color_init(struct intel_crtc *crtc)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > @@ -1077,7 +1061,7 @@ void intel_color_init(struct intel_crtc *crtc)
> >  		else if (INTEL_GEN(dev_priv) >= 8)
> >  			dev_priv->display.color_check = bdw_color_check;
> >  		else
> > -			dev_priv->display.color_check = _intel_color_check;
> > +			dev_priv->display.color_check = ilk_color_check;
> >  	}
> >  
> >  	/* Enable color management support when we have degamma & gamma LUTs. */
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

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