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

[v3 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.

v3: Addressed Matt's review comments. Removed rmw patterns
as suggested by Matt.

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    |   5 ++
 drivers/gpu/drm/i915/intel_color.c | 101 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h   |   3 ++
 4 files changed, 106 insertions(+), 6 deletions(-)

-- 
1.9.1

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

[v3 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.

v3: Addressed Matt's review comments. Removed rmw patterns
as suggested by Matt.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 47baf2fe..b0147bf 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7058,6 +7058,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..7c8c996 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -422,6 +422,7 @@ static void bdw_load_degamma_lut(struct drm_crtc_state *state)
 static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
 {
 	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
+	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
 	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
 	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
 
@@ -464,6 +465,9 @@ 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 (INTEL_GEN(dev_priv) >= 11)
+		intel_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
 }
 
 /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
@@ -523,6 +527,53 @@ 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);
+	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
+	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
+	const uint32_t lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
+	uint32_t 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++) {
+			/*
+			 * First 33 entries represent range from 0 to 1.0
+			 * 34th and 35th entry will represent extended range
+			 * inputs 3.0 and 7.0 respectively, currently clamped
+			 * at 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);
+		}
+	}
+
+	intel_state->gamma_mode |= 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 +657,26 @@ 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;
+
+	if (crtc_state_is_legacy_gamma(state)) {
+		haswell_load_luts(state);
+		return;
+	}
+
+	icl_load_degamma_lut(state);
+	bdw_load_gamma_lut(state, 0);
+
+	intel_state->gamma_mode |= GAMMA_MODE_MODE_10BIT;
+	I915_WRITE(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 +733,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

[v3 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.

v3: Addressed Matt's review comments. Removed rmw patterns
as suggested by Matt.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b0147bf..6470008 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9824,6 +9824,8 @@ 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   OUTPUT_CSC_ENABLE		(1 << 30)
 #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 7c8c996..414e1ca 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -108,10 +108,14 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
 	return result;
 }
 
-static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
+static void ilk_load_ycbcr_conversion_matrix(struct drm_crtc_state *crtc_state)
 {
+	struct drm_crtc *crtc = crtc_state->crtc;
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
-	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
+	struct intel_crtc_state *intel_crtc_state =
+					to_intel_crtc_state(crtc_state);
 
 	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
 	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
@@ -129,7 +133,11 @@ 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)
+		intel_crtc_state->csc_mode |=  CSC_ENABLE;
+	else
+		intel_crtc_state->csc_mode = 0;
 }
 
 static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
@@ -151,7 +159,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
 
 	if (intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
 	    intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
-		ilk_load_ycbcr_conversion_matrix(intel_crtc);
+		ilk_load_ycbcr_conversion_matrix(crtc_state);
 		return;
 	} else if (crtc_state->ctm) {
 		struct drm_color_ctm *ctm = crtc_state->ctm->data;
@@ -239,7 +247,10 @@ 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)
+			intel_crtc_state->csc_mode |=  CSC_ENABLE;
+		else
+			intel_crtc_state->csc_mode = 0;
 	} else {
 		uint32_t mode = CSC_MODE_YUV_TO_RGB;
 
@@ -305,9 +316,15 @@ void intel_color_set_csc(struct drm_crtc_state *crtc_state)
 {
 	struct drm_device *dev = crtc_state->crtc->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->crtc);
+	struct intel_crtc_state *intel_crtc_state =
+				to_intel_crtc_state(crtc_state);
+	enum pipe pipe = intel_crtc->pipe;
 
 	if (dev_priv->display.load_csc_matrix)
 		dev_priv->display.load_csc_matrix(crtc_state);
+
+	I915_WRITE(PIPE_CSC_MODE(pipe), intel_crtc_state->csc_mode);
 }
 
 /* Loads the legacy palette/gamma unit for the CRTC. */
@@ -734,6 +751,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;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a62d77b..e7b77c1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -922,6 +922,9 @@ struct intel_crtc_state {
 	/* Gamma mode programmed on the pipe */
 	uint32_t gamma_mode;
 
+	/* CSC mode programmed on the pipe */
+	uint32_t csc_mode;
+
 	/* bitmask of visible planes (enum plane_id) */
 	u8 active_planes;
 	u8 nv12_planes;
-- 
1.9.1

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

[v3 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.

v3: Rebase

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 1b81d7c..94f455d 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -624,7 +624,8 @@
 	}, \
 	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 (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

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

total: 0 errors, 0 warnings, 1 checks, 112 lines checked
5a32a67a4649 drm/i915/icl: Enable ICL Pipe CSC block
653e346a63d4 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: success for Add support for Gen 11 pipe color features (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5222 -> Patchwork_10957
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  * {igt@runner@aborted}:
    - {fi-icl-y}:         NOTRUN -> FAIL [fdo#108070]

  
#### Possible fixes ####

  * igt@gem_mmap_gtt@basic:
    - fi-glk-dsi:         INCOMPLETE [fdo#103359] / [k.org#198133] -> PASS

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

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#108070]: https://bugs.freedesktop.org/show_bug.cgi?id=108070
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (49 -> 44)
------------------------------

  Additional (1): fi-icl-y 
  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


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

    * Linux: CI_DRM_5222 -> Patchwork_10957

  CI_DRM_5222: a6f85043a0ca86eb0072e69bf92b77f7d9d6d5d5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4735: b05c028ccdb6ac8e8d8499a041bb14dfe358ee26 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10957: 653e346a63d4c9f5d2d2e559ed0b73ea3cce7029 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

653e346a63d4 drm/i915/icl: Add degamma and gamma lut size to gen11 caps
5a32a67a4649 drm/i915/icl: Enable ICL Pipe CSC block
1427600ab4e5 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_10957/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Matt Roper]

On Thu, Nov 29, 2018 at 08:21:41PM +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.
> 
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>  drivers/gpu/drm/i915/intel_color.c | 73 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 47baf2fe..b0147bf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7058,6 +7058,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..7c8c996 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -422,6 +422,7 @@ static void bdw_load_degamma_lut(struct drm_crtc_state *state)
>  static void bdw_load_gamma_lut(struct drm_crtc_state *state, u32 offset)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
> +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>  
> @@ -464,6 +465,9 @@ 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 (INTEL_GEN(dev_priv) >= 11)
> +		intel_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;

icl_load_luts updates this field as soon as this function returns, so it
might be easier to make this update there, at the same point you set
MODE_10BIT.

However the overall use and handling of gamma_mode by the driver seems
strange.  Generally we try to calculate important state variables during
atomic check and then use those derived state values during the commit
programming phase, but we're not following that pattern with this field.
Can we just move the logic to build this field into the atomic check phase
so that we're not trying to update state during the commit?

Actually, looking closer, I'm not sure if we even need this field.  For
most platforms we have a fixed set of bits that we write (e.g.,
PRE_CSC_GAMMA_ENABLE | POST_CSC_GAMMA_ENABLE | GAMMA_MODE_10_BIT for
ICL), so we can just put those directly into the I915_WRITE statement.
Unless I'm overlooking something, the only place where this variable is
used for something else is in haswell_load_luts() where we have to
disable IPS before accessing the LUT's if we're in split gamma mode.
But the driver itself only ever sets 8BIT for Haswell as far as I can
see.  BIOS might have put us in split mode before the driver loaded, but
that might be easier to sanitize during hardware state readout?


>  }
>  
>  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */
> @@ -523,6 +527,53 @@ 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);
> +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
> +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
> +	const uint32_t lut_size = INTEL_INFO(dev_priv)->color.degamma_lut_size;
> +	uint32_t 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++) {
> +			/*
> +			 * First 33 entries represent range from 0 to 1.0
> +			 * 34th and 35th entry will represent extended range
> +			 * inputs 3.0 and 7.0 respectively, currently clamped
> +			 * at 1.0.
> +			 * ToDo: Extend to max 7.0.
> +			 */
> +			uint32_t word =
> +				drm_color_lut_extract(lut[i].red, 16);

A few minor things I notice; not sure if you want to bother addressing
them at the moment or not, but I figured I'd point them out now:

 * Technically we could just set word directly to lut[i].red since we're
   extracting the full 16-bits (so no rounding) and our u16 input can't
   hold any value outside our clamping range of [0,0.99998].

 * It's a minor deviation, but the scaling is slightly off here such
   that the 33rd entry written by this loop (which represents a 1.0
   input) will be clamped to 0xFFFF (0.99998) rather than 1.0.  So
   userspace won't be able to exactly match the linear table we'd
   otherwise program if no userspace LUT was provided.

 * GLK and gen11 deviate from our other platforms in that they just use
   a single value for red, green, and blue rather than accepting
   separate values for each.  GLK punts on this and doesn't even expose
   degamma to userspace (just loads a linear table), whereas your patch
   silently ignores green/blue values, which might be a surprise to
   userspace.  Maybe we should switch GLK code to do what you do here,
   but also add some validation of new table blobs during atomic-check
   such that we reject any table with non-equal values for r/g/b.  While
   we're at it, we could also check to ensure that LUT entries are never
   decreasing, since that's also a hardware requirement documented in
   the bspec.


Matt

> +			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);
> +		}
> +	}
> +
> +	intel_state->gamma_mode |= 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 +657,26 @@ 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;
> +
> +	if (crtc_state_is_legacy_gamma(state)) {
> +		haswell_load_luts(state);
> +		return;
> +	}
> +
> +	icl_load_degamma_lut(state);
> +	bdw_load_gamma_lut(state, 0);
> +
> +	intel_state->gamma_mode |= GAMMA_MODE_MODE_10BIT;
> +	I915_WRITE(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 +733,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
> 

-- 
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

✓ Fi.CI.IGT: success for Add support for Gen 11 pipe color features (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_5222_full -> Patchwork_10957_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_color@pipe-b-gamma:
    - {shard-iclb}:       SKIP -> FAIL +11

  
#### Warnings ####

  * igt@kms_color@pipe-b-ctm-green-to-red:
    - {shard-iclb}:       SKIP -> PASS +19

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_schedule@pi-ringfull-blt:
    - shard-skl:          NOTRUN -> FAIL [fdo#103158]

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#107956] +2

  * igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
    - shard-apl:          PASS -> DMESG-WARN [fdo#107956]

  * igt@kms_chv_cursor_fail@pipe-b-256x256-right-edge:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#107724] / [fdo#108336] +3

  * igt@kms_cursor_crc@cursor-256x85-random:
    - shard-glk:          PASS -> FAIL [fdo#103232] +1

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-apl:          PASS -> FAIL [fdo#103232]

  * igt@kms_draw_crc@draw-method-rgb565-mmap-cpu-ytiled:
    - {shard-iclb}:       PASS -> WARN [fdo#108336]

  * igt@kms_fbcon_fbt@psr-suspend:
    - shard-skl:          NOTRUN -> FAIL [fdo#107882]

  * igt@kms_flip@flip-vs-fences:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#107724] +14

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-move:
    - shard-apl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-glk:          PASS -> FAIL [fdo#103167] / [fdo#105682]
    - shard-apl:          PASS -> FAIL [fdo#103167] / [fdo#105682]

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-cur-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167] +3

  * igt@kms_frontbuffer_tracking@fbc-farfromfence:
    - shard-skl:          PASS -> FAIL [fdo#103167] +2

  * igt@kms_frontbuffer_tracking@fbc-shrfb-scaledprimary:
    - shard-skl:          PASS -> FAIL [fdo#105682]

  * igt@kms_frontbuffer_tracking@fbcpsr-farfromfence:
    - {shard-iclb}:       PASS -> DMESG-FAIL [fdo#107724] +1

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-fullscreen:
    - {shard-iclb}:       PASS -> FAIL [fdo#103167] +4

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#107713]

  * igt@kms_plane@pixel-format-pipe-c-planes:
    - shard-skl:          NOTRUN -> DMESG-WARN [fdo#106885]

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-skl:          PASS -> INCOMPLETE [fdo#104108] / [fdo#107773]

  * igt@kms_plane@plane-position-covered-pipe-a-planes:
    - {shard-iclb}:       PASS -> FAIL [fdo#103166]

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-max:
    - shard-skl:          NOTRUN -> FAIL [fdo#108145] +1

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +2

  * igt@kms_setmode@basic:
    - shard-skl:          NOTRUN -> FAIL [fdo#99912]

  * igt@pm_backlight@fade_with_suspend:
    - shard-skl:          NOTRUN -> FAIL [fdo#107847]

  * igt@pm_rpm@modeset-lpsp-stress-no-wait:
    - shard-skl:          PASS -> INCOMPLETE [fdo#107807]

  * igt@pm_rpm@system-suspend-execbuf:
    - {shard-iclb}:       PASS -> INCOMPLETE [fdo#107713] / [fdo#108840]

  * igt@pm_rpm@universal-planes:
    - {shard-iclb}:       PASS -> DMESG-WARN [fdo#108654] / [fdo#108756]

  * {igt@runner@aborted}:
    - {shard-iclb}:       NOTRUN -> FAIL [fdo#108756]

  * igt@syncobj_wait@multi-wait-for-submit-unsubmitted-signaled:
    - shard-snb:          PASS -> INCOMPLETE [fdo#105411]

  
#### Possible fixes ####

  * igt@drm_import_export@import-close-race-flink:
    - shard-skl:          TIMEOUT [fdo#108667] -> PASS

  * igt@kms_atomic_transition@plane-all-modeset-transition-internal-panels:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] -> PASS +13

  * igt@kms_color@pipe-b-degamma:
    - shard-apl:          FAIL [fdo#104782] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-random:
    - shard-apl:          FAIL [fdo#103232] -> PASS +2

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-apl:          FAIL [fdo#103191] / [fdo#103232] -> PASS +1

  * igt@kms_cursor_crc@cursor-128x42-random:
    - shard-glk:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-offscreen:
    - shard-skl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-256x256-random:
    - {shard-iclb}:       FAIL [fdo#103232] -> PASS +18

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
    - shard-glk:          FAIL [fdo#105363] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-indfb-draw-mmap-gtt:
    - {shard-iclb}:       DMESG-FAIL [fdo#107724] -> PASS +3

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
    - shard-apl:          FAIL [fdo#103167] -> PASS +2

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
    - {shard-iclb}:       FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
    - shard-glk:          FAIL [fdo#103167] -> PASS +3

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-pri-shrfb-draw-render:
    - {shard-iclb}:       DMESG-WARN [fdo#107724] / [fdo#108336] -> PASS +4

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - {shard-iclb}:       DMESG-FAIL [fdo#103166] / [fdo#107724] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - {shard-iclb}:       FAIL [fdo#103166] -> PASS +1

  * igt@kms_psr@suspend:
    - {shard-iclb}:       INCOMPLETE [fdo#107713] -> PASS

  * igt@pm_rpm@legacy-planes:
    - shard-skl:          INCOMPLETE [fdo#105959] / [fdo#107807] -> PASS

  
#### Warnings ####

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - {shard-iclb}:       FAIL [fdo#107725] -> DMESG-WARN [fdo#107724] / [fdo#108336]

  * igt@kms_cursor_crc@cursor-128x42-sliding:
    - {shard-iclb}:       FAIL [fdo#103232] -> DMESG-WARN [fdo#107724] / [fdo#108336]

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-spr-indfb-fullscreen:
    - {shard-iclb}:       FAIL [fdo#103167] -> DMESG-FAIL [fdo#107724]

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

  [fdo#103158]: https://bugs.freedesktop.org/show_bug.cgi?id=103158
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [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#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#105959]: https://bugs.freedesktop.org/show_bug.cgi?id=105959
  [fdo#106885]: https://bugs.freedesktop.org/show_bug.cgi?id=106885
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107773]: https://bugs.freedesktop.org/show_bug.cgi?id=107773
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107847]: https://bugs.freedesktop.org/show_bug.cgi?id=107847
  [fdo#107882]: https://bugs.freedesktop.org/show_bug.cgi?id=107882
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108336]: https://bugs.freedesktop.org/show_bug.cgi?id=108336
  [fdo#108654]: https://bugs.freedesktop.org/show_bug.cgi?id=108654
  [fdo#108667]: https://bugs.freedesktop.org/show_bug.cgi?id=108667
  [fdo#108756]: https://bugs.freedesktop.org/show_bug.cgi?id=108756
  [fdo#108840]: https://bugs.freedesktop.org/show_bug.cgi?id=108840
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (7 -> 7)
------------------------------

  No changes in participating hosts


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

    * Linux: CI_DRM_5222 -> Patchwork_10957

  CI_DRM_5222: a6f85043a0ca86eb0072e69bf92b77f7d9d6d5d5 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4735: b05c028ccdb6ac8e8d8499a041bb14dfe358ee26 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10957: 653e346a63d4c9f5d2d2e559ed0b73ea3cce7029 @ 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_10957/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shankar, Uma]

>-----Original Message-----
>From: Roper, Matthew D
>Sent: Friday, November 30, 2018 4:38 AM
>To: Shankar, Uma <uma.shankar@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Lankhorst, Maarten
><maarten.lankhorst@intel.com>; Syrjala, Ville <ville.syrjala@intel.com>; Sharma,
>Shashank <shashank.sharma@intel.com>
>Subject: Re: [v3 1/3] drm/i915/icl: Add icl pipe degamma and gamma support
>
>On Thu, Nov 29, 2018 at 08:21:41PM +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.
>>
>> v3: Addressed Matt's review comments. Removed rmw patterns as
>> suggested by Matt.
>>
>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h    |  3 ++
>>  drivers/gpu/drm/i915/intel_color.c | 73
>> ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 76 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index 47baf2fe..b0147bf 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7058,6 +7058,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..7c8c996 100644
>> --- a/drivers/gpu/drm/i915/intel_color.c
>> +++ b/drivers/gpu/drm/i915/intel_color.c
>> @@ -422,6 +422,7 @@ static void bdw_load_degamma_lut(struct
>> drm_crtc_state *state)  static void bdw_load_gamma_lut(struct
>> drm_crtc_state *state, u32 offset)  {
>>  	struct drm_i915_private *dev_priv = to_i915(state->crtc->dev);
>> +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>>  	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>>  	uint32_t i, lut_size = INTEL_INFO(dev_priv)->color.gamma_lut_size;
>>
>> @@ -464,6 +465,9 @@ 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 (INTEL_GEN(dev_priv) >= 11)
>> +		intel_state->gamma_mode |= POST_CSC_GAMMA_ENABLE;
>
>icl_load_luts updates this field as soon as this function returns, so it might be
>easier to make this update there, at the same point you set MODE_10BIT.
>
>However the overall use and handling of gamma_mode by the driver seems
>strange.  Generally we try to calculate important state variables during atomic
>check and then use those derived state values during the commit programming
>phase, but we're not following that pattern with this field.
>Can we just move the logic to build this field into the atomic check phase so that
>we're not trying to update state during the commit?
>
>Actually, looking closer, I'm not sure if we even need this field.  For most
>platforms we have a fixed set of bits that we write (e.g.,
>PRE_CSC_GAMMA_ENABLE | POST_CSC_GAMMA_ENABLE |
>GAMMA_MODE_10_BIT for ICL), so we can just put those directly into the
>I915_WRITE statement.
>Unless I'm overlooking something, the only place where this variable is used for
>something else is in haswell_load_luts() where we have to disable IPS before
>accessing the LUT's if we're in split gamma mode.
>But the driver itself only ever sets 8BIT for Haswell as far as I can see.  BIOS might
>have put us in split mode before the driver loaded, but that might be easier to
>sanitize during hardware state readout?

Yeah I agree, we can avoid this state variable altogether. I will try to clean it up
and resend the series.

>
>>  }
>>
>>  /* Loads the palette/gamma unit for the CRTC on Broadwell+. */ @@
>> -523,6 +527,53 @@ 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);
>> +	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>> +	enum pipe pipe = to_intel_crtc(state->crtc)->pipe;
>> +	const uint32_t lut_size = INTEL_INFO(dev_priv)-
>>color.degamma_lut_size;
>> +	uint32_t 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++) {
>> +			/*
>> +			 * First 33 entries represent range from 0 to 1.0
>> +			 * 34th and 35th entry will represent extended range
>> +			 * inputs 3.0 and 7.0 respectively, currently clamped
>> +			 * at 1.0.
>> +			 * ToDo: Extend to max 7.0.
>> +			 */
>> +			uint32_t word =
>> +				drm_color_lut_extract(lut[i].red, 16);
>
>A few minor things I notice; not sure if you want to bother addressing them at the
>moment or not, but I figured I'd point them out now:
>
> * Technically we could just set word directly to lut[i].red since we're
>   extracting the full 16-bits (so no rounding) and our u16 input can't
>   hold any value outside our clamping range of [0,0.99998].

Yes, it's practically NOOP here. Will update this.

> * It's a minor deviation, but the scaling is slightly off here such
>   that the 33rd entry written by this loop (which represents a 1.0
>   input) will be clamped to 0xFFFF (0.99998) rather than 1.0.  So
>   userspace won't be able to exactly match the linear table we'd
>   otherwise program if no userspace LUT was provided.

Yes, with the current data structure definition of lut (16bit entries), its
not possible for userspace to send exact 1.0 (which will require 17bits).
We need to use extended structure in order to get over this corner case
and also to handle extended range 3.0 and 7.0 etc. I tried to introduce that
as part of "drm: Add Enhanced Gamma LUT precision structure" patch in series
https://patchwork.freedesktop.org/series/30875/

We need to use above extension to get over this corner case. 

> * GLK and gen11 deviate from our other platforms in that they just use
>   a single value for red, green, and blue rather than accepting
>   separate values for each.  GLK punts on this and doesn't even expose
>   degamma to userspace (just loads a linear table), whereas your patch
>   silently ignores green/blue values, which might be a surprise to
>   userspace.  Maybe we should switch GLK code to do what you do here,
>   but also add some validation of new table blobs during atomic-check
>   such that we reject any table with non-equal values for r/g/b.  While
>   we're at it, we could also check to ensure that LUT entries are never
>   decreasing, since that's also a hardware requirement documented in
>   the bspec.

Ok sure, I will add that check to make sure userspace requests for non-equal
R, G and B components is not allowed and user is aware that hardware expects
equal LUT values for each component. 

Thanks Matt for your useful comments and inputs.

Regards,
Uma Shankar

>
>Matt
>
>> +			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);
>> +		}
>> +	}
>> +
>> +	intel_state->gamma_mode |= 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 +657,26 @@ 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;
>> +
>> +	if (crtc_state_is_legacy_gamma(state)) {
>> +		haswell_load_luts(state);
>> +		return;
>> +	}
>> +
>> +	icl_load_degamma_lut(state);
>> +	bdw_load_gamma_lut(state, 0);
>> +
>> +	intel_state->gamma_mode |= GAMMA_MODE_MODE_10BIT;
>> +	I915_WRITE(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 +733,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
>>
>
>--
>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: [v3 2/3] drm/i915/icl: Enable ICL Pipe CSC block

[Matt Roper]

On Thu, Nov 29, 2018 at 08:21:42PM +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.
> 
> v3: Addressed Matt's review comments. Removed rmw patterns
> as suggested by Matt.
> 
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h    |  2 ++
>  drivers/gpu/drm/i915/intel_color.c | 28 +++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h   |  3 +++
>  3 files changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b0147bf..6470008 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9824,6 +9824,8 @@ 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   OUTPUT_CSC_ENABLE		(1 << 30)
>  #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 7c8c996..414e1ca 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -108,10 +108,14 @@ static u64 *ctm_mult_by_limited(u64 *result, const u64 *input)
>  	return result;
>  }
>  
> -static void ilk_load_ycbcr_conversion_matrix(struct intel_crtc *intel_crtc)
> +static void ilk_load_ycbcr_conversion_matrix(struct drm_crtc_state *crtc_state)

It's generally preferred to use Intel types for these lower-level
functions rather than DRM types.  It looks like a lot of the preexisting
color management code breaks from this convention, so we might want to
also clean that up in the preexisting code as a separate patch.


>  {
> +	struct drm_crtc *crtc = crtc_state->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
> +	struct intel_crtc_state *intel_crtc_state =
> +					to_intel_crtc_state(crtc_state);
>  
>  	I915_WRITE(PIPE_CSC_PREOFF_HI(pipe), 0);
>  	I915_WRITE(PIPE_CSC_PREOFF_ME(pipe), 0);
> @@ -129,7 +133,11 @@ 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)
> +		intel_crtc_state->csc_mode |=  CSC_ENABLE;

Shouldn't fixed RGB->YUV be done with OUTPUT_CSC_ENABLE now that we have
two CSC's to work with instead of just one?  I.e., I think we can leave
the first pipe CSC for the userspace-defined transformation (or identity
if none is provided) and then enable the output CSC if we need to
convert to YUV and/or compress to limited range.


Matt

> +	else
> +		intel_crtc_state->csc_mode = 0;
>  }
>  
>  static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
> @@ -151,7 +159,7 @@ static void ilk_load_csc_matrix(struct drm_crtc_state *crtc_state)
>  
>  	if (intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
>  	    intel_crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
> -		ilk_load_ycbcr_conversion_matrix(intel_crtc);
> +		ilk_load_ycbcr_conversion_matrix(crtc_state);
>  		return;
>  	} else if (crtc_state->ctm) {
>  		struct drm_color_ctm *ctm = crtc_state->ctm->data;
> @@ -239,7 +247,10 @@ 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)
> +			intel_crtc_state->csc_mode |=  CSC_ENABLE;
> +		else
> +			intel_crtc_state->csc_mode = 0;
>  	} else {
>  		uint32_t mode = CSC_MODE_YUV_TO_RGB;
>  
> @@ -305,9 +316,15 @@ void intel_color_set_csc(struct drm_crtc_state *crtc_state)
>  {
>  	struct drm_device *dev = crtc_state->crtc->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->crtc);
> +	struct intel_crtc_state *intel_crtc_state =
> +				to_intel_crtc_state(crtc_state);
> +	enum pipe pipe = intel_crtc->pipe;
>  
>  	if (dev_priv->display.load_csc_matrix)
>  		dev_priv->display.load_csc_matrix(crtc_state);
> +
> +	I915_WRITE(PIPE_CSC_MODE(pipe), intel_crtc_state->csc_mode);
>  }
>  
>  /* Loads the legacy palette/gamma unit for the CRTC. */
> @@ -734,6 +751,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;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a62d77b..e7b77c1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -922,6 +922,9 @@ struct intel_crtc_state {
>  	/* Gamma mode programmed on the pipe */
>  	uint32_t gamma_mode;
>  
> +	/* CSC mode programmed on the pipe */
> +	uint32_t csc_mode;
> +
>  	/* bitmask of visible planes (enum plane_id) */
>  	u8 active_planes;
>  	u8 nv12_planes;
> -- 
> 1.9.1
> 

-- 
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: [v3 3/3] drm/i915/icl: Add degamma and gamma lut size to gen11 caps

[Matt Roper]

On Thu, Nov 29, 2018 at 08:21:43PM +0530, Uma Shankar wrote:
> Add the degamma and gamma lut sizes to gen11 capability
> structure.

I'd add a comment to the commit message indicating that this doesn't
account for the extended range gamma entries and that we'll wait to
address those (probably with some kind of new segmented gamma ABI) in a
future patch.


Matt

> 
> v2: Reorder the patch as per Maarten's suggestion.
> 
> v3: Rebase
> 
> 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 1b81d7c..94f455d 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -624,7 +624,8 @@
>  	}, \
>  	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
> 

-- 
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