>-----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
