Re: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Apr 03, 2019 at 12:23:06PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com]
> >Sent: Tuesday, April 2, 2019 1:32 AM
> >To: intel-gfx@lists.freedesktop.org
> >Cc: Shankar, Uma <uma.shankar@intel.com>; Roper, Matthew D
> ><matthew.d.roper@intel.com>
> >Subject: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to
> >
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >Using the split gamma mode when we don't have to has the annoying requirement of
> >loading a linear LUT to the unused half. Instead let's make life simpler by switching to
> >the 10bit gamma mode and duplicating each entry.
> >
> >This also allows us to load the software gamma LUT into the hardware degamma
> >LUT, thus removing some of the buggy configurations we currently allow
> >(YCbCr/limited range RGB
> >+ gamma LUT). We do still have other configurations that are
> >also buggy, but those will need more complicated fixes or they just need to be
> >rejected. Sadly GLK doesn't have this flexibility anymore and the degamma and
> >gamma LUTs are very different so no help there.
> >
> >v2: Apply a mask when checking gamma_mode on icl since it
> >    contains more bits than just the gamma mode
> >v3: Rebase due to EXT_GC_MAX/EXT2_GC_MAX changes
> >
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >---
> > drivers/gpu/drm/i915/i915_reg.h    |   2 +
> > drivers/gpu/drm/i915/intel_color.c | 185 ++++++++++++++---------------
> > 2 files changed, 92 insertions(+), 95 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index
> >341f03e00536..bed2c52aebd8 100644
> >--- a/drivers/gpu/drm/i915/i915_reg.h
> >+++ b/drivers/gpu/drm/i915/i915_reg.h
> >@@ -7214,6 +7214,7 @@ enum {
> > #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A,
> >_GAMMA_MODE_B)
> > #define  PRE_CSC_GAMMA_ENABLE	(1 << 31)
> > #define  POST_CSC_GAMMA_ENABLE	(1 << 30)
> >+#define  GAMMA_MODE_MODE_MASK	(3 << 0)
> > #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
> > #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
> > #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
> >@@ -10127,6 +10128,7 @@ enum skl_power_gate {
> > #define   PAL_PREC_SPLIT_MODE		(1 << 31)
> > #define   PAL_PREC_AUTO_INCREMENT	(1 << 15)
> > #define   PAL_PREC_INDEX_VALUE_MASK	(0x3ff << 0)
> >+#define   PAL_PREC_INDEX_VALUE(x)	((x) << 0)
> > #define _PAL_PREC_DATA_A	0x4A404
> > #define _PAL_PREC_DATA_B	0x4AC04
> > #define _PAL_PREC_DATA_C	0x4B404
> >diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >index d5b3060c2645..5ef93c43afcf 100644
> >--- a/drivers/gpu/drm/i915/intel_color.c
> >+++ b/drivers/gpu/drm/i915/intel_color.c
> >@@ -466,115 +466,83 @@ static void skl_color_commit(const struct intel_crtc_state
> >*crtc_state)
> > 		ilk_load_csc_matrix(crtc_state);
> > }
> >
> >-static void bdw_load_degamma_lut(const struct intel_crtc_state *crtc_state)
> >+static void bdw_load_lut_10(struct intel_crtc *crtc,
> >+			    const struct drm_property_blob *blob,
> >+			    u32 prec_index, bool duplicate)
> > {
> >-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >-	const struct drm_property_blob *degamma_lut = crtc_state-
> >>base.degamma_lut;
> >-	u32 i, lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> >+	const struct drm_color_lut *lut = blob->data;
> >+	int i, lut_size = drm_color_lut_size(blob);
> > 	enum pipe pipe = crtc->pipe;
> >
> >-	I915_WRITE(PREC_PAL_INDEX(pipe),
> >-		   PAL_PREC_SPLIT_MODE | PAL_PREC_AUTO_INCREMENT);
> >-
> >-	if (degamma_lut) {
> >-		const struct drm_color_lut *lut = degamma_lut->data;
> >+	I915_WRITE(PREC_PAL_INDEX(pipe), prec_index |
> >+		   PAL_PREC_AUTO_INCREMENT);
> >
> >-		for (i = 0; i < lut_size; i++)
> >-			I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(&lut[i]));
> >-	} else {
> >+	/*
> >+	 * We advertize the split gamma sizes. When not using split
> 
> Typo in advertise.

Just a different language. Though maybe the 'z' isn't quite right even
for US English. "Consistency not included" should be on the packaging.

> 
> >+	 * gamma we just duplicate each entry.
> >+	 *
> >+	 * TODO: expose the full LUT to userspace
> >+	 */
> >+	if (duplicate) {
> > 		for (i = 0; i < lut_size; i++) {
> >-			u32 v = (i * ((1 << 10) - 1)) / (lut_size - 1);
> >-
> >-			I915_WRITE(PREC_PAL_DATA(pipe),
> >-				   (v << 20) | (v << 10) | v);
> >+			I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(&lut[i]));
> >+			I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(&lut[i]));
> > 		}
> >+	} else {
> >+		for (i = 0; i < lut_size; i++)
> >+			I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(&lut[i]));
> > 	}
> >+
> >+	/*
> >+	 * Reset the index, otherwise it prevents the legacy palette to be
> >+	 * written properly.
> >+	 */
> >+	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> > }
> >
> >-static void bdw_load_gamma_lut(const struct intel_crtc_state *crtc_state, u32
> >offset)
> >+static void bdw_load_lut_10_max(struct intel_crtc *crtc)
> > {
> >-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >-	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >-	u32 i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > 	enum pipe pipe = crtc->pipe;
> >
> >-	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> >-
> >-	I915_WRITE(PREC_PAL_INDEX(pipe),
> >-		   (offset ? PAL_PREC_SPLIT_MODE : 0) |
> >-		   PAL_PREC_AUTO_INCREMENT |
> >-		   offset);
> >-
> >-	if (gamma_lut) {
> >-		const struct drm_color_lut *lut = gamma_lut->data;
> >-
> >-		for (i = 0; i < lut_size; i++)
> >-			I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_10(&lut[i]));
> >-
> >-		/*
> >-		 * Program the max register to clamp values > 1.0.
> >-		 * ToDo: Extend the ABI to be able to program values
> >-		 * from 1.0 to 3.0
> >-		 */
> >-		I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), (1 << 16));
> >-		I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), (1 << 16));
> >-		I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), (1 << 16));
> >-
> >-		/*
> >-		 * Program the gc max 2 register to clamp values > 1.0.
> >-		 * ToDo: Extend the ABI to be able to program values
> >-		 * from 3.0 to 7.0
> >-		 */
> >-		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> >-			I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), (1 << 16));
> >-			I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), (1 << 16));
> >-			I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), (1 << 16));
> >-		}
> >-	} else {
> >-		for (i = 0; i < lut_size; i++) {
> >-			u32 v = (i * ((1 << 10) - 1)) / (lut_size - 1);
> >-
> >-			I915_WRITE(PREC_PAL_DATA(pipe),
> >-				   (v << 20) | (v << 10) | v);
> >-		}
> >-
> >-		I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), (1 << 16));
> >-		I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), (1 << 16));
> >-		I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), (1 << 16));
> >-
> >-		/*
> >-		 * Program the gc max 2 register to clamp values > 1.0.
> >-		 * ToDo: Extend the ABI to be able to program values
> >-		 * from 3.0 to 7.0
> >-		 */
> >-		if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> >-			I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), (1 << 16));
> >-			I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), (1 << 16));
> >-			I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), (1 << 16));
> >-		}
> >-	}
> >+	/* Program the max register to clamp values > 1.0. */
> >+	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> >+	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> >+	I915_WRITE(PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> >
> > 	/*
> >-	 * Reset the index, otherwise it prevents the legacy palette to be
> >-	 * written properly.
> >+	 * Program the gc max 2 register to clamp values > 1.0.
> >+	 * ToDo: Extend the ABI to be able to program values
> >+	 * from 3.0 to 7.0
> > 	 */
> >-	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> >+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv)) {
> >+		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> >+		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> >+		I915_WRITE(PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
> >+	}
> > }
> >
> >-/* Loads the palette/gamma unit for the CRTC on Broadwell+. */ -static void
> >broadwell_load_luts(const struct intel_crtc_state *crtc_state)
> >+static void bdw_load_luts(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);
> >+	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >+	const struct drm_property_blob *degamma_lut =
> >+crtc_state->base.degamma_lut;
> >
> >-	if (crtc_state_is_legacy_gamma(crtc_state)) {
> >+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT) {
> > 		i9xx_load_luts(crtc_state);
> >+	} else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT) {
> >+		bdw_load_lut_10(crtc, degamma_lut, PAL_PREC_SPLIT_MODE |
> >+				PAL_PREC_INDEX_VALUE(0), false);
> >+		bdw_load_lut_10_max(crtc);
> >+		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_SPLIT_MODE |
> >+				PAL_PREC_INDEX_VALUE(512),  false);
> > 	} else {
> >-		bdw_load_degamma_lut(crtc_state);
> >-		bdw_load_gamma_lut(crtc_state,
> >-				   INTEL_INFO(dev_priv)->color.degamma_lut_size);
> >+		const struct drm_property_blob *blob = gamma_lut ?: degamma_lut;
> >+
> >+		bdw_load_lut_10(crtc, blob,
> >+				PAL_PREC_INDEX_VALUE(0), true);
> >+		bdw_load_lut_10_max(crtc);
> > 	}
> > }
> >
> >@@ -646,6 +614,9 @@ static void glk_load_degamma_lut_linear(const struct
> >intel_crtc_state *crtc_stat
> >
> > static void glk_load_luts(const struct intel_crtc_state *crtc_state)  {
> >+	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >+
> > 	/*
> > 	 * On GLK+ both pipe CSC and degamma LUT are controlled
> > 	 * by csc_enable. Hence for the cases where the CSC is @@ -659,22 +630,29
> >@@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
> > 	else
> > 		glk_load_degamma_lut_linear(crtc_state);
> >
> >-	if (crtc_state_is_legacy_gamma(crtc_state))
> >+	if (crtc_state->gamma_mode == GAMMA_MODE_MODE_8BIT) {
> > 		i9xx_load_luts(crtc_state);
> >-	else
> >-		bdw_load_gamma_lut(crtc_state, 0);
> >+	} else {
> >+		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0),
> >false);
> >+		bdw_load_lut_10_max(crtc);
> >+	}
> > }
> >
> > static void icl_load_luts(const struct intel_crtc_state *crtc_state)  {
> >+	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >+
> > 	if (crtc_state->base.degamma_lut)
> > 		glk_load_degamma_lut(crtc_state);
> >
> >-	if (crtc_state_is_legacy_gamma(crtc_state))
> >+	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
> >+	    GAMMA_MODE_MODE_8BIT) {
> > 		i9xx_load_luts(crtc_state);
> >-	else
> >-		/* ToDo: Add support for multi segment gamma LUT */
> >-		bdw_load_gamma_lut(crtc_state, 0);
> >+	} else {
> >+		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0),
> >false);
> >+		bdw_load_lut_10_max(crtc);
> >+	}
> > }
> >
> > static void cherryview_load_luts(const struct intel_crtc_state *crtc_state) @@ -
> >959,8 +937,25 @@ 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
> >+	else if (crtc_state->base.gamma_lut &&
> >+		 crtc_state->base.degamma_lut)
> > 		return GAMMA_MODE_MODE_SPLIT;
> >+	else
> >+		return GAMMA_MODE_MODE_10BIT;
> >+}
> >+
> >+static u32 bdw_csc_mode(const struct intel_crtc_state *crtc_state) {
> >+	/*
> >+	 * CSC comes after the LUT in degamma, RGB->YCbCr,
> >+	 * and RGB full->limited range mode.
> >+	 */
> >+	if (crtc_state->base.degamma_lut ||
> >+	    crtc_state->output_format != INTEL_OUTPUT_FORMAT_RGB ||
> >+	    crtc_state->limited_color_range)
> >+		return 0;
> 
> There may be a scenario that non-linear blending is done and then at pipe level,
> some CTM manipulation is expected (adjusting hues, saturation etc) for this we may
> have to apply degamma to make the content linear, then CTM and finally gamma.
> So is it right to presume that  if degamma is there CSC will always be after gamma ?

Yes, this just determines whether we get lut->csc or csc->lut in
non-split gamma modes. For the case you're taking about we'll use
the split gamma mode so it'll be lut->csc->lut regardless of this
CSC_MODE bit.

> 
> >+	return CSC_POSITION_BEFORE_GAMMA;
> > }
> >
> > static int bdw_color_check(struct intel_crtc_state *crtc_state) @@ -982,7 +977,7
> >@@ static int bdw_color_check(struct intel_crtc_state *crtc_state)
> >
> > 	crtc_state->gamma_mode = bdw_gamma_mode(crtc_state);
> >
> >-	crtc_state->csc_mode = 0;
> >+	crtc_state->csc_mode = bdw_csc_mode(crtc_state);
> >
> > 	ret = intel_color_add_affected_planes(crtc_state);
> > 	if (ret)
> >@@ -1116,7 +1111,7 @@ void intel_color_init(struct intel_crtc *crtc)
> > 		else if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > 			dev_priv->display.load_luts = glk_load_luts;
> > 		else if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv))
> >-			dev_priv->display.load_luts = broadwell_load_luts;
> >+			dev_priv->display.load_luts = bdw_load_luts;
> > 		else
> > 			dev_priv->display.load_luts = i9xx_load_luts;
> > 	}
> >--
> >2.19.2
> 

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