[PATCH 0/3] Add support for Gen 11 pipe color features

[PATCH 0/3] Add support for Gen 11 pipe color features

[Uma Shankar]

This patch series adds support for Gen11 pipe degamma, CSC
and gamma hardware blocks.

CRC checks are not working for 10bit gamma but works for 8bit
pallete modes which seems to be due to some rounding errors in pipe.

ToDo: Support for Multi Segmented Gamma will be added later.

v2: Addressed Maarten's review comments and re-ordered the patch
series.

Uma Shankar (3):
  drm/i915/icl: Add icl pipe degamma and gamma support
  drm/i915/icl: Enable ICL Pipe CSC block
  drm/i915/icl: Add degamma and gamma lut size to gen11 caps

 drivers/gpu/drm/i915/i915_pci.c    |  3 +-
 drivers/gpu/drm/i915/i915_reg.h    |  4 ++
 drivers/gpu/drm/i915/intel_color.c | 88 +++++++++++++++++++++++++++++++++++++-
 3 files changed, 92 insertions(+), 3 deletions(-)

-- 
1.9.1

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

[PATCH 1/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Uma Shankar]

Add support for icl pipe degamma and gamma.

v2: Removed a POSTING_READ and corrected the Bit
Definition as per Maarten's comments.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  3 ++
 drivers/gpu/drm/i915/intel_color.c | 74 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bd61f9..3adf689 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6965,6 +6965,9 @@ enum {
 #define GAMMA_MODE_MODE_12BIT	(2 << 0)
 #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
 
+#define PRE_CSC_GAMMA_ENABLE	(1 << 31)
+#define POST_CSC_GAMMA_ENABLE	(1 << 30)
+
 /* DMC/CSR */
 #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
 #define CSR_SSP_BASE_ADDR_GEN9	0x00002FC0
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 5127da2..12c659f 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -424,6 +424,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
 	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
 	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
 	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	uint32_t tmp;
 
 	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
 
@@ -464,6 +465,11 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
 		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
 		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
 	}
+
+	if (IS_ICELAKE(dev_priv)) {
+		tmp = I915_READ(GAMMA_MODE(pipe));
+		I915_WRITE(GAMMA_MODE(pipe), tmp | POST_CSC_GAMMA_ENABLE);
+	}
 }
 
 /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
@@ -523,6 +529,50 @@ static void glk_load_degamma_lut(struct drm_crtc_state *state)
 		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
 }
 
+static void icl_load_degamma_lut(struct drm_crtc_state *state)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
+	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
+	const uint32_t lut_size = 33;
+	uint32_t tmp, i;
+
+	/*
+	 * When setting the auto-increment bit, the hardware seems to
+	 * ignore the index bits, so we need to reset it to index 0
+	 * separately.
+	 */
+	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
+	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
+
+	if (state->degamma_lut) {
+		struct drm_color_lut *lut =
+			(struct drm_color_lut *) state->degamma_lut->data;
+		for (i = 0; i < lut_size; i++) {
+			/*
+			 * Currently Clamp input to 1.0.
+			 * ToDo: Extend to max 7.0.
+			 */
+			uint32_t word =
+				drm_color_lut_extract(lut[i].red, 16);
+			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
+		}
+	} else {
+		/* load a linear table. */
+		for (i = 0; i < lut_size; i++) {
+			uint32_t v = (i * (1 << 16)) / (lut_size - 1);
+
+			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
+		}
+	}
+
+	tmp = I915_READ(GAMMA_MODE(pipe));
+	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
+
+	/* Clamp values > 1.0. */
+	while (i++ < 35)
+		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
+}
+
 static void glk_load_luts(struct drm_crtc_state *state)
 {
 	struct drm_crtc *crtc = state->crtc;
@@ -606,6 +656,28 @@ static void cherryview_load_luts(struct drm_crtc_state *state)
 	i9xx_load_luts_internal(crtc, NULL, to_intel_crtc_state(state));
 }
 
+static void icl_load_luts(struct drm_crtc_state *state)
+{
+	struct drm_crtc *crtc = state->crtc;
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+
+	icl_load_degamma_lut(state);
+
+	if (crtc_state_is_legacy_gamma(state)) {
+		haswell_load_luts(state);
+		return;
+	}
+
+	bdw_load_gamma_lut(state, 0);
+	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
+
+	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
+			intel_state->gamma_mode);
+}
+
 void intel_color_load_luts(struct drm_crtc_state *crtc_state)
 {
 	struct drm_device *dev = crtc_state->crtc->dev;
@@ -662,6 +734,8 @@ void intel_color_init(struct drm_crtc *crtc)
 	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
 		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = glk_load_luts;
+	} else if (IS_ICELAKE(dev_priv)) {
+		dev_priv->display.load_luts = icl_load_luts;
 	} else {
 		dev_priv->display.load_luts = i9xx_load_luts;
 	}
-- 
1.9.1

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

[PATCH 2/3] drm/i915/icl: Enable ICL Pipe CSC block

[Uma Shankar]

Enable ICL pipe csc hardware. CSC block is enabled
in CSC_MODE register instead of PLANE_COLOR_CTL.

v2: Addressed Maarten's review comments.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h    |  1 +
 drivers/gpu/drm/i915/intel_color.c | 14 ++++++++++++--
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 3adf689..857ce79 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9700,6 +9700,7 @@ enum skl_power_gate {
 #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
 #define _PIPE_A_CSC_COEFF_BV	0x49024
 #define _PIPE_A_CSC_MODE	0x49028
+#define   CSC_ENABLE			(1 << 31)
 #define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
 #define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
 #define   CSC_MODE_YUV_TO_RGB		(1 << 0)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index 12c659f..e7042fd 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -129,7 +129,12 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
 	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
 	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
 	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
-	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
+
+	if (INTEL_GEN(dev_priv) >= 11)
+		I915_WRITE(PIPE_CSC_MODE(pipe),
+			I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);
+	else
+		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
 }
 
 static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
@@ -239,7 +244,11 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
 		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
 
-		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
+		if (INTEL_GEN(dev_priv) >= 11)
+			I915_WRITE(PIPE_CSC_MODE(pipe),
+				I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);
+		else
+			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
 	} else {
 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
 
@@ -735,6 +744,7 @@ void intel_color_init(struct drm_crtc *crtc)
 		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = glk_load_luts;
 	} else if (IS_ICELAKE(dev_priv)) {
+		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
 		dev_priv->display.load_luts = icl_load_luts;
 	} else {
 		dev_priv->display.load_luts = i9xx_load_luts;
-- 
1.9.1

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

[PATCH 3/3] drm/i915/icl: Add degamma and gamma lut size to gen11 caps

[Uma Shankar]

Add the degamma and gamma lut sizes to gen11 capability
structure.

v2: Reorder the patch as per Maarten's suggestion.

Signed-off-by: Uma Shankar <uma.shankar@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index 44e7459..3c18ea2 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -603,7 +603,8 @@
 			   TRANSCODER_DSI0_OFFSET, TRANSCODER_DSI1_OFFSET}, \
 	GEN(11), \
 	.ddb_size = 2048, \
-	.has_logical_ring_elsq = 1
+	.has_logical_ring_elsq = 1, \
+	.color = { .degamma_lut_size = 33, .gamma_lut_size = 1024 }
 
 static const struct intel_device_info intel_icelake_11_info = {
 	GEN11_FEATURES,
-- 
1.9.1

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

✗ Fi.CI.CHECKPATCH: warning for Add support for Gen 11 pipe color features (rev2)

[Patchwork]

== Series Details ==

Series: Add support for Gen 11 pipe color features (rev2)
URL   : https://patchwork.freedesktop.org/series/51408/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
019fa0c46108 drm/i915/icl: Add icl pipe degamma and gamma support
-:72: CHECK:SPACING: No space is necessary after a cast
#72: FILE: drivers/gpu/drm/i915/intel_color.c:549:
+			(struct drm_color_lut *) state->degamma_lut->data;

total: 0 errors, 0 warnings, 1 checks, 113 lines checked
c872b768776c drm/i915/icl: Enable ICL Pipe CSC block
-:37: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#37: FILE: drivers/gpu/drm/i915/intel_color.c:135:
+		I915_WRITE(PIPE_CSC_MODE(pipe),
+			I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);

-:50: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#50: FILE: drivers/gpu/drm/i915/intel_color.c:249:
+			I915_WRITE(PIPE_CSC_MODE(pipe),
+				I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);

total: 0 errors, 0 warnings, 2 checks, 39 lines checked
93c7f3af1ed7 drm/i915/icl: Add degamma and gamma lut size to gen11 caps

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

✗ Fi.CI.BAT: failure for Add support for Gen 11 pipe color features (rev2)

[Patchwork]

== Series Details ==

Series: Add support for Gen 11 pipe color features (rev2)
URL   : https://patchwork.freedesktop.org/series/51408/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_5028 -> Patchwork_10563 =

== Summary - FAILURE ==

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/51408/revisions/2/mbox/

== Possible new issues ==

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

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_addfb_basic@clobberred-modifier:
      fi-kbl-8809g:       PASS -> FAIL +11

    igt@kms_addfb_basic@size-max:
      fi-kbl-8809g:       PASS -> WARN +15

    igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
      fi-kbl-8809g:       SKIP -> FAIL +1

    igt@kms_flip@basic-flip-vs-dpms:
      fi-kbl-8809g:       SKIP -> WARN +3

    
    ==== Warnings ====

    igt@kms_addfb_basic@unused-modifier:
      fi-kbl-8809g:       PASS -> SKIP +8

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
      fi-kbl-8809g:       SKIP -> PASS +19

    
== Known issues ==

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

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_frontbuffer_tracking@basic:
      fi-byt-clapper:     PASS -> FAIL (fdo#103167)

    igt@kms_pipe_crc_basic@hang-read-crc-pipe-b:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    
    ==== Possible fixes ====

    igt@pm_rpm@module-reload:
      fi-skl-6600u:       INCOMPLETE (fdo#107807) -> PASS

    
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807


== Participating hosts (47 -> 41) ==

  Missing    (6): fi-kbl-soraka fi-hsw-4770r fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_5028 -> Patchwork_10563

  CI_DRM_5028: 0d4b0d09075cb5334c622099724d7b3af605bed9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4689: fce6109b879f720755399f37701eff30c646f89b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10563: 93c7f3af1ed74c2ac3b79f8c3beae347804379a9 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

93c7f3af1ed7 drm/i915/icl: Add degamma and gamma lut size to gen11 caps
c872b768776c drm/i915/icl: Enable ICL Pipe CSC block
019fa0c46108 drm/i915/icl: Add icl pipe degamma and gamma support

== Logs ==

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

Re: [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Matt Roper]

On Wed, Oct 24, 2018 at 08:30:11PM +0530, Uma Shankar wrote:
> Add support for icl pipe degamma and gamma.
> 
> v2: Removed a POSTING_READ and corrected the Bit
> Definition as per Maarten's comments.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>  drivers/gpu/drm/i915/intel_color.c | 74 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 77 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8bd61f9..3adf689 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6965,6 +6965,9 @@ enum {
>  #define GAMMA_MODE_MODE_12BIT	(2 << 0)
>  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
>  
> +#define PRE_CSC_GAMMA_ENABLE	(1 << 31)
> +#define POST_CSC_GAMMA_ENABLE	(1 << 30)
> +
>  /* DMC/CSR */
>  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
>  #define CSR_SSP_BASE_ADDR_GEN9	0x00002FC0
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 5127da2..12c659f 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -424,6 +424,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
>  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	uint32_t tmp;
>  
>  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>  
> @@ -464,6 +465,11 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
>  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
>  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
>  	}
> +
> +	if (IS_ICELAKE(dev_priv)) {

We probably want to do "INTEL_GEN(dev_priv) >= 11" here to future-proof
this against future gen's (or other platforms that might match gen11
design without being classified as icelake).

> +		tmp = I915_READ(GAMMA_MODE(pipe));
> +		I915_WRITE(GAMMA_MODE(pipe), tmp | POST_CSC_GAMMA_ENABLE);
> +	}

We generally try to avoid read-modify-write approaches to updating
registers; it's better to build the full register value from scratch to
make sure we know exactly what's in it.

There are a few places in this patch you do a r-m-w cycle.  I'd suggest
we add an intel_crtc_state->gamma_mode field and calculate all the
appropriate bits during the atomic check phase.  Then we can just do a

    if (INTEL_GEN(dev_priv) >= 11)
        I915_WRITE(GAMMA_MODE(pipe), intel_crtc_state->gamma_mode);

once somewhere more central any time crtc_state->color_mgmt_changed is
true.

>  }
>  
>  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
> @@ -523,6 +529,50 @@ static void glk_load_degamma_lut(struct drm_crtc_state *state)
>  		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
>  }
>  
> +static void icl_load_degamma_lut(struct drm_crtc_state *state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> +	const uint32_t lut_size = 33;

Isn't this the value from INTEL_INFO(dev)->color.degamma_lut_size?
Actually, upon closer inspection it looks like LUT size, ranges, etc. is
sort of complicated; more comments farther down.

> +	uint32_t tmp, i;
> +
> +	/*
> +	 * When setting the auto-increment bit, the hardware seems to
> +	 * ignore the index bits, so we need to reset it to index 0
> +	 * separately.
> +	 */

The final sentence of the bspec description says (emphasis mine):

        "While in auto increment mode, after performing reads or writes
        to only part of the range, the auto increment bit must be
        cleared *before* resetting the index value."

so I suspect it's technically the first write (which disables the
auto-increment) that ignores the index bits.  Then your second write
both sets the index bits (to 0) and turns auto-increment back on.

> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> +
> +	if (state->degamma_lut) {
> +		struct drm_color_lut *lut =
> +			(struct drm_color_lut *) state->degamma_lut->data;
> +		for (i = 0; i < lut_size; i++) {
> +			/*
> +			 * Currently Clamp input to 1.0.
> +			 * ToDo: Extend to max 7.0.
> +			 */

This comment is a little confusing.  I think what you mean is that this
for() loop covers the first 33 entries, which represent the range of
inputs from 0 to 1.0.  The remaining two entries are for extended range
of inputs (representing gamma values at 3.0 and 7.0) and are not evenly
spaced.

> +			uint32_t word =
> +				drm_color_lut_extract(lut[i].red, 16);
> +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
> +		}

Following the loop is where we'd need to deal with the two extended
range entries, right?  So if we leave dealing with extended range
properly as a TODO for now, then I think we want to fill them in with
linear values.  E.g.,

     I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x30000);
     I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x70000);

Also, I believe the 1.0 input can have a value >= 1.0, so we need to
extract more bits of precision for that.

I'm not really an expert on how this color management stuff gets used by
userspace in practice, but I suspect our current ABI probably needs to
be augmented to deal more complicated gamma LUT's than we support today.
E.g., different ranges with different input values, precision, etc.  Do
we have a clear understanding of if/how userspace wants to deal with
extended range?

> +	} else {
> +		/* load a linear table. */
> +		for (i = 0; i < lut_size; i++) {
> +			uint32_t v = (i * (1 << 16)) / (lut_size - 1);
> +
> +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> +		}

As above, I think we should still fill in the last two entries with
linear values for now.

> +	}
> +
> +	tmp = I915_READ(GAMMA_MODE(pipe));
> +	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
> +
> +	/* Clamp values > 1.0. */
> +	while (i++ < 35)
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> +}
> +
>  static void glk_load_luts(struct drm_crtc_state *state)
>  {
>  	struct drm_crtc *crtc = state->crtc;
> @@ -606,6 +656,28 @@ static void cherryview_load_luts(struct drm_crtc_state *state)
>  	i9xx_load_luts_internal(crtc, NULL, to_intel_crtc_state(state));
>  }
>  
> +static void icl_load_luts(struct drm_crtc_state *state)
> +{
> +	struct drm_crtc *crtc = state->crtc;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +
> +	icl_load_degamma_lut(state);
> +
> +	if (crtc_state_is_legacy_gamma(state)) {
> +		haswell_load_luts(state);
> +		return;
> +	}

This block will only run if there's no degamma LUT, so it might be
slightly easier to read the code if we move this above the
icl_load_degamma_lut() call above?

> +
> +	bdw_load_gamma_lut(state, 0);
> +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;

I think we also need to update PAL_EXT_GC_MAX with a FP3.16 for the 3.0
input value and the PAL_EXT2_GC_MAX for the 7.0 input.  Assuming we want
to leave the ABI details of this until later, I assume we'd handle this
with linear values like I suggested for the degamma extended range
entries?


Matt

> +
> +	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
> +			intel_state->gamma_mode);
> +}
> +
>  void intel_color_load_luts(struct drm_crtc_state *crtc_state)
>  {
>  	struct drm_device *dev = crtc_state->crtc->dev;
> @@ -662,6 +734,8 @@ void intel_color_init(struct drm_crtc *crtc)
>  	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = glk_load_luts;
> +	} else if (IS_ICELAKE(dev_priv)) {
> +		dev_priv->display.load_luts = icl_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
>  	}
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/icl: Enable ICL Pipe CSC block

[Matt Roper]

On Wed, Oct 24, 2018 at 08:30:12PM +0530, Uma Shankar wrote:
> Enable ICL pipe csc hardware. CSC block is enabled
> in CSC_MODE register instead of PLANE_COLOR_CTL.
> 
> v2: Addressed Maarten's review comments.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  1 +
>  drivers/gpu/drm/i915/intel_color.c | 14 ++++++++++++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 3adf689..857ce79 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9700,6 +9700,7 @@ enum skl_power_gate {
>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>  #define _PIPE_A_CSC_COEFF_BV	0x49024
>  #define _PIPE_A_CSC_MODE	0x49028
> +#define   CSC_ENABLE			(1 << 31)
>  #define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
>  #define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
>  #define   CSC_MODE_YUV_TO_RGB		(1 << 0)
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index 12c659f..e7042fd 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -129,7 +129,12 @@ static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
>  	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
>  	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), POSTOFF_RGB_TO_YUV_ME);
>  	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
> -	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		I915_WRITE(PIPE_CSC_MODE(pipe),
> +			I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);

Is this the right approach for gen11?  The bspec details are pretty
sparse, but it looks like with gen11+, we now have two CSC's so we can
do both a userspace-provided CSC matrix (bit 31 in CSC_MODE) and a
separate RGB->YUV matrix (bit 30 in CSC_MODE and the OUTPUT_CSC_*
registers) rather than having to pick just one or the other.

Also, as noted on the other patch, we generally don't want to do
read-modify-write patterns in the driver.


Matt

> +	else
> +		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>  }
>  
>  static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> @@ -239,7 +244,11 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>  		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>  
> -		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +		if (INTEL_GEN(dev_priv) >= 11)
> +			I915_WRITE(PIPE_CSC_MODE(pipe),
> +				I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);
> +		else
> +			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>  	} else {
>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>  
> @@ -735,6 +744,7 @@ void intel_color_init(struct drm_crtc *crtc)
>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = glk_load_luts;
>  	} else if (IS_ICELAKE(dev_priv)) {
> +		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>  		dev_priv->display.load_luts = icl_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Matt Roper]

On Tue, Oct 30, 2018 at 05:03:57PM -0700, Matt Roper wrote:
> On Wed, Oct 24, 2018 at 08:30:11PM +0530, Uma Shankar wrote:
> > Add support for icl pipe degamma and gamma.
> > 
> > v2: Removed a POSTING_READ and corrected the Bit
> > Definition as per Maarten's comments.
> > 
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
> >  drivers/gpu/drm/i915/intel_color.c | 74 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8bd61f9..3adf689 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6965,6 +6965,9 @@ enum {
> >  #define GAMMA_MODE_MODE_12BIT	(2 << 0)
> >  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
> >  
> > +#define PRE_CSC_GAMMA_ENABLE	(1 << 31)
> > +#define POST_CSC_GAMMA_ENABLE	(1 << 30)
> > +
> >  /* DMC/CSR */
> >  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
> >  #define CSR_SSP_BASE_ADDR_GEN9	0x00002FC0
> > diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> > index 5127da2..12c659f 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -424,6 +424,7 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> >  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> >  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> >  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> > +	uint32_t tmp;
> >  
> >  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
> >  
> > @@ -464,6 +465,11 @@ static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
> >  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
> >  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
> >  	}
> > +
> > +	if (IS_ICELAKE(dev_priv)) {
> 
> We probably want to do "INTEL_GEN(dev_priv) >= 11" here to future-proof
> this against future gen's (or other platforms that might match gen11
> design without being classified as icelake).
> 
> > +		tmp = I915_READ(GAMMA_MODE(pipe));
> > +		I915_WRITE(GAMMA_MODE(pipe), tmp | POST_CSC_GAMMA_ENABLE);
> > +	}
> 
> We generally try to avoid read-modify-write approaches to updating
> registers; it's better to build the full register value from scratch to
> make sure we know exactly what's in it.
> 
> There are a few places in this patch you do a r-m-w cycle.  I'd suggest
> we add an intel_crtc_state->gamma_mode field and calculate all the
> appropriate bits during the atomic check phase.  Then we can just do a
> 
>     if (INTEL_GEN(dev_priv) >= 11)
>         I915_WRITE(GAMMA_MODE(pipe), intel_crtc_state->gamma_mode);
> 
> once somewhere more central any time crtc_state->color_mgmt_changed is
> true.
> 
> >  }
> >  
> >  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
> > @@ -523,6 +529,50 @@ static void glk_load_degamma_lut(struct drm_crtc_state *state)
> >  		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> >  }
> >  
> > +static void icl_load_degamma_lut(struct drm_crtc_state *state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> > +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> > +	const uint32_t lut_size = 33;
> 
> Isn't this the value from INTEL_INFO(dev)->color.degamma_lut_size?
> Actually, upon closer inspection it looks like LUT size, ranges, etc. is
> sort of complicated; more comments farther down.
> 
> > +	uint32_t tmp, i;
> > +
> > +	/*
> > +	 * When setting the auto-increment bit, the hardware seems to
> > +	 * ignore the index bits, so we need to reset it to index 0
> > +	 * separately.
> > +	 */
> 
> The final sentence of the bspec description says (emphasis mine):
> 
>         "While in auto increment mode, after performing reads or writes
>         to only part of the range, the auto increment bit must be
>         cleared *before* resetting the index value."
> 
> so I suspect it's technically the first write (which disables the
> auto-increment) that ignores the index bits.  Then your second write
> both sets the index bits (to 0) and turns auto-increment back on.
> 
> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), PRE_CSC_GAMC_AUTO_INCREMENT);
> > +
> > +	if (state->degamma_lut) {
> > +		struct drm_color_lut *lut =
> > +			(struct drm_color_lut *) state->degamma_lut->data;
> > +		for (i = 0; i < lut_size; i++) {
> > +			/*
> > +			 * Currently Clamp input to 1.0.
> > +			 * ToDo: Extend to max 7.0.
> > +			 */
> 
> This comment is a little confusing.  I think what you mean is that this
> for() loop covers the first 33 entries, which represent the range of
> inputs from 0 to 1.0.  The remaining two entries are for extended range
> of inputs (representing gamma values at 3.0 and 7.0) and are not evenly
> spaced.
> 
> > +			uint32_t word =
> > +				drm_color_lut_extract(lut[i].red, 16);
> > +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
> > +		}
> 
> Following the loop is where we'd need to deal with the two extended
> range entries, right?  So if we leave dealing with extended range
> properly as a TODO for now, then I think we want to fill them in with
> linear values.  E.g.,
> 
>      I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x30000);
>      I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x70000);

Actually, it looks like you actually do set these at the bottom of the
function (and clamp to 1.0), so that's fine and you can disregard this
comment.


Matt

> 
> Also, I believe the 1.0 input can have a value >= 1.0, so we need to
> extract more bits of precision for that.
> 
> I'm not really an expert on how this color management stuff gets used by
> userspace in practice, but I suspect our current ABI probably needs to
> be augmented to deal more complicated gamma LUT's than we support today.
> E.g., different ranges with different input values, precision, etc.  Do
> we have a clear understanding of if/how userspace wants to deal with
> extended range?
> 
> > +	} else {
> > +		/* load a linear table. */
> > +		for (i = 0; i < lut_size; i++) {
> > +			uint32_t v = (i * (1 << 16)) / (lut_size - 1);
> > +
> > +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> > +		}
> 
> As above, I think we should still fill in the last two entries with
> linear values for now.
> 
> > +	}
> > +
> > +	tmp = I915_READ(GAMMA_MODE(pipe));
> > +	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
> > +
> > +	/* Clamp values > 1.0. */
> > +	while (i++ < 35)
> > +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));
> > +}
> > +
> >  static void glk_load_luts(struct drm_crtc_state *state)
> >  {
> >  	struct drm_crtc *crtc = state->crtc;
> > @@ -606,6 +656,28 @@ static void cherryview_load_luts(struct drm_crtc_state *state)
> >  	i9xx_load_luts_internal(crtc, NULL, to_intel_crtc_state(state));
> >  }
> >  
> > +static void icl_load_luts(struct drm_crtc_state *state)
> > +{
> > +	struct drm_crtc *crtc = state->crtc;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
> > +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > +
> > +	icl_load_degamma_lut(state);
> > +
> > +	if (crtc_state_is_legacy_gamma(state)) {
> > +		haswell_load_luts(state);
> > +		return;
> > +	}
> 
> This block will only run if there's no degamma LUT, so it might be
> slightly easier to read the code if we move this above the
> icl_load_degamma_lut() call above?
> 
> > +
> > +	bdw_load_gamma_lut(state, 0);
> > +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> 
> I think we also need to update PAL_EXT_GC_MAX with a FP3.16 for the 3.0
> input value and the PAL_EXT2_GC_MAX for the 7.0 input.  Assuming we want
> to leave the ABI details of this until later, I assume we'd handle this
> with linear values like I suggested for the degamma extended range
> entries?
> 
> 
> Matt
> 
> > +
> > +	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
> > +			intel_state->gamma_mode);
> > +}
> > +
> >  void intel_color_load_luts(struct drm_crtc_state *crtc_state)
> >  {
> >  	struct drm_device *dev = crtc_state->crtc->dev;
> > @@ -662,6 +734,8 @@ void intel_color_init(struct drm_crtc *crtc)
> >  	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
> >  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
> >  		dev_priv->display.load_luts = glk_load_luts;
> > +	} else if (IS_ICELAKE(dev_priv)) {
> > +		dev_priv->display.load_luts = icl_load_luts;
> >  	} else {
> >  		dev_priv->display.load_luts = i9xx_load_luts;
> >  	}
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and gamma support

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, November 1, 2018 5:10 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915/icl: Add icl pipe degamma and
>gamma support
>
>On Tue, Oct 30, 2018 at 05:03:57PM -0700, Matt Roper wrote:
>> On Wed, Oct 24, 2018 at 08:30:11PM +0530, Uma Shankar wrote:
>> > Add support for icl pipe degamma and gamma.
>> >
>> > v2: Removed a POSTING_READ and corrected the Bit Definition as per
>> > Maarten's comments.
>> >
>> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>> >  drivers/gpu/drm/i915/intel_color.c | 74
>> > ++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 77 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > b/drivers/gpu/drm/i915/i915_reg.h index 8bd61f9..3adf689 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -6965,6 +6965,9 @@ enum {
>> >  #define GAMMA_MODE_MODE_12BIT	(2 << 0)
>> >  #define GAMMA_MODE_MODE_SPLIT	(3 << 0)
>> >
>> > +#define PRE_CSC_GAMMA_ENABLE	(1 << 31)
>> > +#define POST_CSC_GAMMA_ENABLE	(1 << 30)
>> > +
>> >  /* DMC/CSR */
>> >  #define CSR_PROGRAM(i)		_MMIO(0x80000 + (i) * 4)
>> >  #define CSR_SSP_BASE_ADDR_GEN9	0x00002FC0
>> > diff --git a/drivers/gpu/drm/i915/intel_color.c
>> > b/drivers/gpu/drm/i915/intel_color.c
>> > index 5127da2..12c659f 100644
>> > --- a/drivers/gpu/drm/i915/intel_color.c
>> > +++ b/drivers/gpu/drm/i915/intel_color.c
>> > @@ -424,6 +424,7 @@ static void bdw_load_gamma_lut(struct
>drm_crtc_state *state, u32 offset)
>> >  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>> >  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>> >  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>> > +	uint32_t tmp;
>> >
>> >  	WARN_ON(offset & ~PAL_PREC_INDEX_VALUE_MASK);
>> >
>> > @@ -464,6 +465,11 @@ static void bdw_load_gamma_lut(struct
>drm_crtc_state *state, u32 offset)
>> >  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
>> >  		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
>> >  	}
>> > +
>> > +	if (IS_ICELAKE(dev_priv)) {
>>
>> We probably want to do "INTEL_GEN(dev_priv) >= 11" here to
>> future-proof this against future gen's (or other platforms that might
>> match gen11 design without being classified as icelake).

Ok, will modify this.

>> > +		tmp = I915_READ(GAMMA_MODE(pipe));
>> > +		I915_WRITE(GAMMA_MODE(pipe), tmp |
>POST_CSC_GAMMA_ENABLE);
>> > +	}
>>
>> We generally try to avoid read-modify-write approaches to updating
>> registers; it's better to build the full register value from scratch
>> to make sure we know exactly what's in it.
>> There are a few places in this patch you do a r-m-w cycle.  I'd
>> suggest we add an intel_crtc_state->gamma_mode field and calculate all
>> the appropriate bits during the atomic check phase.  Then we can just
>> do a
>>
>>     if (INTEL_GEN(dev_priv) >= 11)
>>         I915_WRITE(GAMMA_MODE(pipe), intel_crtc_state->gamma_mode);
>>
>> once somewhere more central any time crtc_state->color_mgmt_changed is
>> true.

Sure, will do it in similar way.

>> >  }
>> >
>> >  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */ @@
>> > -523,6 +529,50 @@ static void glk_load_degamma_lut(struct drm_crtc_state
>*state)
>> >  		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16));  }
>> >
>> > +static void icl_load_degamma_lut(struct drm_crtc_state *state) {
>> > +	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>> > +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>> > +	const uint32_t lut_size = 33;
>>
>> Isn't this the value from INTEL_INFO(dev)->color.degamma_lut_size?

Yes, this hardcoding can be dropped from here.

>> Actually, upon closer inspection it looks like LUT size, ranges, etc.
>> is sort of complicated; more comments farther down.

Yeah, with the introduction of multi segmented gamma this is really
not straight forward. Current implementation just handle things without
multi segmented gamma. I will add support of multi segmented gamma
and introduce some kind of tuples (set of 3 or 4 struct elements to define
a segment). Ville has given some ideas on the same.

>>
>> > +	uint32_t tmp, i;
>> > +
>> > +	/*
>> > +	 * When setting the auto-increment bit, the hardware seems to
>> > +	 * ignore the index bits, so we need to reset it to index 0
>> > +	 * separately.
>> > +	 */
>>
>> The final sentence of the bspec description says (emphasis mine):
>>
>>         "While in auto increment mode, after performing reads or writes
>>         to only part of the range, the auto increment bit must be
>>         cleared *before* resetting the index value."
>>
>> so I suspect it's technically the first write (which disables the
>> auto-increment) that ignores the index bits.  Then your second write
>> both sets the index bits (to 0) and turns auto-increment back on.

Yes, this is my understanding as well. So, when we need to reprogram the
LUT, take index to 0 and set auto increment. This is working and all the
LUT values get programmed correctly.

>> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe), 0);
>> > +	I915_WRITE(PRE_CSC_GAMC_INDEX(pipe),
>PRE_CSC_GAMC_AUTO_INCREMENT);
>> > +
>> > +	if (state->degamma_lut) {
>> > +		struct drm_color_lut *lut =
>> > +			(struct drm_color_lut *) state->degamma_lut->data;
>> > +		for (i = 0; i < lut_size; i++) {
>> > +			/*
>> > +			 * Currently Clamp input to 1.0.
>> > +			 * ToDo: Extend to max 7.0.
>> > +			 */
>>
>> This comment is a little confusing.  I think what you mean is that
>> this
>> for() loop covers the first 33 entries, which represent the range of
>> inputs from 0 to 1.0.  The remaining two entries are for extended
>> range of inputs (representing gamma values at 3.0 and 7.0) and are not
>> evenly spaced.

Yes, you are right. I will add bit more explanation here though, to avoid
ambiguity and give more clarity.

>> > +			uint32_t word =
>> > +				drm_color_lut_extract(lut[i].red, 16);
>> > +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), word);
>> > +		}
>>
>> Following the loop is where we'd need to deal with the two extended
>> range entries, right?  So if we leave dealing with extended range
>> properly as a TODO for now, then I think we want to fill them in with
>> linear values.  E.g.,
>>
>>      I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x30000);
>>      I915_WRITE(PRE_CSC_GAMC_DATA(pipe), 0x70000);
>
>Actually, it looks like you actually do set these at the bottom of the function (and
>clamp to 1.0), so that's fine and you can disregard this comment.

Yes, the 34th and 35the entry is programmed to clamp at 1.0.

>
>Matt
>
>>
>> Also, I believe the 1.0 input can have a value >= 1.0, so we need to
>> extract more bits of precision for that.
>>
>> I'm not really an expert on how this color management stuff gets used
>> by userspace in practice, but I suspect our current ABI probably needs
>> to be augmented to deal more complicated gamma LUT's than we support
>today.
>> E.g., different ranges with different input values, precision, etc.
>> Do we have a clear understanding of if/how userspace wants to deal
>> with extended range?

I believe current userspace just goes for 1.0. Yes, we can extend and add and
extended mode as well to deal with these 7.0 ranges. I will try to create some
kind of design and send a RFC for input to handle multi segment as well as such
extended range inputs. Will try to catch Ville as well for his inputs :)

>> > +	} else {
>> > +		/* load a linear table. */
>> > +		for (i = 0; i < lut_size; i++) {
>> > +			uint32_t v = (i * (1 << 16)) / (lut_size - 1);
>> > +
>> > +			I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
>> > +		}
>>
>> As above, I think we should still fill in the last two entries with
>> linear values for now.

This is ok and taken care as mentioned above.

>> > +	}
>> > +
>> > +	tmp = I915_READ(GAMMA_MODE(pipe));
>> > +	I915_WRITE(GAMMA_MODE(pipe), tmp | PRE_CSC_GAMMA_ENABLE);
>> > +
>> > +	/* Clamp values > 1.0. */
>> > +	while (i++ < 35)
>> > +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16)); }
>> > +
>> >  static void glk_load_luts(struct drm_crtc_state *state)  {
>> >  	struct drm_crtc *crtc = state->crtc; @@ -606,6 +656,28 @@ static
>> > void cherryview_load_luts(struct drm_crtc_state *state)
>> >  	i9xx_load_luts_internal(crtc, NULL, to_intel_crtc_state(state));
>> > }
>> >
>> > +static void icl_load_luts(struct drm_crtc_state *state) {
>> > +	struct drm_crtc *crtc = state->crtc;
>> > +	struct drm_device *dev = crtc->dev;
>> > +	struct drm_i915_private *dev_priv = to_i915(dev);
>> > +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>> > +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
>> > +
>> > +	icl_load_degamma_lut(state);
>> > +
>> > +	if (crtc_state_is_legacy_gamma(state)) {
>> > +		haswell_load_luts(state);
>> > +		return;
>> > +	}
>>
>> This block will only run if there's no degamma LUT, so it might be
>> slightly easier to read the code if we move this above the
>> icl_load_degamma_lut() call above?

Sure, I will do that.

>> > +
>> > +	bdw_load_gamma_lut(state, 0);
>> > +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
>>
>> I think we also need to update PAL_EXT_GC_MAX with a FP3.16 for the
>> 3.0 input value and the PAL_EXT2_GC_MAX for the 7.0 input.  Assuming
>> we want to leave the ABI details of this until later, I assume we'd
>> handle this with linear values like I suggested for the degamma
>> extended range entries?

As mentioned above, will try to create some RFC for review to handle extended ranges.

Thanks Matt for your review and valuable comments.

Regards,
Uma Shankar
>>
>> Matt
>>
>> > +
>> > +	I915_WRITE(GAMMA_MODE(pipe), I915_READ(GAMMA_MODE(pipe)) |
>> > +			intel_state->gamma_mode);
>> > +}
>> > +
>> >  void intel_color_load_luts(struct drm_crtc_state *crtc_state)  {
>> >  	struct drm_device *dev = crtc_state->crtc->dev; @@ -662,6 +734,8
>> > @@ void intel_color_init(struct drm_crtc *crtc)
>> >  	} else if (IS_GEMINILAKE(dev_priv) || IS_CANNONLAKE(dev_priv)) {
>> >  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>> >  		dev_priv->display.load_luts = glk_load_luts;
>> > +	} else if (IS_ICELAKE(dev_priv)) {
>> > +		dev_priv->display.load_luts = icl_load_luts;
>> >  	} else {
>> >  		dev_priv->display.load_luts = i9xx_load_luts;
>> >  	}
>> > --
>> > 1.9.1
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>> --
>> Matt Roper
>> Graphics Software Engineer
>> IoTG Platform Enabling & Development
>> Intel Corporation
>> (916) 356-2795
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/icl: Enable ICL Pipe CSC block

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Thursday, November 1, 2018 5:10 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>;
>Lankhorst, Maarten <maarten.lankhorst@intel.com>
>Subject: Re: [Intel-gfx] [PATCH 2/3] drm/i915/icl: Enable ICL Pipe CSC block
>
>On Wed, Oct 24, 2018 at 08:30:12PM +0530, Uma Shankar wrote:
>> Enable ICL pipe csc hardware. CSC block is enabled in CSC_MODE
>> register instead of PLANE_COLOR_CTL.
>>
>> v2: Addressed Maarten's review comments.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    |  1 +
>>  drivers/gpu/drm/i915/intel_color.c | 14 ++++++++++++--
>>  2 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 3adf689..857ce79 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9700,6 +9700,7 @@ enum skl_power_gate {
>>  #define _PIPE_A_CSC_COEFF_RV_GV	0x49020
>>  #define _PIPE_A_CSC_COEFF_BV	0x49024
>>  #define _PIPE_A_CSC_MODE	0x49028
>> +#define   CSC_ENABLE			(1 << 31)
>>  #define   CSC_BLACK_SCREEN_OFFSET	(1 << 2)
>>  #define   CSC_POSITION_BEFORE_GAMMA	(1 << 1)
>>  #define   CSC_MODE_YUV_TO_RGB		(1 << 0)
>> diff --git a/drivers/gpu/drm/i915/intel_color.c
>> b/drivers/gpu/drm/i915/intel_color.c
>> index 12c659f..e7042fd 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -129,7 +129,12 @@ static void ilk_load_ycbcr_conversion_matrix(struct
>intel_crtc *intel_crtc)
>>  	I915_WRITE(PIPE_CSC_POSTOFF_HI(pipe), POSTOFF_RGB_TO_YUV_HI);
>>  	I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe),
>POSTOFF_RGB_TO_YUV_ME);
>>  	I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), POSTOFF_RGB_TO_YUV_LO);
>> -	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>> +
>> +	if (INTEL_GEN(dev_priv) >= 11)
>> +		I915_WRITE(PIPE_CSC_MODE(pipe),
>> +			I915_READ(PIPE_CSC_MODE(pipe)) | CSC_ENABLE);
>
>Is this the right approach for gen11?  The bspec details are pretty sparse, but it
>looks like with gen11+, we now have two CSC's so we can do both a userspace-
>provided CSC matrix (bit 31 in CSC_MODE) and a separate RGB->YUV matrix (bit
>30 in CSC_MODE and the OUTPUT_CSC_*
>registers) rather than having to pick just one or the other.

Yes, actually for RGB->YUV output CSC should be used. So this should set but 30 instead
of normal CSC. As it allows final conversion on non-linear data coming out of pipe after
blending and gamma operation. Will correct this. Thanks !!!

>Also, as noted on the other patch, we generally don't want to do read-modify-
>write patterns in the driver.

Yes, will change this.

Regards,
Uma Shankar

>
>
>Matt
>
>> +	else
>> +		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>>  }
>>
>>  static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state) @@
>> -239,7 +244,11 @@ static void ilk_load_csc_matrix(struct drm_crtc_state
>*crtc_state)
>>  		I915_WRITE(PIPE_CSC_POSTOFF_ME(pipe), postoff);
>>  		I915_WRITE(PIPE_CSC_POSTOFF_LO(pipe), postoff);
>>
>> -		I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>> +		if (INTEL_GEN(dev_priv) >= 11)
>> +			I915_WRITE(PIPE_CSC_MODE(pipe),
>> +				I915_READ(PIPE_CSC_MODE(pipe)) |
>CSC_ENABLE);
>> +		else
>> +			I915_WRITE(PIPE_CSC_MODE(pipe), 0);
>>  	} else {
>>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>>
>> @@ -735,6 +744,7 @@ void intel_color_init(struct drm_crtc *crtc)
>>  		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>>  		dev_priv->display.load_luts = glk_load_luts;
>>  	} else if (IS_ICELAKE(dev_priv)) {
>> +		dev_priv->display.load_csc_matrix = ilk_load_csc_matrix;
>>  		dev_priv->display.load_luts = icl_load_luts;
>>  	} else {
>>  		dev_priv->display.load_luts = i9xx_load_luts;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>--
>Matt Roper
>Graphics Software Engineer
>IoTG Platform Enabling & Development
>Intel Corporation
>(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx