[PATCH 1/4] drm/i915: Disable plane gamma in SKL+ sprite planes

[PATCH 1/4] drm/i915: Disable plane gamma in SKL+ sprite planes

[Ander Conselvan de Oliveira]

The plane gamma tables are never programmed, so just disable it, like it
is done for the primary plane.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9ef5468..c05545f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -226,6 +226,7 @@ skl_update_plane(struct drm_plane *drm_plane,
 	plane_ctl |= skl_plane_ctl_format(fb->format->format);
 	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
 
+	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
 	if (key->flags) {
-- 
2.5.5

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

[PATCH 2/4] drm/i915/glk: Plane color correction register changes

[Ander Conselvan de Oliveira]

From: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>

In Geminilake, the bits for enabling pipe csc, pipe gamma and plane
gamma moved to a new register. So update the plane update functions
to set the right bits.

Pipe CSC is kept disabled though, since enabling that also enables the
dedicated degamma table, and that is not properly programmed yet,
leading to a black screen.

Signed-off-by: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 19 ++++++++++++++++++-
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++++++++++++-----
 3 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8861683..7c240e4 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5874,6 +5874,13 @@ enum {
 #define _PLANE_NV12_BUF_CFG_1_A		0x70278
 #define _PLANE_NV12_BUF_CFG_2_A		0x70378
 
+#define _PLANE_COLOR_CTL_1_A			0x701CC
+#define _PLANE_COLOR_CTL_2_A			0x702CC
+#define _PLANE_COLOR_CTL_3_A			0x703CC
+#define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30)
+#define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23)
+#define   PLANE_COLOR_PLANE_GAMMA_DISABLE	(1 << 13)
+
 #define _PLANE_CTL_1_B				0x71180
 #define _PLANE_CTL_2_B				0x71280
 #define _PLANE_CTL_3_B				0x71380
@@ -5968,7 +5975,17 @@ enum {
 #define PLANE_NV12_BUF_CFG(pipe, plane)	\
 	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
 
-/* SKL new cursor registers */
+#define _PLANE_COLOR_CTL_1_B			0x711CC
+#define _PLANE_COLOR_CTL_2_B			0x712CC
+#define _PLANE_COLOR_CTL_3_B			0x713CC
+#define _PLANE_COLOR_CTL_1(pipe)	\
+	_PIPE(pipe, _PLANE_COLOR_CTL_1_A, _PLANE_COLOR_CTL_1_B)
+#define _PLANE_COLOR_CTL_2(pipe)	\
+	_PIPE(pipe, _PLANE_COLOR_CTL_2_A, _PLANE_COLOR_CTL_2_B)
+#define PLANE_COLOR_CTL(pipe, plane)	\
+	_MMIO_PLANE(plane, _PLANE_COLOR_CTL_1(pipe), _PLANE_COLOR_CTL_2(pipe))
+
+#/* SKL new cursor registers */
 #define _CUR_BUF_CFG_A				0x7017c
 #define _CUR_BUF_CFG_B				0x7117c
 #define CUR_BUF_CFG(pipe)	_MMIO_PIPE(pipe, _CUR_BUF_CFG_A, _CUR_BUF_CFG_B)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0bf8e1b..5313c1f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3386,9 +3386,20 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	int dst_w = drm_rect_width(&plane_state->base.dst);
 	int dst_h = drm_rect_height(&plane_state->base.dst);
 
-	plane_ctl = PLANE_CTL_ENABLE |
-		    PLANE_CTL_PIPE_GAMMA_ENABLE |
-		    PLANE_CTL_PIPE_CSC_ENABLE;
+	plane_ctl = PLANE_CTL_ENABLE;
+
+	if (IS_GEMINILAKE(dev_priv)) {
+		u32 plane_color =
+			PLANE_COLOR_PIPE_GAMMA_ENABLE |
+			PLANE_COLOR_PLANE_GAMMA_DISABLE;
+
+		I915_WRITE(PLANE_COLOR_CTL(pipe, 0), plane_color);
+	} else {
+		plane_ctl |=
+			PLANE_CTL_PIPE_GAMMA_ENABLE |
+			PLANE_CTL_PIPE_CSC_ENABLE |
+			PLANE_CTL_PLANE_GAMMA_DISABLE;
+	}
 
 	plane_ctl |= skl_plane_ctl_format(fb->format->format);
 	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c05545f..25f49a0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -219,14 +219,23 @@ skl_update_plane(struct drm_plane *drm_plane,
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
 
-	plane_ctl = PLANE_CTL_ENABLE |
-		PLANE_CTL_PIPE_GAMMA_ENABLE |
-		PLANE_CTL_PIPE_CSC_ENABLE;
+	plane_ctl = PLANE_CTL_ENABLE;
+
+	if (IS_GEMINILAKE(dev_priv)) {
+		u32 plane_color =
+			PLANE_COLOR_PIPE_GAMMA_ENABLE |
+			PLANE_COLOR_PLANE_GAMMA_DISABLE;
+
+		I915_WRITE(PLANE_COLOR_CTL(pipe, plane_id), plane_color);
+	} else {
+		plane_ctl |=
+			PLANE_CTL_PIPE_GAMMA_ENABLE |
+			PLANE_CTL_PIPE_CSC_ENABLE |
+			PLANE_CTL_PLANE_GAMMA_DISABLE;
+	}
 
 	plane_ctl |= skl_plane_ctl_format(fb->format->format);
 	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
-
-	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
 	plane_ctl |= skl_plane_ctl_rotation(rotation);
 
 	if (key->flags) {
-- 
2.5.5

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

[PATCH 3/4] drm/i915/glk: Program pipe gamma and degamma tables

[Ander Conselvan de Oliveira]

The gamma tables in Geminilake were changed. There is no split-gamma
mode. Instead, there is a dedicated degamma table that is enabled
whenever pipe CSC is enabled.

The dedicated gamma table has 16 bit precision but doesn't support
separate channels. Since that doesn't match the per-channel format of
the degamma LUT property, for now only a linear table is loaded and the
property ignored.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/i915_pci.c    |   1 +
 drivers/gpu/drm/i915/i915_reg.h    |  14 +++++
 drivers/gpu/drm/i915/intel_color.c | 105 +++++++++++++++++++++++++++++++++++++
 3 files changed, 120 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
index ecb487b..df2051b 100644
--- a/drivers/gpu/drm/i915/i915_pci.c
+++ b/drivers/gpu/drm/i915/i915_pci.c
@@ -403,6 +403,7 @@ static const struct intel_device_info intel_geminilake_info = {
 	.platform = INTEL_GEMINILAKE,
 	.is_alpha_support = 1,
 	.ddb_size = 1024,
+	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
 };
 
 static const struct intel_device_info intel_kabylake_info = {
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7c240e4..625d8f7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -8180,12 +8180,26 @@ enum {
 #define _PAL_PREC_EXT_GC_MAX_A	0x4A420
 #define _PAL_PREC_EXT_GC_MAX_B	0x4AC20
 #define _PAL_PREC_EXT_GC_MAX_C	0x4B420
+#define _PAL_PREC_EXT2_GC_MAX_A	0x4A430
+#define _PAL_PREC_EXT2_GC_MAX_B	0x4AC30
+#define _PAL_PREC_EXT2_GC_MAX_C	0x4B430
 
 #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_INDEX_A, _PAL_PREC_INDEX_B)
 #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_DATA_A, _PAL_PREC_DATA_B)
 #define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
 #define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
 
+#define _PRE_CSC_GAMC_INDEX_A	0x4A484
+#define _PRE_CSC_GAMC_INDEX_B	0x4AC84
+#define _PRE_CSC_GAMC_INDEX_C	0x4B484
+#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
+#define _PRE_CSC_GAMC_DATA_A	0x4A488
+#define _PRE_CSC_GAMC_DATA_B	0x4AC88
+#define _PRE_CSC_GAMC_DATA_C	0x4B488
+
+#define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
+#define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
+
 /* pipe CSC & degamma/gamma LUTs on CHV */
 #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
 #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
index d81232b..09e0903 100644
--- a/drivers/gpu/drm/i915/intel_color.c
+++ b/drivers/gpu/drm/i915/intel_color.c
@@ -422,6 +422,108 @@ static void broadwell_load_luts(struct drm_crtc_state *state)
 	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
 }
 
+static void glk_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 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);
+
+	/*
+	 *  FIXME: The pipe degamma table in geminilake doesn't support
+	 *  different values per channel, so this just loads a linear table.
+	 */
+	for (i = 0; i < lut_size; i++) {
+		uint32_t v = (i * ((1 << 16) - 1)) / (lut_size - 1);
+
+		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
+	}
+
+	/* Clamp values > 1.0. */
+	while (i++ < 35)
+		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16) - 1);
+
+}
+
+static void glk_load_gamma_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 = INTEL_INFO(dev_priv)->color.gamma_lut_size;
+	uint32_t i;
+
+	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
+
+	if (state->gamma_lut) {
+		struct drm_color_lut *lut =
+			(struct drm_color_lut *) state->gamma_lut->data;
+
+		for (i = 0; i < lut_size; i++) {
+			uint32_t word =
+			(drm_color_lut_extract(lut[i].red, 10) << 20) |
+			(drm_color_lut_extract(lut[i].green, 10) << 10) |
+			drm_color_lut_extract(lut[i].blue, 10);
+
+			I915_WRITE(PREC_PAL_DATA(pipe), word);
+		}
+
+		/* Program the max register to clamp values > 1.0. */
+		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0),
+			   drm_color_lut_extract(lut[i].red, 16));
+		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1),
+			   drm_color_lut_extract(lut[i].green, 16));
+		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2),
+			   drm_color_lut_extract(lut[i].blue, 16));
+	} else {
+		for (i = 0; i < lut_size; i++) {
+			uint32_t v = (i * ((1 << 10) - 1)) / (lut_size - 1);
+
+			I915_WRITE(PREC_PAL_DATA(pipe),
+				   (v << 20) | (v << 10) | v);
+		}
+
+		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16) - 1);
+		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
+		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
+	}
+}
+
+static void glk_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(state)) {
+		haswell_load_luts(state);
+		return;
+	}
+
+	glk_load_degamma_lut(state);
+	glk_load_gamma_lut(state);
+
+	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
+	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
+	POSTING_READ(GAMMA_MODE(pipe));
+
+	/*
+	 * Reset the index, otherwise it prevents the legacy palette to be
+	 * written properly.
+	 */
+	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
+	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
+}
+
 /* Loads the palette/gamma unit for the CRTC on CherryView. */
 static void cherryview_load_luts(struct drm_crtc_state *state)
 {
@@ -540,6 +642,9 @@ void intel_color_init(struct drm_crtc *crtc)
 		   IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)) {
 		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
 		dev_priv->display.load_luts = broadwell_load_luts;
+	} else if (IS_GEMINILAKE(dev_priv)) {
+		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
+		dev_priv->display.load_luts = glk_load_luts;
 	} else {
 		dev_priv->display.load_luts = i9xx_load_luts;
 	}
-- 
2.5.5

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

[PATCH 4/4] drm/i915/glk: Enable pipe CSC

[Ander Conselvan de Oliveira]

Now that the pre-csc degamma table is set up correctly in Geminilake,
pipe CSC can be enabled without causing a black screen.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 1 +
 drivers/gpu/drm/i915/intel_sprite.c  | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5313c1f..8c7f3c7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3391,6 +3391,7 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
 	if (IS_GEMINILAKE(dev_priv)) {
 		u32 plane_color =
 			PLANE_COLOR_PIPE_GAMMA_ENABLE |
+			PLANE_CTL_PIPE_CSC_ENABLE |
 			PLANE_COLOR_PLANE_GAMMA_DISABLE;
 
 		I915_WRITE(PLANE_COLOR_CTL(pipe, 0), plane_color);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 25f49a0..5a7b95c 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -224,6 +224,7 @@ skl_update_plane(struct drm_plane *drm_plane,
 	if (IS_GEMINILAKE(dev_priv)) {
 		u32 plane_color =
 			PLANE_COLOR_PIPE_GAMMA_ENABLE |
+			PLANE_COLOR_PIPE_CSC_ENABLE |
 			PLANE_COLOR_PLANE_GAMMA_DISABLE;
 
 		I915_WRITE(PLANE_COLOR_CTL(pipe, plane_id), plane_color);
-- 
2.5.5

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

✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915: Disable plane gamma in SKL+ sprite planes

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915: Disable plane gamma in SKL+ sprite planes
URL   : https://patchwork.freedesktop.org/series/18487/
State : failure

== Summary ==

Series 18487v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/18487/revisions/1/mbox/

Test gem_exec_store:
        Subgroup basic-blt:
                incomplete -> PASS       (fi-byt-j1900)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (fi-ivb-3770)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> INCOMPLETE (fi-byt-n2820)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:225  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:211  pass:179  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:225  dwarn:1   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:222  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

d0150572848ac878b63afdd3d4fcd9456635024d drm-tip: 2017y-01m-24d-13h-35m-17s UTC integration manifest
a5be7f5 drm/i915/glk: Enable pipe CSC
1880e62 drm/i915/glk: Program pipe gamma and degamma tables
c8eee70 drm/i915/glk: Plane color correction register changes
7d4c114 drm/i915: Disable plane gamma in SKL+ sprite planes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3593/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Disable plane gamma in SKL+ sprite planes

[Ville Syrjälä]

On Tue, Jan 24, 2017 at 03:35:52PM +0200, Ander Conselvan de Oliveira wrote:
> The plane gamma tables are never programmed, so just disable it, like it
> is done for the primary plane.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9ef5468..c05545f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -226,6 +226,7 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
>  
> +	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;

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

Though what I'd like to see is someone eliminating this primary vs.
sprite code duplication entirely. I think (or at lest hope) that it
should be trivial since I added the plane_id thing.

>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
>  	if (key->flags) {
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915/glk: Plane color correction register changes

[Ville Syrjälä]

On Tue, Jan 24, 2017 at 03:35:53PM +0200, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
> 
> In Geminilake, the bits for enabling pipe csc, pipe gamma and plane
> gamma moved to a new register. So update the plane update functions
> to set the right bits.
> 
> Pipe CSC is kept disabled though, since enabling that also enables the
> dedicated degamma table, and that is not properly programmed yet,
> leading to a black screen.
> 
> Signed-off-by: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      | 19 ++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
>  drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++++++++++++-----
>  3 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8861683..7c240e4 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5874,6 +5874,13 @@ enum {
>  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
>  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
>  
> +#define _PLANE_COLOR_CTL_1_A			0x701CC
> +#define _PLANE_COLOR_CTL_2_A			0x702CC
> +#define _PLANE_COLOR_CTL_3_A			0x703CC

I'd prefer that we try to keep the register defines in order based
on the offset.

Have you've tested this stuff on real hw? Bspec seems to have become
rather illegible as of late, so I had to jump hoops to even find the
register offsets for these, and even then it seems to be telling me
that these registers do and do not exist at the same time.

> +#define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30)
> +#define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23)
> +#define   PLANE_COLOR_PLANE_GAMMA_DISABLE	(1 << 13)

Offsets and bits seem to match what I eventually managed to dig up
from the spec.

> +
>  #define _PLANE_CTL_1_B				0x71180
>  #define _PLANE_CTL_2_B				0x71280
>  #define _PLANE_CTL_3_B				0x71380
> @@ -5968,7 +5975,17 @@ enum {
>  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
>  	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
>  
> -/* SKL new cursor registers */
> +#define _PLANE_COLOR_CTL_1_B			0x711CC
> +#define _PLANE_COLOR_CTL_2_B			0x712CC
> +#define _PLANE_COLOR_CTL_3_B			0x713CC
> +#define _PLANE_COLOR_CTL_1(pipe)	\
> +	_PIPE(pipe, _PLANE_COLOR_CTL_1_A, _PLANE_COLOR_CTL_1_B)
> +#define _PLANE_COLOR_CTL_2(pipe)	\
> +	_PIPE(pipe, _PLANE_COLOR_CTL_2_A, _PLANE_COLOR_CTL_2_B)
> +#define PLANE_COLOR_CTL(pipe, plane)	\
> +	_MMIO_PLANE(plane, _PLANE_COLOR_CTL_1(pipe), _PLANE_COLOR_CTL_2(pipe))
> +
> +#/* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
>  #define _CUR_BUF_CFG_B				0x7117c
>  #define CUR_BUF_CFG(pipe)	_MMIO_PIPE(pipe, _CUR_BUF_CFG_A, _CUR_BUF_CFG_B)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0bf8e1b..5313c1f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3386,9 +3386,20 @@ static void skylake_update_primary_plane(struct drm_plane *plane,
>  	int dst_w = drm_rect_width(&plane_state->base.dst);
>  	int dst_h = drm_rect_height(&plane_state->base.dst);
>  
> -	plane_ctl = PLANE_CTL_ENABLE |
> -		    PLANE_CTL_PIPE_GAMMA_ENABLE |
> -		    PLANE_CTL_PIPE_CSC_ENABLE;
> +	plane_ctl = PLANE_CTL_ENABLE;
> +
> +	if (IS_GEMINILAKE(dev_priv)) {
> +		u32 plane_color =
> +			PLANE_COLOR_PIPE_GAMMA_ENABLE |
> +			PLANE_COLOR_PLANE_GAMMA_DISABLE;
> +
> +		I915_WRITE(PLANE_COLOR_CTL(pipe, 0), plane_color);

Use plane_id please. 'plane_color' seems a little pointless.
Or are we expecting to OR bits in conditionally?

> +	} else {
> +		plane_ctl |=
> +			PLANE_CTL_PIPE_GAMMA_ENABLE |
> +			PLANE_CTL_PIPE_CSC_ENABLE |
> +			PLANE_CTL_PLANE_GAMMA_DISABLE;
> +	}
>  
>  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c05545f..25f49a0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -219,14 +219,23 @@ skl_update_plane(struct drm_plane *drm_plane,
>  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
>  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
>  
> -	plane_ctl = PLANE_CTL_ENABLE |
> -		PLANE_CTL_PIPE_GAMMA_ENABLE |
> -		PLANE_CTL_PIPE_CSC_ENABLE;
> +	plane_ctl = PLANE_CTL_ENABLE;
> +
> +	if (IS_GEMINILAKE(dev_priv)) {
> +		u32 plane_color =
> +			PLANE_COLOR_PIPE_GAMMA_ENABLE |
> +			PLANE_COLOR_PLANE_GAMMA_DISABLE;
> +
> +		I915_WRITE(PLANE_COLOR_CTL(pipe, plane_id), plane_color);
> +	} else {
> +		plane_ctl |=
> +			PLANE_CTL_PIPE_GAMMA_ENABLE |
> +			PLANE_CTL_PIPE_CSC_ENABLE |
> +			PLANE_CTL_PLANE_GAMMA_DISABLE;
> +	}
>  
>  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> -
> -	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>  	plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
>  	if (key->flags) {
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/4] drm/i915/glk: Program pipe gamma and degamma tables

[Ville Syrjälä]

On Tue, Jan 24, 2017 at 03:35:54PM +0200, Ander Conselvan de Oliveira wrote:
> The gamma tables in Geminilake were changed. There is no split-gamma
> mode. Instead, there is a dedicated degamma table that is enabled
> whenever pipe CSC is enabled.
> 
> The dedicated gamma table has 16 bit precision but doesn't support
> separate channels. Since that doesn't match the per-channel format of
> the degamma LUT property, for now only a linear table is loaded and the
> property ignored.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_pci.c    |   1 +
>  drivers/gpu/drm/i915/i915_reg.h    |  14 +++++
>  drivers/gpu/drm/i915/intel_color.c | 105 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> index ecb487b..df2051b 100644
> --- a/drivers/gpu/drm/i915/i915_pci.c
> +++ b/drivers/gpu/drm/i915/i915_pci.c
> @@ -403,6 +403,7 @@ static const struct intel_device_info intel_geminilake_info = {
>  	.platform = INTEL_GEMINILAKE,
>  	.is_alpha_support = 1,
>  	.ddb_size = 1024,
> +	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
>  };
>  
>  static const struct intel_device_info intel_kabylake_info = {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7c240e4..625d8f7 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8180,12 +8180,26 @@ enum {
>  #define _PAL_PREC_EXT_GC_MAX_A	0x4A420
>  #define _PAL_PREC_EXT_GC_MAX_B	0x4AC20
>  #define _PAL_PREC_EXT_GC_MAX_C	0x4B420
> +#define _PAL_PREC_EXT2_GC_MAX_A	0x4A430
> +#define _PAL_PREC_EXT2_GC_MAX_B	0x4AC30
> +#define _PAL_PREC_EXT2_GC_MAX_C	0x4B430
>  
>  #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_INDEX_A, _PAL_PREC_INDEX_B)
>  #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe, _PAL_PREC_DATA_A, _PAL_PREC_DATA_B)
>  #define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
>  #define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe, _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
>  
> +#define _PRE_CSC_GAMC_INDEX_A	0x4A484
> +#define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> +#define _PRE_CSC_GAMC_INDEX_C	0x4B484
> +#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> +#define _PRE_CSC_GAMC_DATA_A	0x4A488
> +#define _PRE_CSC_GAMC_DATA_B	0x4AC88
> +#define _PRE_CSC_GAMC_DATA_C	0x4B488
> +
> +#define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe, _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
> +#define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe, _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
> +
>  /* pipe CSC & degamma/gamma LUTs on CHV */
>  #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
>  #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
> diff --git a/drivers/gpu/drm/i915/intel_color.c b/drivers/gpu/drm/i915/intel_color.c
> index d81232b..09e0903 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -422,6 +422,108 @@ static void broadwell_load_luts(struct drm_crtc_state *state)
>  	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
>  }
>  
> +static void glk_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 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);
> +
> +	/*
> +	 *  FIXME: The pipe degamma table in geminilake doesn't support
> +	 *  different values per channel, so this just loads a linear table.
> +	 */
> +	for (i = 0; i < lut_size; i++) {
> +		uint32_t v = (i * ((1 << 16) - 1)) / (lut_size - 1);
> +
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> +	}
> +
> +	/* Clamp values > 1.0. */
> +	while (i++ < 35)
> +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16) - 1);
> +

Useless empty line.

I do still wonder how we ended up with the 0.16 scheme for LUTs. I'm
pretty sure 8.24 or something like that was the plane. Oh well, it is
what it is.

> +}
> +
> +static void glk_load_gamma_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 = INTEL_INFO(dev_priv)->color.gamma_lut_size;
> +	uint32_t i;
> +
> +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> +
> +	if (state->gamma_lut) {
> +		struct drm_color_lut *lut =
> +			(struct drm_color_lut *) state->gamma_lut->data;

const perhaps?

> +
> +		for (i = 0; i < lut_size; i++) {
> +			uint32_t word =
> +			(drm_color_lut_extract(lut[i].red, 10) << 20) |
> +			(drm_color_lut_extract(lut[i].green, 10) << 10) |
> +			drm_color_lut_extract(lut[i].blue, 10);

A bit strange calling it a "word", but that's what we apparently do
elsewhere.

> +
> +			I915_WRITE(PREC_PAL_DATA(pipe), word);
> +		}
> +
> +		/* Program the max register to clamp values > 1.0. */
> +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0),
> +			   drm_color_lut_extract(lut[i].red, 16));
> +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1),
> +			   drm_color_lut_extract(lut[i].green, 16));
> +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2),
> +			   drm_color_lut_extract(lut[i].blue, 16));
> +	} else {
> +		for (i = 0; i < lut_size; i++) {
> +			uint32_t v = (i * ((1 << 10) - 1)) / (lut_size - 1);
> +
> +			I915_WRITE(PREC_PAL_DATA(pipe),
> +				   (v << 20) | (v << 10) | v);
> +		}
> +
> +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16) - 1);
> +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
> +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
> +	}
> +}

Hmm. Am I imagining it, or is that code pretty much identical to the
"broadwell" code?

I think all this LUT code really needs to some heavy handed renaming
to make things not confusing. But that's a separate issue.

> +
> +static void glk_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(state)) {
> +		haswell_load_luts(state);
> +		return;
> +	}
> +
> +	glk_load_degamma_lut(state);
> +	glk_load_gamma_lut(state);
> +
> +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> +	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
> +	POSTING_READ(GAMMA_MODE(pipe));
> +
> +	/*
> +	 * Reset the index, otherwise it prevents the legacy palette to be
> +	 * written properly.
> +	 */
> +	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> +	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> +}
> +
>  /* Loads the palette/gamma unit for the CRTC on CherryView. */
>  static void cherryview_load_luts(struct drm_crtc_state *state)
>  {
> @@ -540,6 +642,9 @@ void intel_color_init(struct drm_crtc *crtc)
>  		   IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)) {
>  		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
>  		dev_priv->display.load_luts = broadwell_load_luts;
> +	} else if (IS_GEMINILAKE(dev_priv)) {
> +		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> +		dev_priv->display.load_luts = glk_load_luts;
>  	} else {
>  		dev_priv->display.load_luts = i9xx_load_luts;
>  	}
> -- 
> 2.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/4] drm/i915/glk: Plane color correction register changes

[Ander Conselvan De Oliveira]

On Tue, 2017-01-24 at 18:16 +0200, Ville Syrjälä wrote:
> On Tue, Jan 24, 2017 at 03:35:53PM +0200, Ander Conselvan de Oliveira wrote:
> > 
> > From: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@intel.com>
> > 
> > In Geminilake, the bits for enabling pipe csc, pipe gamma and plane
> > gamma moved to a new register. So update the plane update functions
> > to set the right bits.
> > 
> > Pipe CSC is kept disabled though, since enabling that also enables the
> > dedicated degamma table, and that is not properly programmed yet,
> > leading to a black screen.
> > 
> > Signed-off-by: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@inte
> > l.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h      | 19 ++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
> >  drivers/gpu/drm/i915/intel_sprite.c  | 19 ++++++++++++++-----
> >  3 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 8861683..7c240e4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -5874,6 +5874,13 @@ enum {
> >  #define _PLANE_NV12_BUF_CFG_1_A		0x70278
> >  #define _PLANE_NV12_BUF_CFG_2_A		0x70378
> >  
> > +#define _PLANE_COLOR_CTL_1_A			0x701CC
> > +#define _PLANE_COLOR_CTL_2_A			0x702CC
> > +#define _PLANE_COLOR_CTL_3_A			0x703CC
> I'd prefer that we try to keep the register defines in order based
> on the offset.

Do you mean stuffing this into the list above? I found that less legible, but
ok.

> Have you've tested this stuff on real hw? Bspec seems to have become
> rather illegible as of late, so I had to jump hoops to even find the
> register offsets for these, and even then it seems to be telling me
> that these registers do and do not exist at the same time.

Yep, I tested this with kms_pipe_color, and it did the right thing for the CTM
tests, although I had to workaround it wanting a programmable degamma LUT. I
should probably clean that patch up and submit.

> > 
> > +#define   PLANE_COLOR_PIPE_GAMMA_ENABLE		(1 << 30)
> > +#define   PLANE_COLOR_PIPE_CSC_ENABLE		(1 << 23)
> > +#define   PLANE_COLOR_PLANE_GAMMA_DISABLE	(1 << 13)
> Offsets and bits seem to match what I eventually managed to dig up
> from the spec.
> 
> > 
> > +
> >  #define _PLANE_CTL_1_B				0x71180
> >  #define _PLANE_CTL_2_B				0x71280
> >  #define _PLANE_CTL_3_B				0x71380
> > @@ -5968,7 +5975,17 @@ enum {
> >  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
> >  	_MMIO_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe),
> > _PLANE_NV12_BUF_CFG_2(pipe))
> >  
> > -/* SKL new cursor registers */
> > +#define _PLANE_COLOR_CTL_1_B			0x711CC
> > +#define _PLANE_COLOR_CTL_2_B			0x712CC
> > +#define _PLANE_COLOR_CTL_3_B			0x713CC
> > +#define _PLANE_COLOR_CTL_1(pipe)	\
> > +	_PIPE(pipe, _PLANE_COLOR_CTL_1_A, _PLANE_COLOR_CTL_1_B)
> > +#define _PLANE_COLOR_CTL_2(pipe)	\
> > +	_PIPE(pipe, _PLANE_COLOR_CTL_2_A, _PLANE_COLOR_CTL_2_B)
> > +#define PLANE_COLOR_CTL(pipe, plane)	\
> > +	_MMIO_PLANE(plane, _PLANE_COLOR_CTL_1(pipe),
> > _PLANE_COLOR_CTL_2(pipe))
> > +
> > +#/* SKL new cursor registers */
> >  #define _CUR_BUF_CFG_A				0x7017c
> >  #define _CUR_BUF_CFG_B				0x7117c
> >  #define CUR_BUF_CFG(pipe)	_MMIO_PIPE(pipe, _CUR_BUF_CFG_A,
> > _CUR_BUF_CFG_B)
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 0bf8e1b..5313c1f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3386,9 +3386,20 @@ static void skylake_update_primary_plane(struct
> > drm_plane *plane,
> >  	int dst_w = drm_rect_width(&plane_state->base.dst);
> >  	int dst_h = drm_rect_height(&plane_state->base.dst);
> >  
> > -	plane_ctl = PLANE_CTL_ENABLE |
> > -		    PLANE_CTL_PIPE_GAMMA_ENABLE |
> > -		    PLANE_CTL_PIPE_CSC_ENABLE;
> > +	plane_ctl = PLANE_CTL_ENABLE;
> > +
> > +	if (IS_GEMINILAKE(dev_priv)) {
> > +		u32 plane_color =
> > +			PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > +			PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > +
> > +		I915_WRITE(PLANE_COLOR_CTL(pipe, 0), plane_color);
> Use plane_id please.

Will do.

>  'plane_color' seems a little pointless.
> Or are we expecting to OR bits in conditionally?

I found that a bit more readable. Will change.

Ander

> 
> > 
> > +	} else {
> > +		plane_ctl |=
> > +			PLANE_CTL_PIPE_GAMMA_ENABLE |
> > +			PLANE_CTL_PIPE_CSC_ENABLE |
> > +			PLANE_CTL_PLANE_GAMMA_DISABLE;
> > +	}
> >  
> >  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
> >  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> > b/drivers/gpu/drm/i915/intel_sprite.c
> > index c05545f..25f49a0 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -219,14 +219,23 @@ skl_update_plane(struct drm_plane *drm_plane,
> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >  
> > -	plane_ctl = PLANE_CTL_ENABLE |
> > -		PLANE_CTL_PIPE_GAMMA_ENABLE |
> > -		PLANE_CTL_PIPE_CSC_ENABLE;
> > +	plane_ctl = PLANE_CTL_ENABLE;
> > +
> > +	if (IS_GEMINILAKE(dev_priv)) {
> > +		u32 plane_color =
> > +			PLANE_COLOR_PIPE_GAMMA_ENABLE |
> > +			PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > +
> > +		I915_WRITE(PLANE_COLOR_CTL(pipe, plane_id), plane_color);
> > +	} else {
> > +		plane_ctl |=
> > +			PLANE_CTL_PIPE_GAMMA_ENABLE |
> > +			PLANE_CTL_PIPE_CSC_ENABLE |
> > +			PLANE_CTL_PLANE_GAMMA_DISABLE;
> > +	}
> >  
> >  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
> >  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> > -
> > -	plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >  	plane_ctl |= skl_plane_ctl_rotation(rotation);
> >  
> >  	if (key->flags) {
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915/glk: Program pipe gamma and degamma tables

[Ander Conselvan De Oliveira]

On Tue, 2017-01-24 at 20:05 +0200, Ville Syrjälä wrote:
> On Tue, Jan 24, 2017 at 03:35:54PM +0200, Ander Conselvan de Oliveira wrote:
> > 
> > The gamma tables in Geminilake were changed. There is no split-gamma
> > mode. Instead, there is a dedicated degamma table that is enabled
> > whenever pipe CSC is enabled.
> > 
> > The dedicated gamma table has 16 bit precision but doesn't support
> > separate channels. Since that doesn't match the per-channel format of
> > the degamma LUT property, for now only a linear table is loaded and the
> > property ignored.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@inte
> > l.com>
> > ---
> >  drivers/gpu/drm/i915/i915_pci.c    |   1 +
> >  drivers/gpu/drm/i915/i915_reg.h    |  14 +++++
> >  drivers/gpu/drm/i915/intel_color.c | 105
> > +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 120 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> > b/drivers/gpu/drm/i915/i915_pci.c
> > index ecb487b..df2051b 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -403,6 +403,7 @@ static const struct intel_device_info
> > intel_geminilake_info = {
> >  	.platform = INTEL_GEMINILAKE,
> >  	.is_alpha_support = 1,
> >  	.ddb_size = 1024,
> > +	.color = { .degamma_lut_size = 0, .gamma_lut_size = 1024 }
> >  };
> >  
> >  static const struct intel_device_info intel_kabylake_info = {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 7c240e4..625d8f7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8180,12 +8180,26 @@ enum {
> >  #define _PAL_PREC_EXT_GC_MAX_A	0x4A420
> >  #define _PAL_PREC_EXT_GC_MAX_B	0x4AC20
> >  #define _PAL_PREC_EXT_GC_MAX_C	0x4B420
> > +#define _PAL_PREC_EXT2_GC_MAX_A	0x4A430
> > +#define _PAL_PREC_EXT2_GC_MAX_B	0x4AC30
> > +#define _PAL_PREC_EXT2_GC_MAX_C	0x4B430
> >  
> >  #define PREC_PAL_INDEX(pipe)		_MMIO_PIPE(pipe,
> > _PAL_PREC_INDEX_A, _PAL_PREC_INDEX_B)
> >  #define PREC_PAL_DATA(pipe)		_MMIO_PIPE(pipe,
> > _PAL_PREC_DATA_A, _PAL_PREC_DATA_B)
> >  #define PREC_PAL_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> > _PAL_PREC_GC_MAX_A, _PAL_PREC_GC_MAX_B) + (i) * 4)
> >  #define PREC_PAL_EXT_GC_MAX(pipe, i)	_MMIO(_PIPE(pipe,
> > _PAL_PREC_EXT_GC_MAX_A, _PAL_PREC_EXT_GC_MAX_B) + (i) * 4)
> >  
> > +#define _PRE_CSC_GAMC_INDEX_A	0x4A484
> > +#define _PRE_CSC_GAMC_INDEX_B	0x4AC84
> > +#define _PRE_CSC_GAMC_INDEX_C	0x4B484
> > +#define   PRE_CSC_GAMC_AUTO_INCREMENT	(1 << 10)
> > +#define _PRE_CSC_GAMC_DATA_A	0x4A488
> > +#define _PRE_CSC_GAMC_DATA_B	0x4AC88
> > +#define _PRE_CSC_GAMC_DATA_C	0x4B488
> > +
> > +#define PRE_CSC_GAMC_INDEX(pipe)	_MMIO_PIPE(pipe,
> > _PRE_CSC_GAMC_INDEX_A, _PRE_CSC_GAMC_INDEX_B)
> > +#define PRE_CSC_GAMC_DATA(pipe)		_MMIO_PIPE(pipe,
> > _PRE_CSC_GAMC_DATA_A, _PRE_CSC_GAMC_DATA_B)
> > +
> >  /* pipe CSC & degamma/gamma LUTs on CHV */
> >  #define _CGM_PIPE_A_CSC_COEFF01	(VLV_DISPLAY_BASE + 0x67900)
> >  #define _CGM_PIPE_A_CSC_COEFF23	(VLV_DISPLAY_BASE + 0x67904)
> > diff --git a/drivers/gpu/drm/i915/intel_color.c
> > b/drivers/gpu/drm/i915/intel_color.c
> > index d81232b..09e0903 100644
> > --- a/drivers/gpu/drm/i915/intel_color.c
> > +++ b/drivers/gpu/drm/i915/intel_color.c
> > @@ -422,6 +422,108 @@ static void broadwell_load_luts(struct drm_crtc_state
> > *state)
> >  	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> >  }
> >  
> > +static void glk_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 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);
> > +
> > +	/*
> > +	 *  FIXME: The pipe degamma table in geminilake doesn't support
> > +	 *  different values per channel, so this just loads a linear
> > table.
> > +	 */
> > +	for (i = 0; i < lut_size; i++) {
> > +		uint32_t v = (i * ((1 << 16) - 1)) / (lut_size - 1);
> > +
> > +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), v);
> > +	}
> > +
> > +	/* Clamp values > 1.0. */
> > +	while (i++ < 35)
> > +		I915_WRITE(PRE_CSC_GAMC_DATA(pipe), (1 << 16) - 1);
> > +
> Useless empty line.
> 
> I do still wonder how we ended up with the 0.16 scheme for LUTs. I'm
> pretty sure 8.24 or something like that was the plane. Oh well, it is
> what it is.
> 
> > 
> > +}
> > +
> > +static void glk_load_gamma_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 = INTEL_INFO(dev_priv)-
> > >color.gamma_lut_size;
> > +	uint32_t i;
> > +
> > +	I915_WRITE(PREC_PAL_INDEX(pipe), PAL_PREC_AUTO_INCREMENT);
> > +
> > +	if (state->gamma_lut) {
> > +		struct drm_color_lut *lut =
> > +			(struct drm_color_lut *) state->gamma_lut->data;
> const perhaps?
> 
> > 
> > +
> > +		for (i = 0; i < lut_size; i++) {
> > +			uint32_t word =
> > +			(drm_color_lut_extract(lut[i].red, 10) << 20) |
> > +			(drm_color_lut_extract(lut[i].green, 10) << 10) |
> > +			drm_color_lut_extract(lut[i].blue, 10);
> A bit strange calling it a "word", but that's what we apparently do
> elsewhere.
> 
> > 
> > +
> > +			I915_WRITE(PREC_PAL_DATA(pipe), word);
> > +		}
> > +
> > +		/* Program the max register to clamp values > 1.0. */
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0),
> > +			   drm_color_lut_extract(lut[i].red, 16));
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1),
> > +			   drm_color_lut_extract(lut[i].green, 16));
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2),
> > +			   drm_color_lut_extract(lut[i].blue, 16));
> > +	} else {
> > +		for (i = 0; i < lut_size; i++) {
> > +			uint32_t v = (i * ((1 << 10) - 1)) / (lut_size -
> > 1);
> > +
> > +			I915_WRITE(PREC_PAL_DATA(pipe),
> > +				   (v << 20) | (v << 10) | v);
> > +		}
> > +
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 0), (1 << 16) - 1);
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 1), (1 << 16) - 1);
> > +		I915_WRITE(PREC_PAL_GC_MAX(pipe, 2), (1 << 16) - 1);
> > +	}
> > +}
> Hmm. Am I imagining it, or is that code pretty much identical to the
> "broadwell" code?

It is pretty much the same. The broadwell code uses split gamma, so the table
starts from the 512th entry and relies on the load of the degamma lut to leave
the index pointing there. I couldn't figure out how to merge the two without
obscuring this. I could call this function bdw_load_gamma_lut() and add a
parameter to tell where to start, but it just looks weird that the lenght of the
table comes from device info, but the start index would be hardcoded.

Probably still better than duplicated code, so just do that.

Ander

> 
> I think all this LUT code really needs to some heavy handed renaming
> to make things not confusing. But that's a separate issue.
> 
> > 
> > +
> > +static void glk_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(state)) {
> > +		haswell_load_luts(state);
> > +		return;
> > +	}
> > +
> > +	glk_load_degamma_lut(state);
> > +	glk_load_gamma_lut(state);
> > +
> > +	intel_state->gamma_mode = GAMMA_MODE_MODE_10BIT;
> > +	I915_WRITE(GAMMA_MODE(pipe), GAMMA_MODE_MODE_10BIT);
> > +	POSTING_READ(GAMMA_MODE(pipe));
> > +
> > +	/*
> > +	 * Reset the index, otherwise it prevents the legacy palette to be
> > +	 * written properly.
> > +	 */
> > +	I915_WRITE(PREC_PAL_INDEX(pipe), 0);
> > +	I915_WRITE(PIPE_CSC_MODE(pipe), 0);
> > +}
> > +
> >  /* Loads the palette/gamma unit for the CRTC on CherryView. */
> >  static void cherryview_load_luts(struct drm_crtc_state *state)
> >  {
> > @@ -540,6 +642,9 @@ void intel_color_init(struct drm_crtc *crtc)
> >  		   IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)) {
> >  		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> >  		dev_priv->display.load_luts = broadwell_load_luts;
> > +	} else if (IS_GEMINILAKE(dev_priv)) {
> > +		dev_priv->display.load_csc_matrix = i9xx_load_csc_matrix;
> > +		dev_priv->display.load_luts = glk_load_luts;
> >  	} else {
> >  		dev_priv->display.load_luts = i9xx_load_luts;
> >  	}
> > -- 
> > 2.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx