Re: [Intel-gfx] [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to
>-Original Message- >From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com] >Sent: Wednesday, April 3, 2019 6:10 PM >To: Shankar, Uma >Cc: intel-gfx@lists.freedesktop.org; Roper, Matthew D > >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 +, 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 ; Roper, Matthew D >> > >> >Subject: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't >> >have to >> > >> >From: Ville Syrjälä >> > >> >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ä >> >--- >> > 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); >> >
Re: [Intel-gfx] [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 +, 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 ; Roper, Matthew D > > > >Subject: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to > > > >From: Ville Syrjälä > > > >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ä > >--- > > 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_A0x4A404 > > #define _PAL_PREC_DATA_B0x4AC04 > > #define _PAL_PREC_DATA_C0x4B404 > >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[
Re: [Intel-gfx] [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to
>-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 ; Roper, Matthew D > >Subject: [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to > >From: Ville Syrjälä > >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ä >--- > 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. >+ * 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
[Intel-gfx] [PATCH v2 2/7] drm/i915: Don't use split gamma when we don't have to
From: Ville Syrjälä 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ä --- 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 +* 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