[PATCH v2 0/4] Enable Multi-segmented-gamma for ICL

[PATCH v2 0/4] Enable Multi-segmented-gamma for ICL

[Shashank Sharma]

This patch series enables programming of Multi-segmented-gamma
palette for ICL.

Shashank Sharma (3):
  drm/i915: Change gamma/degamma_lut_size data type to u32
  drm/i915: Rename ivb_load_lut_10_max
  drm/i915/icl: Add Multi-segmented gamma support

Uma Shankar (1):
  drm/i915/icl: Add register definitions for Multi Segmented gamma

 drivers/gpu/drm/i915/i915_pci.c          |   3 +-
 drivers/gpu/drm/i915/i915_reg.h          |  19 ++++
 drivers/gpu/drm/i915/intel_color.c       | 139 +++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_device_info.h |   4 +-
 4 files changed, 151 insertions(+), 14 deletions(-)

-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/4] drm/i915: Change gamma/degamma_lut_size data type to u32

[Shashank Sharma]

Currently, data type of gamma_lut_size & degamma_lut_size elements
in intel_device_info is u16, which means it can accommodate maximum
64k values. In case of ICL multisegmented gamma, the size of gamma
LUT is 256K.

This patch changes the data type of both of these elements to u32.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Uma Shankar <uma.shankar@intel.com>

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_device_info.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 5a2e17d6146b..67677c356716 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -179,8 +179,8 @@ struct intel_device_info {
 	int cursor_offsets[I915_MAX_PIPES];
 
 	struct color_luts {
-		u16 degamma_lut_size;
-		u16 gamma_lut_size;
+		u32 degamma_lut_size;
+		u32 gamma_lut_size;
 		u32 degamma_lut_tests;
 		u32 gamma_lut_tests;
 	} color;
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 2/4] drm/i915/icl: Add register definitions for Multi Segmented gamma

[Shashank Sharma]

From: Uma Shankar <uma.shankar@intel.com>

Add macros to define multi segmented gamma registers

V2: Addressed Ville's comments:
    	Add gen-lable before bit definition
    Addressed Jani's comment
	- Use REG_GENMASK() and REG_BIT()

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6f0a0866c802..7d10b8d00d64 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7198,7 +7198,10 @@ enum {
 #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
 #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
 #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
+/* ivb-bdw */
 #define  GAMMA_MODE_MODE_SPLIT	(3 << 0)
+/* icl + */
+#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0)
 
 /* DMC/CSR */
 #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
@@ -10145,6 +10148,22 @@ enum skl_power_gate {
 #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
 #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
 
+/* Add registers for Gen11 Multi Segmented Gamma Mode */
+#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_DATA_A	0x4A40C
+#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
+
+#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
+					_PAL_PREC_MULTI_SEG_INDEX_A, \
+					_PAL_PREC_MULTI_SEG_INDEX_B)
+#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
+					_PAL_PREC_MULTI_SEG_DATA_A, \
+					_PAL_PREC_MULTI_SEG_DATA_B)
+
 /* pipe CSC & degamma/gamma LUTs on CHV */
 #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
 #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 3/4] drm/i915: Rename ivb_load_lut_10_max

[Shashank Sharma]

This patch renames function ivb_load_lut_10_max to
ivb_load_lut_ext_max.

Cc: Uma Shankar <uma.shankar@intel.com>
Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_color.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 962db1236970..6c341bea514c 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -607,7 +607,7 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
 	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
 }
 
-static void ivb_load_lut_10_max(struct intel_crtc *crtc)
+static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
 {
 	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	enum pipe pipe = crtc->pipe;
@@ -640,7 +640,7 @@ static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
 	} else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT) {
 		ivb_load_lut_10(crtc, degamma_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(0));
-		ivb_load_lut_10_max(crtc);
+		ivb_load_lut_ext_max(crtc);
 		ivb_load_lut_10(crtc, gamma_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(512));
 	} else {
@@ -648,7 +648,7 @@ static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
 
 		ivb_load_lut_10(crtc, blob,
 				PAL_PREC_INDEX_VALUE(0));
-		ivb_load_lut_10_max(crtc);
+		ivb_load_lut_ext_max(crtc);
 	}
 }
 
@@ -663,7 +663,7 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state)
 	} else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT) {
 		bdw_load_lut_10(crtc, degamma_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(0));
-		ivb_load_lut_10_max(crtc);
+		ivb_load_lut_ext_max(crtc);
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_SPLIT_MODE |
 				PAL_PREC_INDEX_VALUE(512));
 	} else {
@@ -671,7 +671,7 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state)
 
 		bdw_load_lut_10(crtc, blob,
 				PAL_PREC_INDEX_VALUE(0));
-		ivb_load_lut_10_max(crtc);
+		ivb_load_lut_ext_max(crtc);
 	}
 }
 
@@ -763,7 +763,7 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 		i9xx_load_luts(crtc_state);
 	} else {
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
-		ivb_load_lut_10_max(crtc);
+		ivb_load_lut_ext_max(crtc);
 	}
 }
 
@@ -780,7 +780,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 		i9xx_load_luts(crtc_state);
 	} else {
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
-		ivb_load_lut_10_max(crtc);
+		ivb_load_lut_ext_max(crtc);
 	}
 }
 
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Shashank Sharma]

ICL introduces a new gamma correction mode in display engine, called
multi-segmented-gamma mode. This mode allows users to program the
darker region of the gamma curve with sueprfine precision. An
example use case for this is HDR curves (like PQ ST-2084).

If we plot a gamma correction curve from value range between 0.0 to 1.0,
ICL's multi-segment has 3 different sections:
- superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
- fine segment: 257 values, ranges between 0 - 1/(128)
- corase segment: 257 values, ranges between 0 - 1

This patch:
- Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
  so that userspace can program with highest precision supported.
- Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
- Adds functions to program/detect multi-segment gamma.

V2: Addressed review comments from Ville
    - separate function for superfine and fine segments.
    - remove enum for segments.
    - reuse last entry of the LUT as gc_max value.
    - replace if() ....cond with switch...case in icl_load_luts.
    - add an entry variable, instead of 'word'

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c    |   3 +-
 drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
 2 files changed, 123 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index ffa2ee70a03d..83698951760b 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
 	GEN(11), \
 	.ddb_size = 2048, \
 	.has_logical_ring_elsq = 1, \
-	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
+	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
+
 
 static const struct intel_device_info intel_icelake_11_info = {
 	GEN11_FEATURES,
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 6c341bea514c..49831e8d02fb 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -41,6 +41,9 @@
 #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
 
 #define LEGACY_LUT_LENGTH		256
+#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
+#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
+
 /*
  * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
  * format). This macro takes the coefficient we want transformed and the
@@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
 	}
 }
 
+/* ilk+ "12.4" interpolated format (high 10 bits) */
+static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
+{
+	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
+		(color->blue >> 6);
+}
+
+/* ilk+ "12.4" interpolated format (low 6 bits) */
+static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
+{
+	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
+		(color->blue & 0x3f);
+}
+
+static void
+icl_load_gcmax(const struct intel_crtc_state *crtc_state,
+			const struct drm_color_lut *entry)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	enum pipe pipe = crtc->pipe;
+
+	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
+	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
+	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
+	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
+}
+
+static void
+icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
+	const struct drm_color_lut *lut = blob->data;
+	enum pipe pipe = crtc->pipe;
+	u32 i;
+
+	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
+		return;
+
+	/*
+	 * Every entry in the multi-segment LUT is corresponding to a superfine
+	 * segment step which is 1/(8 * 128 * 256).
+	 *
+	 * Superfine segment has 9 entries, corresponding to values
+	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
+	 */
+	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+
+	for (i = 0; i < 9; i++) {
+		const struct drm_color_lut *entry = &lut[i];
+
+		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
+			   ilk_lut_12p4_udw(entry));
+		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
+			   ilk_lut_12p4_ldw(entry));
+	}
+}
+
+static void
+icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
+	const struct drm_color_lut *lut = blob->data;
+	const struct drm_color_lut *entry;
+	enum pipe pipe = crtc->pipe;
+	u32 i;
+
+	if (!lut || (drm_color_lut_size(blob) < ICL_GAMMA_MULTISEG_LUT_LENGTH))
+		return;
+
+	/*
+	 * Every entry in the multi-segment LUT is corresponding to a superfine
+	 * segment step which is 1/(8*128*256).
+	 *
+	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
+	 * ... 256/(128*256). So in order to program fine segment of LUT we
+	 * need to pick every 8'th entry in LUT, and program 256 indexes.
+	 * Fine segment's index 0 is programmed in HW, and it starts from
+	 * index 1.
+	 */
+	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+	for (i = 1; i < 257; i++) {
+		entry = &lut[i * 8];
+		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
+		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
+	}
+
+	/*
+	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
+	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
+	 * above, we need to pick every 8 * 128 = 1024th entry in LUT, and
+	 * program 256 of those.
+	 */
+	for (i = 0; i < 256; i++) {
+		entry = &lut[i * 1024];
+		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
+		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
+	}
+
+	icl_load_gcmax(crtc_state, entry);
+	ivb_load_lut_ext_max(crtc);
+}
+
 static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 {
 	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
@@ -775,10 +885,17 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
 	if (crtc_state->base.degamma_lut)
 		glk_load_degamma_lut(crtc_state);
 
-	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
-	    GAMMA_MODE_MODE_8BIT) {
+	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
+	case GAMMA_MODE_MODE_8BIT:
 		i9xx_load_luts(crtc_state);
-	} else {
+		break;
+
+	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
+		icl_program_gamma_superfine_segment(crtc_state);
+		icl_program_gamma_multi_segment(crtc_state);
+		break;
+
+	default:
 		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
 		ivb_load_lut_ext_max(crtc);
 	}
@@ -1209,7 +1326,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
 	    crtc_state_is_legacy_gamma(crtc_state))
 		gamma_mode |= GAMMA_MODE_MODE_8BIT;
 	else
-		gamma_mode |= GAMMA_MODE_MODE_10BIT;
+		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
 
 	return gamma_mode;
 }
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Enable Multi-segmented-gamma for ICL

[Patchwork]

== Series Details ==

Series: Enable Multi-segmented-gamma for ICL
URL   : https://patchwork.freedesktop.org/series/60126/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
dc4b88abc4e0 drm/i915: Change gamma/degamma_lut_size data type to u32
8811262cd9fa drm/i915/icl: Add register definitions for Multi Segmented gamma
7bc8c7b747f3 drm/i915: Rename ivb_load_lut_10_max
2a7a87a80902 drm/i915/icl: Add Multi-segmented gamma support
-:89: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#89: FILE: drivers/gpu/drm/i915/intel_color.c:789:
+icl_load_gcmax(const struct intel_crtc_state *crtc_state,
+			const struct drm_color_lut *entry)

total: 0 errors, 0 warnings, 1 checks, 159 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Enable Multi-segmented-gamma for ICL

[Patchwork]

== Series Details ==

Series: Enable Multi-segmented-gamma for ICL
URL   : https://patchwork.freedesktop.org/series/60126/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6018 -> Patchwork_12915
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/60126/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-blb-e6850:       [INCOMPLETE][1] ([fdo#107718]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/fi-blb-e6850/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/fi-blb-e6850/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-skl-iommu:       [FAIL][3] ([fdo#104108]) -> [FAIL][4] ([fdo#104108] / [fdo#108602])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/fi-skl-iommu/igt@runner@aborted.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/fi-skl-iommu/igt@runner@aborted.html

  
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602


Participating hosts (51 -> 45)
------------------------------

  Additional (2): fi-icl-y fi-bdw-5557u 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-skl-guc fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper 


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

  * Linux: CI_DRM_6018 -> Patchwork_12915

  CI_DRM_6018: 03fe208c324d490589fef877aed0005bc8946451 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4971: fc5e0467eb6913d21ad932aa8a31c77fdb5a9c77 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12915: 2a7a87a80902a2561002f64e9b448dfe66b3d84e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2a7a87a80902 drm/i915/icl: Add Multi-segmented gamma support
7bc8c7b747f3 drm/i915: Rename ivb_load_lut_10_max
8811262cd9fa drm/i915/icl: Add register definitions for Multi Segmented gamma
dc4b88abc4e0 drm/i915: Change gamma/degamma_lut_size data type to u32

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for Enable Multi-segmented-gamma for ICL

[Patchwork]

== Series Details ==

Series: Enable Multi-segmented-gamma for ICL
URL   : https://patchwork.freedesktop.org/series/60126/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6018_full -> Patchwork_12915_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_12915_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_12915_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_12915_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_mocs_settings@mocs-rc6-render:
    - shard-skl:          NOTRUN -> [INCOMPLETE][1] +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl4/igt@gem_mocs_settings@mocs-rc6-render.html

  * igt@gem_pwrite@small-cpu-random:
    - shard-skl:          [PASS][2] -> [INCOMPLETE][3] +1 similar issue
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl9/igt@gem_pwrite@small-cpu-random.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl9/igt@gem_pwrite@small-cpu-random.html

  * igt@kms_color@pipe-b-ctm-0-25:
    - shard-iclb:         [PASS][4] -> [FAIL][5] +5 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb6/igt@kms_color@pipe-b-ctm-0-25.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb5/igt@kms_color@pipe-b-ctm-0-25.html

  
#### Warnings ####

  * igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-shrfb-draw-pwrite:
    - shard-skl:          [SKIP][6] ([fdo#109271]) -> [INCOMPLETE][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl10/igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-shrfb-draw-pwrite.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl10/igt@kms_frontbuffer_tracking@psr-2p-primscrn-pri-shrfb-draw-pwrite.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - shard-skl:          [PASS][8] -> [INCOMPLETE][9] ([fdo#104108] / [fdo#107773])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl7/igt@gem_exec_suspend@basic-s3.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl4/igt@gem_exec_suspend@basic-s3.html

  * igt@gem_workarounds@suspend-resume:
    - shard-apl:          [PASS][10] -> [DMESG-WARN][11] ([fdo#108566]) +4 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-apl4/igt@gem_workarounds@suspend-resume.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-apl3/igt@gem_workarounds@suspend-resume.html

  * igt@i915_pm_rpm@basic-rte:
    - shard-skl:          [PASS][12] -> [INCOMPLETE][13] ([fdo#107807]) +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl8/igt@i915_pm_rpm@basic-rte.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl1/igt@i915_pm_rpm@basic-rte.html

  * igt@i915_pm_rpm@gem-execbuf:
    - shard-hsw:          [PASS][14] -> [INCOMPLETE][15] ([fdo#103540] / [fdo#107803] / [fdo#107807])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-hsw6/igt@i915_pm_rpm@gem-execbuf.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-hsw1/igt@i915_pm_rpm@gem-execbuf.html

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-glk:          [PASS][16] -> [INCOMPLETE][17] ([fdo#103359] / [k.org#198133]) +1 similar issue
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-glk3/igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-glk1/igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a.html

  * igt@kms_flip@plain-flip-ts-check-interruptible:
    - shard-skl:          [PASS][18] -> [FAIL][19] ([fdo#100368])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl6/igt@kms_flip@plain-flip-ts-check-interruptible.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl6/igt@kms_flip@plain-flip-ts-check-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite:
    - shard-skl:          [PASS][20] -> [FAIL][21] ([fdo#103167]) +2 similar issues
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl10/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl9/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite:
    - shard-iclb:         [PASS][22] -> [FAIL][23] ([fdo#103167]) +2 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-render:
    - shard-iclb:         [PASS][24] -> [INCOMPLETE][25] ([fdo#106978] / [fdo#107713])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-render.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-draw-render.html

  * igt@kms_plane_alpha_blend@pipe-a-coverage-7efc:
    - shard-skl:          [PASS][26] -> [FAIL][27] ([fdo#108145])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl10/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl9/igt@kms_plane_alpha_blend@pipe-a-coverage-7efc.html

  * igt@kms_plane_scaling@pipe-b-scaler-with-rotation:
    - shard-glk:          [PASS][28] -> [SKIP][29] ([fdo#109271] / [fdo#109278]) +1 similar issue
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-glk9/igt@kms_plane_scaling@pipe-b-scaler-with-rotation.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-glk5/igt@kms_plane_scaling@pipe-b-scaler-with-rotation.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [PASS][30] -> [FAIL][31] ([fdo#100047])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb7/igt@kms_sysfs_edid_timing.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb3/igt@kms_sysfs_edid_timing.html

  * igt@tools_test@tools_test:
    - shard-glk:          [PASS][32] -> [SKIP][33] ([fdo#109271]) +1 similar issue
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-glk7/igt@tools_test@tools_test.html
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-glk7/igt@tools_test@tools_test.html

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@medium-copy:
    - shard-skl:          [INCOMPLETE][34] -> [PASS][35]
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl5/igt@gem_mmap_gtt@medium-copy.html
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl1/igt@gem_mmap_gtt@medium-copy.html

  * igt@i915_pm_rpm@modeset-lpsp-stress-no-wait:
    - shard-skl:          [INCOMPLETE][36] ([fdo#107807]) -> [PASS][37]
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl8/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl6/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html

  * igt@i915_suspend@sysfs-reader:
    - shard-apl:          [DMESG-WARN][38] ([fdo#108566]) -> [PASS][39] +4 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-apl5/igt@i915_suspend@sysfs-reader.html
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-apl5/igt@i915_suspend@sysfs-reader.html

  * igt@kms_available_modes_crc@available_mode_test_crc:
    - shard-iclb:         [FAIL][40] ([fdo#106641]) -> [PASS][41]
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb7/igt@kms_available_modes_crc@available_mode_test_crc.html
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb3/igt@kms_available_modes_crc@available_mode_test_crc.html

  * igt@kms_color@pipe-b-gamma:
    - shard-iclb:         [FAIL][42] ([fdo#104782]) -> [PASS][43] +5 similar issues
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb1/igt@kms_color@pipe-b-gamma.html
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb7/igt@kms_color@pipe-b-gamma.html

  * igt@kms_flip@2x-flip-vs-suspend:
    - shard-glk:          [INCOMPLETE][44] ([fdo#103359] / [k.org#198133]) -> [PASS][45]
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-glk2/igt@kms_flip@2x-flip-vs-suspend.html
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-glk1/igt@kms_flip@2x-flip-vs-suspend.html

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-skl:          [FAIL][46] ([fdo#105363]) -> [PASS][47]
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl9/igt@kms_flip@flip-vs-expired-vblank.html
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl9/igt@kms_flip@flip-vs-expired-vblank.html

  * igt@kms_flip@flip-vs-suspend:
    - shard-hsw:          [INCOMPLETE][48] ([fdo#103540]) -> [PASS][49]
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-hsw5/igt@kms_flip@flip-vs-suspend.html
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-hsw6/igt@kms_flip@flip-vs-suspend.html

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-iclb:         [FAIL][50] ([fdo#103167]) -> [PASS][51] +5 similar issues
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move.html
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-skl:          [INCOMPLETE][52] ([fdo#104108]) -> [PASS][53]
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl2/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-glk:          [SKIP][54] ([fdo#109271]) -> [PASS][55] +1 similar issue
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-glk6/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-glk9/igt@kms_plane@pixel-format-pipe-c-planes-source-clamping.html

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
    - shard-skl:          [FAIL][56] ([fdo#108145]) -> [PASS][57]
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html

  * igt@kms_psr@psr2_cursor_render:
    - shard-iclb:         [SKIP][58] ([fdo#109441]) -> [PASS][59]
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-iclb3/igt@kms_psr@psr2_cursor_render.html
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-iclb2/igt@kms_psr@psr2_cursor_render.html

  * igt@perf_pmu@idle-no-semaphores-bcs0:
    - shard-apl:          [INCOMPLETE][60] ([fdo#103927]) -> [PASS][61] +1 similar issue
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-apl1/igt@perf_pmu@idle-no-semaphores-bcs0.html
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-apl7/igt@perf_pmu@idle-no-semaphores-bcs0.html

  
#### Warnings ####

  * igt@kms_flip@2x-flip-vs-modeset:
    - shard-skl:          [INCOMPLETE][62] -> [SKIP][63] ([fdo#109271])
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6018/shard-skl9/igt@kms_flip@2x-flip-vs-modeset.html
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/shard-skl4/igt@kms_flip@2x-flip-vs-modeset.html

  
  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#106641]: https://bugs.freedesktop.org/show_bug.cgi?id=106641
  [fdo#106978]: https://bugs.freedesktop.org/show_bug.cgi?id=106978
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * Linux: CI_DRM_6018 -> Patchwork_12915

  CI_DRM_6018: 03fe208c324d490589fef877aed0005bc8946451 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4971: fc5e0467eb6913d21ad932aa8a31c77fdb5a9c77 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_12915: 2a7a87a80902a2561002f64e9b448dfe66b3d84e @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_12915/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915/icl: Add register definitions for Multi Segmented gamma

[Ville Syrjälä]

On Tue, Apr 30, 2019 at 08:51:06PM +0530, Shashank Sharma wrote:
> From: Uma Shankar <uma.shankar@intel.com>
> 
> Add macros to define multi segmented gamma registers
> 
> V2: Addressed Ville's comments:
>     	Add gen-lable before bit definition
>     Addressed Jani's comment
> 	- Use REG_GENMASK() and REG_BIT()
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6f0a0866c802..7d10b8d00d64 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7198,7 +7198,10 @@ enum {
>  #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
>  #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
>  #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
> +/* ivb-bdw */
>  #define  GAMMA_MODE_MODE_SPLIT	(3 << 0)
> +/* icl + */
> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0)

I would put the comments at the end of the line
#define ... /* ivb-bdw */

>  
>  /* DMC/CSR */
>  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
> @@ -10145,6 +10148,22 @@ enum skl_power_gate {
>  #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
>  #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
>  
> +/* Add registers for Gen11 Multi Segmented Gamma Mode */

Weird comment. 's/Add registers for //' might make it somewhat useful.
And no point in capitalizing every word. This isn't a movie title/etc.

With those sorted this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +#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_DATA_A	0x4A40C
> +#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
> +
> +#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
> +					_PAL_PREC_MULTI_SEG_INDEX_A, \
> +					_PAL_PREC_MULTI_SEG_INDEX_B)
> +#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
> +					_PAL_PREC_MULTI_SEG_DATA_A, \
> +					_PAL_PREC_MULTI_SEG_DATA_B)
> +
>  /* pipe CSC & degamma/gamma LUTs on CHV */
>  #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
>  #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 3/4] drm/i915: Rename ivb_load_lut_10_max

[Ville Syrjälä]

On Tue, Apr 30, 2019 at 08:51:07PM +0530, Shashank Sharma wrote:
> This patch renames function ivb_load_lut_10_max to
> ivb_load_lut_ext_max.
> 
> Cc: Uma Shankar <uma.shankar@intel.com>
> Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_color.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 962db1236970..6c341bea514c 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -607,7 +607,7 @@ static void bdw_load_lut_10(struct intel_crtc *crtc,
>  	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
>  }
>  
> -static void ivb_load_lut_10_max(struct intel_crtc *crtc)
> +static void ivb_load_lut_ext_max(struct intel_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum pipe pipe = crtc->pipe;
> @@ -640,7 +640,7 @@ static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
>  	} else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT) {
>  		ivb_load_lut_10(crtc, degamma_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(0));
> -		ivb_load_lut_10_max(crtc);
> +		ivb_load_lut_ext_max(crtc);
>  		ivb_load_lut_10(crtc, gamma_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(512));
>  	} else {
> @@ -648,7 +648,7 @@ static void ivb_load_luts(const struct intel_crtc_state *crtc_state)
>  
>  		ivb_load_lut_10(crtc, blob,
>  				PAL_PREC_INDEX_VALUE(0));
> -		ivb_load_lut_10_max(crtc);
> +		ivb_load_lut_ext_max(crtc);
>  	}
>  }
>  
> @@ -663,7 +663,7 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state)
>  	} else if (crtc_state->gamma_mode == GAMMA_MODE_MODE_SPLIT) {
>  		bdw_load_lut_10(crtc, degamma_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(0));
> -		ivb_load_lut_10_max(crtc);
> +		ivb_load_lut_ext_max(crtc);
>  		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_SPLIT_MODE |
>  				PAL_PREC_INDEX_VALUE(512));
>  	} else {
> @@ -671,7 +671,7 @@ static void bdw_load_luts(const struct intel_crtc_state *crtc_state)
>  
>  		bdw_load_lut_10(crtc, blob,
>  				PAL_PREC_INDEX_VALUE(0));
> -		ivb_load_lut_10_max(crtc);
> +		ivb_load_lut_ext_max(crtc);
>  	}
>  }
>  
> @@ -763,7 +763,7 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>  		i9xx_load_luts(crtc_state);
>  	} else {
>  		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
> -		ivb_load_lut_10_max(crtc);
> +		ivb_load_lut_ext_max(crtc);
>  	}
>  }
>  
> @@ -780,7 +780,7 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  		i9xx_load_luts(crtc_state);
>  	} else {
>  		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
> -		ivb_load_lut_10_max(crtc);
> +		ivb_load_lut_ext_max(crtc);
>  	}
>  }
>  
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Ville Syrjälä]

On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
> ICL introduces a new gamma correction mode in display engine, called
> multi-segmented-gamma mode. This mode allows users to program the
> darker region of the gamma curve with sueprfine precision. An
> example use case for this is HDR curves (like PQ ST-2084).
> 
> If we plot a gamma correction curve from value range between 0.0 to 1.0,
> ICL's multi-segment has 3 different sections:
> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
> - fine segment: 257 values, ranges between 0 - 1/(128)
> - corase segment: 257 values, ranges between 0 - 1
> 
> This patch:
> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
>   so that userspace can program with highest precision supported.
> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
> - Adds functions to program/detect multi-segment gamma.
> 
> V2: Addressed review comments from Ville
>     - separate function for superfine and fine segments.
>     - remove enum for segments.
>     - reuse last entry of the LUT as gc_max value.
>     - replace if() ....cond with switch...case in icl_load_luts.
>     - add an entry variable, instead of 'word'
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pci.c    |   3 +-
>  drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
>  2 files changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index ffa2ee70a03d..83698951760b 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
>  	GEN(11), \
>  	.ddb_size = 2048, \
>  	.has_logical_ring_elsq = 1, \
> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }

Ugh. Thats one big LUT. But looks correct.

> +

Bogus newline.

>  
>  static const struct intel_device_info intel_icelake_11_info = {
>  	GEN11_FEATURES,
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 6c341bea514c..49831e8d02fb 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -41,6 +41,9 @@
>  #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>  
>  #define LEGACY_LUT_LENGTH		256
> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
> +
>  /*
>   * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>   * format). This macro takes the coefficient we want transformed and the
> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>  	}
>  }
>  
> +/* ilk+ "12.4" interpolated format (high 10 bits) */
> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
> +{
> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
> +		(color->blue >> 6);
> +}
> +
> +/* ilk+ "12.4" interpolated format (low 6 bits) */
> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
> +{
> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
> +		(color->blue & 0x3f);
> +}
> +
> +static void
> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
> +			const struct drm_color_lut *entry)

Indentation looks off. Also s/entry/color/ to match the other similarish
funcs maybe?

> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +
> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
> +}
> +
> +static void
> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> +	const struct drm_color_lut *lut = blob->data;
> +	enum pipe pipe = crtc->pipe;
> +	u32 i;
> +
> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
> +		return;

These checks aren't needed. Just dead code.

> +
> +	/*
> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
> +	 * segment step which is 1/(8 * 128 * 256).
> +	 *
> +	 * Superfine segment has 9 entries, corresponding to values
> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
> +	 */
> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +
> +	for (i = 0; i < 9; i++) {
> +		const struct drm_color_lut *entry = &lut[i];
> +
> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> +			   ilk_lut_12p4_udw(entry));

ldw should come before udw.

> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> +			   ilk_lut_12p4_ldw(entry));
> +	}
> +}
> +
> +static void
> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> +	const struct drm_color_lut *lut = blob->data;
> +	const struct drm_color_lut *entry;

'entry' declaration can be moved into the loops.

> +	enum pipe pipe = crtc->pipe;
> +	u32 i;
> +
> +	if (!lut || (drm_color_lut_size(blob) < ICL_GAMMA_MULTISEG_LUT_LENGTH))
> +		return;

More checks that aren't needed.

> +
> +	/*
> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
> +	 * segment step which is 1/(8*128*256).
> +	 *
> +	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
> +	 * ... 256/(128*256). So in order to program fine segment of LUT we
> +	 * need to pick every 8'th entry in LUT, and program 256 indexes.
> +	 * Fine segment's index 0 is programmed in HW, and it starts from
> +	 * index 1.

The wording here is a bit confusing. I guess the problem is what to call
things. PAL_PREC_INDEX[0/1] is what we program, but that maps to the point
seg2[1] with seg2[0] being unused by the hw. Well, the spec says it's
implicit but IIRC I was told long ago that it's not actually used.

Not sure how to word that in the best way. Maybe something like?

/*
 * Fine segment (seg2) ...
 *
 * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
 * with seg2[0] being unused by the hardware.
 */

Not sure that's any clearer.

> +	 */
> +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +	for (i = 1; i < 257; i++) {
> +		entry = &lut[i * 8];
> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> +	}
> +
> +	/*
> +	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
> +	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
> +	 * above, we need to pick every 8 * 128 = 1024th entry in LUT, and
> +	 * program 256 of those.
> +	 */

Could make a note here stating that seg3[0] and seg3[1] are also unused
by the hardware, even though we have to program them to advance the
index. I don't see it mentioned in the spec, but this one I definitely
remember confirming from Art way back when. However I never verified
that on actual hw. We could also consider just programming those two
entries to 0 and start the actual coarse segment programming from index 2.
Or we could skip them by reprogramming the index directly.

> +	for (i = 0; i < 256; i++) {
> +		entry = &lut[i * 1024];

s/1024/8 * 128/ maybe?

> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> +	}
> +
> +	icl_load_gcmax(crtc_state, entry);
> +	ivb_load_lut_ext_max(crtc);
> +}
> +
>  static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  {
>  	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> @@ -775,10 +885,17 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>  	if (crtc_state->base.degamma_lut)
>  		glk_load_degamma_lut(crtc_state);
>  
> -	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
> -	    GAMMA_MODE_MODE_8BIT) {
> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> +	case GAMMA_MODE_MODE_8BIT:
>  		i9xx_load_luts(crtc_state);
> -	} else {
> +		break;
> +
> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> +		icl_program_gamma_superfine_segment(crtc_state);
> +		icl_program_gamma_multi_segment(crtc_state);
> +		break;
> +
> +	default:
>  		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>  		ivb_load_lut_ext_max(crtc);
>  	}
> @@ -1209,7 +1326,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>  	    crtc_state_is_legacy_gamma(crtc_state))
>  		gamma_mode |= GAMMA_MODE_MODE_8BIT;
>  	else
> -		gamma_mode |= GAMMA_MODE_MODE_10BIT;
> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
>  
>  	return gamma_mode;
>  }
> -- 
> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/4] drm/i915/icl: Add register definitions for Multi Segmented gamma

[Sharma, Shashank]

On 5/3/2019 8:35 PM, Ville Syrjälä wrote:
> On Tue, Apr 30, 2019 at 08:51:06PM +0530, Shashank Sharma wrote:
>> From: Uma Shankar <uma.shankar@intel.com>
>>
>> Add macros to define multi segmented gamma registers
>>
>> V2: Addressed Ville's comments:
>>      	Add gen-lable before bit definition
>>      Addressed Jani's comment
>> 	- Use REG_GENMASK() and REG_BIT()
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h | 19 +++++++++++++++++++
>>   1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6f0a0866c802..7d10b8d00d64 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7198,7 +7198,10 @@ enum {
>>   #define  GAMMA_MODE_MODE_8BIT	(0 << 0)
>>   #define  GAMMA_MODE_MODE_10BIT	(1 << 0)
>>   #define  GAMMA_MODE_MODE_12BIT	(2 << 0)
>> +/* ivb-bdw */
>>   #define  GAMMA_MODE_MODE_SPLIT	(3 << 0)
>> +/* icl + */
>> +#define  GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED	(3 << 0)
> I would put the comments at the end of the line
> #define ... /* ivb-bdw */
>
>>   
>>   /* DMC/CSR */
>>   #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
>> @@ -10145,6 +10148,22 @@ enum skl_power_gate {
>>   #define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
>>   #define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
>>   
>> +/* Add registers for Gen11 Multi Segmented Gamma Mode */
> Weird comment. 's/Add registers for //' might make it somewhat useful.
> And no point in capitalizing every word. This isn't a movie title/etc.
>
> With those sorted this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review, will do the changes as suggested.

- Shashank

>> +#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_DATA_A	0x4A40C
>> +#define _PAL_PREC_MULTI_SEG_DATA_B	0x4AC0C
>> +
>> +#define PREC_PAL_MULTI_SEG_INDEX(pipe)	_MMIO_PIPE(pipe, \
>> +					_PAL_PREC_MULTI_SEG_INDEX_A, \
>> +					_PAL_PREC_MULTI_SEG_INDEX_B)
>> +#define PREC_PAL_MULTI_SEG_DATA(pipe)	_MMIO_PIPE(pipe, \
>> +					_PAL_PREC_MULTI_SEG_DATA_A, \
>> +					_PAL_PREC_MULTI_SEG_DATA_B)
>> +
>>   /* pipe CSC & degamma/gamma LUTs on CHV */
>>   #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
>>   #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
>> -- 
>> 2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Sharma, Shashank]

Regards

Shashank

On 5/3/2019 9:20 PM, Ville Syrjälä wrote:
> On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
>> ICL introduces a new gamma correction mode in display engine, called
>> multi-segmented-gamma mode. This mode allows users to program the
>> darker region of the gamma curve with sueprfine precision. An
>> example use case for this is HDR curves (like PQ ST-2084).
>>
>> If we plot a gamma correction curve from value range between 0.0 to 1.0,
>> ICL's multi-segment has 3 different sections:
>> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
>> - fine segment: 257 values, ranges between 0 - 1/(128)
>> - corase segment: 257 values, ranges between 0 - 1
>>
>> This patch:
>> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
>>    so that userspace can program with highest precision supported.
>> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
>> - Adds functions to program/detect multi-segment gamma.
>>
>> V2: Addressed review comments from Ville
>>      - separate function for superfine and fine segments.
>>      - remove enum for segments.
>>      - reuse last entry of the LUT as gc_max value.
>>      - replace if() ....cond with switch...case in icl_load_luts.
>>      - add an entry variable, instead of 'word'
>>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_pci.c    |   3 +-
>>   drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
>>   2 files changed, 123 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>> index ffa2ee70a03d..83698951760b 100644
>> --- a/drivers/gpu/drm/i915/i915_pci.c
>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
>>   	GEN(11), \
>>   	.ddb_size = 2048, \
>>   	.has_logical_ring_elsq = 1, \
>> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
> Ugh. Thats one big LUT. But looks correct.
>
>> +
> Bogus newline.
Got it.
>>   
>>   static const struct intel_device_info intel_icelake_11_info = {
>>   	GEN11_FEATURES,
>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>> index 6c341bea514c..49831e8d02fb 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -41,6 +41,9 @@
>>   #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>>   
>>   #define LEGACY_LUT_LENGTH		256
>> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
>> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
>> +
>>   /*
>>    * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>>    * format). This macro takes the coefficient we want transformed and the
>> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>>   	}
>>   }
>>   
>> +/* ilk+ "12.4" interpolated format (high 10 bits) */
>> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>> +{
>> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
>> +		(color->blue >> 6);
>> +}
>> +
>> +/* ilk+ "12.4" interpolated format (low 6 bits) */
>> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
>> +{
>> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
>> +		(color->blue & 0x3f);
>> +}
>> +
>> +static void
>> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>> +			const struct drm_color_lut *entry)
> Indentation looks off. Also s/entry/color/ to match the other similarish
> funcs maybe?
Sure.
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	enum pipe pipe = crtc->pipe;
>> +
>> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
>> +}
>> +
>> +static void
>> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>> +	const struct drm_color_lut *lut = blob->data;
>> +	enum pipe pipe = crtc->pipe;
>> +	u32 i;
>> +
>> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
>> +		return;
> These checks aren't needed. Just dead code.
Will remove this and similars.
>> +
>> +	/*
>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
>> +	 * segment step which is 1/(8 * 128 * 256).
>> +	 *
>> +	 * Superfine segment has 9 entries, corresponding to values
>> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>> +	 */
>> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> +
>> +	for (i = 0; i < 9; i++) {
>> +		const struct drm_color_lut *entry = &lut[i];
>> +
>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>> +			   ilk_lut_12p4_udw(entry));
> ldw should come before udw.
Got it.
>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>> +			   ilk_lut_12p4_ldw(entry));
>> +	}
>> +}
>> +
>> +static void
>> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>> +{
>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>> +	const struct drm_color_lut *lut = blob->data;
>> +	const struct drm_color_lut *entry;
> 'entry' declaration can be moved into the loops.
Its being used in multiple loops, also being used for GCMax outside the 
loop.
>> +	enum pipe pipe = crtc->pipe;
>> +	u32 i;
>> +
>> +	if (!lut || (drm_color_lut_size(blob) < ICL_GAMMA_MULTISEG_LUT_LENGTH))
>> +		return;
> More checks that aren't needed.
Got it.
>> +
>> +	/*
>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
>> +	 * segment step which is 1/(8*128*256).
>> +	 *
>> +	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
>> +	 * ... 256/(128*256). So in order to program fine segment of LUT we
>> +	 * need to pick every 8'th entry in LUT, and program 256 indexes.
>> +	 * Fine segment's index 0 is programmed in HW, and it starts from
>> +	 * index 1.
> The wording here is a bit confusing. I guess the problem is what to call
> things. PAL_PREC_INDEX[0/1] is what we program, but that maps to the point
> seg2[1] with seg2[0] being unused by the hw. Well, the spec says it's
> implicit but IIRC I was told long ago that it's not actually used.
>
> Not sure how to word that in the best way. Maybe something like?
>
> /*
>   * Fine segment (seg2) ...
>   *
>   * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>   * with seg2[0] being unused by the hardware.
>   */
>
> Not sure that's any clearer.
Ok, will try to come up with something in similar lines.
>> +	 */
>> +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>> +	for (i = 1; i < 257; i++) {
>> +		entry = &lut[i * 8];
>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> +	}
>> +
>> +	/*
>> +	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
>> +	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
>> +	 * above, we need to pick every 8 * 128 = 1024th entry in LUT, and
>> +	 * program 256 of those.
>> +	 */
> Could make a note here stating that seg3[0] and seg3[1] are also unused
> by the hardware, even though we have to program them to advance the
> index. I don't see it mentioned in the spec, but this one I definitely
> remember confirming from Art way back when. However I never verified
> that on actual hw. We could also consider just programming those two
> entries to 0 and start the actual coarse segment programming from index 2.
> Or we could skip them by reprogramming the index directly.
If they are not being used, does it matter what and if we program into 
them ? We can add a note though, mentioning this.
>> +	for (i = 0; i < 256; i++) {
>> +		entry = &lut[i * 1024];
> s/1024/8 * 128/ maybe?

Sure.

Regards

Shashank

>
>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>> +	}
>> +
>> +	icl_load_gcmax(crtc_state, entry);
>> +	ivb_load_lut_ext_max(crtc);
>> +}
>> +
>>   static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>   {
>>   	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>> @@ -775,10 +885,17 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>   	if (crtc_state->base.degamma_lut)
>>   		glk_load_degamma_lut(crtc_state);
>>   
>> -	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
>> -	    GAMMA_MODE_MODE_8BIT) {
>> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>> +	case GAMMA_MODE_MODE_8BIT:
>>   		i9xx_load_luts(crtc_state);
>> -	} else {
>> +		break;
>> +
>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>> +		icl_program_gamma_superfine_segment(crtc_state);
>> +		icl_program_gamma_multi_segment(crtc_state);
>> +		break;
>> +
>> +	default:
>>   		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>>   		ivb_load_lut_ext_max(crtc);
>>   	}
>> @@ -1209,7 +1326,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>>   	    crtc_state_is_legacy_gamma(crtc_state))
>>   		gamma_mode |= GAMMA_MODE_MODE_8BIT;
>>   	else
>> -		gamma_mode |= GAMMA_MODE_MODE_10BIT;
>> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
>>   
>>   	return gamma_mode;
>>   }
>> -- 
>> 2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Ville Syrjälä]

On Mon, May 06, 2019 at 04:09:33PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> On 5/3/2019 9:20 PM, Ville Syrjälä wrote:
> > On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
> >> ICL introduces a new gamma correction mode in display engine, called
> >> multi-segmented-gamma mode. This mode allows users to program the
> >> darker region of the gamma curve with sueprfine precision. An
> >> example use case for this is HDR curves (like PQ ST-2084).
> >>
> >> If we plot a gamma correction curve from value range between 0.0 to 1.0,
> >> ICL's multi-segment has 3 different sections:
> >> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
> >> - fine segment: 257 values, ranges between 0 - 1/(128)
> >> - corase segment: 257 values, ranges between 0 - 1
> >>
> >> This patch:
> >> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
> >>    so that userspace can program with highest precision supported.
> >> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
> >> - Adds functions to program/detect multi-segment gamma.
> >>
> >> V2: Addressed review comments from Ville
> >>      - separate function for superfine and fine segments.
> >>      - remove enum for segments.
> >>      - reuse last entry of the LUT as gc_max value.
> >>      - replace if() ....cond with switch...case in icl_load_luts.
> >>      - add an entry variable, instead of 'word'
> >>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_pci.c    |   3 +-
> >>   drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
> >>   2 files changed, 123 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> >> index ffa2ee70a03d..83698951760b 100644
> >> --- a/drivers/gpu/drm/i915/i915_pci.c
> >> +++ b/drivers/gpu/drm/i915/i915_pci.c
> >> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
> >>   	GEN(11), \
> >>   	.ddb_size = 2048, \
> >>   	.has_logical_ring_elsq = 1, \
> >> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
> >> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
> > Ugh. Thats one big LUT. But looks correct.
> >
> >> +
> > Bogus newline.
> Got it.
> >>   
> >>   static const struct intel_device_info intel_icelake_11_info = {
> >>   	GEN11_FEATURES,
> >> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >> index 6c341bea514c..49831e8d02fb 100644
> >> --- a/drivers/gpu/drm/i915/intel_color.c
> >> +++ b/drivers/gpu/drm/i915/intel_color.c
> >> @@ -41,6 +41,9 @@
> >>   #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
> >>   
> >>   #define LEGACY_LUT_LENGTH		256
> >> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
> >> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
> >> +
> >>   /*
> >>    * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> >>    * format). This macro takes the coefficient we want transformed and the
> >> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
> >>   	}
> >>   }
> >>   
> >> +/* ilk+ "12.4" interpolated format (high 10 bits) */
> >> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
> >> +{
> >> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
> >> +		(color->blue >> 6);
> >> +}
> >> +
> >> +/* ilk+ "12.4" interpolated format (low 6 bits) */
> >> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
> >> +{
> >> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
> >> +		(color->blue & 0x3f);
> >> +}
> >> +
> >> +static void
> >> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
> >> +			const struct drm_color_lut *entry)
> > Indentation looks off. Also s/entry/color/ to match the other similarish
> > funcs maybe?
> Sure.
> >> +{
> >> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> +	enum pipe pipe = crtc->pipe;
> >> +
> >> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
> >> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
> >> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
> >> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
> >> +}
> >> +
> >> +static void
> >> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
> >> +{
> >> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> >> +	const struct drm_color_lut *lut = blob->data;
> >> +	enum pipe pipe = crtc->pipe;
> >> +	u32 i;
> >> +
> >> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
> >> +		return;
> > These checks aren't needed. Just dead code.
> Will remove this and similars.
> >> +
> >> +	/*
> >> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
> >> +	 * segment step which is 1/(8 * 128 * 256).
> >> +	 *
> >> +	 * Superfine segment has 9 entries, corresponding to values
> >> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
> >> +	 */
> >> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> >> +
> >> +	for (i = 0; i < 9; i++) {
> >> +		const struct drm_color_lut *entry = &lut[i];
> >> +
> >> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> >> +			   ilk_lut_12p4_udw(entry));
> > ldw should come before udw.
> Got it.
> >> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> >> +			   ilk_lut_12p4_ldw(entry));
> >> +	}
> >> +}
> >> +
> >> +static void
> >> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
> >> +{
> >> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> >> +	const struct drm_color_lut *lut = blob->data;
> >> +	const struct drm_color_lut *entry;
> > 'entry' declaration can be moved into the loops.
> Its being used in multiple loops,

IMO still better to move into the loops. We don't want to pass any
information between the loops.

> also being used for GCMax outside the 
> loop.

Hmm. That might be an arguemnt for keeping it out. But the current
gcmax usage looks broken. You're programming the same value into
the last PAL_PREC index and GCMAX.

> >> +	enum pipe pipe = crtc->pipe;
> >> +	u32 i;
> >> +
> >> +	if (!lut || (drm_color_lut_size(blob) < ICL_GAMMA_MULTISEG_LUT_LENGTH))
> >> +		return;
> > More checks that aren't needed.
> Got it.
> >> +
> >> +	/*
> >> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
> >> +	 * segment step which is 1/(8*128*256).
> >> +	 *
> >> +	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
> >> +	 * ... 256/(128*256). So in order to program fine segment of LUT we
> >> +	 * need to pick every 8'th entry in LUT, and program 256 indexes.
> >> +	 * Fine segment's index 0 is programmed in HW, and it starts from
> >> +	 * index 1.
> > The wording here is a bit confusing. I guess the problem is what to call
> > things. PAL_PREC_INDEX[0/1] is what we program, but that maps to the point
> > seg2[1] with seg2[0] being unused by the hw. Well, the spec says it's
> > implicit but IIRC I was told long ago that it's not actually used.
> >
> > Not sure how to word that in the best way. Maybe something like?
> >
> > /*
> >   * Fine segment (seg2) ...
> >   *
> >   * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
> >   * with seg2[0] being unused by the hardware.
> >   */
> >
> > Not sure that's any clearer.
> Ok, will try to come up with something in similar lines.
> >> +	 */
> >> +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> >> +	for (i = 1; i < 257; i++) {
> >> +		entry = &lut[i * 8];
> >> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> >> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> >> +	}
> >> +
> >> +	/*
> >> +	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
> >> +	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
> >> +	 * above, we need to pick every 8 * 128 = 1024th entry in LUT, and
> >> +	 * program 256 of those.
> >> +	 */
> > Could make a note here stating that seg3[0] and seg3[1] are also unused
> > by the hardware, even though we have to program them to advance the
> > index. I don't see it mentioned in the spec, but this one I definitely
> > remember confirming from Art way back when. However I never verified
> > that on actual hw. We could also consider just programming those two
> > entries to 0 and start the actual coarse segment programming from index 2.
> > Or we could skip them by reprogramming the index directly.
> If they are not being used, does it matter what and if we program into 
> them ? We can add a note though, mentioning this.

It shouldn't matter what we programing into them. But as mentioned I
never actually confirmed this on actual hardware. Would be nice to
double check that so we don't end up with incorrect comment.

> >> +	for (i = 0; i < 256; i++) {
> >> +		entry = &lut[i * 1024];
> > s/1024/8 * 128/ maybe?
> 
> Sure.
> 
> Regards
> 
> Shashank
> 
> >
> >> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
> >> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
> >> +	}
> >> +
> >> +	icl_load_gcmax(crtc_state, entry);
> >> +	ivb_load_lut_ext_max(crtc);
> >> +}
> >> +
> >>   static void icl_load_luts(const struct intel_crtc_state *crtc_state)
> >>   {
> >>   	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
> >> @@ -775,10 +885,17 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
> >>   	if (crtc_state->base.degamma_lut)
> >>   		glk_load_degamma_lut(crtc_state);
> >>   
> >> -	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
> >> -	    GAMMA_MODE_MODE_8BIT) {
> >> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
> >> +	case GAMMA_MODE_MODE_8BIT:
> >>   		i9xx_load_luts(crtc_state);
> >> -	} else {
> >> +		break;
> >> +
> >> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
> >> +		icl_program_gamma_superfine_segment(crtc_state);
> >> +		icl_program_gamma_multi_segment(crtc_state);
> >> +		break;
> >> +
> >> +	default:
> >>   		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
> >>   		ivb_load_lut_ext_max(crtc);
> >>   	}
> >> @@ -1209,7 +1326,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
> >>   	    crtc_state_is_legacy_gamma(crtc_state))
> >>   		gamma_mode |= GAMMA_MODE_MODE_8BIT;
> >>   	else
> >> -		gamma_mode |= GAMMA_MODE_MODE_10BIT;
> >> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
> >>   
> >>   	return gamma_mode;
> >>   }
> >> -- 
> >> 2.17.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Sharma, Shashank]

On 5/6/2019 5:55 PM, Ville Syrjälä wrote:
> On Mon, May 06, 2019 at 04:09:33PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>> On 5/3/2019 9:20 PM, Ville Syrjälä wrote:
>>> On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
>>>> ICL introduces a new gamma correction mode in display engine, called
>>>> multi-segmented-gamma mode. This mode allows users to program the
>>>> darker region of the gamma curve with sueprfine precision. An
>>>> example use case for this is HDR curves (like PQ ST-2084).
>>>>
>>>> If we plot a gamma correction curve from value range between 0.0 to 1.0,
>>>> ICL's multi-segment has 3 different sections:
>>>> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
>>>> - fine segment: 257 values, ranges between 0 - 1/(128)
>>>> - corase segment: 257 values, ranges between 0 - 1
>>>>
>>>> This patch:
>>>> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
>>>>     so that userspace can program with highest precision supported.
>>>> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
>>>> - Adds functions to program/detect multi-segment gamma.
>>>>
>>>> V2: Addressed review comments from Ville
>>>>       - separate function for superfine and fine segments.
>>>>       - remove enum for segments.
>>>>       - reuse last entry of the LUT as gc_max value.
>>>>       - replace if() ....cond with switch...case in icl_load_luts.
>>>>       - add an entry variable, instead of 'word'
>>>>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>
>>>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_pci.c    |   3 +-
>>>>    drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
>>>>    2 files changed, 123 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>>>> index ffa2ee70a03d..83698951760b 100644
>>>> --- a/drivers/gpu/drm/i915/i915_pci.c
>>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>>>> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
>>>>    	GEN(11), \
>>>>    	.ddb_size = 2048, \
>>>>    	.has_logical_ring_elsq = 1, \
>>>> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
>>>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
>>> Ugh. Thats one big LUT. But looks correct.
>>>
>>>> +
>>> Bogus newline.
>> Got it.
>>>>    
>>>>    static const struct intel_device_info intel_icelake_11_info = {
>>>>    	GEN11_FEATURES,
>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>> index 6c341bea514c..49831e8d02fb 100644
>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>> @@ -41,6 +41,9 @@
>>>>    #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>>>>    
>>>>    #define LEGACY_LUT_LENGTH		256
>>>> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
>>>> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
>>>> +
>>>>    /*
>>>>     * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>>>>     * format). This macro takes the coefficient we want transformed and the
>>>> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>>>>    	}
>>>>    }
>>>>    
>>>> +/* ilk+ "12.4" interpolated format (high 10 bits) */
>>>> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>>>> +{
>>>> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
>>>> +		(color->blue >> 6);
>>>> +}
>>>> +
>>>> +/* ilk+ "12.4" interpolated format (low 6 bits) */
>>>> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
>>>> +{
>>>> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
>>>> +		(color->blue & 0x3f);
>>>> +}
>>>> +
>>>> +static void
>>>> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>>>> +			const struct drm_color_lut *entry)
>>> Indentation looks off. Also s/entry/color/ to match the other similarish
>>> funcs maybe?
>> Sure.
>>>> +{
>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +	enum pipe pipe = crtc->pipe;
>>>> +
>>>> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
>>>> +}
>>>> +
>>>> +static void
>>>> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>>> +{
>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>> +	const struct drm_color_lut *lut = blob->data;
>>>> +	enum pipe pipe = crtc->pipe;
>>>> +	u32 i;
>>>> +
>>>> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
>>>> +		return;
>>> These checks aren't needed. Just dead code.
>> Will remove this and similars.
>>>> +
>>>> +	/*
>>>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
>>>> +	 * segment step which is 1/(8 * 128 * 256).
>>>> +	 *
>>>> +	 * Superfine segment has 9 entries, corresponding to values
>>>> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>>>> +	 */
>>>> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>> +
>>>> +	for (i = 0; i < 9; i++) {
>>>> +		const struct drm_color_lut *entry = &lut[i];
>>>> +
>>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>>> +			   ilk_lut_12p4_udw(entry));
>>> ldw should come before udw.
>> Got it.
>>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>>> +			   ilk_lut_12p4_ldw(entry));
>>>> +	}
>>>> +}
>>>> +
>>>> +static void
>>>> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>> +{
>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>> +	const struct drm_color_lut *lut = blob->data;
>>>> +	const struct drm_color_lut *entry;
>>> 'entry' declaration can be moved into the loops.
>> Its being used in multiple loops,
> IMO still better to move into the loops. We don't want to pass any
> information between the loops.
>
>> also being used for GCMax outside the
>> loop.
> Hmm. That might be an arguemnt for keeping it out. But the current
> gcmax usage looks broken. You're programming the same value into
> the last PAL_PREC index and GCMAX.

Isn't this what you wanted ? IIRC, your recommendation was to program 
the highest values user wants to program in GCMAX reg. We also had a 
discussion on how user cant program 1.0, as we have LUT depth on 16 bit 
only, and we decided to use the last value of the LUT as GCMAX. Did I 
misunderstand anything there ?

- Shashank

>>>> +	enum pipe pipe = crtc->pipe;
>>>> +	u32 i;
>>>> +
>>>> +	if (!lut || (drm_color_lut_size(blob) < ICL_GAMMA_MULTISEG_LUT_LENGTH))
>>>> +		return;
>>> More checks that aren't needed.
>> Got it.
>>>> +
>>>> +	/*
>>>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
>>>> +	 * segment step which is 1/(8*128*256).
>>>> +	 *
>>>> +	 * Fine segment's step is 1/(128 * 256) ie 1/(128 * 256),  2/(128*256)
>>>> +	 * ... 256/(128*256). So in order to program fine segment of LUT we
>>>> +	 * need to pick every 8'th entry in LUT, and program 256 indexes.
>>>> +	 * Fine segment's index 0 is programmed in HW, and it starts from
>>>> +	 * index 1.
>>> The wording here is a bit confusing. I guess the problem is what to call
>>> things. PAL_PREC_INDEX[0/1] is what we program, but that maps to the point
>>> seg2[1] with seg2[0] being unused by the hw. Well, the spec says it's
>>> implicit but IIRC I was told long ago that it's not actually used.
>>>
>>> Not sure how to word that in the best way. Maybe something like?
>>>
>>> /*
>>>    * Fine segment (seg2) ...
>>>    *
>>>    * PAL_PREC_INDEX[0] and PAL_PREC_INDEX[1] map to seg2[1],
>>>    * with seg2[0] being unused by the hardware.
>>>    */
>>>
>>> Not sure that's any clearer.
>> Ok, will try to come up with something in similar lines.
>>>> +	 */
>>>> +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>> +	for (i = 1; i < 257; i++) {
>>>> +		entry = &lut[i * 8];
>>>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>>>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * Coarse segment's starts from index 0 and it's step is 1/256 ie 0,
>>>> +	 * 1/256, 2/256 ...256/256. As per the description of each entry in LUT
>>>> +	 * above, we need to pick every 8 * 128 = 1024th entry in LUT, and
>>>> +	 * program 256 of those.
>>>> +	 */
>>> Could make a note here stating that seg3[0] and seg3[1] are also unused
>>> by the hardware, even though we have to program them to advance the
>>> index. I don't see it mentioned in the spec, but this one I definitely
>>> remember confirming from Art way back when. However I never verified
>>> that on actual hw. We could also consider just programming those two
>>> entries to 0 and start the actual coarse segment programming from index 2.
>>> Or we could skip them by reprogramming the index directly.
>> If they are not being used, does it matter what and if we program into
>> them ? We can add a note though, mentioning this.
> It shouldn't matter what we programing into them. But as mentioned I
> never actually confirmed this on actual hardware. Would be nice to
> double check that so we don't end up with incorrect comment.
>
>>>> +	for (i = 0; i < 256; i++) {
>>>> +		entry = &lut[i * 1024];
>>> s/1024/8 * 128/ maybe?
>> Sure.
>>
>> Regards
>>
>> Shashank
>>
>>>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_udw(entry));
>>>> +		I915_WRITE(PREC_PAL_DATA(pipe), ilk_lut_12p4_ldw(entry));
>>>> +	}
>>>> +
>>>> +	icl_load_gcmax(crtc_state, entry);
>>>> +	ivb_load_lut_ext_max(crtc);
>>>> +}
>>>> +
>>>>    static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>>>    {
>>>>    	const struct drm_property_blob *gamma_lut = crtc_state->base.gamma_lut;
>>>> @@ -775,10 +885,17 @@ static void icl_load_luts(const struct intel_crtc_state *crtc_state)
>>>>    	if (crtc_state->base.degamma_lut)
>>>>    		glk_load_degamma_lut(crtc_state);
>>>>    
>>>> -	if ((crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) ==
>>>> -	    GAMMA_MODE_MODE_8BIT) {
>>>> +	switch (crtc_state->gamma_mode & GAMMA_MODE_MODE_MASK) {
>>>> +	case GAMMA_MODE_MODE_8BIT:
>>>>    		i9xx_load_luts(crtc_state);
>>>> -	} else {
>>>> +		break;
>>>> +
>>>> +	case GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED:
>>>> +		icl_program_gamma_superfine_segment(crtc_state);
>>>> +		icl_program_gamma_multi_segment(crtc_state);
>>>> +		break;
>>>> +
>>>> +	default:
>>>>    		bdw_load_lut_10(crtc, gamma_lut, PAL_PREC_INDEX_VALUE(0));
>>>>    		ivb_load_lut_ext_max(crtc);
>>>>    	}
>>>> @@ -1209,7 +1326,7 @@ static u32 icl_gamma_mode(const struct intel_crtc_state *crtc_state)
>>>>    	    crtc_state_is_legacy_gamma(crtc_state))
>>>>    		gamma_mode |= GAMMA_MODE_MODE_8BIT;
>>>>    	else
>>>> -		gamma_mode |= GAMMA_MODE_MODE_10BIT;
>>>> +		gamma_mode |= GAMMA_MODE_MODE_12BIT_MULTI_SEGMENTED;
>>>>    
>>>>    	return gamma_mode;
>>>>    }
>>>> -- 
>>>> 2.17.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Ville Syrjälä]

On Mon, May 06, 2019 at 06:25:19PM +0530, Sharma, Shashank wrote:
> 
> On 5/6/2019 5:55 PM, Ville Syrjälä wrote:
> > On Mon, May 06, 2019 at 04:09:33PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >> On 5/3/2019 9:20 PM, Ville Syrjälä wrote:
> >>> On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
> >>>> ICL introduces a new gamma correction mode in display engine, called
> >>>> multi-segmented-gamma mode. This mode allows users to program the
> >>>> darker region of the gamma curve with sueprfine precision. An
> >>>> example use case for this is HDR curves (like PQ ST-2084).
> >>>>
> >>>> If we plot a gamma correction curve from value range between 0.0 to 1.0,
> >>>> ICL's multi-segment has 3 different sections:
> >>>> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
> >>>> - fine segment: 257 values, ranges between 0 - 1/(128)
> >>>> - corase segment: 257 values, ranges between 0 - 1
> >>>>
> >>>> This patch:
> >>>> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
> >>>>     so that userspace can program with highest precision supported.
> >>>> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
> >>>> - Adds functions to program/detect multi-segment gamma.
> >>>>
> >>>> V2: Addressed review comments from Ville
> >>>>       - separate function for superfine and fine segments.
> >>>>       - remove enum for segments.
> >>>>       - reuse last entry of the LUT as gc_max value.
> >>>>       - replace if() ....cond with switch...case in icl_load_luts.
> >>>>       - add an entry variable, instead of 'word'
> >>>>
> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>
> >>>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/i915_pci.c    |   3 +-
> >>>>    drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
> >>>>    2 files changed, 123 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> >>>> index ffa2ee70a03d..83698951760b 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_pci.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
> >>>> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
> >>>>    	GEN(11), \
> >>>>    	.ddb_size = 2048, \
> >>>>    	.has_logical_ring_elsq = 1, \
> >>>> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
> >>>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
> >>> Ugh. Thats one big LUT. But looks correct.
> >>>
> >>>> +
> >>> Bogus newline.
> >> Got it.
> >>>>    
> >>>>    static const struct intel_device_info intel_icelake_11_info = {
> >>>>    	GEN11_FEATURES,
> >>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >>>> index 6c341bea514c..49831e8d02fb 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_color.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_color.c
> >>>> @@ -41,6 +41,9 @@
> >>>>    #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
> >>>>    
> >>>>    #define LEGACY_LUT_LENGTH		256
> >>>> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
> >>>> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
> >>>> +
> >>>>    /*
> >>>>     * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> >>>>     * format). This macro takes the coefficient we want transformed and the
> >>>> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
> >>>>    	}
> >>>>    }
> >>>>    
> >>>> +/* ilk+ "12.4" interpolated format (high 10 bits) */
> >>>> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
> >>>> +{
> >>>> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
> >>>> +		(color->blue >> 6);
> >>>> +}
> >>>> +
> >>>> +/* ilk+ "12.4" interpolated format (low 6 bits) */
> >>>> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
> >>>> +{
> >>>> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
> >>>> +		(color->blue & 0x3f);
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
> >>>> +			const struct drm_color_lut *entry)
> >>> Indentation looks off. Also s/entry/color/ to match the other similarish
> >>> funcs maybe?
> >> Sure.
> >>>> +{
> >>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>>> +	enum pipe pipe = crtc->pipe;
> >>>> +
> >>>> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
> >>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
> >>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
> >>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
> >>>> +{
> >>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> >>>> +	const struct drm_color_lut *lut = blob->data;
> >>>> +	enum pipe pipe = crtc->pipe;
> >>>> +	u32 i;
> >>>> +
> >>>> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
> >>>> +		return;
> >>> These checks aren't needed. Just dead code.
> >> Will remove this and similars.
> >>>> +
> >>>> +	/*
> >>>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
> >>>> +	 * segment step which is 1/(8 * 128 * 256).
> >>>> +	 *
> >>>> +	 * Superfine segment has 9 entries, corresponding to values
> >>>> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
> >>>> +	 */
> >>>> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> >>>> +
> >>>> +	for (i = 0; i < 9; i++) {
> >>>> +		const struct drm_color_lut *entry = &lut[i];
> >>>> +
> >>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> >>>> +			   ilk_lut_12p4_udw(entry));
> >>> ldw should come before udw.
> >> Got it.
> >>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> >>>> +			   ilk_lut_12p4_ldw(entry));
> >>>> +	}
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
> >>>> +{
> >>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> >>>> +	const struct drm_color_lut *lut = blob->data;
> >>>> +	const struct drm_color_lut *entry;
> >>> 'entry' declaration can be moved into the loops.
> >> Its being used in multiple loops,
> > IMO still better to move into the loops. We don't want to pass any
> > information between the loops.
> >
> >> also being used for GCMax outside the
> >> loop.
> > Hmm. That might be an arguemnt for keeping it out. But the current
> > gcmax usage looks broken. You're programming the same value into
> > the last PAL_PREC index and GCMAX.
> 
> Isn't this what you wanted ? IIRC, your recommendation was to program 
> the highest values user wants to program in GCMAX reg. We also had a 
> discussion on how user cant program 1.0, as we have LUT depth on 16 bit 
> only, and we decided to use the last value of the LUT as GCMAX. Did I 
> misunderstand anything there ?

It should be:
PAL_PREC[max] = lut[size-2];
GCMAX = lut[size-1];

Oh, we also need to increase the LUT size by one to make that
work correctly.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Sharma, Shashank]

Regards

Shashank

On 5/6/2019 6:41 PM, Ville Syrjälä wrote:
> On Mon, May 06, 2019 at 06:25:19PM +0530, Sharma, Shashank wrote:
>> On 5/6/2019 5:55 PM, Ville Syrjälä wrote:
>>> On Mon, May 06, 2019 at 04:09:33PM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>> On 5/3/2019 9:20 PM, Ville Syrjälä wrote:
>>>>> On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
>>>>>> ICL introduces a new gamma correction mode in display engine, called
>>>>>> multi-segmented-gamma mode. This mode allows users to program the
>>>>>> darker region of the gamma curve with sueprfine precision. An
>>>>>> example use case for this is HDR curves (like PQ ST-2084).
>>>>>>
>>>>>> If we plot a gamma correction curve from value range between 0.0 to 1.0,
>>>>>> ICL's multi-segment has 3 different sections:
>>>>>> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
>>>>>> - fine segment: 257 values, ranges between 0 - 1/(128)
>>>>>> - corase segment: 257 values, ranges between 0 - 1
>>>>>>
>>>>>> This patch:
>>>>>> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
>>>>>>      so that userspace can program with highest precision supported.
>>>>>> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
>>>>>> - Adds functions to program/detect multi-segment gamma.
>>>>>>
>>>>>> V2: Addressed review comments from Ville
>>>>>>        - separate function for superfine and fine segments.
>>>>>>        - remove enum for segments.
>>>>>>        - reuse last entry of the LUT as gc_max value.
>>>>>>        - replace if() ....cond with switch...case in icl_load_luts.
>>>>>>        - add an entry variable, instead of 'word'
>>>>>>
>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>
>>>>>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/i915_pci.c    |   3 +-
>>>>>>     drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
>>>>>>     2 files changed, 123 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
>>>>>> index ffa2ee70a03d..83698951760b 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_pci.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
>>>>>> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
>>>>>>     	GEN(11), \
>>>>>>     	.ddb_size = 2048, \
>>>>>>     	.has_logical_ring_elsq = 1, \
>>>>>> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
>>>>>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
>>>>> Ugh. Thats one big LUT. But looks correct.
>>>>>
>>>>>> +
>>>>> Bogus newline.
>>>> Got it.
>>>>>>     
>>>>>>     static const struct intel_device_info intel_icelake_11_info = {
>>>>>>     	GEN11_FEATURES,
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
>>>>>> index 6c341bea514c..49831e8d02fb 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_color.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
>>>>>> @@ -41,6 +41,9 @@
>>>>>>     #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
>>>>>>     
>>>>>>     #define LEGACY_LUT_LENGTH		256
>>>>>> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
>>>>>> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
>>>>>> +
>>>>>>     /*
>>>>>>      * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
>>>>>>      * format). This macro takes the coefficient we want transformed and the
>>>>>> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
>>>>>>     	}
>>>>>>     }
>>>>>>     
>>>>>> +/* ilk+ "12.4" interpolated format (high 10 bits) */
>>>>>> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
>>>>>> +{
>>>>>> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
>>>>>> +		(color->blue >> 6);
>>>>>> +}
>>>>>> +
>>>>>> +/* ilk+ "12.4" interpolated format (low 6 bits) */
>>>>>> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
>>>>>> +{
>>>>>> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
>>>>>> +		(color->blue & 0x3f);
>>>>>> +}
>>>>>> +
>>>>>> +static void
>>>>>> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
>>>>>> +			const struct drm_color_lut *entry)
>>>>> Indentation looks off. Also s/entry/color/ to match the other similarish
>>>>> funcs maybe?
>>>> Sure.
>>>>>> +{
>>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>>> +	enum pipe pipe = crtc->pipe;
>>>>>> +
>>>>>> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
>>>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
>>>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
>>>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
>>>>>> +}
>>>>>> +
>>>>>> +static void
>>>>>> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
>>>>>> +{
>>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>>>> +	const struct drm_color_lut *lut = blob->data;
>>>>>> +	enum pipe pipe = crtc->pipe;
>>>>>> +	u32 i;
>>>>>> +
>>>>>> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
>>>>>> +		return;
>>>>> These checks aren't needed. Just dead code.
>>>> Will remove this and similars.
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
>>>>>> +	 * segment step which is 1/(8 * 128 * 256).
>>>>>> +	 *
>>>>>> +	 * Superfine segment has 9 entries, corresponding to values
>>>>>> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
>>>>>> +	 */
>>>>>> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
>>>>>> +
>>>>>> +	for (i = 0; i < 9; i++) {
>>>>>> +		const struct drm_color_lut *entry = &lut[i];
>>>>>> +
>>>>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>>>>> +			   ilk_lut_12p4_udw(entry));
>>>>> ldw should come before udw.
>>>> Got it.
>>>>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
>>>>>> +			   ilk_lut_12p4_ldw(entry));
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>> +static void
>>>>>> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
>>>>>> +{
>>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
>>>>>> +	const struct drm_color_lut *lut = blob->data;
>>>>>> +	const struct drm_color_lut *entry;
>>>>> 'entry' declaration can be moved into the loops.
>>>> Its being used in multiple loops,
>>> IMO still better to move into the loops. We don't want to pass any
>>> information between the loops.
>>>
>>>> also being used for GCMax outside the
>>>> loop.
>>> Hmm. That might be an arguemnt for keeping it out. But the current
>>> gcmax usage looks broken. You're programming the same value into
>>> the last PAL_PREC index and GCMAX.
>> Isn't this what you wanted ? IIRC, your recommendation was to program
>> the highest values user wants to program in GCMAX reg. We also had a
>> discussion on how user cant program 1.0, as we have LUT depth on 16 bit
>> only, and we decided to use the last value of the LUT as GCMAX. Did I
>> misunderstand anything there ?
> It should be:
> PAL_PREC[max] = lut[size-2];
> GCMAX = lut[size-1];
>
> Oh, we also need to increase the LUT size by one to make that
> work correctly.

Yep, that's why I was not so sure about this, and thought programming a 
1.0 is a better idea.

So now the new size would be 257 * 256 * 128 = 263,168 entries.I will do 
changes and send V3.

Regards

Shashank

>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 4/4] drm/i915/icl: Add Multi-segmented gamma support

[Ville Syrjälä]

On Mon, May 06, 2019 at 06:44:43PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> On 5/6/2019 6:41 PM, Ville Syrjälä wrote:
> > On Mon, May 06, 2019 at 06:25:19PM +0530, Sharma, Shashank wrote:
> >> On 5/6/2019 5:55 PM, Ville Syrjälä wrote:
> >>> On Mon, May 06, 2019 at 04:09:33PM +0530, Sharma, Shashank wrote:
> >>>> Regards
> >>>>
> >>>> Shashank
> >>>>
> >>>> On 5/3/2019 9:20 PM, Ville Syrjälä wrote:
> >>>>> On Tue, Apr 30, 2019 at 08:51:08PM +0530, Shashank Sharma wrote:
> >>>>>> ICL introduces a new gamma correction mode in display engine, called
> >>>>>> multi-segmented-gamma mode. This mode allows users to program the
> >>>>>> darker region of the gamma curve with sueprfine precision. An
> >>>>>> example use case for this is HDR curves (like PQ ST-2084).
> >>>>>>
> >>>>>> If we plot a gamma correction curve from value range between 0.0 to 1.0,
> >>>>>> ICL's multi-segment has 3 different sections:
> >>>>>> - superfine segment: 9 values, ranges between 0 - 1/(128 * 256)
> >>>>>> - fine segment: 257 values, ranges between 0 - 1/(128)
> >>>>>> - corase segment: 257 values, ranges between 0 - 1
> >>>>>>
> >>>>>> This patch:
> >>>>>> - Changes gamma LUTs size for ICL/GEN11 to 262144 entries (8 * 128 * 256),
> >>>>>>      so that userspace can program with highest precision supported.
> >>>>>> - Changes default gamma mode (non-legacy) to multi-segmented-gamma mode.
> >>>>>> - Adds functions to program/detect multi-segment gamma.
> >>>>>>
> >>>>>> V2: Addressed review comments from Ville
> >>>>>>        - separate function for superfine and fine segments.
> >>>>>>        - remove enum for segments.
> >>>>>>        - reuse last entry of the LUT as gc_max value.
> >>>>>>        - replace if() ....cond with switch...case in icl_load_luts.
> >>>>>>        - add an entry variable, instead of 'word'
> >>>>>>
> >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>>>
> >>>>>> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/i915/i915_pci.c    |   3 +-
> >>>>>>     drivers/gpu/drm/i915/intel_color.c | 125 ++++++++++++++++++++++++++++-
> >>>>>>     2 files changed, 123 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> >>>>>> index ffa2ee70a03d..83698951760b 100644
> >>>>>> --- a/drivers/gpu/drm/i915/i915_pci.c
> >>>>>> +++ b/drivers/gpu/drm/i915/i915_pci.c
> >>>>>> @@ -749,7 +749,8 @@ static const struct intel_device_info intel_cannonlake_info = {
> >>>>>>     	GEN(11), \
> >>>>>>     	.ddb_size = 2048, \
> >>>>>>     	.has_logical_ring_elsq = 1, \
> >>>>>> -	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
> >>>>>> +	.color = { .degamma_lut_size = 33, .gamma_lut_size = 262144 }
> >>>>> Ugh. Thats one big LUT. But looks correct.
> >>>>>
> >>>>>> +
> >>>>> Bogus newline.
> >>>> Got it.
> >>>>>>     
> >>>>>>     static const struct intel_device_info intel_icelake_11_info = {
> >>>>>>     	GEN11_FEATURES,
> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> >>>>>> index 6c341bea514c..49831e8d02fb 100644
> >>>>>> --- a/drivers/gpu/drm/i915/intel_color.c
> >>>>>> +++ b/drivers/gpu/drm/i915/intel_color.c
> >>>>>> @@ -41,6 +41,9 @@
> >>>>>>     #define CTM_COEFF_ABS(coeff)		((coeff) & (CTM_COEFF_SIGN - 1))
> >>>>>>     
> >>>>>>     #define LEGACY_LUT_LENGTH		256
> >>>>>> +#define ICL_GAMMA_MULTISEG_LUT_LENGTH		(256 * 128 * 8)
> >>>>>> +#define ICL_GAMMA_SUPERFINE_SEG_LENGTH	9
> >>>>>> +
> >>>>>>     /*
> >>>>>>      * Extract the CSC coefficient from a CTM coefficient (in U32.32 fixed point
> >>>>>>      * format). This macro takes the coefficient we want transformed and the
> >>>>>> @@ -767,6 +770,113 @@ static void glk_load_luts(const struct intel_crtc_state *crtc_state)
> >>>>>>     	}
> >>>>>>     }
> >>>>>>     
> >>>>>> +/* ilk+ "12.4" interpolated format (high 10 bits) */
> >>>>>> +static u32 ilk_lut_12p4_ldw(const struct drm_color_lut *color)
> >>>>>> +{
> >>>>>> +	return (color->red >> 6) << 20 | (color->green >> 6) << 10 |
> >>>>>> +		(color->blue >> 6);
> >>>>>> +}
> >>>>>> +
> >>>>>> +/* ilk+ "12.4" interpolated format (low 6 bits) */
> >>>>>> +static u32 ilk_lut_12p4_udw(const struct drm_color_lut *color)
> >>>>>> +{
> >>>>>> +	return (color->red & 0x3f) << 24 | (color->green & 0x3f) << 14 |
> >>>>>> +		(color->blue & 0x3f);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void
> >>>>>> +icl_load_gcmax(const struct intel_crtc_state *crtc_state,
> >>>>>> +			const struct drm_color_lut *entry)
> >>>>> Indentation looks off. Also s/entry/color/ to match the other similarish
> >>>>> funcs maybe?
> >>>> Sure.
> >>>>>> +{
> >>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>>>>> +	enum pipe pipe = crtc->pipe;
> >>>>>> +
> >>>>>> +	/* Fixme: LUT entries are 16 bit only, so we can prog 0xFFFF max */
> >>>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), entry->red);
> >>>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), entry->green);
> >>>>>> +	I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), entry->blue);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void
> >>>>>> +icl_program_gamma_superfine_segment(const struct intel_crtc_state *crtc_state)
> >>>>>> +{
> >>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>>>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> >>>>>> +	const struct drm_color_lut *lut = blob->data;
> >>>>>> +	enum pipe pipe = crtc->pipe;
> >>>>>> +	u32 i;
> >>>>>> +
> >>>>>> +	if (!lut || drm_color_lut_size(blob) < ICL_GAMMA_SUPERFINE_SEG_LENGTH)
> >>>>>> +		return;
> >>>>> These checks aren't needed. Just dead code.
> >>>> Will remove this and similars.
> >>>>>> +
> >>>>>> +	/*
> >>>>>> +	 * Every entry in the multi-segment LUT is corresponding to a superfine
> >>>>>> +	 * segment step which is 1/(8 * 128 * 256).
> >>>>>> +	 *
> >>>>>> +	 * Superfine segment has 9 entries, corresponding to values
> >>>>>> +	 * 0, 1/(8 * 128 * 256), 2/(8 * 128 * 256) .... 8/(8 * 128 * 256).
> >>>>>> +	 */
> >>>>>> +	I915_WRITE(PREC_PAL_MULTI_SEG_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> >>>>>> +
> >>>>>> +	for (i = 0; i < 9; i++) {
> >>>>>> +		const struct drm_color_lut *entry = &lut[i];
> >>>>>> +
> >>>>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> >>>>>> +			   ilk_lut_12p4_udw(entry));
> >>>>> ldw should come before udw.
> >>>> Got it.
> >>>>>> +		I915_WRITE(PREC_PAL_MULTI_SEG_DATA(pipe),
> >>>>>> +			   ilk_lut_12p4_ldw(entry));
> >>>>>> +	}
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void
> >>>>>> +icl_program_gamma_multi_segment(const struct intel_crtc_state *crtc_state)
> >>>>>> +{
> >>>>>> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >>>>>> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >>>>>> +	const struct drm_property_blob *blob = crtc_state->base.gamma_lut;
> >>>>>> +	const struct drm_color_lut *lut = blob->data;
> >>>>>> +	const struct drm_color_lut *entry;
> >>>>> 'entry' declaration can be moved into the loops.
> >>>> Its being used in multiple loops,
> >>> IMO still better to move into the loops. We don't want to pass any
> >>> information between the loops.
> >>>
> >>>> also being used for GCMax outside the
> >>>> loop.
> >>> Hmm. That might be an arguemnt for keeping it out. But the current
> >>> gcmax usage looks broken. You're programming the same value into
> >>> the last PAL_PREC index and GCMAX.
> >> Isn't this what you wanted ? IIRC, your recommendation was to program
> >> the highest values user wants to program in GCMAX reg. We also had a
> >> discussion on how user cant program 1.0, as we have LUT depth on 16 bit
> >> only, and we decided to use the last value of the LUT as GCMAX. Did I
> >> misunderstand anything there ?
> > It should be:
> > PAL_PREC[max] = lut[size-2];
> > GCMAX = lut[size-1];
> >
> > Oh, we also need to increase the LUT size by one to make that
> > work correctly.
> 
> Yep, that's why I was not so sure about this, and thought programming a 
> 1.0 is a better idea.
> 
> So now the new size would be 257 * 256 * 128 = 263,168 entries.I will do 
> changes and send V3.

Should be just 256*128*8+1=262145

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx