[Intel-gfx] [PATCH 00/13] drm/i915: Gamma/DSB prep work

[Intel-gfx] [PATCH 00/13] drm/i915: Gamma/DSB prep work

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

A bit of cleanups/refactoring/etc. around the gamma and
DSB code. The eventual aim is to get the DSB to succesfully
load the LUTs for us, but we're not there yet.

Ville Syrjälä (13):
  drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit
  drm/i915: Clean up GAMMA_MODE defines
  drm/i915: Define skl+ palette anti-collision bit
  drm/i915: Clean up various indexed LUT registers
  drm/i915: Standardize auto-increment LUT load procedure
  drm/i915: Document LUT "max" register precision
  drm/i915: Move the DSB->mmio fallback into the LUT code
  drm/i915: Move the DSB setup/cleaup into the color code
  drm/i915: Make DSB lower level
  drm/i915: Disable DSB usage specifically for LUTs
  Revert "drm/i915: Disable DSB usage for now"
  drm/i915: Use ilk_lut_write*() for all ilk+ gamma modes
  drm/i915: Do state check for color management changes

 drivers/gpu/drm/i915/display/intel_color.c    | 245 +++++++++++-------
 drivers/gpu/drm/i915/display/intel_color.h    |   2 +
 drivers/gpu/drm/i915/display/intel_display.c  |  25 +-
 drivers/gpu/drm/i915/display/intel_display.h  |   8 +
 drivers/gpu/drm/i915/display/intel_dsb.c      |  95 +++----
 drivers/gpu/drm/i915/display/intel_dsb.h      |  13 +-
 .../drm/i915/display/intel_modeset_verify.c   |   2 +
 drivers/gpu/drm/i915/i915_pci.c               |   2 +-
 drivers/gpu/drm/i915/i915_reg.h               |  45 ++--
 9 files changed, 250 insertions(+), 187 deletions(-)

-- 
2.37.4

[Intel-gfx] [PATCH 01/13] drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

s/GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED/GAMMA_MODE_MODE_12BIT_MULTI_SEG/
to make this thing slightly shorter.

Also fix up the platform comment while at it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 10 +++++-----
 drivers/gpu/drm/i915/i915_reg.h            |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 842d58da3128..956b221860e6 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1212,7 +1212,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 	case GAMMA_MODE_MODE_8BIT:
 		ilk_load_lut_8(crtc, post_csc_lut);
 		break;
-	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
 		icl_program_gamma_superfine_segment(crtc_state);
 		icl_program_gamma_multi_segment(crtc_state);
 		ivb_load_lut_ext_max(crtc_state);
@@ -2091,7 +2091,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
 	else if (DISPLAY_VER(i915) >= 13)
 		gamma_mode |= GAMMA_MODE_MODE_10BIT;
 	else
-		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
+		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEG;
 
 	return gamma_mode;
 }
@@ -2283,7 +2283,7 @@ static int icl_post_csc_lut_precision(const struct intel_crtc_state *crtc_state)
 		return 8;
 	case GAMMA_MODE_MODE_10BIT:
 		return 10;
-	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
 		return 16;
 	default:
 		MISSING_CASE(crtc_state->gamma_mode);
@@ -2455,7 +2455,7 @@ static bool icl_lut_equal(const struct intel_crtc_state *crtc_state,
 
 	/* hw readout broken except for the super fine segment :( */
 	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
-	    GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED)
+	    GAMMA_MODE_MODE_12BIT_MULTI_SEG)
 		check_size = 9;
 
 	return intel_lut_equal(blob1, blob2, check_size,
@@ -2971,7 +2971,7 @@ static void icl_read_luts(struct intel_crtc_state *crtc_state)
 	case GAMMA_MODE_MODE_10BIT:
 		crtc_state->post_csc_lut = bdw_read_lut_10(crtc, PAL_PREC_INDEX_VALUE(0));
 		break;
-	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
 		crtc_state->post_csc_lut = icl_read_lut_multi_segment(crtc);
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0b90fe6a28f7..b1c314093737 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5316,7 +5316,7 @@
 #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
 #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
 #define  GAMMA_MODE_MODE_SPLIT	(3 << 0) /* ivb-bdw */
-#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0) /* icl + */
+#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	(3 << 0) /* icl-tgl */
 
 /* Display Internal Timeout Register */
 #define RM_TIMEOUT		_MMIO(0x42060)
-- 
2.37.4

[Intel-gfx] [PATCH 02/13] drm/i915: Clean up GAMMA_MODE defines

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use REG_BIT() & co. for GAMMA_MODE bits.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b1c314093737..52d289f55ce1 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5309,14 +5309,14 @@
 #define _GAMMA_MODE_A		0x4a480
 #define _GAMMA_MODE_B		0x4ac80
 #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)
-#define  GAMMA_MODE_MODE_SPLIT	(3 << 0) /* ivb-bdw */
-#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	(3 << 0) /* icl-tgl */
+#define  PRE_CSC_GAMMA_ENABLE			REG_BIT(31) /* icl+ */
+#define  POST_CSC_GAMMA_ENABLE			REG_BIT(30) /* icl+ */
+#define  GAMMA_MODE_MODE_MASK			REG_GENMASK(1, 0)
+#define  GAMMA_MODE_MODE_8BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 0)
+#define  GAMMA_MODE_MODE_10BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 1)
+#define  GAMMA_MODE_MODE_12BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 2)
+#define  GAMMA_MODE_MODE_SPLIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 3) /* ivb-bdw */
+#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 3) /* icl-tgl */
 
 /* Display Internal Timeout Register */
 #define RM_TIMEOUT		_MMIO(0x42060)
-- 
2.37.4

[Intel-gfx] [PATCH 03/13] drm/i915: Define skl+ palette anti-collision bit

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've been frobbing the palette anti-collision logic bit
while playing around with DSB. Not sure we'll have real
use for this but let's define the bit anyways so I don't
have to carry it around locally.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 52d289f55ce1..80ac50d80af4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5311,6 +5311,7 @@
 #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A, _GAMMA_MODE_B)
 #define  PRE_CSC_GAMMA_ENABLE			REG_BIT(31) /* icl+ */
 #define  POST_CSC_GAMMA_ENABLE			REG_BIT(30) /* icl+ */
+#define  PALETTE_ANTICOL_DISABLE		REG_BIT(15) /* skl+ */
 #define  GAMMA_MODE_MODE_MASK			REG_GENMASK(1, 0)
 #define  GAMMA_MODE_MODE_8BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 0)
 #define  GAMMA_MODE_MODE_10BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 1)
-- 
2.37.4

[Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT registers

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Use REG_BIT() & co. for the LUT index registers, and also
use the REG_FIELD_PREP() stuff a bit more consistently when
generating the values for said registers.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
 drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
 2 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 956b221860e6..c960c2aaf328 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
 	enum pipe pipe = crtc->pipe;
 
 	for (i = 0; i < lut_size; i++) {
-		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
+		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+				  prec_index + i);
 		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
 				  ilk_lut_10(&lut[i]));
 	}
@@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 }
 
 /* On BDW+ the index auto increment mode actually works */
@@ -933,7 +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
 	enum pipe pipe = crtc->pipe;
 
 	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  prec_index | PAL_PREC_AUTO_INCREMENT);
+			  PAL_PREC_AUTO_INCREMENT |
+			  prec_index);
 
 	for (i = 0; i < lut_size; i++)
 		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
@@ -943,7 +946,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 }
 
 static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
@@ -1049,9 +1053,11 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
 	 * ignore the index bits, so we need to reset it to index 0
 	 * separately.
 	 */
-	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
 	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
-			  PRE_CSC_GAMC_AUTO_INCREMENT);
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
+	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
+			  PRE_CSC_GAMC_AUTO_INCREMENT |
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	for (i = 0; i < lut_size; i++) {
 		/*
@@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 * seg2[0] being unused by the hardware.
 	 */
 	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
-			    PAL_PREC_AUTO_INCREMENT);
+			    PAL_PREC_AUTO_INCREMENT |
+			    PAL_PREC_INDEX_VALUE(0));
+
 	for (i = 1; i < 257; i++) {
 		entry = &lut[i * 8];
 		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
@@ -2756,7 +2764,8 @@ static struct drm_property_blob *ivb_read_lut_10(struct intel_crtc *crtc,
 		ilk_lut_10_pack(&lut[i], val);
 	}
 
-	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 
 	return blob;
 }
@@ -2811,7 +2820,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
 	lut = blob->data;
 
 	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  prec_index | PAL_PREC_AUTO_INCREMENT);
+			  PAL_PREC_AUTO_INCREMENT |
+			  prec_index);
 
 	for (i = 0; i < lut_size; i++) {
 		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe));
@@ -2819,7 +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
 		ilk_lut_10_pack(&lut[i], val);
 	}
 
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  PAL_PREC_INDEX_VALUE(0));
 
 	return blob;
 }
@@ -2876,9 +2887,11 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
 	 * ignore the index bits, so we need to reset it to index 0
 	 * separately.
 	 */
-	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
 	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
-			  PRE_CSC_GAMC_AUTO_INCREMENT);
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
+	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
+			  PRE_CSC_GAMC_AUTO_INCREMENT |
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	for (i = 0; i < lut_size; i++) {
 		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
@@ -2888,7 +2901,8 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
 		lut[i].blue = val;
 	}
 
-	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
+	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
+			  PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	return blob;
 }
@@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
 	lut = blob->data;
 
 	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
-			  PAL_PREC_AUTO_INCREMENT);
+			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
+			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 
 	for (i = 0; i < 9; i++) {
 		u32 ldw = intel_de_read_fw(i915, PREC_PAL_MULTI_SEG_DATA(pipe));
@@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
 		ilk_lut_12p4_pack(&lut[i], ldw, udw);
 	}
 
-	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
+	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
+			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 
 	/*
 	 * FIXME readouts from PAL_PREC_DATA register aren't giving
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 80ac50d80af4..22fb9fd78483 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7531,11 +7531,10 @@ enum skl_power_gate {
 #define _PAL_PREC_INDEX_A	0x4A400
 #define _PAL_PREC_INDEX_B	0x4AC00
 #define _PAL_PREC_INDEX_C	0x4B400
-#define   PAL_PREC_10_12_BIT		(0 << 31)
-#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_SPLIT_MODE		REG_BIT(31)
+#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
+#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
+#define   PAL_PREC_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
 #define _PAL_PREC_DATA_A	0x4A404
 #define _PAL_PREC_DATA_B	0x4AC04
 #define _PAL_PREC_DATA_C	0x4B404
@@ -7559,7 +7558,9 @@ enum skl_power_gate {
 #define _PRE_CSC_GAMC_INDEX_A	0x4A484
 #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
 #define _PRE_CSC_GAMC_INDEX_C	0x4B484
-#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
+#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
+#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)
+#define   PRE_CSC_GAMC_INDEX_VALUE(x)	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
 #define _PRE_CSC_GAMC_DATA_A	0x4A488
 #define _PRE_CSC_GAMC_DATA_B	0x4AC88
 #define _PRE_CSC_GAMC_DATA_C	0x4B488
@@ -7570,8 +7571,9 @@ enum skl_power_gate {
 /* ICL Multi segmented gamma */
 #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
 #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
-#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		REG_BIT(15)
-#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
+#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
+#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
+#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
 
 #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
 #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
-- 
2.37.4

[Intel-gfx] [PATCH 05/13] drm/i915: Standardize auto-increment LUT load procedure

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Various gamma units on various platforms have some problems loading
the LUT index and auto-increment bit at the same time. We have to
do this in two steps. The first known case was the glk degamma LUT,
but at least ADL has another known case.

We're not going to suffer too badly from a couple of extra register
writes here, so let's just standardize on this practice for all
auto-increment LUT loads/reads. This way we never have to worry about
this specific issue again. And for good measure always reset the
index back to zero at the end (we already did this in a few places).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index c960c2aaf328..bd7e781d9d07 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -934,6 +934,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
 	int i, lut_size = drm_color_lut_size(blob);
 	enum pipe pipe = crtc->pipe;
 
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  prec_index);
 	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
 			  PAL_PREC_AUTO_INCREMENT |
 			  prec_index);
@@ -1138,7 +1140,10 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
 	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
 	 */
 	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
-			    PAL_PREC_AUTO_INCREMENT);
+			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
+	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
+			    PAL_PREC_AUTO_INCREMENT |
+			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 
 	for (i = 0; i < 9; i++) {
 		const struct drm_color_lut *entry = &lut[i];
@@ -1148,6 +1153,9 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
 		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
 					    ilk_lut_12p4_udw(entry));
 	}
+
+	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
+			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 }
 
 static void
@@ -1170,6 +1178,8 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
 	 * seg2[0] being unused by the hardware.
 	 */
+	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
+			    PAL_PREC_INDEX_VALUE(0));
 	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
 			    PAL_PREC_AUTO_INCREMENT |
 			    PAL_PREC_INDEX_VALUE(0));
@@ -1202,6 +1212,9 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 					    ilk_lut_12p4_udw(entry));
 	}
 
+	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
+			    PAL_PREC_INDEX_VALUE(0));
+
 	/* The last entry in the LUT is to be programmed in GCMAX */
 	entry = &lut[256 * 8 * 128];
 	ivb_load_lut_max(crtc_state, entry);
@@ -2819,6 +2832,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
 
 	lut = blob->data;
 
+	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
+			  prec_index);
 	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
 			  PAL_PREC_AUTO_INCREMENT |
 			  prec_index);
@@ -2947,6 +2962,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
 
 	lut = blob->data;
 
+	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
+			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
 			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
 			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
-- 
2.37.4

[Intel-gfx] [PATCH 06/13] drm/i915: Document LUT "max" register precision

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Document the precision of the LUT "max" registers, just
so we don't have to dig through the spec so much.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 22fb9fd78483..cd0a445814c7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3620,7 +3620,7 @@
 
 #define  _PIPEAGCMAX           0x70010
 #define  _PIPEBGCMAX           0x71010
-#define PIPEGCMAX(pipe, i)     _MMIO_PIPE2(pipe, _PIPEAGCMAX + (i) * 4)
+#define PIPEGCMAX(pipe, i)     _MMIO_PIPE2(pipe, _PIPEAGCMAX + (i) * 4) /* u1.16 */
 
 #define _PIPE_ARB_CTL_A			0x70028 /* icl+ */
 #define PIPE_ARB_CTL(pipe)		_MMIO_PIPE2(pipe, _PIPE_ARB_CTL_A)
@@ -5304,7 +5304,7 @@
 
 #define  _PREC_PIPEAGCMAX              0x4d000
 #define  _PREC_PIPEBGCMAX              0x4d010
-#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX, _PIPEBGCMAX) + (i) * 4)
+#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX, _PIPEBGCMAX) + (i) * 4) /* u1.16 */
 
 #define _GAMMA_MODE_A		0x4a480
 #define _GAMMA_MODE_B		0x4ac80
@@ -7551,9 +7551,9 @@ enum skl_power_gate {
 
 #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_INDEX_A, _PAL_PREC_INDEX_B)
 #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_DATA_A, _PAL_PREC_DATA_B)
-#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
-#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
-#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4)
+#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4) /* u1.16 */
+#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4) /* u3.16 */
+#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4) /* glk+, u3.16 */
 
 #define _PRE_CSC_GAMC_INDEX_A	0x4A484
 #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
-- 
2.37.4

[Intel-gfx] [PATCH 07/13] drm/i915: Move the DSB->mmio fallback into the LUT code

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The use of DSB has to be done differently on a case by case basis.
So no way this kind of blind mmio fallback in the guts of the DSB
code will work properly. Move it at least one level up into the
LUT loading code. Not sure if this is the way we want do the
DSB vs. mmio handling in the end, but at least it's a bit
closer than what we had before.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 94 ++++++++++++++--------
 drivers/gpu/drm/i915/display/intel_dsb.c   | 18 +----
 2 files changed, 62 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index bd7e781d9d07..5a4f794e1d08 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -836,6 +836,28 @@ static void i965_load_luts(const struct intel_crtc_state *crtc_state)
 	}
 }
 
+static void ilk_lut_write(const struct intel_crtc_state *crtc_state,
+			  i915_reg_t reg, u32 val)
+{
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+
+	if (crtc_state->dsb)
+		intel_dsb_reg_write(crtc_state, reg, val);
+	else
+		intel_de_write_fw(i915, reg, val);
+}
+
+static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
+				  i915_reg_t reg, u32 val)
+{
+	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
+
+	if (crtc_state->dsb)
+		intel_dsb_indexed_reg_write(crtc_state, reg, val);
+	else
+		intel_de_write_fw(i915, reg, val);
+}
+
 static void ilk_load_lut_8(struct intel_crtc *crtc,
 			   const struct drm_property_blob *blob)
 {
@@ -958,9 +980,9 @@ static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
 	enum pipe pipe = crtc->pipe;
 
 	/* Program the max register to clamp values > 1.0. */
-	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
-	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
-	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
+	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
+	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
+	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
 }
 
 static void glk_load_lut_ext2_max(const struct intel_crtc_state *crtc_state)
@@ -969,9 +991,9 @@ static void glk_load_lut_ext2_max(const struct intel_crtc_state *crtc_state)
 	enum pipe pipe = crtc->pipe;
 
 	/* Program the max register to clamp values > 1.0. */
-	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
-	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
-	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
+	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
+	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
+	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
 }
 
 static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
@@ -1118,9 +1140,9 @@ ivb_load_lut_max(const struct intel_crtc_state *crtc_state,
 	enum pipe pipe = crtc->pipe;
 
 	/* FIXME LUT entries are 16 bit only, so we can prog 0xFFFF max */
-	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 0), color->red);
-	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 1), color->green);
-	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 2), color->blue);
+	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 0), color->red);
+	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 1), color->green);
+	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 2), color->blue);
 }
 
 static void
@@ -1139,23 +1161,23 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
 	 * 9 entries, corresponding to values 0, 1/(8 * 128 * 256),
 	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
 	 */
-	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
-			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
-	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
-			    PAL_PREC_AUTO_INCREMENT |
-			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
+		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
+		      PAL_PREC_AUTO_INCREMENT |
+		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 
 	for (i = 0; i < 9; i++) {
 		const struct drm_color_lut *entry = &lut[i];
 
-		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
-					    ilk_lut_12p4_ldw(entry));
-		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
-					    ilk_lut_12p4_udw(entry));
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
+				      ilk_lut_12p4_ldw(entry));
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
+				      ilk_lut_12p4_udw(entry));
 	}
 
-	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
-			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
+		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
 }
 
 static void
@@ -1178,18 +1200,19 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
 	 * seg2[0] being unused by the hardware.
 	 */
-	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
-			    PAL_PREC_INDEX_VALUE(0));
-	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
-			    PAL_PREC_AUTO_INCREMENT |
-			    PAL_PREC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      PAL_PREC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      PAL_PREC_AUTO_INCREMENT |
+		      PAL_PREC_INDEX_VALUE(0));
 
 	for (i = 1; i < 257; i++) {
 		entry = &lut[i * 8];
-		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
-					    ilk_lut_12p4_ldw(entry));
-		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
-					    ilk_lut_12p4_udw(entry));
+
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
+				      ilk_lut_12p4_ldw(entry));
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
+				      ilk_lut_12p4_udw(entry));
 	}
 
 	/*
@@ -1206,14 +1229,15 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
 	 */
 	for (i = 0; i < 256; i++) {
 		entry = &lut[i * 8 * 128];
-		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
-					    ilk_lut_12p4_ldw(entry));
-		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
-					    ilk_lut_12p4_udw(entry));
+
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
+				      ilk_lut_12p4_ldw(entry));
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
+				      ilk_lut_12p4_udw(entry));
 	}
 
-	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
-			    PAL_PREC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      PAL_PREC_INDEX_VALUE(0));
 
 	/* The last entry in the LUT is to be programmed in GCMAX */
 	entry = &lut[256 * 8 * 128];
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 1e1c6107d51b..b4f0356c2463 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -129,14 +129,9 @@ void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
 	struct intel_dsb *dsb = crtc_state->dsb;
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	u32 *buf;
+	u32 *buf = dsb->cmd_buf;
 	u32 reg_val;
 
-	if (!dsb) {
-		intel_de_write_fw(dev_priv, reg, val);
-		return;
-	}
-	buf = dsb->cmd_buf;
 	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
 		drm_dbg_kms(&dev_priv->drm, "DSB buffer overflow\n");
 		return;
@@ -205,16 +200,9 @@ void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct intel_dsb *dsb;
-	u32 *buf;
+	struct intel_dsb *dsb = crtc_state->dsb;
+	u32 *buf = dsb->cmd_buf;
 
-	dsb = crtc_state->dsb;
-	if (!dsb) {
-		intel_de_write_fw(dev_priv, reg, val);
-		return;
-	}
-
-	buf = dsb->cmd_buf;
 	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
 		drm_dbg_kms(&dev_priv->drm, "DSB buffer overflow\n");
 		return;
-- 
2.37.4

[Intel-gfx] [PATCH 08/13] drm/i915: Move the DSB setup/cleaup into the color code

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Since the color management code is the only user of the DSB
at the moment move the DSB prepare/cleanup there too. The
code has to anyway make decisions on whether to use the DSB
or not (and how to use it). Also we'll need a place where we
actually generate the DSB command buffer ahead of time rather
than the current situation where it gets generated too late
during the mmio programming of the hardware.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c   | 10 ++++++++
 drivers/gpu/drm/i915/display/intel_color.h   |  2 ++
 drivers/gpu/drm/i915/display/intel_display.c | 25 ++++++++------------
 drivers/gpu/drm/i915/display/intel_display.h |  8 +++++++
 4 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 5a4f794e1d08..5a8652407f30 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1389,6 +1389,16 @@ void intel_color_commit_arm(const struct intel_crtc_state *crtc_state)
 	i915->display.funcs.color->color_commit_arm(crtc_state);
 }
 
+void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
+{
+	intel_dsb_prepare(crtc_state);
+}
+
+void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
+{
+	intel_dsb_cleanup(crtc_state);
+}
+
 static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
diff --git a/drivers/gpu/drm/i915/display/intel_color.h b/drivers/gpu/drm/i915/display/intel_color.h
index 1c6b1755f6d2..d620b5b1e2a6 100644
--- a/drivers/gpu/drm/i915/display/intel_color.h
+++ b/drivers/gpu/drm/i915/display/intel_color.h
@@ -17,6 +17,8 @@ void intel_color_init_hooks(struct drm_i915_private *i915);
 int intel_color_init(struct drm_i915_private *i915);
 void intel_color_crtc_init(struct intel_crtc *crtc);
 int intel_color_check(struct intel_crtc_state *crtc_state);
+void intel_color_prepare_commit(struct intel_crtc_state *crtc_state);
+void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state);
 void intel_color_commit_noarm(const struct intel_crtc_state *crtc_state);
 void intel_color_commit_arm(const struct intel_crtc_state *crtc_state);
 void intel_color_load_luts(const struct intel_crtc_state *crtc_state);
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 32b257157186..45d7996f5c1a 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -93,7 +93,6 @@
 #include "intel_dp_link_training.h"
 #include "intel_dpio_phy.h"
 #include "intel_dpt.h"
-#include "intel_dsb.h"
 #include "intel_fbc.h"
 #include "intel_fbdev.h"
 #include "intel_fdi.h"
@@ -6931,7 +6930,7 @@ static int intel_atomic_prepare_commit(struct intel_atomic_state *state)
 
 	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
 		if (intel_crtc_needs_color_update(crtc_state))
-			intel_dsb_prepare(crtc_state);
+			intel_color_prepare_commit(crtc_state);
 	}
 
 	return 0;
@@ -7382,24 +7381,18 @@ static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_stat
 		    &wait_reset);
 }
 
-static void intel_cleanup_dsbs(struct intel_atomic_state *state)
-{
-	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
-	struct intel_crtc *crtc;
-	int i;
-
-	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
-					    new_crtc_state, i)
-		intel_dsb_cleanup(old_crtc_state);
-}
-
 static void intel_atomic_cleanup_work(struct work_struct *work)
 {
 	struct intel_atomic_state *state =
 		container_of(work, struct intel_atomic_state, base.commit_work);
 	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	struct intel_crtc_state *old_crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	for_each_old_intel_crtc_in_state(state, crtc, old_crtc_state, i)
+		intel_color_cleanup_commit(old_crtc_state);
 
-	intel_cleanup_dsbs(state);
 	drm_atomic_helper_cleanup_planes(&i915->drm, &state->base);
 	drm_atomic_helper_commit_cleanup_done(&state->base);
 	drm_atomic_state_put(&state->base);
@@ -7590,6 +7583,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 		 * DSB cleanup is done in cleanup_work aligning with framebuffer
 		 * cleanup. So copy and reset the dsb structure to sync with
 		 * commit_done and later do dsb cleanup in cleanup_work.
+		 *
+		 * FIXME get rid of this funny new->old swapping
 		 */
 		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
 	}
@@ -7740,7 +7735,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 		i915_sw_fence_commit(&state->commit_ready);
 
 		for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
-			intel_dsb_cleanup(new_crtc_state);
+			intel_color_cleanup_commit(new_crtc_state);
 
 		drm_atomic_helper_cleanup_planes(dev, &state->base);
 		intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
index 714030136b7f..ef73730f32b0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -440,6 +440,14 @@ enum hpd_pin {
 	     (__i)++) \
 		for_each_if(plane)
 
+#define for_each_old_intel_crtc_in_state(__state, crtc, old_crtc_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->base.dev->mode_config.num_crtc && \
+		     ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+		      (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), 1); \
+	     (__i)++) \
+		for_each_if(crtc)
+
 #define for_each_new_intel_plane_in_state(__state, plane, new_plane_state, __i) \
 	for ((__i) = 0; \
 	     (__i) < (__state)->base.dev->mode_config.num_total_plane && \
-- 
2.37.4

[Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We could have many different uses for the DSB(s) during a
single commit, so the current approach of passing the whole
crtc_state to the DSB functions is far too high level. Lower
the abstraction a little bit so each DSB user can decide where
to stick the command buffer/etc.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 17 +++--
 drivers/gpu/drm/i915/display/intel_dsb.c   | 79 ++++++++++------------
 drivers/gpu/drm/i915/display/intel_dsb.h   | 13 ++--
 3 files changed, 55 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 5a8652407f30..2715f1b617e1 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -842,7 +842,7 @@ static void ilk_lut_write(const struct intel_crtc_state *crtc_state,
 	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
 
 	if (crtc_state->dsb)
-		intel_dsb_reg_write(crtc_state, reg, val);
+		intel_dsb_reg_write(crtc_state->dsb, reg, val);
 	else
 		intel_de_write_fw(i915, reg, val);
 }
@@ -853,7 +853,7 @@ static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
 	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
 
 	if (crtc_state->dsb)
-		intel_dsb_indexed_reg_write(crtc_state, reg, val);
+		intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
 	else
 		intel_de_write_fw(i915, reg, val);
 }
@@ -1273,7 +1273,8 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 		break;
 	}
 
-	intel_dsb_commit(crtc_state);
+	if (crtc_state->dsb)
+		intel_dsb_commit(crtc_state->dsb);
 }
 
 static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
@@ -1391,12 +1392,18 @@ void intel_color_commit_arm(const struct intel_crtc_state *crtc_state)
 
 void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
 {
-	intel_dsb_prepare(crtc_state);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+
+	crtc_state->dsb = intel_dsb_prepare(crtc);
 }
 
 void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
 {
-	intel_dsb_cleanup(crtc_state);
+	if (!crtc_state->dsb)
+		return;
+
+	intel_dsb_cleanup(crtc_state->dsb);
+	crtc_state->dsb = NULL;
 }
 
 static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index b4f0356c2463..ab74bfc89465 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -24,8 +24,10 @@ enum dsb_id {
 
 struct intel_dsb {
 	enum dsb_id id;
+
 	u32 *cmd_buf;
 	struct i915_vma *vma;
+	struct intel_crtc *crtc;
 
 	/*
 	 * free_pos will point the first free entry position
@@ -113,7 +115,7 @@ static bool intel_dsb_disable_engine(struct drm_i915_private *i915,
 /**
  * intel_dsb_indexed_reg_write() -Write to the DSB context for auto
  * increment register.
- * @crtc_state: intel_crtc_state structure
+ * @dsb: DSB context
  * @reg: register address.
  * @val: value.
  *
@@ -123,11 +125,10 @@ static bool intel_dsb_disable_engine(struct drm_i915_private *i915,
  * is done through mmio write.
  */
 
-void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
 				 i915_reg_t reg, u32 val)
 {
-	struct intel_dsb *dsb = crtc_state->dsb;
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct intel_crtc *crtc = dsb->crtc;
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 *buf = dsb->cmd_buf;
 	u32 reg_val;
@@ -195,12 +196,11 @@ void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
  * and rest all erroneous condition register programming is done
  * through mmio write.
  */
-void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_reg_write(struct intel_dsb *dsb,
 			 i915_reg_t reg, u32 val)
 {
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
+	struct intel_crtc *crtc = dsb->crtc;
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
-	struct intel_dsb *dsb = crtc_state->dsb;
 	u32 *buf = dsb->cmd_buf;
 
 	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
@@ -217,17 +217,14 @@ void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
 
 /**
  * intel_dsb_commit() - Trigger workload execution of DSB.
- * @crtc_state: intel_crtc_state structure
+ * @dsb: DSB context
  *
  * This function is used to do actual write to hardware using DSB.
- * On errors, fall back to MMIO. Also this function help to reset the context.
  */
-void intel_dsb_commit(const struct intel_crtc_state *crtc_state)
+void intel_dsb_commit(struct intel_dsb *dsb)
 {
-	struct intel_dsb *dsb = crtc_state->dsb;
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
-	struct drm_device *dev = crtc->base.dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *crtc = dsb->crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
 	u32 tail;
 
@@ -274,14 +271,13 @@ void intel_dsb_commit(const struct intel_crtc_state *crtc_state)
 
 /**
  * intel_dsb_prepare() - Allocate, pin and map the DSB command buffer.
- * @crtc_state: intel_crtc_state structure to prepare associated dsb instance.
+ * @crtc: the CRTC
  *
  * This function prepare the command buffer which is used to store dsb
  * instructions with data.
  */
-void intel_dsb_prepare(struct intel_crtc_state *crtc_state)
+struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc)
 {
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
 	struct intel_dsb *dsb;
 	struct drm_i915_gem_object *obj;
@@ -290,63 +286,60 @@ void intel_dsb_prepare(struct intel_crtc_state *crtc_state)
 	intel_wakeref_t wakeref;
 
 	if (!HAS_DSB(i915))
-		return;
+		return NULL;
 
 	dsb = kmalloc(sizeof(*dsb), GFP_KERNEL);
-	if (!dsb) {
-		drm_err(&i915->drm, "DSB object creation failed\n");
-		return;
-	}
+	if (!dsb)
+		goto out;
 
 	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
 
 	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
-	if (IS_ERR(obj)) {
-		kfree(dsb);
-		goto out;
-	}
+	if (IS_ERR(obj))
+		goto out_put_rpm;
 
 	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
 	if (IS_ERR(vma)) {
 		i915_gem_object_put(obj);
-		kfree(dsb);
-		goto out;
+		goto out_put_rpm;
 	}
 
 	buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
 	if (IS_ERR(buf)) {
 		i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
-		kfree(dsb);
-		goto out;
+		goto out_put_rpm;
 	}
 
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+
 	dsb->id = DSB1;
 	dsb->vma = vma;
+	dsb->crtc = crtc;
 	dsb->cmd_buf = buf;
 	dsb->free_pos = 0;
 	dsb->ins_start_offset = 0;
-	crtc_state->dsb = dsb;
+
+	return dsb;
+
+out_put_rpm:
+	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+	kfree(dsb);
 out:
-	if (!crtc_state->dsb)
-		drm_info(&i915->drm,
-			 "DSB queue setup failed, will fallback to MMIO for display HW programming\n");
+	drm_info_once(&i915->drm,
+		      "DSB queue setup failed, will fallback to MMIO for display HW programming\n");
 
-	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
+	return NULL;
 }
 
 /**
  * intel_dsb_cleanup() - To cleanup DSB context.
- * @crtc_state: intel_crtc_state structure to cleanup associated dsb instance.
+ * @dsb: DSB context
  *
  * This function cleanup the DSB context by unpinning and releasing
  * the VMA object associated with it.
  */
-void intel_dsb_cleanup(struct intel_crtc_state *crtc_state)
+void intel_dsb_cleanup(struct intel_dsb *dsb)
 {
-	if (!crtc_state->dsb)
-		return;
-
-	i915_vma_unpin_and_release(&crtc_state->dsb->vma, I915_VMA_RELEASE_MAP);
-	kfree(crtc_state->dsb);
-	crtc_state->dsb = NULL;
+	i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
+	kfree(dsb);
 }
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
index 74dd2b3343bb..25f13c4d5389 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.h
+++ b/drivers/gpu/drm/i915/display/intel_dsb.h
@@ -10,14 +10,15 @@
 
 #include "i915_reg_defs.h"
 
-struct intel_crtc_state;
+struct intel_crtc;
+struct intel_dsb;
 
-void intel_dsb_prepare(struct intel_crtc_state *crtc_state);
-void intel_dsb_cleanup(struct intel_crtc_state *crtc_state);
-void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
+struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc);
+void intel_dsb_cleanup(struct intel_dsb *dsb);
+void intel_dsb_reg_write(struct intel_dsb *dsb,
 			 i915_reg_t reg, u32 val);
-void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
+void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
 				 i915_reg_t reg, u32 val);
-void intel_dsb_commit(const struct intel_crtc_state *crtc_state);
+void intel_dsb_commit(struct intel_dsb *dsb);
 
 #endif
-- 
2.37.4

[Intel-gfx] [PATCH 10/13] drm/i915: Disable DSB usage specifically for LUTs

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The DSB has problem loading the LUTs at the moment. Some
of that is due to the palette anti collision logic, some
due to what seem real hw issues. Disable it the whole
thing locally in the color management code for now.

Note that we currently have this weird situation where on
adl+ we load parts of the LUT with DSB and parts with mmio.
That is due to the fact that only some parts of the LUT code
are using the DSB register write functions (ivb_load_lut_ext*()),
while the rest is using pure mmio (bdw_load_lut_10()). So now
we'll go back to pure mmio temporarily, until the DSB issues
get fixed (at which point we should be going for pure DSB).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 2715f1b617e1..9978d21f1634 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1394,6 +1394,9 @@ void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
+	/* FIXME DSB has issues loading LUTs, disable it for now */
+	return;
+
 	crtc_state->dsb = intel_dsb_prepare(crtc);
 }
 
-- 
2.37.4

[Intel-gfx] [PATCH 11/13] Revert "drm/i915: Disable DSB usage for now"

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit 99510e1afb4863a225207146bd988064c5fd0629.

DSB is now getting disabled locally in the color management
code so we don't need to apply this big hammer via the device
info (not that we have other DSB users at the moment).

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 414b4bfd514b..d8f0f512c944 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -889,7 +889,7 @@ static const struct intel_device_info jsl_info = {
 	TGL_CURSOR_OFFSETS, \
 	.has_global_mocs = 1, \
 	.has_pxp = 1, \
-	.display.has_dsb = 0 /* FIXME: LUT load is broken with DSB */
+	.display.has_dsb = 1
 
 static const struct intel_device_info tgl_info = {
 	GEN12_FEATURES,
-- 
2.37.4

[Intel-gfx] [PATCH 12/13] drm/i915: Use ilk_lut_write*() for all ilk+ gamma modes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We could use the dsb to load the LUT in any gamma mode, not just
when using the multi-segment mode. So replace the direct mmio
on all ilk+ paths with the wrapper.

There are a few functions (ilk_load_lut_10(), ivb_load_lut_10())
that would never be used on a platform with dsb so we could
skip those, but probably better to keep all this 100% consistent
to avoid people getting confused and copy pasting the wrong thing
when adding a new gamma mode.

The gmch stuff I left with direct mmio since those are fairly
distinct and shouldn't cause too much confusion. Although
I've also pondered about converting everything over to dsb
command buffers and just executing it on the CPU when the
real hw is not available. But dunno if that would actually
be a good idea or not...

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 106 ++++++++++-----------
 1 file changed, 50 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 9978d21f1634..d57631b0bb9a 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -858,10 +858,10 @@ static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
 		intel_de_write_fw(i915, reg, val);
 }
 
-static void ilk_load_lut_8(struct intel_crtc *crtc,
+static void ilk_load_lut_8(const struct intel_crtc_state *crtc_state,
 			   const struct drm_property_blob *blob)
 {
-	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	const struct drm_color_lut *lut;
 	enum pipe pipe = crtc->pipe;
 	int i;
@@ -872,36 +872,35 @@ static void ilk_load_lut_8(struct intel_crtc *crtc,
 	lut = blob->data;
 
 	for (i = 0; i < 256; i++)
-		intel_de_write_fw(i915, LGC_PALETTE(pipe, i),
-				  i9xx_lut_8(&lut[i]));
+		ilk_lut_write(crtc_state, LGC_PALETTE(pipe, i),
+			      i9xx_lut_8(&lut[i]));
 }
 
-static void ilk_load_lut_10(struct intel_crtc *crtc,
+static void ilk_load_lut_10(const struct intel_crtc_state *crtc_state,
 			    const struct drm_property_blob *blob)
 {
-	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	const struct drm_color_lut *lut = blob->data;
 	int i, lut_size = drm_color_lut_size(blob);
 	enum pipe pipe = crtc->pipe;
 
 	for (i = 0; i < lut_size; i++)
-		intel_de_write_fw(i915, PREC_PALETTE(pipe, i),
-				  ilk_lut_10(&lut[i]));
+		ilk_lut_write(crtc_state, PREC_PALETTE(pipe, i),
+			      ilk_lut_10(&lut[i]));
 }
 
 static void ilk_load_luts(const struct intel_crtc_state *crtc_state)
 {
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
 	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
 	const struct drm_property_blob *blob = post_csc_lut ?: pre_csc_lut;
 
 	switch (crtc_state->gamma_mode) {
 	case GAMMA_MODE_MODE_8BIT:
-		ilk_load_lut_8(crtc, blob);
+		ilk_load_lut_8(crtc_state, blob);
 		break;
 	case GAMMA_MODE_MODE_10BIT:
-		ilk_load_lut_10(crtc, blob);
+		ilk_load_lut_10(crtc_state, blob);
 		break;
 	default:
 		MISSING_CASE(crtc_state->gamma_mode);
@@ -922,56 +921,56 @@ static int ivb_lut_10_size(u32 prec_index)
  * "Restriction : Index auto increment mode is not
  *  supported and must not be enabled."
  */
-static void ivb_load_lut_10(struct intel_crtc *crtc,
+static void ivb_load_lut_10(const struct intel_crtc_state *crtc_state,
 			    const struct drm_property_blob *blob,
 			    u32 prec_index)
 {
-	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	const struct drm_color_lut *lut = blob->data;
 	int i, lut_size = drm_color_lut_size(blob);
 	enum pipe pipe = crtc->pipe;
 
 	for (i = 0; i < lut_size; i++) {
-		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-				  prec_index + i);
-		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
-				  ilk_lut_10(&lut[i]));
+		ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+			      prec_index + i);
+		ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
+			      ilk_lut_10(&lut[i]));
 	}
 
 	/*
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  PAL_PREC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      PAL_PREC_INDEX_VALUE(0));
 }
 
 /* On BDW+ the index auto increment mode actually works */
-static void bdw_load_lut_10(struct intel_crtc *crtc,
+static void bdw_load_lut_10(const struct intel_crtc_state *crtc_state,
 			    const struct drm_property_blob *blob,
 			    u32 prec_index)
 {
-	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	const struct drm_color_lut *lut = blob->data;
 	int i, lut_size = drm_color_lut_size(blob);
 	enum pipe pipe = crtc->pipe;
 
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  prec_index);
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  PAL_PREC_AUTO_INCREMENT |
-			  prec_index);
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      prec_index);
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      PAL_PREC_AUTO_INCREMENT |
+		      prec_index);
 
 	for (i = 0; i < lut_size; i++)
-		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
-				  ilk_lut_10(&lut[i]));
+		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
+				      ilk_lut_10(&lut[i]));
 
 	/*
 	 * Reset the index, otherwise it prevents the legacy palette to be
 	 * written properly.
 	 */
-	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
-			  PAL_PREC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
+		      PAL_PREC_INDEX_VALUE(0));
 }
 
 static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
@@ -998,24 +997,23 @@ static void glk_load_lut_ext2_max(const struct intel_crtc_state *crtc_state)
 
 static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
 {
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
 	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
 	const struct drm_property_blob *blob = post_csc_lut ?: pre_csc_lut;
 
 	switch (crtc_state->gamma_mode) {
 	case GAMMA_MODE_MODE_8BIT:
-		ilk_load_lut_8(crtc, blob);
+		ilk_load_lut_8(crtc_state, blob);
 		break;
 	case GAMMA_MODE_MODE_SPLIT:
-		ivb_load_lut_10(crtc, pre_csc_lut, PAL_PREC_SPLIT_MODE |
+		ivb_load_lut_10(crtc_state, pre_csc_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc_state);
-		ivb_load_lut_10(crtc, post_csc_lut, PAL_PREC_SPLIT_MODE |
+		ivb_load_lut_10(crtc_state, post_csc_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(512));
 		break;
 	case GAMMA_MODE_MODE_10BIT:
-		ivb_load_lut_10(crtc, blob,
+		ivb_load_lut_10(crtc_state, blob,
 				PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc_state);
 		break;
@@ -1027,25 +1025,23 @@ static void ivb_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->uapi.crtc);
 	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
 	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
 	const struct drm_property_blob *blob = post_csc_lut ?: pre_csc_lut;
 
 	switch (crtc_state->gamma_mode) {
 	case GAMMA_MODE_MODE_8BIT:
-		ilk_load_lut_8(crtc, blob);
+		ilk_load_lut_8(crtc_state, blob);
 		break;
 	case GAMMA_MODE_MODE_SPLIT:
-		bdw_load_lut_10(crtc, pre_csc_lut, PAL_PREC_SPLIT_MODE |
+		bdw_load_lut_10(crtc_state, pre_csc_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc_state);
-		bdw_load_lut_10(crtc, post_csc_lut, PAL_PREC_SPLIT_MODE |
+		bdw_load_lut_10(crtc_state, post_csc_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(512));
 		break;
 	case GAMMA_MODE_MODE_10BIT:
-
-		bdw_load_lut_10(crtc, blob,
+		bdw_load_lut_10(crtc_state, blob,
 				PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc_state);
 		break;
@@ -1077,11 +1073,11 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
 	 * ignore the index bits, so we need to reset it to index 0
 	 * separately.
 	 */
-	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
-			  PRE_CSC_GAMC_INDEX_VALUE(0));
-	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
-			  PRE_CSC_GAMC_AUTO_INCREMENT |
-			  PRE_CSC_GAMC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe),
+		      PRE_CSC_GAMC_INDEX_VALUE(0));
+	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe),
+		      PRE_CSC_GAMC_AUTO_INCREMENT |
+		      PRE_CSC_GAMC_INDEX_VALUE(0));
 
 	for (i = 0; i < lut_size; i++) {
 		/*
@@ -1097,32 +1093,31 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
 		 * ToDo: Extend to max 7.0. Enable 32 bit input value
 		 * as compared to just 16 to achieve this.
 		 */
-		intel_de_write_fw(i915, PRE_CSC_GAMC_DATA(pipe),
-				  lut[i].green);
+		ilk_lut_write_indexed(crtc_state, PRE_CSC_GAMC_DATA(pipe),
+				      lut[i].green);
 	}
 
 	/* Clamp values > 1.0. */
 	while (i++ < glk_degamma_lut_size(i915))
-		intel_de_write_fw(i915, PRE_CSC_GAMC_DATA(pipe), 1 << 16);
+		ilk_lut_write_indexed(crtc_state, PRE_CSC_GAMC_DATA(pipe), 1 << 16);
 
-	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
+	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe), 0);
 }
 
 static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
 	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
 	if (pre_csc_lut)
 		glk_load_degamma_lut(crtc_state, pre_csc_lut);
 
 	switch (crtc_state->gamma_mode) {
 	case GAMMA_MODE_MODE_8BIT:
-		ilk_load_lut_8(crtc, post_csc_lut);
+		ilk_load_lut_8(crtc_state, post_csc_lut);
 		break;
 	case GAMMA_MODE_MODE_10BIT:
-		bdw_load_lut_10(crtc, post_csc_lut, PAL_PREC_INDEX_VALUE(0));
+		bdw_load_lut_10(crtc_state, post_csc_lut, PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc_state);
 		glk_load_lut_ext2_max(crtc_state);
 		break;
@@ -1248,14 +1243,13 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
 	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 
 	if (pre_csc_lut)
 		glk_load_degamma_lut(crtc_state, pre_csc_lut);
 
 	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
 	case GAMMA_MODE_MODE_8BIT:
-		ilk_load_lut_8(crtc, post_csc_lut);
+		ilk_load_lut_8(crtc_state, post_csc_lut);
 		break;
 	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
 		icl_program_gamma_superfine_segment(crtc_state);
@@ -1264,7 +1258,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 		glk_load_lut_ext2_max(crtc_state);
 		break;
 	case GAMMA_MODE_MODE_10BIT:
-		bdw_load_lut_10(crtc, post_csc_lut, PAL_PREC_INDEX_VALUE(0));
+		bdw_load_lut_10(crtc_state, post_csc_lut, PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc_state);
 		glk_load_lut_ext2_max(crtc_state);
 		break;
-- 
2.37.4

[Intel-gfx] [PATCH 13/13] drm/i915: Do state check for color management changes

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

In order to validate LUT programming more thoroughly let's
do a state check for all color management updates as well.

Not sure we really want this outside CI. It is rather heavy
and color management updates could become rather common
with all the HDR/etc. stuff happening. Maybe we should have
an extra knob for this that we could enable in CI?

v2: Skip for initial_commit to avoid FDI dotclock
    sanity checks/etc. tripping up

Reviewed-by: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_modeset_verify.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_modeset_verify.c b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
index 842d70f0dfd2..9e4767e1b900 100644
--- a/drivers/gpu/drm/i915/display/intel_modeset_verify.c
+++ b/drivers/gpu/drm/i915/display/intel_modeset_verify.c
@@ -228,6 +228,8 @@ void intel_modeset_verify_crtc(struct intel_crtc *crtc,
 			       struct intel_crtc_state *new_crtc_state)
 {
 	if (!intel_crtc_needs_modeset(new_crtc_state) &&
+	    (!intel_crtc_needs_color_update(new_crtc_state) ||
+	     new_crtc_state->inherited) &&
 	    !intel_crtc_needs_fastset(new_crtc_state))
 		return;
 
-- 
2.37.4

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Gamma/DSB prep work

[Patchwork]

== Series Details ==

Series: drm/i915: Gamma/DSB prep work
URL   : https://patchwork.freedesktop.org/series/111262/
State : warning

== Summary ==

Error: dim checkpatch failed
8f9716b0038b drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit
725e303baa7f drm/i915: Clean up GAMMA_MODE defines
-:35: WARNING:LONG_LINE_COMMENT: line length of 101 exceeds 100 columns
#35: FILE: drivers/gpu/drm/i915/i915_reg.h:5318:
+#define  GAMMA_MODE_MODE_SPLIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 3) /* ivb-bdw */

-:36: WARNING:LONG_LINE_COMMENT: line length of 101 exceeds 100 columns
#36: FILE: drivers/gpu/drm/i915/i915_reg.h:5319:
+#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 3) /* icl-tgl */

total: 0 errors, 2 warnings, 0 checks, 22 lines checked
67b6bbe92e61 drm/i915: Define skl+ palette anti-collision bit
bbcbe1d1b741 drm/i915: Clean up various indexed LUT registers
-:197: WARNING:LONG_LINE: line length of 104 exceeds 100 columns
#197: FILE: drivers/gpu/drm/i915/i915_reg.h:7576:
+#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))

total: 0 errors, 1 warnings, 0 checks, 162 lines checked
1c9ea03dbbbe drm/i915: Standardize auto-increment LUT load procedure
8a2f660eccfe drm/i915: Document LUT "max" register precision
-:32: WARNING:LONG_LINE_COMMENT: line length of 105 exceeds 100 columns
#32: FILE: drivers/gpu/drm/i915/i915_reg.h:5307:
+#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX, _PIPEBGCMAX) + (i) * 4) /* u1.16 */

-:43: WARNING:LONG_LINE: line length of 120 exceeds 100 columns
#43: FILE: drivers/gpu/drm/i915/i915_reg.h:7554:
+#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4) /* u1.16 */

-:44: WARNING:LONG_LINE: line length of 128 exceeds 100 columns
#44: FILE: drivers/gpu/drm/i915/i915_reg.h:7555:
+#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4) /* u3.16 */

-:45: WARNING:LONG_LINE: line length of 136 exceeds 100 columns
#45: FILE: drivers/gpu/drm/i915/i915_reg.h:7556:
+#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4) /* glk+, u3.16 */

total: 0 errors, 4 warnings, 0 checks, 28 lines checked
c56e8ed19533 drm/i915: Move the DSB->mmio fallback into the LUT code
643c7b5484d7 drm/i915: Move the DSB setup/cleaup into the color code
-:131: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__state' - possible side-effects?
#131: FILE: drivers/gpu/drm/i915/display/intel_display.h:443:
+#define for_each_old_intel_crtc_in_state(__state, crtc, old_crtc_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->base.dev->mode_config.num_crtc && \
+		     ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+		      (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), 1); \
+	     (__i)++) \
+		for_each_if(crtc)

-:131: CHECK:MACRO_ARG_REUSE: Macro argument reuse 'crtc' - possible side-effects?
#131: FILE: drivers/gpu/drm/i915/display/intel_display.h:443:
+#define for_each_old_intel_crtc_in_state(__state, crtc, old_crtc_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->base.dev->mode_config.num_crtc && \
+		     ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+		      (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), 1); \
+	     (__i)++) \
+		for_each_if(crtc)

-:131: CHECK:MACRO_ARG_REUSE: Macro argument reuse '__i' - possible side-effects?
#131: FILE: drivers/gpu/drm/i915/display/intel_display.h:443:
+#define for_each_old_intel_crtc_in_state(__state, crtc, old_crtc_state, __i) \
+	for ((__i) = 0; \
+	     (__i) < (__state)->base.dev->mode_config.num_crtc && \
+		     ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+		      (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), 1); \
+	     (__i)++) \
+		for_each_if(crtc)

-:135: WARNING:LONG_LINE: line length of 105 exceeds 100 columns
#135: FILE: drivers/gpu/drm/i915/display/intel_display.h:447:
+		      (old_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].old_state), 1); \

total: 0 errors, 1 warnings, 3 checks, 99 lines checked
3376369f655d drm/i915: Make DSB lower level
254311de0fb7 drm/i915: Disable DSB usage specifically for LUTs
029ab662a202 Revert "drm/i915: Disable DSB usage for now"
fcb5168a004e drm/i915: Use ilk_lut_write*() for all ilk+ gamma modes
042ad2520eaf drm/i915: Do state check for color management changes

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: Gamma/DSB prep work

[Patchwork]

== Series Details ==

Series: drm/i915: Gamma/DSB prep work
URL   : https://patchwork.freedesktop.org/series/111262/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.DOCS: warning for drm/i915: Gamma/DSB prep work

[Patchwork]

== Series Details ==

Series: drm/i915: Gamma/DSB prep work
URL   : https://patchwork.freedesktop.org/series/111262/
State : warning

== Summary ==

Error: make htmldocs had i915 warnings
./drivers/gpu/drm/i915/display/intel_dsb.c:201: warning: Excess function parameter 'crtc_state' description in 'intel_dsb_reg_write'
./drivers/gpu/drm/i915/display/intel_dsb.c:201: warning: Function parameter or member 'dsb' not described in 'intel_dsb_reg_write'
./drivers/gpu/drm/i915/display/intel_dsb.c:201: warning: Excess function parameter 'crtc_state' description in 'intel_dsb_reg_write'
./drivers/gpu/drm/i915/gt/intel_gt_mcr.c:739: warning: expecting prototype for intel_gt_mcr_wait_for_reg_fw(). Prototype was for intel_gt_mcr_wait_for_reg() instead
./drivers/gpu/drm/i915/gt/intel_gt_mcr.c:739: warning: expecting prototype for intel_gt_mcr_wait_for_reg_fw(). Prototype was for intel_gt_mcr_wait_for_reg() instead

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Gamma/DSB prep work

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 14148 bytes --]

== Series Details ==

Series: drm/i915: Gamma/DSB prep work
URL   : https://patchwork.freedesktop.org/series/111262/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_12424 -> Patchwork_111262v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/index.html

Participating hosts (37 -> 37)
------------------------------

  Additional (2): fi-icl-u2 bat-dg1-6 
  Missing    (2): fi-ctg-p8600 fi-ilk-m540 

Known issues
------------

  Here are the changes found in Patchwork_111262v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@debugfs_test@basic-hwmon:
    - fi-icl-u2:          NOTRUN -> [SKIP][1] ([i915#7456])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@debugfs_test@basic-hwmon.html

  * igt@gem_huc_copy@huc-copy:
    - fi-icl-u2:          NOTRUN -> [SKIP][2] ([i915#2190])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@random-engines:
    - fi-icl-u2:          NOTRUN -> [SKIP][3] ([i915#4613]) +3 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@gem_lmem_swapping@random-engines.html

  * igt@gem_mmap@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][4] ([i915#4083])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@gem_mmap@basic.html

  * igt@gem_render_tiled_blits@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][5] ([i915#4079]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@gem_render_tiled_blits@basic.html

  * igt@gem_tiled_fence_blits@basic:
    - bat-dg1-6:          NOTRUN -> [SKIP][6] ([i915#4077]) +2 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@gem_tiled_fence_blits@basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - bat-dg1-6:          NOTRUN -> [SKIP][7] ([i915#7561])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg1-6:          NOTRUN -> [SKIP][8] ([i915#6621])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [PASS][9] -> [INCOMPLETE][10] ([i915#4785])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - fi-bdw-gvtdvm:      NOTRUN -> [INCOMPLETE][11] ([i915#146])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-bdw-gvtdvm/igt@i915_suspend@basic-s2idle-without-i915.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       [PASS][12] -> [FAIL][13] ([fdo#103375])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg1-6:          NOTRUN -> [SKIP][14] ([i915#4215])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg1-6:          NOTRUN -> [SKIP][15] ([i915#4212]) +7 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - bat-dg1-5:          NOTRUN -> [SKIP][16] ([fdo#111827])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-5/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - bat-dg1-6:          NOTRUN -> [SKIP][17] ([fdo#111827]) +8 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          NOTRUN -> [SKIP][18] ([fdo#111827]) +8 similar issues
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
    - fi-bdw-gvtdvm:      NOTRUN -> [SKIP][19] ([fdo#109271] / [fdo#111827]) +7 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-bdw-gvtdvm/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - fi-icl-u2:          NOTRUN -> [SKIP][20] ([i915#4103])
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html
    - bat-dg1-6:          NOTRUN -> [SKIP][21] ([i915#4103] / [i915#4213])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_flip@basic-plain-flip:
    - fi-bdw-gvtdvm:      NOTRUN -> [SKIP][22] ([fdo#109271]) +31 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-bdw-gvtdvm/igt@kms_flip@basic-plain-flip.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-icl-u2:          NOTRUN -> [SKIP][23] ([fdo#109285])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@kms_force_connector_basic@force-load-detect.html
    - bat-dg1-6:          NOTRUN -> [SKIP][24] ([fdo#109285])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@sprite_plane_onoff:
    - bat-dg1-6:          NOTRUN -> [SKIP][25] ([i915#1072] / [i915#4078]) +3 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_psr@sprite_plane_onoff.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg1-6:          NOTRUN -> [SKIP][26] ([i915#3555])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@kms_setmode@basic-clone-single-crtc.html
    - fi-icl-u2:          NOTRUN -> [SKIP][27] ([i915#3555])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-gtt:
    - bat-dg1-6:          NOTRUN -> [SKIP][28] ([i915#3708] / [i915#4077]) +1 similar issue
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@prime_vgem@basic-gtt.html

  * igt@prime_vgem@basic-read:
    - bat-dg1-6:          NOTRUN -> [SKIP][29] ([i915#3708]) +3 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-userptr:
    - fi-icl-u2:          NOTRUN -> [SKIP][30] ([fdo#109295] / [i915#3301])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-icl-u2/igt@prime_vgem@basic-userptr.html
    - bat-dg1-6:          NOTRUN -> [SKIP][31] ([i915#3708] / [i915#4873])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-6/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-hsw-4770:        NOTRUN -> [FAIL][32] ([fdo#109271] / [i915#4312] / [i915#5594])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-hsw-4770/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@gem_exec_parallel@engines@contexts:
    - fi-bdw-gvtdvm:      [INCOMPLETE][33] ([i915#7506]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/fi-bdw-gvtdvm/igt@gem_exec_parallel@engines@contexts.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-bdw-gvtdvm/igt@gem_exec_parallel@engines@contexts.html

  * igt@gem_exec_suspend@basic-s3@smem:
    - {bat-rplp-1}:       [DMESG-WARN][35] ([i915#2867]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-rplp-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_module_load@reload:
    - {bat-rpls-2}:       [DMESG-WARN][37] ([i915#6434]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/bat-rpls-2/igt@i915_module_load@reload.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-rpls-2/igt@i915_module_load@reload.html

  * igt@i915_selftest@live@gt_lrc:
    - bat-dg1-5:          [INCOMPLETE][39] -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/bat-dg1-5/igt@i915_selftest@live@gt_lrc.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-dg1-5/igt@i915_selftest@live@gt_lrc.html
    - {bat-rpls-1}:       [INCOMPLETE][41] ([i915#4983]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/bat-rpls-1/igt@i915_selftest@live@gt_lrc.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/bat-rpls-1/igt@i915_selftest@live@gt_lrc.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       [FAIL][43] ([i915#6298]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  
#### Warnings ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-rkl-11600:       [FAIL][45] ([fdo#103375]) -> [INCOMPLETE][46] ([i915#6179])
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12424/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#146]: https://gitlab.freedesktop.org/drm/intel/issues/146
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2867]: https://gitlab.freedesktop.org/drm/intel/issues/2867
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4873]: https://gitlab.freedesktop.org/drm/intel/issues/4873
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5278]: https://gitlab.freedesktop.org/drm/intel/issues/5278
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594
  [i915#6179]: https://gitlab.freedesktop.org/drm/intel/issues/6179
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456
  [i915#7506]: https://gitlab.freedesktop.org/drm/intel/issues/7506
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561


Build changes
-------------

  * Linux: CI_DRM_12424 -> Patchwork_111262v1

  CI-20190529: 20190529
  CI_DRM_12424: c8b2ce6e20662ef30130e65f473b1ff5362765e3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7072: 69ba7163475925cdc69aebbdfa0e87453ae165c7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_111262v1: c8b2ce6e20662ef30130e65f473b1ff5362765e3 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

d9872bfc61b7 drm/i915: Do state check for color management changes
03f19d122d54 drm/i915: Use ilk_lut_write*() for all ilk+ gamma modes
96966b0cdd6c Revert "drm/i915: Disable DSB usage for now"
e8e159f5085c drm/i915: Disable DSB usage specifically for LUTs
85ad86f741eb drm/i915: Make DSB lower level
ffc0bb36e4f2 drm/i915: Move the DSB setup/cleaup into the color code
7bc8ab2a948d drm/i915: Move the DSB->mmio fallback into the LUT code
ba7dc364457c drm/i915: Document LUT "max" register precision
a117833bbed4 drm/i915: Standardize auto-increment LUT load procedure
8c0fa502b949 drm/i915: Clean up various indexed LUT registers
062c98f7dbf1 drm/i915: Define skl+ palette anti-collision bit
2861e424544d drm/i915: Clean up GAMMA_MODE defines
806e23806f89 drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_111262v1/index.html

[-- Attachment #2: Type: text/html, Size: 16329 bytes --]

Re: [Intel-gfx] [PATCH 01/13] drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit

[Nautiyal, Ankit K]

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> s/GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED/GAMMA_MODE_MODE_12BIT_MULTI_SEG/
> to make this thing slightly shorter.
>
> Also fix up the platform comment while at it.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 10 +++++-----
>   drivers/gpu/drm/i915/i915_reg.h            |  2 +-
>   2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 842d58da3128..956b221860e6 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1212,7 +1212,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>   	case GAMMA_MODE_MODE_8BIT:
>   		ilk_load_lut_8(crtc, post_csc_lut);
>   		break;
> -	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>   		icl_program_gamma_superfine_segment(crtc_state);
>   		icl_program_gamma_multi_segment(crtc_state);
>   		ivb_load_lut_ext_max(crtc_state);
> @@ -2091,7 +2091,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>   	else if (DISPLAY_VER(i915) >= 13)
>   		gamma_mode |= GAMMA_MODE_MODE_10BIT;
>   	else
> -		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEG;
>   
>   	return gamma_mode;
>   }
> @@ -2283,7 +2283,7 @@ static int icl_post_csc_lut_precision(const struct intel_crtc_state *crtc_state)
>   		return 8;
>   	case GAMMA_MODE_MODE_10BIT:
>   		return 10;
> -	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>   		return 16;
>   	default:
>   		MISSING_CASE(crtc_state->gamma_mode);
> @@ -2455,7 +2455,7 @@ static bool icl_lut_equal(const struct intel_crtc_state *crtc_state,
>   
>   	/* hw readout broken except for the super fine segment :( */
>   	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
> -	    GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED)
> +	    GAMMA_MODE_MODE_12BIT_MULTI_SEG)
>   		check_size = 9;
>   
>   	return intel_lut_equal(blob1, blob2, check_size,
> @@ -2971,7 +2971,7 @@ static void icl_read_luts(struct intel_crtc_state *crtc_state)
>   	case GAMMA_MODE_MODE_10BIT:
>   		crtc_state->post_csc_lut = bdw_read_lut_10(crtc, PAL_PREC_INDEX_VALUE(0));
>   		break;
> -	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>   		crtc_state->post_csc_lut = icl_read_lut_multi_segment(crtc);
>   		break;
>   	default:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0b90fe6a28f7..b1c314093737 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5316,7 +5316,7 @@
>   #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
>   #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
>   #define  GAMMA_MODE_MODE_SPLIT	(3 << 0) /* ivb-bdw */
> -#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0) /* icl + */
> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	(3 << 0) /* icl-tgl */
>   
>   /* Display Internal Timeout Register */
>   #define RM_TIMEOUT		_MMIO(0x42060)

Re: [Intel-gfx] [PATCH 02/13] drm/i915: Clean up GAMMA_MODE defines

[Nautiyal, Ankit K]

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use REG_BIT() & co. for GAMMA_MODE bits.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b1c314093737..52d289f55ce1 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5309,14 +5309,14 @@
>   #define _GAMMA_MODE_A		0x4a480
>   #define _GAMMA_MODE_B		0x4ac80
>   #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)
> -#define  GAMMA_MODE_MODE_SPLIT	(3 << 0) /* ivb-bdw */
> -#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	(3 << 0) /* icl-tgl */
> +#define  PRE_CSC_GAMMA_ENABLE			REG_BIT(31) /* icl+ */
> +#define  POST_CSC_GAMMA_ENABLE			REG_BIT(30) /* icl+ */
> +#define  GAMMA_MODE_MODE_MASK			REG_GENMASK(1, 0)
> +#define  GAMMA_MODE_MODE_8BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 0)
> +#define  GAMMA_MODE_MODE_10BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 1)
> +#define  GAMMA_MODE_MODE_12BIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 2)
> +#define  GAMMA_MODE_MODE_SPLIT			REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 3) /* ivb-bdw */
> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 3) /* icl-tgl */
>   
>   /* Display Internal Timeout Register */
>   #define RM_TIMEOUT		_MMIO(0x42060)

Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT registers

[Nautiyal, Ankit K]

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Use REG_BIT() & co. for the LUT index registers, and also
> use the REG_FIELD_PREP() stuff a bit more consistently when
> generating the values for said registers.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
>   drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
>   2 files changed, 41 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 956b221860e6..c960c2aaf328 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
>   	enum pipe pipe = crtc->pipe;
>   
>   	for (i = 0; i < lut_size; i++) {
> -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
> +		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +				  prec_index + i);
>   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
>   				  ilk_lut_10(&lut[i]));
>   	}
> @@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
>   	 * Reset the index, otherwise it prevents the legacy palette to be
>   	 * written properly.
>   	 */
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   }
>   
>   /* On BDW+ the index auto increment mode actually works */
> @@ -933,7 +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>   	enum pipe pipe = crtc->pipe;
>   
>   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> +			  PAL_PREC_AUTO_INCREMENT |
> +			  prec_index);
>   
>   	for (i = 0; i < lut_size; i++)
>   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> @@ -943,7 +946,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>   	 * Reset the index, otherwise it prevents the legacy palette to be
>   	 * written properly.
>   	 */
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   }
>   
>   static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
> @@ -1049,9 +1053,11 @@ static void glk_load_degamma_lut(const struct intel_crtc_state *crtc_state,
>   	 * ignore the index bits, so we need to reset it to index 0
>   	 * separately.
>   	 */
> -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
>   	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> +	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
>   
>   	for (i = 0; i < lut_size; i++) {
>   		/*
> @@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   	 * seg2[0] being unused by the hardware.
>   	 */
>   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT);
> +			    PAL_PREC_AUTO_INCREMENT |
> +			    PAL_PREC_INDEX_VALUE(0));
> +
>   	for (i = 1; i < 257; i++) {
>   		entry = &lut[i * 8];
>   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> @@ -2756,7 +2764,8 @@ static struct drm_property_blob *ivb_read_lut_10(struct intel_crtc *crtc,
>   		ilk_lut_10_pack(&lut[i], val);
>   	}
>   
> -	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   
>   	return blob;
>   }
> @@ -2811,7 +2820,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
>   	lut = blob->data;
>   
>   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> +			  PAL_PREC_AUTO_INCREMENT |
> +			  prec_index);
>   
>   	for (i = 0; i < lut_size; i++) {
>   		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe));
> @@ -2819,7 +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
>   		ilk_lut_10_pack(&lut[i], val);
>   	}
>   
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  PAL_PREC_INDEX_VALUE(0));
>   
>   	return blob;
>   }
> @@ -2876,9 +2887,11 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
>   	 * ignore the index bits, so we need to reset it to index 0
>   	 * separately.
>   	 */
> -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
>   	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
>   
>   	for (i = 0; i < lut_size; i++) {
>   		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
> @@ -2888,7 +2901,8 @@ static struct drm_property_blob *glk_read_degamma_lut(struct intel_crtc *crtc)
>   		lut[i].blue = val;
>   	}
>   
> -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> +			  PRE_CSC_GAMC_INDEX_VALUE(0));
>   
>   	return blob;
>   }
> @@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
>   	lut = blob->data;
>   
>   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			  PAL_PREC_AUTO_INCREMENT);
> +			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
> +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   
>   	for (i = 0; i < 9; i++) {
>   		u32 ldw = intel_de_read_fw(i915, PREC_PAL_MULTI_SEG_DATA(pipe));
> @@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
>   		ilk_lut_12p4_pack(&lut[i], ldw, udw);
>   	}
>   
> -	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
> +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   
>   	/*
>   	 * FIXME readouts from PAL_PREC_DATA register aren't giving
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 80ac50d80af4..22fb9fd78483 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7531,11 +7531,10 @@ enum skl_power_gate {
>   #define _PAL_PREC_INDEX_A	0x4A400
>   #define _PAL_PREC_INDEX_B	0x4AC00
>   #define _PAL_PREC_INDEX_C	0x4B400
> -#define   PAL_PREC_10_12_BIT		(0 << 31)
> -#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_SPLIT_MODE		REG_BIT(31)
> +#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
> +#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
> +#define   PAL_PREC_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
>   #define _PAL_PREC_DATA_A	0x4A404
>   #define _PAL_PREC_DATA_B	0x4AC04
>   #define _PAL_PREC_DATA_C	0x4B404
> @@ -7559,7 +7558,9 @@ enum skl_power_gate {
>   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
>   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
>   #define _PRE_CSC_GAMC_INDEX_C	0x4B484
> -#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> +#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
> +#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)


PRE_CSC_GAMC_INDEX_VALUE_MASK till TGL seem to be using bits 0:5. For 
ADL+ this seem to be 0:7 though. Would it make sense to use separate masks?


Regards,

Ankit


> +#define   PRE_CSC_GAMC_INDEX_VALUE(x)	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
>   #define _PRE_CSC_GAMC_DATA_A	0x4A488
>   #define _PRE_CSC_GAMC_DATA_B	0x4AC88
>   #define _PRE_CSC_GAMC_DATA_C	0x4B488
> @@ -7570,8 +7571,9 @@ enum skl_power_gate {
>   /* ICL Multi segmented gamma */
>   #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
>   #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> -#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT		REG_BIT(15)
> -#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
> +#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
> +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4, 0)
> +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
>   
>   #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
>   #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Standardize auto-increment LUT load procedure

[Nautiyal, Ankit K]

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Various gamma units on various platforms have some problems loading
> the LUT index and auto-increment bit at the same time. We have to
> do this in two steps. The first known case was the glk degamma LUT,
> but at least ADL has another known case.
>
> We're not going to suffer too badly from a couple of extra register
> writes here, so let's just standardize on this practice for all
> auto-increment LUT loads/reads. This way we never have to worry about
> this specific issue again. And for good measure always reset the
> index back to zero at the end (we already did this in a few places).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index c960c2aaf328..bd7e781d9d07 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -934,6 +934,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>   	int i, lut_size = drm_color_lut_size(blob);
>   	enum pipe pipe = crtc->pipe;
>   
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  prec_index);
>   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
>   			  PAL_PREC_AUTO_INCREMENT |
>   			  prec_index);
> @@ -1138,7 +1140,10 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>   	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
>   	 */
>   	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT);
> +			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			    PAL_PREC_AUTO_INCREMENT |
> +			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   
>   	for (i = 0; i < 9; i++) {
>   		const struct drm_color_lut *entry = &lut[i];
> @@ -1148,6 +1153,9 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
>   					    ilk_lut_12p4_udw(entry));
>   	}
> +
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   }
>   
>   static void
> @@ -1170,6 +1178,8 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>   	 * seg2[0] being unused by the hardware.
>   	 */
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> +			    PAL_PREC_INDEX_VALUE(0));
>   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
>   			    PAL_PREC_AUTO_INCREMENT |
>   			    PAL_PREC_INDEX_VALUE(0));
> @@ -1202,6 +1212,9 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   					    ilk_lut_12p4_udw(entry));
>   	}
>   
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> +			    PAL_PREC_INDEX_VALUE(0));
> +
>   	/* The last entry in the LUT is to be programmed in GCMAX */
>   	entry = &lut[256 * 8 * 128];
>   	ivb_load_lut_max(crtc_state, entry);
> @@ -2819,6 +2832,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc *crtc,
>   
>   	lut = blob->data;
>   
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  prec_index);
>   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
>   			  PAL_PREC_AUTO_INCREMENT |
>   			  prec_index);
> @@ -2947,6 +2962,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
>   
>   	lut = blob->data;
>   
> +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
>   			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
>   			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));

Re: [Intel-gfx] [PATCH 06/13] drm/i915: Document LUT "max" register precision

[Nautiyal, Ankit K]

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Document the precision of the LUT "max" registers, just
> so we don't have to dig through the spec so much.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_reg.h | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 22fb9fd78483..cd0a445814c7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3620,7 +3620,7 @@
>   
>   #define  _PIPEAGCMAX           0x70010
>   #define  _PIPEBGCMAX           0x71010
> -#define PIPEGCMAX(pipe, i)     _MMIO_PIPE2(pipe, _PIPEAGCMAX + (i) * 4)
> +#define PIPEGCMAX(pipe, i)     _MMIO_PIPE2(pipe, _PIPEAGCMAX + (i) * 4) /* u1.16 */
>   
>   #define _PIPE_ARB_CTL_A			0x70028 /* icl+ */
>   #define PIPE_ARB_CTL(pipe)		_MMIO_PIPE2(pipe, _PIPE_ARB_CTL_A)
> @@ -5304,7 +5304,7 @@
>   
>   #define  _PREC_PIPEAGCMAX              0x4d000
>   #define  _PREC_PIPEBGCMAX              0x4d010
> -#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX, _PIPEBGCMAX) + (i) * 4)
> +#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX, _PIPEBGCMAX) + (i) * 4) /* u1.16 */
>   
>   #define _GAMMA_MODE_A		0x4a480
>   #define _GAMMA_MODE_B		0x4ac80
> @@ -7551,9 +7551,9 @@ enum skl_power_gate {
>   
>   #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_INDEX_A, _PAL_PREC_INDEX_B)
>   #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_DATA_A, _PAL_PREC_DATA_B)
> -#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
> -#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
> -#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4)
> +#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4) /* u1.16 */
> +#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4) /* u3.16 */
> +#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4) /* glk+, u3.16 */
>   
>   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
>   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Move the DSB->mmio fallback into the LUT code

[Nautiyal, Ankit K]

Makes sense to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The use of DSB has to be done differently on a case by case basis.
> So no way this kind of blind mmio fallback in the guts of the DSB
> code will work properly. Move it at least one level up into the
> LUT loading code. Not sure if this is the way we want do the
> DSB vs. mmio handling in the end, but at least it's a bit
> closer than what we had before.	
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 94 ++++++++++++++--------
>   drivers/gpu/drm/i915/display/intel_dsb.c   | 18 +----
>   2 files changed, 62 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index bd7e781d9d07..5a4f794e1d08 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -836,6 +836,28 @@ static void i965_load_luts(const struct intel_crtc_state *crtc_state)
>   	}
>   }
>   
> +static void ilk_lut_write(const struct intel_crtc_state *crtc_state,
> +			  i915_reg_t reg, u32 val)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (crtc_state->dsb)
> +		intel_dsb_reg_write(crtc_state, reg, val);
> +	else
> +		intel_de_write_fw(i915, reg, val);
> +}
> +
> +static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
> +				  i915_reg_t reg, u32 val)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (crtc_state->dsb)
> +		intel_dsb_indexed_reg_write(crtc_state, reg, val);
> +	else
> +		intel_de_write_fw(i915, reg, val);
> +}
> +
>   static void ilk_load_lut_8(struct intel_crtc *crtc,
>   			   const struct drm_property_blob *blob)
>   {
> @@ -958,9 +980,9 @@ static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state)
>   	enum pipe pipe = crtc->pipe;
>   
>   	/* Program the max register to clamp values > 1.0. */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>   }
>   
>   static void glk_load_lut_ext2_max(const struct intel_crtc_state *crtc_state)
> @@ -969,9 +991,9 @@ static void glk_load_lut_ext2_max(const struct intel_crtc_state *crtc_state)
>   	enum pipe pipe = crtc->pipe;
>   
>   	/* Program the max register to clamp values > 1.0. */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
>   }
>   
>   static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
> @@ -1118,9 +1140,9 @@ ivb_load_lut_max(const struct intel_crtc_state *crtc_state,
>   	enum pipe pipe = crtc->pipe;
>   
>   	/* FIXME LUT entries are 16 bit only, so we can prog 0xFFFF max */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 0), color->red);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 1), color->green);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 2), color->blue);
> +	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 0), color->red);
> +	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 1), color->green);
> +	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>   }
>   
>   static void
> @@ -1139,23 +1161,23 @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>   	 * 9 entries, corresponding to values 0, 1/(8 * 128 * 256),
>   	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
>   	 */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT |
> -			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +		      PAL_PREC_AUTO_INCREMENT |
> +		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   
>   	for (i = 0; i < 9; i++) {
>   		const struct drm_color_lut *entry = &lut[i];
>   
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
> -					    ilk_lut_12p4_ldw(entry));
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
> -					    ilk_lut_12p4_udw(entry));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
> +				      ilk_lut_12p4_ldw(entry));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
> +				      ilk_lut_12p4_udw(entry));
>   	}
>   
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>   }
>   
>   static void
> @@ -1178,18 +1200,19 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>   	 * seg2[0] being unused by the hardware.
>   	 */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_INDEX_VALUE(0));
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT |
> -			    PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_AUTO_INCREMENT |
> +		      PAL_PREC_INDEX_VALUE(0));
>   
>   	for (i = 1; i < 257; i++) {
>   		entry = &lut[i * 8];
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_ldw(entry));
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_udw(entry));
> +
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_ldw(entry));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_udw(entry));
>   	}
>   
>   	/*
> @@ -1206,14 +1229,15 @@ icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>   	 */
>   	for (i = 0; i < 256; i++) {
>   		entry = &lut[i * 8 * 128];
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_ldw(entry));
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_udw(entry));
> +
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_ldw(entry));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_udw(entry));
>   	}
>   
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_INDEX_VALUE(0));
>   
>   	/* The last entry in the LUT is to be programmed in GCMAX */
>   	entry = &lut[256 * 8 * 128];
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 1e1c6107d51b..b4f0356c2463 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -129,14 +129,9 @@ void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
>   	struct intel_dsb *dsb = crtc_state->dsb;
>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 *buf;
> +	u32 *buf = dsb->cmd_buf;
>   	u32 reg_val;
>   
> -	if (!dsb) {
> -		intel_de_write_fw(dev_priv, reg, val);
> -		return;
> -	}
> -	buf = dsb->cmd_buf;
>   	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
>   		drm_dbg_kms(&dev_priv->drm, "DSB buffer overflow\n");
>   		return;
> @@ -205,16 +200,9 @@ void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
>   {
>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_dsb *dsb;
> -	u32 *buf;
> +	struct intel_dsb *dsb = crtc_state->dsb;
> +	u32 *buf = dsb->cmd_buf;
>   
> -	dsb = crtc_state->dsb;
> -	if (!dsb) {
> -		intel_de_write_fw(dev_priv, reg, val);
> -		return;
> -	}
> -
> -	buf = dsb->cmd_buf;
>   	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
>   		drm_dbg_kms(&dev_priv->drm, "DSB buffer overflow\n");
>   		return;

Re: [Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level

[Nautiyal, Ankit K]

Patch looks good to me.

There are couple of minor nitpicks mentioned inline.

In any case this is:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We could have many different uses for the DSB(s) during a
> single commit, so the current approach of passing the whole
> crtc_state to the DSB functions is far too high level. Lower
> the abstraction a little bit so each DSB user can decide where
> to stick the command buffer/etc.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 17 +++--
>   drivers/gpu/drm/i915/display/intel_dsb.c   | 79 ++++++++++------------
>   drivers/gpu/drm/i915/display/intel_dsb.h   | 13 ++--
>   3 files changed, 55 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 5a8652407f30..2715f1b617e1 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -842,7 +842,7 @@ static void ilk_lut_write(const struct intel_crtc_state *crtc_state,
>   	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
>   
>   	if (crtc_state->dsb)
> -		intel_dsb_reg_write(crtc_state, reg, val);
> +		intel_dsb_reg_write(crtc_state->dsb, reg, val);
>   	else
>   		intel_de_write_fw(i915, reg, val);
>   }
> @@ -853,7 +853,7 @@ static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
>   	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
>   
>   	if (crtc_state->dsb)
> -		intel_dsb_indexed_reg_write(crtc_state, reg, val);
> +		intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
>   	else
>   		intel_de_write_fw(i915, reg, val);
>   }
> @@ -1273,7 +1273,8 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>   		break;
>   	}
>   
> -	intel_dsb_commit(crtc_state);
> +	if (crtc_state->dsb)
> +		intel_dsb_commit(crtc_state->dsb);
>   }
>   
>   static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color)
> @@ -1391,12 +1392,18 @@ void intel_color_commit_arm(const struct intel_crtc_state *crtc_state)
>   
>   void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
>   {
> -	intel_dsb_prepare(crtc_state);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +
> +	crtc_state->dsb = intel_dsb_prepare(crtc);
>   }
>   
>   void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
>   {
> -	intel_dsb_cleanup(crtc_state);
> +	if (!crtc_state->dsb)
> +		return;
> +
> +	intel_dsb_cleanup(crtc_state->dsb);
> +	crtc_state->dsb = NULL;
>   }
>   
>   static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index b4f0356c2463..ab74bfc89465 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -24,8 +24,10 @@ enum dsb_id {
>   
>   struct intel_dsb {
>   	enum dsb_id id;
> +

Is this extra line required?


>   	u32 *cmd_buf;
>   	struct i915_vma *vma;
> +	struct intel_crtc *crtc;
>   
>   	/*
>   	 * free_pos will point the first free entry position
> @@ -113,7 +115,7 @@ static bool intel_dsb_disable_engine(struct drm_i915_private *i915,
>   /**
>    * intel_dsb_indexed_reg_write() -Write to the DSB context for auto
>    * increment register.
> - * @crtc_state: intel_crtc_state structure
> + * @dsb: DSB context
>    * @reg: register address.
>    * @val: value.
>    *
> @@ -123,11 +125,10 @@ static bool intel_dsb_disable_engine(struct drm_i915_private *i915,
>    * is done through mmio write.
>    */
>   
> -void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
>   				 i915_reg_t reg, u32 val)
>   {
> -	struct intel_dsb *dsb = crtc_state->dsb;
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct intel_crtc *crtc = dsb->crtc;
>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   	u32 *buf = dsb->cmd_buf;
>   	u32 reg_val;
> @@ -195,12 +196,11 @@ void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
>    * and rest all erroneous condition register programming is done
>    * through mmio write.
>    */
> -void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
> +void intel_dsb_reg_write(struct intel_dsb *dsb,
>   			 i915_reg_t reg, u32 val)
>   {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> +	struct intel_crtc *crtc = dsb->crtc;
>   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_dsb *dsb = crtc_state->dsb;
>   	u32 *buf = dsb->cmd_buf;
>   
>   	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
> @@ -217,17 +217,14 @@ void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
>   
>   /**
>    * intel_dsb_commit() - Trigger workload execution of DSB.
> - * @crtc_state: intel_crtc_state structure
> + * @dsb: DSB context
>    *
>    * This function is used to do actual write to hardware using DSB.
> - * On errors, fall back to MMIO. Also this function help to reset the context.
>    */
> -void intel_dsb_commit(const struct intel_crtc_state *crtc_state)
> +void intel_dsb_commit(struct intel_dsb *dsb)
>   {
> -	struct intel_dsb *dsb = crtc_state->dsb;
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> -	struct drm_device *dev = crtc->base.dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *crtc = dsb->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>   	enum pipe pipe = crtc->pipe;
>   	u32 tail;
>   
> @@ -274,14 +271,13 @@ void intel_dsb_commit(const struct intel_crtc_state *crtc_state)
>   
>   /**
>    * intel_dsb_prepare() - Allocate, pin and map the DSB command buffer.
> - * @crtc_state: intel_crtc_state structure to prepare associated dsb instance.
> + * @crtc: the CRTC


We can perhaps document the return type, the dsb context here.

Regards,

Ankit


>    *
>    * This function prepare the command buffer which is used to store dsb
>    * instructions with data.
>    */
> -void intel_dsb_prepare(struct intel_crtc_state *crtc_state)
> +struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc)
>   {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>   	struct intel_dsb *dsb;
>   	struct drm_i915_gem_object *obj;
> @@ -290,63 +286,60 @@ void intel_dsb_prepare(struct intel_crtc_state *crtc_state)
>   	intel_wakeref_t wakeref;
>   
>   	if (!HAS_DSB(i915))
> -		return;
> +		return NULL;
>   
>   	dsb = kmalloc(sizeof(*dsb), GFP_KERNEL);
> -	if (!dsb) {
> -		drm_err(&i915->drm, "DSB object creation failed\n");
> -		return;
> -	}
> +	if (!dsb)
> +		goto out;
>   
>   	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
>   
>   	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
> -	if (IS_ERR(obj)) {
> -		kfree(dsb);
> -		goto out;
> -	}
> +	if (IS_ERR(obj))
> +		goto out_put_rpm;
>   
>   	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
>   	if (IS_ERR(vma)) {
>   		i915_gem_object_put(obj);
> -		kfree(dsb);
> -		goto out;
> +		goto out_put_rpm;
>   	}
>   
>   	buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
>   	if (IS_ERR(buf)) {
>   		i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
> -		kfree(dsb);
> -		goto out;
> +		goto out_put_rpm;
>   	}
>   
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +
>   	dsb->id = DSB1;
>   	dsb->vma = vma;
> +	dsb->crtc = crtc;
>   	dsb->cmd_buf = buf;
>   	dsb->free_pos = 0;
>   	dsb->ins_start_offset = 0;
> -	crtc_state->dsb = dsb;
> +
> +	return dsb;
> +
> +out_put_rpm:
> +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	kfree(dsb);
>   out:
> -	if (!crtc_state->dsb)
> -		drm_info(&i915->drm,
> -			 "DSB queue setup failed, will fallback to MMIO for display HW programming\n");
> +	drm_info_once(&i915->drm,
> +		      "DSB queue setup failed, will fallback to MMIO for display HW programming\n");
>   
> -	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> +	return NULL;
>   }
>   
>   /**
>    * intel_dsb_cleanup() - To cleanup DSB context.
> - * @crtc_state: intel_crtc_state structure to cleanup associated dsb instance.
> + * @dsb: DSB context
>    *
>    * This function cleanup the DSB context by unpinning and releasing
>    * the VMA object associated with it.
>    */
> -void intel_dsb_cleanup(struct intel_crtc_state *crtc_state)
> +void intel_dsb_cleanup(struct intel_dsb *dsb)
>   {
> -	if (!crtc_state->dsb)
> -		return;
> -
> -	i915_vma_unpin_and_release(&crtc_state->dsb->vma, I915_VMA_RELEASE_MAP);
> -	kfree(crtc_state->dsb);
> -	crtc_state->dsb = NULL;
> +	i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
> +	kfree(dsb);
>   }
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 74dd2b3343bb..25f13c4d5389 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -10,14 +10,15 @@
>   
>   #include "i915_reg_defs.h"
>   
> -struct intel_crtc_state;
> +struct intel_crtc;
> +struct intel_dsb;
>   
> -void intel_dsb_prepare(struct intel_crtc_state *crtc_state);
> -void intel_dsb_cleanup(struct intel_crtc_state *crtc_state);
> -void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
> +struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc);
> +void intel_dsb_cleanup(struct intel_dsb *dsb);
> +void intel_dsb_reg_write(struct intel_dsb *dsb,
>   			 i915_reg_t reg, u32 val);
> -void intel_dsb_indexed_reg_write(const struct intel_crtc_state *crtc_state,
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
>   				 i915_reg_t reg, u32 val);
> -void intel_dsb_commit(const struct intel_crtc_state *crtc_state);
> +void intel_dsb_commit(struct intel_dsb *dsb);
>   
>   #endif

Re: [Intel-gfx] [PATCH 10/13] drm/i915: Disable DSB usage specifically for LUTs

[Nautiyal, Ankit K]

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The DSB has problem loading the LUTs at the moment. Some
> of that is due to the palette anti collision logic, some
> due to what seem real hw issues. Disable it the whole
> thing locally in the color management code for now.
>
> Note that we currently have this weird situation where on
> adl+ we load parts of the LUT with DSB and parts with mmio.
> That is due to the fact that only some parts of the LUT code
> are using the DSB register write functions (ivb_load_lut_ext*()),
> while the rest is using pure mmio (bdw_load_lut_10()). So now
> we'll go back to pure mmio temporarily, until the DSB issues
> get fixed (at which point we should be going for pure DSB).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_color.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
> index 2715f1b617e1..9978d21f1634 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1394,6 +1394,9 @@ void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
>   {
>   	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>   
> +	/* FIXME DSB has issues loading LUTs, disable it for now */
> +	return;
> +
>   	crtc_state->dsb = intel_dsb_prepare(crtc);
>   }
>   

Re: [Intel-gfx] [PATCH 11/13] Revert "drm/i915: Disable DSB usage for now"

[Nautiyal, Ankit K]

On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> This reverts commit 99510e1afb4863a225207146bd988064c5fd0629.
>
> DSB is now getting disabled locally in the color management
> code so we don't need to apply this big hammer via the device
> info (not that we have other DSB users at the moment).
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_pci.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index 414b4bfd514b..d8f0f512c944 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -889,7 +889,7 @@ static const struct intel_device_info jsl_info = {
>   	TGL_CURSOR_OFFSETS, \
>   	.has_global_mocs = 1, \
>   	.has_pxp = 1, \
> -	.display.has_dsb = 0 /* FIXME: LUT load is broken with DSB */
> +	.display.has_dsb = 1
>   
>   static const struct intel_device_info tgl_info = {
>   	GEN12_FEATURES,

Re: [Intel-gfx] [PATCH 01/13] drm/i915: Shorten GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:56 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 01/13] drm/i915: Shorten
> GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED a bit
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> s/GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED/GAMMA_MODE_MODE_12B
> IT_MULTI_SEG/
> to make this thing slightly shorter.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> 
> Also fix up the platform comment while at it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 10 +++++-----
>  drivers/gpu/drm/i915/i915_reg.h            |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 842d58da3128..956b221860e6 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1212,7 +1212,7 @@ static void icl_load_luts(const struct intel_crtc_state
> *crtc_state)
>  	case GAMMA_MODE_MODE_8BIT:
>  		ilk_load_lut_8(crtc, post_csc_lut);
>  		break;
> -	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>  		icl_program_gamma_superfine_segment(crtc_state);
>  		icl_program_gamma_multi_segment(crtc_state);
>  		ivb_load_lut_ext_max(crtc_state);
> @@ -2091,7 +2091,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state
> *crtc_state)
>  	else if (DISPLAY_VER(i915) >= 13)
>  		gamma_mode |= GAMMA_MODE_MODE_10BIT;
>  	else
> -		gamma_mode |=
> GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEG;
> 
>  	return gamma_mode;
>  }
> @@ -2283,7 +2283,7 @@ static int icl_post_csc_lut_precision(const struct
> intel_crtc_state *crtc_state)
>  		return 8;
>  	case GAMMA_MODE_MODE_10BIT:
>  		return 10;
> -	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>  		return 16;
>  	default:
>  		MISSING_CASE(crtc_state->gamma_mode);
> @@ -2455,7 +2455,7 @@ static bool icl_lut_equal(const struct intel_crtc_state
> *crtc_state,
> 
>  	/* hw readout broken except for the super fine segment :( */
>  	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
> -	    GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED)
> +	    GAMMA_MODE_MODE_12BIT_MULTI_SEG)
>  		check_size = 9;
> 
>  	return intel_lut_equal(blob1, blob2, check_size, @@ -2971,7 +2971,7 @@
> static void icl_read_luts(struct intel_crtc_state *crtc_state)
>  	case GAMMA_MODE_MODE_10BIT:
>  		crtc_state->post_csc_lut = bdw_read_lut_10(crtc,
> PAL_PREC_INDEX_VALUE(0));
>  		break;
> -	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>  		crtc_state->post_csc_lut = icl_read_lut_multi_segment(crtc);
>  		break;
>  	default:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0b90fe6a28f7..b1c314093737 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5316,7 +5316,7 @@
>  #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
>  #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
>  #define  GAMMA_MODE_MODE_SPLIT	(3 << 0) /* ivb-bdw */
> -#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0) /* icl + */
> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEG	(3 << 0) /* icl-tgl */
> 
>  /* Display Internal Timeout Register */
>  #define RM_TIMEOUT		_MMIO(0x42060)
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 03/13] drm/i915: Define skl+ palette anti-collision bit

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:56 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 03/13] drm/i915: Define skl+ palette anti-collision bit
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I've been frobbing the palette anti-collision logic bit while playing around with DSB.
> Not sure we'll have real use for this but let's define the bit anyways so I don't have to
> carry it around locally.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 52d289f55ce1..80ac50d80af4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5311,6 +5311,7 @@
>  #define GAMMA_MODE(pipe) _MMIO_PIPE(pipe, _GAMMA_MODE_A,
> _GAMMA_MODE_B)
>  #define  PRE_CSC_GAMMA_ENABLE			REG_BIT(31) /* icl+ */
>  #define  POST_CSC_GAMMA_ENABLE			REG_BIT(30) /* icl+ */
> +#define  PALETTE_ANTICOL_DISABLE		REG_BIT(15) /* skl+ */
>  #define  GAMMA_MODE_MODE_MASK			REG_GENMASK(1, 0)
>  #define  GAMMA_MODE_MODE_8BIT
> 	REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 0)
>  #define  GAMMA_MODE_MODE_10BIT
> 	REG_FIELD_PREP(GAMMA_MODE_MODE_MASK, 1)
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT registers

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Nautiyal,
> Ankit K
> Sent: Thursday, December 1, 2022 11:16 AM
> To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT
> registers
> 
> 
> On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Use REG_BIT() & co. for the LUT index registers, and also use the
> > REG_FIELD_PREP() stuff a bit more consistently when generating the
> > values for said registers.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
> >   drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
> >   2 files changed, 41 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 956b221860e6..c960c2aaf328 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> >   	enum pipe pipe = crtc->pipe;
> >
> >   	for (i = 0; i < lut_size; i++) {
> > -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
> > +		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +				  prec_index + i);
> >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> >   				  ilk_lut_10(&lut[i]));
> >   	}
> > @@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> >   	 * Reset the index, otherwise it prevents the legacy palette to be
> >   	 * written properly.
> >   	 */
> > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >   }
> >
> >   /* On BDW+ the index auto increment mode actually works */ @@ -933,7
> > +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
> >   	enum pipe pipe = crtc->pipe;
> >
> >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > +			  PAL_PREC_AUTO_INCREMENT |
> > +			  prec_index);
> >
> >   	for (i = 0; i < lut_size; i++)
> >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe), @@ -943,7 +946,8
> @@
> > static void bdw_load_lut_10(struct intel_crtc *crtc,
> >   	 * Reset the index, otherwise it prevents the legacy palette to be
> >   	 * written properly.
> >   	 */
> > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >   }
> >
> >   static void ivb_load_lut_ext_max(const struct intel_crtc_state
> > *crtc_state) @@ -1049,9 +1053,11 @@ static void glk_load_degamma_lut(const
> struct intel_crtc_state *crtc_state,
> >   	 * ignore the index bits, so we need to reset it to index 0
> >   	 * separately.
> >   	 */
> > -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
> >   	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > +	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> >
> >   	for (i = 0; i < lut_size; i++) {
> >   		/*
> > @@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
> >   	 * seg2[0] being unused by the hardware.
> >   	 */
> >   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> > -			    PAL_PREC_AUTO_INCREMENT);
> > +			    PAL_PREC_AUTO_INCREMENT |
> > +			    PAL_PREC_INDEX_VALUE(0));
> > +
> >   	for (i = 1; i < 257; i++) {
> >   		entry = &lut[i * 8];
> >   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> @@
> > -2756,7 +2764,8 @@ static struct drm_property_blob *ivb_read_lut_10(struct
> intel_crtc *crtc,
> >   		ilk_lut_10_pack(&lut[i], val);
> >   	}
> >
> > -	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >
> >   	return blob;
> >   }
> > @@ -2811,7 +2820,8 @@ static struct drm_property_blob
> *bdw_read_lut_10(struct intel_crtc *crtc,
> >   	lut = blob->data;
> >
> >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > +			  PAL_PREC_AUTO_INCREMENT |
> > +			  prec_index);
> >
> >   	for (i = 0; i < lut_size; i++) {
> >   		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe)); @@ -
> 2819,7
> > +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct intel_crtc
> *crtc,
> >   		ilk_lut_10_pack(&lut[i], val);
> >   	}
> >
> > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > +			  PAL_PREC_INDEX_VALUE(0));
> >
> >   	return blob;
> >   }
> > @@ -2876,9 +2887,11 @@ static struct drm_property_blob
> *glk_read_degamma_lut(struct intel_crtc *crtc)
> >   	 * ignore the index bits, so we need to reset it to index 0
> >   	 * separately.
> >   	 */
> > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> >   	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> >
> >   	for (i = 0; i < lut_size; i++) {
> >   		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
> @@
> > -2888,7 +2901,8 @@ static struct drm_property_blob
> *glk_read_degamma_lut(struct intel_crtc *crtc)
> >   		lut[i].blue = val;
> >   	}
> >
> > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> >
> >   	return blob;
> >   }
> > @@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> >   	lut = blob->data;
> >
> >   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > -			  PAL_PREC_AUTO_INCREMENT);
> > +			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
> > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> >
> >   	for (i = 0; i < 9; i++) {
> >   		u32 ldw = intel_de_read_fw(i915,
> PREC_PAL_MULTI_SEG_DATA(pipe));
> > @@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> >   		ilk_lut_12p4_pack(&lut[i], ldw, udw);
> >   	}
> >
> > -	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
> > +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> >
> >   	/*
> >   	 * FIXME readouts from PAL_PREC_DATA register aren't giving diff
> > --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 80ac50d80af4..22fb9fd78483
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7531,11 +7531,10 @@ enum skl_power_gate {
> >   #define _PAL_PREC_INDEX_A	0x4A400
> >   #define _PAL_PREC_INDEX_B	0x4AC00
> >   #define _PAL_PREC_INDEX_C	0x4B400
> > -#define   PAL_PREC_10_12_BIT		(0 << 31)
> > -#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_SPLIT_MODE		REG_BIT(31)
> > +#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
> > +#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
> > +#define   PAL_PREC_INDEX_VALUE(x)
> 	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
> >   #define _PAL_PREC_DATA_A	0x4A404
> >   #define _PAL_PREC_DATA_B	0x4AC04
> >   #define _PAL_PREC_DATA_C	0x4B404
> > @@ -7559,7 +7558,9 @@ enum skl_power_gate {
> >   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
> >   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> >   #define _PRE_CSC_GAMC_INDEX_C	0x4B484
> > -#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> > +#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
> > +#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)
> 
> 
> PRE_CSC_GAMC_INDEX_VALUE_MASK till TGL seem to be using bits 0:5. For
> ADL+ this seem to be 0:7 though. Would it make sense to use separate masks?

We are using it mostly to reset the counter. Keeping a mask 0:7 should be a superset
and may cover the 0:5 case. Though technically it looks a bit off though.

Regards,
Uma Shankar

> 
> Regards,
> 
> Ankit
> 
> 
> > +#define   PRE_CSC_GAMC_INDEX_VALUE(x)
> 	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
> >   #define _PRE_CSC_GAMC_DATA_A	0x4A488
> >   #define _PRE_CSC_GAMC_DATA_B	0x4AC88
> >   #define _PRE_CSC_GAMC_DATA_C	0x4B488
> > @@ -7570,8 +7571,9 @@ enum skl_power_gate {
> >   /* ICL Multi segmented gamma */
> >   #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
> >   #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> > -#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT
> 	REG_BIT(15)
> > -#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4,
> 0)
> > +#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
> > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4,
> 0)
> > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)
> 	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
> >
> >   #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
> >   #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C

Re: [Intel-gfx] [PATCH 05/13] drm/i915: Standardize auto-increment LUT load procedure

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 05/13] drm/i915: Standardize auto-increment LUT load
> procedure
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Various gamma units on various platforms have some problems loading the LUT
> index and auto-increment bit at the same time. We have to do this in two steps. The
> first known case was the glk degamma LUT, but at least ADL has another known
> case.
> 
> We're not going to suffer too badly from a couple of extra register writes here, so
> let's just standardize on this practice for all auto-increment LUT loads/reads. This
> way we never have to worry about this specific issue again. And for good measure
> always reset the index back to zero at the end (we already did this in a few places).

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index c960c2aaf328..bd7e781d9d07 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -934,6 +934,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>  	int i, lut_size = drm_color_lut_size(blob);
>  	enum pipe pipe = crtc->pipe;
> 
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  prec_index);
>  	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
>  			  PAL_PREC_AUTO_INCREMENT |
>  			  prec_index);
> @@ -1138,7 +1140,10 @@ icl_program_gamma_superfine_segment(const struct
> intel_crtc_state *crtc_state)
>  	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
>  	 */
>  	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT);
> +			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			    PAL_PREC_AUTO_INCREMENT |
> +			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> 
>  	for (i = 0; i < 9; i++) {
>  		const struct drm_color_lut *entry = &lut[i]; @@ -1148,6 +1153,9
> @@ icl_program_gamma_superfine_segment(const struct intel_crtc_state
> *crtc_state)
>  		intel_dsb_indexed_reg_write(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
>  					    ilk_lut_12p4_udw(entry));
>  	}
> +
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>  }
> 
>  static void
> @@ -1170,6 +1178,8 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
>  	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>  	 * seg2[0] being unused by the hardware.
>  	 */
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> +			    PAL_PREC_INDEX_VALUE(0));
>  	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
>  			    PAL_PREC_AUTO_INCREMENT |
>  			    PAL_PREC_INDEX_VALUE(0));
> @@ -1202,6 +1212,9 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
>  					    ilk_lut_12p4_udw(entry));
>  	}
> 
> +	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> +			    PAL_PREC_INDEX_VALUE(0));
> +
>  	/* The last entry in the LUT is to be programmed in GCMAX */
>  	entry = &lut[256 * 8 * 128];
>  	ivb_load_lut_max(crtc_state, entry);
> @@ -2819,6 +2832,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct
> intel_crtc *crtc,
> 
>  	lut = blob->data;
> 
> +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> +			  prec_index);
>  	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
>  			  PAL_PREC_AUTO_INCREMENT |
>  			  prec_index);
> @@ -2947,6 +2962,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> 
>  	lut = blob->data;
> 
> +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>  	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
>  			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
>  			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 06/13] drm/i915: Document LUT "max" register precision

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 06/13] drm/i915: Document LUT "max" register precision
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Document the precision of the LUT "max" registers, just so we don't have to dig
> through the spec so much.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 22fb9fd78483..cd0a445814c7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3620,7 +3620,7 @@
> 
>  #define  _PIPEAGCMAX           0x70010
>  #define  _PIPEBGCMAX           0x71010
> -#define PIPEGCMAX(pipe, i)     _MMIO_PIPE2(pipe, _PIPEAGCMAX + (i) * 4)
> +#define PIPEGCMAX(pipe, i)     _MMIO_PIPE2(pipe, _PIPEAGCMAX + (i) * 4) /* u1.16
> */
> 
>  #define _PIPE_ARB_CTL_A			0x70028 /* icl+ */
>  #define PIPE_ARB_CTL(pipe)		_MMIO_PIPE2(pipe, _PIPE_ARB_CTL_A)
> @@ -5304,7 +5304,7 @@
> 
>  #define  _PREC_PIPEAGCMAX              0x4d000
>  #define  _PREC_PIPEBGCMAX              0x4d010
> -#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX,
> _PIPEBGCMAX) + (i) * 4)
> +#define PREC_PIPEGCMAX(pipe, i)        _MMIO(_PIPE(pipe, _PIPEAGCMAX,
> _PIPEBGCMAX) + (i) * 4) /* u1.16 */
> 
>  #define _GAMMA_MODE_A		0x4a480
>  #define _GAMMA_MODE_B		0x4ac80
> @@ -7551,9 +7551,9 @@ enum skl_power_gate {
> 
>  #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_INDEX_A,
> _PAL_PREC_INDEX_B)
>  #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_DATA_A,
> _PAL_PREC_DATA_B)
> -#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
> -#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
> -#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4)
> +#define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4) /* u1.16 */
> +#define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4) /* u3.16 */
> +#define PREC_PAL_EXT2_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> _PAL_PREC_EXT2_GC_MAX_A, _PAL_PREC_EXT2_GC_MAX_B) + (i) * 4) /* glk+,
> u3.16 */
> 
>  #define _PRE_CSC_GAMC_INDEX_A	0x4A484
>  #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 07/13] drm/i915: Move the DSB->mmio fallback into the LUT code

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 07/13] drm/i915: Move the DSB->mmio fallback into the
> LUT code
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The use of DSB has to be done differently on a case by case basis.
> So no way this kind of blind mmio fallback in the guts of the DSB code will work
> properly. Move it at least one level up into the LUT loading code. Not sure if this is
> the way we want do the DSB vs. mmio handling in the end, but at least it's a bit
> closer than what we had before.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 94 ++++++++++++++--------
>  drivers/gpu/drm/i915/display/intel_dsb.c   | 18 +----
>  2 files changed, 62 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index bd7e781d9d07..5a4f794e1d08 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -836,6 +836,28 @@ static void i965_load_luts(const struct intel_crtc_state
> *crtc_state)
>  	}
>  }
> 
> +static void ilk_lut_write(const struct intel_crtc_state *crtc_state,
> +			  i915_reg_t reg, u32 val)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (crtc_state->dsb)
> +		intel_dsb_reg_write(crtc_state, reg, val);
> +	else
> +		intel_de_write_fw(i915, reg, val);
> +}
> +
> +static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
> +				  i915_reg_t reg, u32 val)
> +{
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (crtc_state->dsb)
> +		intel_dsb_indexed_reg_write(crtc_state, reg, val);
> +	else
> +		intel_de_write_fw(i915, reg, val);
> +}
> +
>  static void ilk_load_lut_8(struct intel_crtc *crtc,
>  			   const struct drm_property_blob *blob)  { @@ -958,9
> +980,9 @@ static void ivb_load_lut_ext_max(const struct intel_crtc_state
> *crtc_state)
>  	enum pipe pipe = crtc->pipe;
> 
>  	/* Program the max register to clamp values > 1.0. */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 0), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 1), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT_GC_MAX(pipe, 2), 1 << 16);
>  }
> 
>  static void glk_load_lut_ext2_max(const struct intel_crtc_state *crtc_state) @@ -
> 969,9 +991,9 @@ static void glk_load_lut_ext2_max(const struct intel_crtc_state
> *crtc_state)
>  	enum pipe pipe = crtc->pipe;
> 
>  	/* Program the max register to clamp values > 1.0. */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 0), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 1), 1 << 16);
> +	ilk_lut_write(crtc_state, PREC_PAL_EXT2_GC_MAX(pipe, 2), 1 << 16);
>  }
> 
>  static void ivb_load_luts(const struct intel_crtc_state *crtc_state) @@ -1118,9
> +1140,9 @@ ivb_load_lut_max(const struct intel_crtc_state *crtc_state,
>  	enum pipe pipe = crtc->pipe;
> 
>  	/* FIXME LUT entries are 16 bit only, so we can prog 0xFFFF max */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 0), color->red);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 1), color->green);
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_GC_MAX(pipe, 2), color->blue);
> +	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 0), color->red);
> +	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 1), color->green);
> +	ilk_lut_write(crtc_state, PREC_PAL_GC_MAX(pipe, 2), color->blue);
>  }
> 
>  static void
> @@ -1139,23 +1161,23 @@ icl_program_gamma_superfine_segment(const struct
> intel_crtc_state *crtc_state)
>  	 * 9 entries, corresponding to values 0, 1/(8 * 128 * 256),
>  	 * 2/(8 * 128 * 256) ... 8/(8 * 128 * 256).
>  	 */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT |
> -			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +		      PAL_PREC_AUTO_INCREMENT |
> +		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> 
>  	for (i = 0; i < 9; i++) {
>  		const struct drm_color_lut *entry = &lut[i];
> 
> -		intel_dsb_indexed_reg_write(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
> -					    ilk_lut_12p4_ldw(entry));
> -		intel_dsb_indexed_reg_write(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
> -					    ilk_lut_12p4_udw(entry));
> +		ilk_lut_write_indexed(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
> +				      ilk_lut_12p4_ldw(entry));
> +		ilk_lut_write_indexed(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
> +				      ilk_lut_12p4_udw(entry));
>  	}
> 
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> -			    PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe),
> +		      PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
>  }
> 
>  static void
> @@ -1178,18 +1200,19 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
>  	 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>  	 * seg2[0] being unused by the hardware.
>  	 */
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_INDEX_VALUE(0));
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_AUTO_INCREMENT |
> -			    PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_AUTO_INCREMENT |
> +		      PAL_PREC_INDEX_VALUE(0));
> 
>  	for (i = 1; i < 257; i++) {
>  		entry = &lut[i * 8];
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_ldw(entry));
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_udw(entry));
> +
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_ldw(entry));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_udw(entry));
>  	}
> 
>  	/*
> @@ -1206,14 +1229,15 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
>  	 */
>  	for (i = 0; i < 256; i++) {
>  		entry = &lut[i * 8 * 128];
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_ldw(entry));
> -		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> -					    ilk_lut_12p4_udw(entry));
> +
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_ldw(entry));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_12p4_udw(entry));
>  	}
> 
> -	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> -			    PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_INDEX_VALUE(0));
> 
>  	/* The last entry in the LUT is to be programmed in GCMAX */
>  	entry = &lut[256 * 8 * 128];
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 1e1c6107d51b..b4f0356c2463 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -129,14 +129,9 @@ void intel_dsb_indexed_reg_write(const struct
> intel_crtc_state *crtc_state,
>  	struct intel_dsb *dsb = crtc_state->dsb;
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 *buf;
> +	u32 *buf = dsb->cmd_buf;
>  	u32 reg_val;
> 
> -	if (!dsb) {
> -		intel_de_write_fw(dev_priv, reg, val);
> -		return;
> -	}
> -	buf = dsb->cmd_buf;
>  	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
>  		drm_dbg_kms(&dev_priv->drm, "DSB buffer overflow\n");
>  		return;
> @@ -205,16 +200,9 @@ void intel_dsb_reg_write(const struct intel_crtc_state
> *crtc_state,  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	struct intel_dsb *dsb;
> -	u32 *buf;
> +	struct intel_dsb *dsb = crtc_state->dsb;
> +	u32 *buf = dsb->cmd_buf;
> 
> -	dsb = crtc_state->dsb;
> -	if (!dsb) {
> -		intel_de_write_fw(dev_priv, reg, val);
> -		return;
> -	}
> -
> -	buf = dsb->cmd_buf;
>  	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
>  		drm_dbg_kms(&dev_priv->drm, "DSB buffer overflow\n");
>  		return;
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 08/13] drm/i915: Move the DSB setup/cleaup into the color code

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 08/13] drm/i915: Move the DSB setup/cleaup into the
> color code
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Since the color management code is the only user of the DSB at the moment move
> the DSB prepare/cleanup there too. The code has to anyway make decisions on
> whether to use the DSB or not (and how to use it). Also we'll need a place where we
> actually generate the DSB command buffer ahead of time rather than the current
> situation where it gets generated too late during the mmio programming of the
> hardware.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c   | 10 ++++++++
>  drivers/gpu/drm/i915/display/intel_color.h   |  2 ++
>  drivers/gpu/drm/i915/display/intel_display.c | 25 ++++++++------------
> drivers/gpu/drm/i915/display/intel_display.h |  8 +++++++
>  4 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 5a4f794e1d08..5a8652407f30 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1389,6 +1389,16 @@ void intel_color_commit_arm(const struct
> intel_crtc_state *crtc_state)
>  	i915->display.funcs.color->color_commit_arm(crtc_state);
>  }
> 
> +void intel_color_prepare_commit(struct intel_crtc_state *crtc_state) {
> +	intel_dsb_prepare(crtc_state);
> +}
> +
> +void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state) {
> +	intel_dsb_cleanup(crtc_state);
> +}
> +
>  static bool intel_can_preload_luts(const struct intel_crtc_state *new_crtc_state)  {
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> diff --git a/drivers/gpu/drm/i915/display/intel_color.h
> b/drivers/gpu/drm/i915/display/intel_color.h
> index 1c6b1755f6d2..d620b5b1e2a6 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.h
> +++ b/drivers/gpu/drm/i915/display/intel_color.h
> @@ -17,6 +17,8 @@ void intel_color_init_hooks(struct drm_i915_private *i915);  int
> intel_color_init(struct drm_i915_private *i915);  void intel_color_crtc_init(struct
> intel_crtc *crtc);  int intel_color_check(struct intel_crtc_state *crtc_state);
> +void intel_color_prepare_commit(struct intel_crtc_state *crtc_state);
> +void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state);
>  void intel_color_commit_noarm(const struct intel_crtc_state *crtc_state);  void
> intel_color_commit_arm(const struct intel_crtc_state *crtc_state);  void
> intel_color_load_luts(const struct intel_crtc_state *crtc_state); diff --git
> a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 32b257157186..45d7996f5c1a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -93,7 +93,6 @@
>  #include "intel_dp_link_training.h"
>  #include "intel_dpio_phy.h"
>  #include "intel_dpt.h"
> -#include "intel_dsb.h"
>  #include "intel_fbc.h"
>  #include "intel_fbdev.h"
>  #include "intel_fdi.h"
> @@ -6931,7 +6930,7 @@ static int intel_atomic_prepare_commit(struct
> intel_atomic_state *state)
> 
>  	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
>  		if (intel_crtc_needs_color_update(crtc_state))
> -			intel_dsb_prepare(crtc_state);
> +			intel_color_prepare_commit(crtc_state);
>  	}
> 
>  	return 0;
> @@ -7382,24 +7381,18 @@ static void intel_atomic_commit_fence_wait(struct
> intel_atomic_state *intel_stat
>  		    &wait_reset);
>  }
> 
> -static void intel_cleanup_dsbs(struct intel_atomic_state *state) -{
> -	struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> -	struct intel_crtc *crtc;
> -	int i;
> -
> -	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> -					    new_crtc_state, i)
> -		intel_dsb_cleanup(old_crtc_state);
> -}
> -
>  static void intel_atomic_cleanup_work(struct work_struct *work)  {
>  	struct intel_atomic_state *state =
>  		container_of(work, struct intel_atomic_state, base.commit_work);
>  	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	struct intel_crtc_state *old_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_old_intel_crtc_in_state(state, crtc, old_crtc_state, i)
> +		intel_color_cleanup_commit(old_crtc_state);
> 
> -	intel_cleanup_dsbs(state);
>  	drm_atomic_helper_cleanup_planes(&i915->drm, &state->base);
>  	drm_atomic_helper_commit_cleanup_done(&state->base);
>  	drm_atomic_state_put(&state->base);
> @@ -7590,6 +7583,8 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
>  		 * DSB cleanup is done in cleanup_work aligning with framebuffer
>  		 * cleanup. So copy and reset the dsb structure to sync with
>  		 * commit_done and later do dsb cleanup in cleanup_work.
> +		 *
> +		 * FIXME get rid of this funny new->old swapping
>  		 */
>  		old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb);
>  	}
> @@ -7740,7 +7735,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		i915_sw_fence_commit(&state->commit_ready);
> 
>  		for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> -			intel_dsb_cleanup(new_crtc_state);
> +			intel_color_cleanup_commit(new_crtc_state);
> 
>  		drm_atomic_helper_cleanup_planes(dev, &state->base);
>  		intel_runtime_pm_put(&dev_priv->runtime_pm, state->wakeref);
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index 714030136b7f..ef73730f32b0 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -440,6 +440,14 @@ enum hpd_pin {
>  	     (__i)++) \
>  		for_each_if(plane)
> 
> +#define for_each_old_intel_crtc_in_state(__state, crtc, old_crtc_state, __i) \
> +	for ((__i) = 0; \
> +	     (__i) < (__state)->base.dev->mode_config.num_crtc && \
> +		     ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
> +		      (old_crtc_state) = to_intel_crtc_state((__state)-
> >base.crtcs[__i].old_state), 1); \
> +	     (__i)++) \
> +		for_each_if(crtc)
> +
>  #define for_each_new_intel_plane_in_state(__state, plane, new_plane_state, __i) \
>  	for ((__i) = 0; \
>  	     (__i) < (__state)->base.dev->mode_config.num_total_plane && \
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Nautiyal,
> Ankit K
> Sent: Thursday, December 1, 2022 11:52 AM
> To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 09/13] drm/i915: Make DSB lower level
> 
> Patch looks good to me.
> 
> There are couple of minor nitpicks mentioned inline.
> 
> In any case this is:
> 
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

Looks good to me as well.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> 
> On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > We could have many different uses for the DSB(s) during a single
> > commit, so the current approach of passing the whole crtc_state to the
> > DSB functions is far too high level. Lower the abstraction a little
> > bit so each DSB user can decide where to stick the command buffer/etc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_color.c | 17 +++--
> >   drivers/gpu/drm/i915/display/intel_dsb.c   | 79 ++++++++++------------
> >   drivers/gpu/drm/i915/display/intel_dsb.h   | 13 ++--
> >   3 files changed, 55 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > b/drivers/gpu/drm/i915/display/intel_color.c
> > index 5a8652407f30..2715f1b617e1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > @@ -842,7 +842,7 @@ static void ilk_lut_write(const struct intel_crtc_state
> *crtc_state,
> >   	struct drm_i915_private *i915 =
> > to_i915(crtc_state->uapi.crtc->dev);
> >
> >   	if (crtc_state->dsb)
> > -		intel_dsb_reg_write(crtc_state, reg, val);
> > +		intel_dsb_reg_write(crtc_state->dsb, reg, val);
> >   	else
> >   		intel_de_write_fw(i915, reg, val);
> >   }
> > @@ -853,7 +853,7 @@ static void ilk_lut_write_indexed(const struct
> intel_crtc_state *crtc_state,
> >   	struct drm_i915_private *i915 =
> > to_i915(crtc_state->uapi.crtc->dev);
> >
> >   	if (crtc_state->dsb)
> > -		intel_dsb_indexed_reg_write(crtc_state, reg, val);
> > +		intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
> >   	else
> >   		intel_de_write_fw(i915, reg, val);
> >   }
> > @@ -1273,7 +1273,8 @@ static void icl_load_luts(const struct intel_crtc_state
> *crtc_state)
> >   		break;
> >   	}
> >
> > -	intel_dsb_commit(crtc_state);
> > +	if (crtc_state->dsb)
> > +		intel_dsb_commit(crtc_state->dsb);
> >   }
> >
> >   static u32 chv_cgm_degamma_ldw(const struct drm_color_lut *color) @@
> > -1391,12 +1392,18 @@ void intel_color_commit_arm(const struct
> > intel_crtc_state *crtc_state)
> >
> >   void intel_color_prepare_commit(struct intel_crtc_state *crtc_state)
> >   {
> > -	intel_dsb_prepare(crtc_state);
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +
> > +	crtc_state->dsb = intel_dsb_prepare(crtc);
> >   }
> >
> >   void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
> >   {
> > -	intel_dsb_cleanup(crtc_state);
> > +	if (!crtc_state->dsb)
> > +		return;
> > +
> > +	intel_dsb_cleanup(crtc_state->dsb);
> > +	crtc_state->dsb = NULL;
> >   }
> >
> >   static bool intel_can_preload_luts(const struct intel_crtc_state
> > *new_crtc_state) diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index b4f0356c2463..ab74bfc89465 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -24,8 +24,10 @@ enum dsb_id {
> >
> >   struct intel_dsb {
> >   	enum dsb_id id;
> > +
> 
> Is this extra line required?
> 
> 
> >   	u32 *cmd_buf;
> >   	struct i915_vma *vma;
> > +	struct intel_crtc *crtc;
> >
> >   	/*
> >   	 * free_pos will point the first free entry position @@ -113,7
> > +115,7 @@ static bool intel_dsb_disable_engine(struct drm_i915_private *i915,
> >   /**
> >    * intel_dsb_indexed_reg_write() -Write to the DSB context for auto
> >    * increment register.
> > - * @crtc_state: intel_crtc_state structure
> > + * @dsb: DSB context
> >    * @reg: register address.
> >    * @val: value.
> >    *
> > @@ -123,11 +125,10 @@ static bool intel_dsb_disable_engine(struct
> drm_i915_private *i915,
> >    * is done through mmio write.
> >    */
> >
> > -void intel_dsb_indexed_reg_write(const struct intel_crtc_state
> > *crtc_state,
> > +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
> >   				 i915_reg_t reg, u32 val)
> >   {
> > -	struct intel_dsb *dsb = crtc_state->dsb;
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct intel_crtc *crtc = dsb->crtc;
> >   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >   	u32 *buf = dsb->cmd_buf;
> >   	u32 reg_val;
> > @@ -195,12 +196,11 @@ void intel_dsb_indexed_reg_write(const struct
> intel_crtc_state *crtc_state,
> >    * and rest all erroneous condition register programming is done
> >    * through mmio write.
> >    */
> > -void intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
> > +void intel_dsb_reg_write(struct intel_dsb *dsb,
> >   			 i915_reg_t reg, u32 val)
> >   {
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	struct intel_crtc *crtc = dsb->crtc;
> >   	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > -	struct intel_dsb *dsb = crtc_state->dsb;
> >   	u32 *buf = dsb->cmd_buf;
> >
> >   	if (drm_WARN_ON(&dev_priv->drm, dsb->free_pos >= DSB_BUF_SIZE)) {
> > @@ -217,17 +217,14 @@ void intel_dsb_reg_write(const struct
> > intel_crtc_state *crtc_state,
> >
> >   /**
> >    * intel_dsb_commit() - Trigger workload execution of DSB.
> > - * @crtc_state: intel_crtc_state structure
> > + * @dsb: DSB context
> >    *
> >    * This function is used to do actual write to hardware using DSB.
> > - * On errors, fall back to MMIO. Also this function help to reset the context.
> >    */
> > -void intel_dsb_commit(const struct intel_crtc_state *crtc_state)
> > +void intel_dsb_commit(struct intel_dsb *dsb)
> >   {
> > -	struct intel_dsb *dsb = crtc_state->dsb;
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > -	struct drm_device *dev = crtc->base.dev;
> > -	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_crtc *crtc = dsb->crtc;
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >   	enum pipe pipe = crtc->pipe;
> >   	u32 tail;
> >
> > @@ -274,14 +271,13 @@ void intel_dsb_commit(const struct
> > intel_crtc_state *crtc_state)
> >
> >   /**
> >    * intel_dsb_prepare() - Allocate, pin and map the DSB command buffer.
> > - * @crtc_state: intel_crtc_state structure to prepare associated dsb instance.
> > + * @crtc: the CRTC
> 
> 
> We can perhaps document the return type, the dsb context here.
> 
> Regards,
> 
> Ankit
> 
> 
> >    *
> >    * This function prepare the command buffer which is used to store dsb
> >    * instructions with data.
> >    */
> > -void intel_dsb_prepare(struct intel_crtc_state *crtc_state)
> > +struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc)
> >   {
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> >   	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >   	struct intel_dsb *dsb;
> >   	struct drm_i915_gem_object *obj;
> > @@ -290,63 +286,60 @@ void intel_dsb_prepare(struct intel_crtc_state
> *crtc_state)
> >   	intel_wakeref_t wakeref;
> >
> >   	if (!HAS_DSB(i915))
> > -		return;
> > +		return NULL;
> >
> >   	dsb = kmalloc(sizeof(*dsb), GFP_KERNEL);
> > -	if (!dsb) {
> > -		drm_err(&i915->drm, "DSB object creation failed\n");
> > -		return;
> > -	}
> > +	if (!dsb)
> > +		goto out;
> >
> >   	wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> >
> >   	obj = i915_gem_object_create_internal(i915, DSB_BUF_SIZE);
> > -	if (IS_ERR(obj)) {
> > -		kfree(dsb);
> > -		goto out;
> > -	}
> > +	if (IS_ERR(obj))
> > +		goto out_put_rpm;
> >
> >   	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
> >   	if (IS_ERR(vma)) {
> >   		i915_gem_object_put(obj);
> > -		kfree(dsb);
> > -		goto out;
> > +		goto out_put_rpm;
> >   	}
> >
> >   	buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
> >   	if (IS_ERR(buf)) {
> >   		i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
> > -		kfree(dsb);
> > -		goto out;
> > +		goto out_put_rpm;
> >   	}
> >
> > +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > +
> >   	dsb->id = DSB1;
> >   	dsb->vma = vma;
> > +	dsb->crtc = crtc;
> >   	dsb->cmd_buf = buf;
> >   	dsb->free_pos = 0;
> >   	dsb->ins_start_offset = 0;
> > -	crtc_state->dsb = dsb;
> > +
> > +	return dsb;
> > +
> > +out_put_rpm:
> > +	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > +	kfree(dsb);
> >   out:
> > -	if (!crtc_state->dsb)
> > -		drm_info(&i915->drm,
> > -			 "DSB queue setup failed, will fallback to MMIO for display
> HW programming\n");
> > +	drm_info_once(&i915->drm,
> > +		      "DSB queue setup failed, will fallback to MMIO for display HW
> > +programming\n");
> >
> > -	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > +	return NULL;
> >   }
> >
> >   /**
> >    * intel_dsb_cleanup() - To cleanup DSB context.
> > - * @crtc_state: intel_crtc_state structure to cleanup associated dsb instance.
> > + * @dsb: DSB context
> >    *
> >    * This function cleanup the DSB context by unpinning and releasing
> >    * the VMA object associated with it.
> >    */
> > -void intel_dsb_cleanup(struct intel_crtc_state *crtc_state)
> > +void intel_dsb_cleanup(struct intel_dsb *dsb)
> >   {
> > -	if (!crtc_state->dsb)
> > -		return;
> > -
> > -	i915_vma_unpin_and_release(&crtc_state->dsb->vma,
> I915_VMA_RELEASE_MAP);
> > -	kfree(crtc_state->dsb);
> > -	crtc_state->dsb = NULL;
> > +	i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
> > +	kfree(dsb);
> >   }
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h
> > b/drivers/gpu/drm/i915/display/intel_dsb.h
> > index 74dd2b3343bb..25f13c4d5389 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> > @@ -10,14 +10,15 @@
> >
> >   #include "i915_reg_defs.h"
> >
> > -struct intel_crtc_state;
> > +struct intel_crtc;
> > +struct intel_dsb;
> >
> > -void intel_dsb_prepare(struct intel_crtc_state *crtc_state); -void
> > intel_dsb_cleanup(struct intel_crtc_state *crtc_state); -void
> > intel_dsb_reg_write(const struct intel_crtc_state *crtc_state,
> > +struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc); void
> > +intel_dsb_cleanup(struct intel_dsb *dsb); void
> > +intel_dsb_reg_write(struct intel_dsb *dsb,
> >   			 i915_reg_t reg, u32 val);
> > -void intel_dsb_indexed_reg_write(const struct intel_crtc_state
> > *crtc_state,
> > +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
> >   				 i915_reg_t reg, u32 val);
> > -void intel_dsb_commit(const struct intel_crtc_state *crtc_state);
> > +void intel_dsb_commit(struct intel_dsb *dsb);
> >
> >   #endif

Re: [Intel-gfx] [PATCH 10/13] drm/i915: Disable DSB usage specifically for LUTs

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 10/13] drm/i915: Disable DSB usage specifically for LUTs
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The DSB has problem loading the LUTs at the moment. Some of that is due to the
> palette anti collision logic, some due to what seem real hw issues. Disable it the
> whole thing locally in the color management code for now.
> 
> Note that we currently have this weird situation where on
> adl+ we load parts of the LUT with DSB and parts with mmio.
> That is due to the fact that only some parts of the LUT code are using the DSB
> register write functions (ivb_load_lut_ext*()), while the rest is using pure mmio
> (bdw_load_lut_10()). So now we'll go back to pure mmio temporarily, until the DSB
> issues get fixed (at which point we should be going for pure DSB).

Yeah, we need to debug and analyze the failure with DSB. If needed,
we can raise a hardware sighting.

For now, let's keep it disabled:
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 2715f1b617e1..9978d21f1634 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1394,6 +1394,9 @@ void intel_color_prepare_commit(struct intel_crtc_state
> *crtc_state)  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> 
> +	/* FIXME DSB has issues loading LUTs, disable it for now */
> +	return;
> +
>  	crtc_state->dsb = intel_dsb_prepare(crtc);  }
> 
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 11/13] Revert "drm/i915: Disable DSB usage for now"

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 11/13] Revert "drm/i915: Disable DSB usage for now"
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This reverts commit 99510e1afb4863a225207146bd988064c5fd0629.
> 
> DSB is now getting disabled locally in the color management code so we don't need
> to apply this big hammer via the device info (not that we have other DSB users at the
> moment).

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c index
> 414b4bfd514b..d8f0f512c944 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -889,7 +889,7 @@ static const struct intel_device_info jsl_info = {
>  	TGL_CURSOR_OFFSETS, \
>  	.has_global_mocs = 1, \
>  	.has_pxp = 1, \
> -	.display.has_dsb = 0 /* FIXME: LUT load is broken with DSB */
> +	.display.has_dsb = 1
> 
>  static const struct intel_device_info tgl_info = {
>  	GEN12_FEATURES,
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 12/13] drm/i915: Use ilk_lut_write*() for all ilk+ gamma modes

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Wednesday, November 23, 2022 8:57 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 12/13] drm/i915: Use ilk_lut_write*() for all ilk+ gamma
> modes
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We could use the dsb to load the LUT in any gamma mode, not just when using the
> multi-segment mode. So replace the direct mmio on all ilk+ paths with the wrapper.
> 
> There are a few functions (ilk_load_lut_10(), ivb_load_lut_10()) that would never be
> used on a platform with dsb so we could skip those, but probably better to keep all
> this 100% consistent to avoid people getting confused and copy pasting the wrong
> thing when adding a new gamma mode.
> 
> The gmch stuff I left with direct mmio since those are fairly distinct and shouldn't
> cause too much confusion. Although I've also pondered about converting everything
> over to dsb command buffers and just executing it on the CPU when the real hw is
> not available. But dunno if that would actually be a good idea or not...

Current approach looks good to me. Converting all to DSB command buffers would be
ideal for consistency, but otherwise also is fine.

Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_color.c | 106 ++++++++++-----------
>  1 file changed, 50 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 9978d21f1634..d57631b0bb9a 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -858,10 +858,10 @@ static void ilk_lut_write_indexed(const struct
> intel_crtc_state *crtc_state,
>  		intel_de_write_fw(i915, reg, val);
>  }
> 
> -static void ilk_load_lut_8(struct intel_crtc *crtc,
> +static void ilk_load_lut_8(const struct intel_crtc_state *crtc_state,
>  			   const struct drm_property_blob *blob)  {
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_color_lut *lut;
>  	enum pipe pipe = crtc->pipe;
>  	int i;
> @@ -872,36 +872,35 @@ static void ilk_load_lut_8(struct intel_crtc *crtc,
>  	lut = blob->data;
> 
>  	for (i = 0; i < 256; i++)
> -		intel_de_write_fw(i915, LGC_PALETTE(pipe, i),
> -				  i9xx_lut_8(&lut[i]));
> +		ilk_lut_write(crtc_state, LGC_PALETTE(pipe, i),
> +			      i9xx_lut_8(&lut[i]));
>  }
> 
> -static void ilk_load_lut_10(struct intel_crtc *crtc,
> +static void ilk_load_lut_10(const struct intel_crtc_state *crtc_state,
>  			    const struct drm_property_blob *blob)  {
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_color_lut *lut = blob->data;
>  	int i, lut_size = drm_color_lut_size(blob);
>  	enum pipe pipe = crtc->pipe;
> 
>  	for (i = 0; i < lut_size; i++)
> -		intel_de_write_fw(i915, PREC_PALETTE(pipe, i),
> -				  ilk_lut_10(&lut[i]));
> +		ilk_lut_write(crtc_state, PREC_PALETTE(pipe, i),
> +			      ilk_lut_10(&lut[i]));
>  }
> 
>  static void ilk_load_luts(const struct intel_crtc_state *crtc_state)  {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
>  	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
>  	const struct drm_property_blob *blob = post_csc_lut ?: pre_csc_lut;
> 
>  	switch (crtc_state->gamma_mode) {
>  	case GAMMA_MODE_MODE_8BIT:
> -		ilk_load_lut_8(crtc, blob);
> +		ilk_load_lut_8(crtc_state, blob);
>  		break;
>  	case GAMMA_MODE_MODE_10BIT:
> -		ilk_load_lut_10(crtc, blob);
> +		ilk_load_lut_10(crtc_state, blob);
>  		break;
>  	default:
>  		MISSING_CASE(crtc_state->gamma_mode);
> @@ -922,56 +921,56 @@ static int ivb_lut_10_size(u32 prec_index)
>   * "Restriction : Index auto increment mode is not
>   *  supported and must not be enabled."
>   */
> -static void ivb_load_lut_10(struct intel_crtc *crtc,
> +static void ivb_load_lut_10(const struct intel_crtc_state *crtc_state,
>  			    const struct drm_property_blob *blob,
>  			    u32 prec_index)
>  {
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_color_lut *lut = blob->data;
>  	int i, lut_size = drm_color_lut_size(blob);
>  	enum pipe pipe = crtc->pipe;
> 
>  	for (i = 0; i < lut_size; i++) {
> -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -				  prec_index + i);
> -		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> -				  ilk_lut_10(&lut[i]));
> +		ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +			      prec_index + i);
> +		ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
> +			      ilk_lut_10(&lut[i]));
>  	}
> 
>  	/*
>  	 * Reset the index, otherwise it prevents the legacy palette to be
>  	 * written properly.
>  	 */
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_INDEX_VALUE(0));
>  }
> 
>  /* On BDW+ the index auto increment mode actually works */ -static void
> bdw_load_lut_10(struct intel_crtc *crtc,
> +static void bdw_load_lut_10(const struct intel_crtc_state *crtc_state,
>  			    const struct drm_property_blob *blob,
>  			    u32 prec_index)
>  {
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_color_lut *lut = blob->data;
>  	int i, lut_size = drm_color_lut_size(blob);
>  	enum pipe pipe = crtc->pipe;
> 
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  prec_index);
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  PAL_PREC_AUTO_INCREMENT |
> -			  prec_index);
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      prec_index);
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_AUTO_INCREMENT |
> +		      prec_index);
> 
>  	for (i = 0; i < lut_size; i++)
> -		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> -				  ilk_lut_10(&lut[i]));
> +		ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> +				      ilk_lut_10(&lut[i]));
> 
>  	/*
>  	 * Reset the index, otherwise it prevents the legacy palette to be
>  	 * written properly.
>  	 */
> -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> -			  PAL_PREC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe),
> +		      PAL_PREC_INDEX_VALUE(0));
>  }
> 
>  static void ivb_load_lut_ext_max(const struct intel_crtc_state *crtc_state) @@ -
> 998,24 +997,23 @@ static void glk_load_lut_ext2_max(const struct intel_crtc_state
> *crtc_state)
> 
>  static void ivb_load_luts(const struct intel_crtc_state *crtc_state)  {
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
>  	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
>  	const struct drm_property_blob *blob = post_csc_lut ?: pre_csc_lut;
> 
>  	switch (crtc_state->gamma_mode) {
>  	case GAMMA_MODE_MODE_8BIT:
> -		ilk_load_lut_8(crtc, blob);
> +		ilk_load_lut_8(crtc_state, blob);
>  		break;
>  	case GAMMA_MODE_MODE_SPLIT:
> -		ivb_load_lut_10(crtc, pre_csc_lut, PAL_PREC_SPLIT_MODE |
> +		ivb_load_lut_10(crtc_state, pre_csc_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc_state);
> -		ivb_load_lut_10(crtc, post_csc_lut, PAL_PREC_SPLIT_MODE |
> +		ivb_load_lut_10(crtc_state, post_csc_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(512));
>  		break;
>  	case GAMMA_MODE_MODE_10BIT:
> -		ivb_load_lut_10(crtc, blob,
> +		ivb_load_lut_10(crtc_state, blob,
>  				PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc_state);
>  		break;
> @@ -1027,25 +1025,23 @@ static void ivb_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->uapi.crtc);
>  	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
>  	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
>  	const struct drm_property_blob *blob = post_csc_lut ?: pre_csc_lut;
> 
>  	switch (crtc_state->gamma_mode) {
>  	case GAMMA_MODE_MODE_8BIT:
> -		ilk_load_lut_8(crtc, blob);
> +		ilk_load_lut_8(crtc_state, blob);
>  		break;
>  	case GAMMA_MODE_MODE_SPLIT:
> -		bdw_load_lut_10(crtc, pre_csc_lut, PAL_PREC_SPLIT_MODE |
> +		bdw_load_lut_10(crtc_state, pre_csc_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc_state);
> -		bdw_load_lut_10(crtc, post_csc_lut, PAL_PREC_SPLIT_MODE |
> +		bdw_load_lut_10(crtc_state, post_csc_lut, PAL_PREC_SPLIT_MODE
> |
>  				PAL_PREC_INDEX_VALUE(512));
>  		break;
>  	case GAMMA_MODE_MODE_10BIT:
> -
> -		bdw_load_lut_10(crtc, blob,
> +		bdw_load_lut_10(crtc_state, blob,
>  				PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc_state);
>  		break;
> @@ -1077,11 +1073,11 @@ static void glk_load_degamma_lut(const struct
> intel_crtc_state *crtc_state,
>  	 * ignore the index bits, so we need to reset it to index 0
>  	 * separately.
>  	 */
> -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> -			  PRE_CSC_GAMC_INDEX_VALUE(0));
> -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> -			  PRE_CSC_GAMC_AUTO_INCREMENT |
> -			  PRE_CSC_GAMC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe),
> +		      PRE_CSC_GAMC_INDEX_VALUE(0));
> +	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe),
> +		      PRE_CSC_GAMC_AUTO_INCREMENT |
> +		      PRE_CSC_GAMC_INDEX_VALUE(0));
> 
>  	for (i = 0; i < lut_size; i++) {
>  		/*
> @@ -1097,32 +1093,31 @@ static void glk_load_degamma_lut(const struct
> intel_crtc_state *crtc_state,
>  		 * ToDo: Extend to max 7.0. Enable 32 bit input value
>  		 * as compared to just 16 to achieve this.
>  		 */
> -		intel_de_write_fw(i915, PRE_CSC_GAMC_DATA(pipe),
> -				  lut[i].green);
> +		ilk_lut_write_indexed(crtc_state, PRE_CSC_GAMC_DATA(pipe),
> +				      lut[i].green);
>  	}
> 
>  	/* Clamp values > 1.0. */
>  	while (i++ < glk_degamma_lut_size(i915))
> -		intel_de_write_fw(i915, PRE_CSC_GAMC_DATA(pipe), 1 << 16);
> +		ilk_lut_write_indexed(crtc_state, PRE_CSC_GAMC_DATA(pipe), 1 <<
> 16);
> 
> -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
> +	ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe), 0);
>  }
> 
>  static void glk_load_luts(const struct intel_crtc_state *crtc_state)  {
>  	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
>  	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> 
>  	if (pre_csc_lut)
>  		glk_load_degamma_lut(crtc_state, pre_csc_lut);
> 
>  	switch (crtc_state->gamma_mode) {
>  	case GAMMA_MODE_MODE_8BIT:
> -		ilk_load_lut_8(crtc, post_csc_lut);
> +		ilk_load_lut_8(crtc_state, post_csc_lut);
>  		break;
>  	case GAMMA_MODE_MODE_10BIT:
> -		bdw_load_lut_10(crtc, post_csc_lut, PAL_PREC_INDEX_VALUE(0));
> +		bdw_load_lut_10(crtc_state, post_csc_lut,
> PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc_state);
>  		glk_load_lut_ext2_max(crtc_state);
>  		break;
> @@ -1248,14 +1243,13 @@ static void icl_load_luts(const struct intel_crtc_state
> *crtc_state)  {
>  	const struct drm_property_blob *pre_csc_lut = crtc_state->pre_csc_lut;
>  	const struct drm_property_blob *post_csc_lut = crtc_state->post_csc_lut;
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> 
>  	if (pre_csc_lut)
>  		glk_load_degamma_lut(crtc_state, pre_csc_lut);
> 
>  	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>  	case GAMMA_MODE_MODE_8BIT:
> -		ilk_load_lut_8(crtc, post_csc_lut);
> +		ilk_load_lut_8(crtc_state, post_csc_lut);
>  		break;
>  	case GAMMA_MODE_MODE_12BIT_MULTI_SEG:
>  		icl_program_gamma_superfine_segment(crtc_state);
> @@ -1264,7 +1258,7 @@ static void icl_load_luts(const struct intel_crtc_state
> *crtc_state)
>  		glk_load_lut_ext2_max(crtc_state);
>  		break;
>  	case GAMMA_MODE_MODE_10BIT:
> -		bdw_load_lut_10(crtc, post_csc_lut, PAL_PREC_INDEX_VALUE(0));
> +		bdw_load_lut_10(crtc_state, post_csc_lut,
> PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc_state);
>  		glk_load_lut_ext2_max(crtc_state);
>  		break;
> --
> 2.37.4

Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various indexed LUT registers

[Shankar, Uma]

> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> > Nautiyal, Ankit K
> > Sent: Thursday, December 1, 2022 11:16 AM
> > To: Ville Syrjala <ville.syrjala@linux.intel.com>;
> > intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 04/13] drm/i915: Clean up various
> > indexed LUT registers
> >
> >
> > On 11/23/2022 8:56 PM, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Use REG_BIT() & co. for the LUT index registers, and also use the
> > > REG_FIELD_PREP() stuff a bit more consistently when generating the
> > > values for said registers.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/display/intel_color.c | 46 +++++++++++++++-------
> > >   drivers/gpu/drm/i915/i915_reg.h            | 18 +++++----
> > >   2 files changed, 41 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> > > b/drivers/gpu/drm/i915/display/intel_color.c
> > > index 956b221860e6..c960c2aaf328 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_color.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_color.c
> > > @@ -910,7 +910,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> > >   	enum pipe pipe = crtc->pipe;
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > > -		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), prec_index++);
> > > +		intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +				  prec_index + i);
> > >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe),
> > >   				  ilk_lut_10(&lut[i]));
> > >   	}
> > > @@ -919,7 +920,8 @@ static void ivb_load_lut_10(struct intel_crtc *crtc,
> > >   	 * Reset the index, otherwise it prevents the legacy palette to be
> > >   	 * written properly.
> > >   	 */
> > > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >   }
> > >
> > >   /* On BDW+ the index auto increment mode actually works */ @@
> > > -933,7
> > > +935,8 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
> > >   	enum pipe pipe = crtc->pipe;
> > >
> > >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > > +			  PAL_PREC_AUTO_INCREMENT |
> > > +			  prec_index);
> > >
> > >   	for (i = 0; i < lut_size; i++)
> > >   		intel_de_write_fw(i915, PREC_PAL_DATA(pipe), @@ -943,7 +946,8
> > @@
> > > static void bdw_load_lut_10(struct intel_crtc *crtc,
> > >   	 * Reset the index, otherwise it prevents the legacy palette to be
> > >   	 * written properly.
> > >   	 */
> > > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >   }
> > >
> > >   static void ivb_load_lut_ext_max(const struct intel_crtc_state
> > > *crtc_state) @@ -1049,9 +1053,11 @@ static void
> > > glk_load_degamma_lut(const
> > struct intel_crtc_state *crtc_state,
> > >   	 * ignore the index bits, so we need to reset it to index 0
> > >   	 * separately.
> > >   	 */
> > > -	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe), 0);
> > >   	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > > +	intel_de_write_fw(i915, PRE_CSC_GAMC_INDEX(pipe),
> > > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > >   		/*
> > > @@ -1165,7 +1171,9 @@ icl_program_gamma_multi_segment(const struct
> > intel_crtc_state *crtc_state)
> > >   	 * seg2[0] being unused by the hardware.
> > >   	 */
> > >   	intel_dsb_reg_write(crtc_state, PREC_PAL_INDEX(pipe),
> > > -			    PAL_PREC_AUTO_INCREMENT);
> > > +			    PAL_PREC_AUTO_INCREMENT |
> > > +			    PAL_PREC_INDEX_VALUE(0));
> > > +
> > >   	for (i = 1; i < 257; i++) {
> > >   		entry = &lut[i * 8];
> > >   		intel_dsb_indexed_reg_write(crtc_state, PREC_PAL_DATA(pipe),
> > @@
> > > -2756,7 +2764,8 @@ static struct drm_property_blob
> > > *ivb_read_lut_10(struct
> > intel_crtc *crtc,
> > >   		ilk_lut_10_pack(&lut[i], val);
> > >   	}
> > >
> > > -	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(dev_priv, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >
> > >   	return blob;
> > >   }
> > > @@ -2811,7 +2820,8 @@ static struct drm_property_blob
> > *bdw_read_lut_10(struct intel_crtc *crtc,
> > >   	lut = blob->data;
> > >
> > >   	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > -			  prec_index | PAL_PREC_AUTO_INCREMENT);
> > > +			  PAL_PREC_AUTO_INCREMENT |
> > > +			  prec_index);
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > >   		u32 val = intel_de_read_fw(i915, PREC_PAL_DATA(pipe)); @@ -
> > 2819,7
> > > +2829,8 @@ static struct drm_property_blob *bdw_read_lut_10(struct
> > > +intel_crtc
> > *crtc,
> > >   		ilk_lut_10_pack(&lut[i], val);
> > >   	}
> > >
> > > -	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_INDEX(pipe),
> > > +			  PAL_PREC_INDEX_VALUE(0));
> > >
> > >   	return blob;
> > >   }
> > > @@ -2876,9 +2887,11 @@ static struct drm_property_blob
> > *glk_read_degamma_lut(struct intel_crtc *crtc)
> > >   	 * ignore the index bits, so we need to reset it to index 0
> > >   	 * separately.
> > >   	 */
> > > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> > >   	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > > -			  PRE_CSC_GAMC_AUTO_INCREMENT);
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > > +			  PRE_CSC_GAMC_AUTO_INCREMENT |
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > >
> > >   	for (i = 0; i < lut_size; i++) {
> > >   		u32 val = intel_de_read_fw(dev_priv, PRE_CSC_GAMC_DATA(pipe));
> > @@
> > > -2888,7 +2901,8 @@ static struct drm_property_blob
> > *glk_read_degamma_lut(struct intel_crtc *crtc)
> > >   		lut[i].blue = val;
> > >   	}
> > >
> > > -	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe), 0);
> > > +	intel_de_write_fw(dev_priv, PRE_CSC_GAMC_INDEX(pipe),
> > > +			  PRE_CSC_GAMC_INDEX_VALUE(0));
> > >
> > >   	return blob;
> > >   }
> > > @@ -2934,7 +2948,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> > >   	lut = blob->data;
> > >
> > >   	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > > -			  PAL_PREC_AUTO_INCREMENT);
> > > +			  PAL_PREC_MULTI_SEG_AUTO_INCREMENT |
> > > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> > >
> > >   	for (i = 0; i < 9; i++) {
> > >   		u32 ldw = intel_de_read_fw(i915,
> > PREC_PAL_MULTI_SEG_DATA(pipe));
> > > @@ -2943,7 +2958,8 @@ icl_read_lut_multi_segment(struct intel_crtc *crtc)
> > >   		ilk_lut_12p4_pack(&lut[i], ldw, udw);
> > >   	}
> > >
> > > -	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe), 0);
> > > +	intel_de_write_fw(i915, PREC_PAL_MULTI_SEG_INDEX(pipe),
> > > +			  PAL_PREC_MULTI_SEG_INDEX_VALUE(0));
> > >
> > >   	/*
> > >   	 * FIXME readouts from PAL_PREC_DATA register aren't giving diff
> > > --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h index 80ac50d80af4..22fb9fd78483
> > > 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7531,11 +7531,10 @@ enum skl_power_gate {
> > >   #define _PAL_PREC_INDEX_A	0x4A400
> > >   #define _PAL_PREC_INDEX_B	0x4AC00
> > >   #define _PAL_PREC_INDEX_C	0x4B400
> > > -#define   PAL_PREC_10_12_BIT		(0 << 31)
> > > -#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_SPLIT_MODE		REG_BIT(31)
> > > +#define   PAL_PREC_AUTO_INCREMENT	REG_BIT(15)
> > > +#define   PAL_PREC_INDEX_VALUE_MASK	REG_GENMASK(9, 0)
> > > +#define   PAL_PREC_INDEX_VALUE(x)
> > 	REG_FIELD_PREP(PAL_PREC_INDEX_VALUE_MASK, (x))
> > >   #define _PAL_PREC_DATA_A	0x4A404
> > >   #define _PAL_PREC_DATA_B	0x4AC04
> > >   #define _PAL_PREC_DATA_C	0x4B404
> > > @@ -7559,7 +7558,9 @@ enum skl_power_gate {
> > >   #define _PRE_CSC_GAMC_INDEX_A	0x4A484
> > >   #define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> > >   #define _PRE_CSC_GAMC_INDEX_C	0x4B484
> > > -#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> > > +#define   PRE_CSC_GAMC_AUTO_INCREMENT	REG_BIT(10)
> > > +#define   PRE_CSC_GAMC_INDEX_VALUE_MASK	REG_GENMASK(7, 0)
> >
> >
> > PRE_CSC_GAMC_INDEX_VALUE_MASK till TGL seem to be using bits 0:5. For
> > ADL+ this seem to be 0:7 though. Would it make sense to use separate masks?
> 
> We are using it mostly to reset the counter. Keeping a mask 0:7 should be a superset
> and may cover the 0:5 case. Though technically it looks a bit off though.

Leaving to your discretion Ville. Looks good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Regards,
> Uma Shankar
> 
> >
> > Regards,
> >
> > Ankit
> >
> >
> > > +#define   PRE_CSC_GAMC_INDEX_VALUE(x)
> > 	REG_FIELD_PREP(PRE_CSC_GAMC_INDEX_VALUE_MASK, (x))
> > >   #define _PRE_CSC_GAMC_DATA_A	0x4A488
> > >   #define _PRE_CSC_GAMC_DATA_B	0x4AC88
> > >   #define _PRE_CSC_GAMC_DATA_C	0x4B488
> > > @@ -7570,8 +7571,9 @@ enum skl_power_gate {
> > >   /* ICL Multi segmented gamma */
> > >   #define _PAL_PREC_MULTI_SEG_INDEX_A	0x4A408
> > >   #define _PAL_PREC_MULTI_SEG_INDEX_B	0x4AC08
> > > -#define  PAL_PREC_MULTI_SEGMENT_AUTO_INCREMENT
> > 	REG_BIT(15)
> > > -#define  PAL_PREC_MULTI_SEGMENT_INDEX_VALUE_MASK	REG_GENMASK(4,
> > 0)
> > > +#define   PAL_PREC_MULTI_SEG_AUTO_INCREMENT	REG_BIT(15)
> > > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK	REG_GENMASK(4,
> > 0)
> > > +#define   PAL_PREC_MULTI_SEG_INDEX_VALUE(x)
> > 	REG_FIELD_PREP(PAL_PREC_MULTI_SEG_INDEX_VALUE_MASK, (x))
> > >
> > >   #define _PAL_PREC_MULTI_SEG_DATA_A	0x4A40C
> > >   #define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C