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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
>Sent: Wednesday, April 3, 2019 6:10 PM
>To: Shankar, Uma <uma.shankar@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Roper, Matthew D
><matthew.d.roper@xxxxxxxxx>
>Subject: Re: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to
>
>On Wed, Apr 03, 2019 at 12:23:06PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjala [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
>> >Sent: Tuesday, April 2, 2019 1:32 AM
>> >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> >Cc: Shankar, Uma <uma.shankar@xxxxxxxxx>; Roper, Matthew D
>> ><matthew.d.roper@xxxxxxxxx>
>> >Subject: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't
>> >have to
>> >
>> >From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> >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@xxxxxxxxxxxxxxx>
>> >---
>> > 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.

Yeah agree :). Opened this on outlook and got this spell check. 

>>
>> >+	 * 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.

Ok yeah, Split gamma mode by default configures itself as degamma ->csc>gamma, we should be
good here.

Changes look ok to me:
Reviewed-by: Uma Shankar <uma.shankar@xxxxxxxxx>

>>
>> >+	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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux