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