>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
>Sent: Wednesday, April 3, 2019 6:10 PM
>To: Shankar, Uma <uma.shan...@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Roper, Matthew D
><matthew.d.ro...@intel.com>
>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.syrj...@linux.intel.com]
>> >Sent: Tuesday, April 2, 2019 1:32 AM
>> >To: intel-gfx@lists.freedesktop.org
>> >Cc: Shankar, Uma <uma.shan...@intel.com>; Roper, Matthew D
>> ><matthew.d.ro...@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.syrj...@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.syrj...@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.
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.shan...@intel.com>
>>
>> >+ 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
