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