[02/13] drm/i915: Split the gamma/csc enable bits from the plane_ctl() function

Submitted by Ville Syrjälä on Jan. 11, 2019, 5:08 p.m.

Details

Message ID 20190111170823.4441-3-ville.syrjala@linux.intel.com
State New
Headers show
Series "Enable/disable gamma/csc dynamically and fix C8" ( rev: 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Ville Syrjälä Jan. 11, 2019, 5:08 p.m.
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On g4x+ the pipe gamma enable bit for the primary plane affects
the pipe bottom color as well. The same for the pipe csc enable
bit on ilk+. Thus we must configure those bits correctly even
when the primary plane is disabled.

To make the feasible let's split those settings from the
plane_ctl() function into a seprate funciton that we can
call from the ->disable_plane() hook as well.

For consistency we'll do that on all the plane types. While
that has no real benefits at this time, it'll become useful
when we start to control the pipe gamma/csc enable bits
dynamically when we overhaul the color management code.

On pre-g4x there doesn't appear to be any way to gamma
correct the pipe bottom color, but sticking to the same
pattern doesn't hurt. And it'll still help us to do
crtc state readout correctly for the pipe gamma enable
bit for the color management overhaul.

An alternative apporach would be to still precompute these
bits into plane_state->ctl, but that would require that we
run through the plane check even when the plane isn't logically
enabled on any crtc. Currently that condition causes us to
short circuit the entire thing and not call ->check_plane().
There would also be some chicken and egg problems with
->check_plane() vs. crtc color state check that would
requite splitting certain things into multiple steps.
So all in all this seems like the easier route.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +-
 drivers/gpu/drm/i915/intel_sprite.c  |  54 ++++++++---
 3 files changed, 139 insertions(+), 46 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5dc0de89c49e..a3871db4703b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3180,28 +3180,38 @@  i9xx_plane_max_stride(struct intel_plane *plane,
 	}
 }
 
+static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
+	u32 dspcntr = 0;
+
+	dspcntr |= DISPPLANE_GAMMA_ENABLE;
+
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+
+	if (INTEL_GEN(dev_priv) < 5)
+		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
+
+	return dspcntr;
+}
+
 static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
 			  const struct intel_plane_state *plane_state)
 {
 	struct drm_i915_private *dev_priv =
 		to_i915(plane_state->base.plane->dev);
-	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
 	const struct drm_framebuffer *fb = plane_state->base.fb;
 	unsigned int rotation = plane_state->base.rotation;
 	u32 dspcntr;
 
-	dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
+	dspcntr = DISPLAY_PLANE_ENABLE;
 
 	if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||
 	    IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
 		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
 
-	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
-
-	if (INTEL_GEN(dev_priv) < 5)
-		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
-
 	switch (fb->format->format) {
 	case DRM_FORMAT_C8:
 		dspcntr |= DISPPLANE_8BPP;
@@ -3329,11 +3339,13 @@  static void i9xx_update_plane(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
 	u32 linear_offset;
-	u32 dspcntr = plane_state->ctl;
 	int x = plane_state->color_plane[0].x;
 	int y = plane_state->color_plane[0].y;
 	unsigned long irqflags;
 	u32 dspaddr_offset;
+	u32 dspcntr;
+
+	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
 
 	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
 
@@ -3393,10 +3405,23 @@  static void i9xx_disable_plane(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
 	unsigned long irqflags;
+	u32 dspcntr;
+
+	/*
+	 * DSPCNTR pipe gamma enable on g4x+ and pipe csc
+	 * enable on ilk+ affect the pipe bottom color as
+	 * well, so we must configure them even if the plane
+	 * is disabled.
+	 *
+	 * On pre-g4x there is no way to gamma correct the
+	 * pipe bottom color but we'll keep on doing this
+	 * anyway.
+	 */
+	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
 
 	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
 
-	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
+	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
 	if (INTEL_GEN(dev_priv) >= 4)
 		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
 	else
@@ -3631,6 +3656,20 @@  static u32 cnl_plane_ctl_flip(unsigned int reflect)
 	return 0;
 }
 
+u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	u32 plane_ctl = 0;
+
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		return plane_ctl;
+
+	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
+	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
+
+	return plane_ctl;
+}
+
 u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
 		  const struct intel_plane_state *plane_state)
 {
@@ -3645,10 +3684,7 @@  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
 
 	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
 		plane_ctl |= skl_plane_ctl_alpha(plane_state);
-		plane_ctl |=
-			PLANE_CTL_PIPE_GAMMA_ENABLE |
-			PLANE_CTL_PIPE_CSC_ENABLE |
-			PLANE_CTL_PLANE_GAMMA_DISABLE;
+		plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
 
 		if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
 			plane_ctl |= PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
@@ -3673,19 +3709,27 @@  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
 	return plane_ctl;
 }
 
+u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	u32 plane_color_ctl = 0;
+
+	if (INTEL_GEN(dev_priv) >= 11)
+		return plane_color_ctl;
+
+	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
+	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
+
+	return plane_color_ctl;
+}
+
 u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
 			const struct intel_plane_state *plane_state)
 {
-	struct drm_i915_private *dev_priv =
-		to_i915(plane_state->base.plane->dev);
 	const struct drm_framebuffer *fb = plane_state->base.fb;
 	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
 	u32 plane_color_ctl = 0;
 
-	if (INTEL_GEN(dev_priv) < 11) {
-		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
-		plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
-	}
 	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
 	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
 
@@ -9867,11 +9911,15 @@  i845_cursor_max_stride(struct intel_plane *plane,
 	return 2048;
 }
 
+static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	return CURSOR_GAMMA_ENABLE;
+}
+
 static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
 			   const struct intel_plane_state *plane_state)
 {
 	return CURSOR_ENABLE |
-		CURSOR_GAMMA_ENABLE |
 		CURSOR_FORMAT_ARGB |
 		CURSOR_STRIDE(plane_state->color_plane[0].stride);
 }
@@ -9941,7 +9989,9 @@  static void i845_update_cursor(struct intel_plane *plane,
 		unsigned int width = plane_state->base.crtc_w;
 		unsigned int height = plane_state->base.crtc_h;
 
-		cntl = plane_state->ctl;
+		cntl = plane_state->ctl |
+			i845_cursor_ctl_crtc(crtc_state);
+
 		size = (height << 12) | width;
 
 		base = intel_cursor_base(plane_state);
@@ -10006,27 +10056,36 @@  i9xx_cursor_max_stride(struct intel_plane *plane,
 	return plane->base.dev->mode_config.cursor_width * 4;
 }
 
-static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
-			   const struct intel_plane_state *plane_state)
+static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
 {
-	struct drm_i915_private *dev_priv =
-		to_i915(plane_state->base.plane->dev);
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
+	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
 	u32 cntl = 0;
 
-	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
-		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
+	if (INTEL_GEN(dev_priv) >= 11)
+		return cntl;
 
-	if (INTEL_GEN(dev_priv) <= 10) {
-		cntl |= MCURSOR_GAMMA_ENABLE;
+	cntl |= MCURSOR_GAMMA_ENABLE;
 
-		if (HAS_DDI(dev_priv))
-			cntl |= MCURSOR_PIPE_CSC_ENABLE;
-	}
+	if (HAS_DDI(dev_priv))
+		cntl |= MCURSOR_PIPE_CSC_ENABLE;
 
 	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
 		cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
 
+	return cntl;
+}
+
+static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
+			   const struct intel_plane_state *plane_state)
+{
+	struct drm_i915_private *dev_priv =
+		to_i915(plane_state->base.plane->dev);
+	u32 cntl = 0;
+
+	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
+		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
+
 	switch (plane_state->base.crtc_w) {
 	case 64:
 		cntl |= MCURSOR_MODE_64_ARGB_AX;
@@ -10151,7 +10210,8 @@  static void i9xx_update_cursor(struct intel_plane *plane,
 	unsigned long irqflags;
 
 	if (plane_state && plane_state->base.visible) {
-		cntl = plane_state->ctl;
+		cntl = plane_state->ctl |
+			i9xx_cursor_ctl_crtc(crtc_state);
 
 		if (plane_state->base.crtc_h != plane_state->base.crtc_w)
 			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h - 1);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3b051fdd0fce..88ac42b2d7ed 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1752,9 +1752,10 @@  static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
 
 u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
 			const struct intel_plane_state *plane_state);
+u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state);
 u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
 		  const struct intel_plane_state *plane_state);
-u32 glk_color_ctl(const struct intel_plane_state *plane_state);
+u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
 u32 skl_plane_stride(const struct intel_plane_state *plane_state,
 		     int plane);
 int skl_check_plane_surface(struct intel_plane_state *plane_state);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8f3982c03925..a45ef98b2f8d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -484,9 +484,16 @@  skl_program_plane(struct intel_plane *plane,
 	struct intel_plane *linked = plane_state->linked_plane;
 	const struct drm_framebuffer *fb = plane_state->base.fb;
 	u8 alpha = plane_state->base.alpha >> 8;
+	u32 plane_color_ctl = 0;
 	unsigned long irqflags;
 	u32 keymsk, keymax;
 
+	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
+
+	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
+		plane_color_ctl = plane_state->color_ctl |
+			glk_plane_color_ctl_crtc(crtc_state);
+
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -533,8 +540,7 @@  skl_program_plane(struct intel_plane *plane,
 	}
 
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
-		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
-			      plane_state->color_ctl);
+		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id), plane_color_ctl);
 
 	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
 		icl_program_input_csc(plane, crtc_state, plane_state);
@@ -731,6 +737,11 @@  vlv_update_clrc(const struct intel_plane_state *plane_state)
 		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
 }
 
+static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	return SP_GAMMA_ENABLE;
+}
+
 static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
 			  const struct intel_plane_state *plane_state)
 {
@@ -739,7 +750,7 @@  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 sprctl;
 
-	sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
+	sprctl = SP_ENABLE;
 
 	switch (fb->format->format) {
 	case DRM_FORMAT_YUYV:
@@ -806,7 +817,6 @@  vlv_update_plane(struct intel_plane *plane,
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
 	enum plane_id plane_id = plane->id;
-	u32 sprctl = plane_state->ctl;
 	u32 sprsurf_offset = plane_state->color_plane[0].offset;
 	u32 linear_offset;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
@@ -817,6 +827,9 @@  vlv_update_plane(struct intel_plane *plane,
 	uint32_t x = plane_state->color_plane[0].x;
 	uint32_t y = plane_state->color_plane[0].y;
 	unsigned long irqflags;
+	u32 sprctl;
+
+	sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
 
 	/* Sizes are 0 based */
 	crtc_w--;
@@ -897,6 +910,19 @@  vlv_plane_get_hw_state(struct intel_plane *plane,
 	return ret;
 }
 
+static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
+	u32 sprctl = 0;
+
+	sprctl |= SPRITE_GAMMA_ENABLE;
+
+	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+		sprctl |= SPRITE_PIPE_CSC_ENABLE;
+
+	return sprctl;
+}
+
 static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
 			  const struct intel_plane_state *plane_state)
 {
@@ -907,14 +933,11 @@  static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 sprctl;
 
-	sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
+	sprctl = SPRITE_ENABLE;
 
 	if (IS_IVYBRIDGE(dev_priv))
 		sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
 
-	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
-		sprctl |= SPRITE_PIPE_CSC_ENABLE;
-
 	switch (fb->format->format) {
 	case DRM_FORMAT_XBGR8888:
 		sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
@@ -966,7 +989,6 @@  ivb_update_plane(struct intel_plane *plane,
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
-	u32 sprctl = plane_state->ctl, sprscale = 0;
 	u32 sprsurf_offset = plane_state->color_plane[0].offset;
 	u32 linear_offset;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
@@ -978,8 +1000,11 @@  ivb_update_plane(struct intel_plane *plane,
 	uint32_t y = plane_state->color_plane[0].y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
+	u32 sprctl, sprscale = 0;
 	unsigned long irqflags;
 
+	sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
+
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;
@@ -1074,6 +1099,11 @@  g4x_sprite_max_stride(struct intel_plane *plane,
 	return 16384;
 }
 
+static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
+{
+	return DVS_GAMMA_ENABLE;
+}
+
 static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
 			  const struct intel_plane_state *plane_state)
 {
@@ -1084,7 +1114,7 @@  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
 	u32 dvscntr;
 
-	dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
+	dvscntr = DVS_ENABLE;
 
 	if (IS_GEN(dev_priv, 6))
 		dvscntr |= DVS_TRICKLE_FEED_DISABLE;
@@ -1140,7 +1170,6 @@  g4x_update_plane(struct intel_plane *plane,
 {
 	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
 	enum pipe pipe = plane->pipe;
-	u32 dvscntr = plane_state->ctl, dvsscale = 0;
 	u32 dvssurf_offset = plane_state->color_plane[0].offset;
 	u32 linear_offset;
 	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
@@ -1152,8 +1181,11 @@  g4x_update_plane(struct intel_plane *plane,
 	uint32_t y = plane_state->color_plane[0].y;
 	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
 	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
+	u32 dvscntr, dvsscale = 0;
 	unsigned long irqflags;
 
+	dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
+
 	/* Sizes are 0 based */
 	src_w--;
 	src_h--;

Comments

On Fri, Jan 11, 2019 at 07:08:12PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On g4x+ the pipe gamma enable bit for the primary plane affects
> the pipe bottom color as well. The same for the pipe csc enable
> bit on ilk+. Thus we must configure those bits correctly even
> when the primary plane is disabled.

This is only true for <gen9, right?  Starting with gen9, we have
dedicated bits that control this, so I don't think the primay plane's
settings should have any impact when disabled.  I.e., we also need the
bits set in this patch:

        https://patchwork.freedesktop.org/patch/271109/

> To make the feasible let's split those settings from the
> plane_ctl() function into a seprate funciton that we can
> call from the ->disable_plane() hook as well.

Is calling it from ->disable_plane() enough?  If we just disable the
primary plane, then those bits will remain set while the crtc remains
active.  But if you then disable the whole crtc and re-enable it again
later, won't we have lost the bits at that point?


Matt

> 
> For consistency we'll do that on all the plane types. While
> that has no real benefits at this time, it'll become useful
> when we start to control the pipe gamma/csc enable bits
> dynamically when we overhaul the color management code.
> 
> On pre-g4x there doesn't appear to be any way to gamma
> correct the pipe bottom color, but sticking to the same
> pattern doesn't hurt. And it'll still help us to do
> crtc state readout correctly for the pipe gamma enable
> bit for the color management overhaul.
> 
> An alternative apporach would be to still precompute these
> bits into plane_state->ctl, but that would require that we
> run through the plane check even when the plane isn't logically
> enabled on any crtc. Currently that condition causes us to
> short circuit the entire thing and not call ->check_plane().
> There would also be some chicken and egg problems with
> ->check_plane() vs. crtc color state check that would
> requite splitting certain things into multiple steps.
> So all in all this seems like the easier route.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
>  drivers/gpu/drm/i915/intel_sprite.c  |  54 ++++++++---
>  3 files changed, 139 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5dc0de89c49e..a3871db4703b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3180,28 +3180,38 @@ i9xx_plane_max_stride(struct intel_plane *plane,
>  	}
>  }
>  
> +static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 dspcntr = 0;
> +
> +	dspcntr |= DISPPLANE_GAMMA_ENABLE;
> +
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> +
> +	if (INTEL_GEN(dev_priv) < 5)
> +		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> +
> +	return dspcntr;
> +}
> +
>  static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
>  			  const struct intel_plane_state *plane_state)
>  {
>  	struct drm_i915_private *dev_priv =
>  		to_i915(plane_state->base.plane->dev);
> -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	unsigned int rotation = plane_state->base.rotation;
>  	u32 dspcntr;
>  
> -	dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
> +	dspcntr = DISPLAY_PLANE_ENABLE;
>  
>  	if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||
>  	    IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
>  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
>  
> -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> -
> -	if (INTEL_GEN(dev_priv) < 5)
> -		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> -
>  	switch (fb->format->format) {
>  	case DRM_FORMAT_C8:
>  		dspcntr |= DISPPLANE_8BPP;
> @@ -3329,11 +3339,13 @@ static void i9xx_update_plane(struct intel_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
>  	u32 linear_offset;
> -	u32 dspcntr = plane_state->ctl;
>  	int x = plane_state->color_plane[0].x;
>  	int y = plane_state->color_plane[0].y;
>  	unsigned long irqflags;
>  	u32 dspaddr_offset;
> +	u32 dspcntr;
> +
> +	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
>  
>  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
>  
> @@ -3393,10 +3405,23 @@ static void i9xx_disable_plane(struct intel_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
>  	unsigned long irqflags;
> +	u32 dspcntr;
> +
> +	/*
> +	 * DSPCNTR pipe gamma enable on g4x+ and pipe csc
> +	 * enable on ilk+ affect the pipe bottom color as
> +	 * well, so we must configure them even if the plane
> +	 * is disabled.
> +	 *
> +	 * On pre-g4x there is no way to gamma correct the
> +	 * pipe bottom color but we'll keep on doing this
> +	 * anyway.
> +	 */
> +	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
>  
>  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
>  
> -	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
> +	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
>  	if (INTEL_GEN(dev_priv) >= 4)
>  		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
>  	else
> @@ -3631,6 +3656,20 @@ static u32 cnl_plane_ctl_flip(unsigned int reflect)
>  	return 0;
>  }
>  
> +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +	u32 plane_ctl = 0;
> +
> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +		return plane_ctl;
> +
> +	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> +	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
> +
> +	return plane_ctl;
> +}
> +
>  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>  		  const struct intel_plane_state *plane_state)
>  {
> @@ -3645,10 +3684,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>  
>  	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
>  		plane_ctl |= skl_plane_ctl_alpha(plane_state);
> -		plane_ctl |=
> -			PLANE_CTL_PIPE_GAMMA_ENABLE |
> -			PLANE_CTL_PIPE_CSC_ENABLE |
> -			PLANE_CTL_PLANE_GAMMA_DISABLE;
> +		plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
>  
>  		if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
>  			plane_ctl |= PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
> @@ -3673,19 +3709,27 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>  	return plane_ctl;
>  }
>  
> +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +	u32 plane_color_ctl = 0;
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		return plane_color_ctl;
> +
> +	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> +	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> +
> +	return plane_color_ctl;
> +}
> +
>  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
>  			const struct intel_plane_state *plane_state)
>  {
> -	struct drm_i915_private *dev_priv =
> -		to_i915(plane_state->base.plane->dev);
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
>  	u32 plane_color_ctl = 0;
>  
> -	if (INTEL_GEN(dev_priv) < 11) {
> -		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> -		plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> -	}
>  	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
>  	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
>  
> @@ -9867,11 +9911,15 @@ i845_cursor_max_stride(struct intel_plane *plane,
>  	return 2048;
>  }
>  
> +static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	return CURSOR_GAMMA_ENABLE;
> +}
> +
>  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
>  			   const struct intel_plane_state *plane_state)
>  {
>  	return CURSOR_ENABLE |
> -		CURSOR_GAMMA_ENABLE |
>  		CURSOR_FORMAT_ARGB |
>  		CURSOR_STRIDE(plane_state->color_plane[0].stride);
>  }
> @@ -9941,7 +9989,9 @@ static void i845_update_cursor(struct intel_plane *plane,
>  		unsigned int width = plane_state->base.crtc_w;
>  		unsigned int height = plane_state->base.crtc_h;
>  
> -		cntl = plane_state->ctl;
> +		cntl = plane_state->ctl |
> +			i845_cursor_ctl_crtc(crtc_state);
> +
>  		size = (height << 12) | width;
>  
>  		base = intel_cursor_base(plane_state);
> @@ -10006,27 +10056,36 @@ i9xx_cursor_max_stride(struct intel_plane *plane,
>  	return plane->base.dev->mode_config.cursor_width * 4;
>  }
>  
> -static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> -			   const struct intel_plane_state *plane_state)
> +static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_i915_private *dev_priv =
> -		to_i915(plane_state->base.plane->dev);
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	u32 cntl = 0;
>  
> -	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> -		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		return cntl;
>  
> -	if (INTEL_GEN(dev_priv) <= 10) {
> -		cntl |= MCURSOR_GAMMA_ENABLE;
> +	cntl |= MCURSOR_GAMMA_ENABLE;
>  
> -		if (HAS_DDI(dev_priv))
> -			cntl |= MCURSOR_PIPE_CSC_ENABLE;
> -	}
> +	if (HAS_DDI(dev_priv))
> +		cntl |= MCURSOR_PIPE_CSC_ENABLE;
>  
>  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
>  		cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
>  
> +	return cntl;
> +}
> +
> +static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> +			   const struct intel_plane_state *plane_state)
> +{
> +	struct drm_i915_private *dev_priv =
> +		to_i915(plane_state->base.plane->dev);
> +	u32 cntl = 0;
> +
> +	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> +		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> +
>  	switch (plane_state->base.crtc_w) {
>  	case 64:
>  		cntl |= MCURSOR_MODE_64_ARGB_AX;
> @@ -10151,7 +10210,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
>  	unsigned long irqflags;
>  
>  	if (plane_state && plane_state->base.visible) {
> -		cntl = plane_state->ctl;
> +		cntl = plane_state->ctl |
> +			i9xx_cursor_ctl_crtc(crtc_state);
>  
>  		if (plane_state->base.crtc_h != plane_state->base.crtc_w)
>  			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h - 1);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3b051fdd0fce..88ac42b2d7ed 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1752,9 +1752,10 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
>  
>  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
>  			const struct intel_plane_state *plane_state);
> +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state);
>  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>  		  const struct intel_plane_state *plane_state);
> -u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
>  u32 skl_plane_stride(const struct intel_plane_state *plane_state,
>  		     int plane);
>  int skl_check_plane_surface(struct intel_plane_state *plane_state);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8f3982c03925..a45ef98b2f8d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -484,9 +484,16 @@ skl_program_plane(struct intel_plane *plane,
>  	struct intel_plane *linked = plane_state->linked_plane;
>  	const struct drm_framebuffer *fb = plane_state->base.fb;
>  	u8 alpha = plane_state->base.alpha >> 8;
> +	u32 plane_color_ctl = 0;
>  	unsigned long irqflags;
>  	u32 keymsk, keymax;
>  
> +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> +
> +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> +		plane_color_ctl = plane_state->color_ctl |
> +			glk_plane_color_ctl_crtc(crtc_state);
> +
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -533,8 +540,7 @@ skl_program_plane(struct intel_plane *plane,
>  	}
>  
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> -		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> -			      plane_state->color_ctl);
> +		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id), plane_color_ctl);
>  
>  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
>  		icl_program_input_csc(plane, crtc_state, plane_state);
> @@ -731,6 +737,11 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
>  		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
>  }
>  
> +static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	return SP_GAMMA_ENABLE;
> +}
> +
>  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>  			  const struct intel_plane_state *plane_state)
>  {
> @@ -739,7 +750,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 sprctl;
>  
> -	sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
> +	sprctl = SP_ENABLE;
>  
>  	switch (fb->format->format) {
>  	case DRM_FORMAT_YUYV:
> @@ -806,7 +817,6 @@ vlv_update_plane(struct intel_plane *plane,
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
>  	enum plane_id plane_id = plane->id;
> -	u32 sprctl = plane_state->ctl;
>  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
>  	u32 linear_offset;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> @@ -817,6 +827,9 @@ vlv_update_plane(struct intel_plane *plane,
>  	uint32_t x = plane_state->color_plane[0].x;
>  	uint32_t y = plane_state->color_plane[0].y;
>  	unsigned long irqflags;
> +	u32 sprctl;
> +
> +	sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
>  
>  	/* Sizes are 0 based */
>  	crtc_w--;
> @@ -897,6 +910,19 @@ vlv_plane_get_hw_state(struct intel_plane *plane,
>  	return ret;
>  }
>  
> +static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> +	u32 sprctl = 0;
> +
> +	sprctl |= SPRITE_GAMMA_ENABLE;
> +
> +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> +		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> +
> +	return sprctl;
> +}
> +
>  static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
>  			  const struct intel_plane_state *plane_state)
>  {
> @@ -907,14 +933,11 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 sprctl;
>  
> -	sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
> +	sprctl = SPRITE_ENABLE;
>  
>  	if (IS_IVYBRIDGE(dev_priv))
>  		sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
>  
> -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> -		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> -
>  	switch (fb->format->format) {
>  	case DRM_FORMAT_XBGR8888:
>  		sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
> @@ -966,7 +989,6 @@ ivb_update_plane(struct intel_plane *plane,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
> -	u32 sprctl = plane_state->ctl, sprscale = 0;
>  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
>  	u32 linear_offset;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> @@ -978,8 +1000,11 @@ ivb_update_plane(struct intel_plane *plane,
>  	uint32_t y = plane_state->color_plane[0].y;
>  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
>  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> +	u32 sprctl, sprscale = 0;
>  	unsigned long irqflags;
>  
> +	sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> +
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> @@ -1074,6 +1099,11 @@ g4x_sprite_max_stride(struct intel_plane *plane,
>  	return 16384;
>  }
>  
> +static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> +{
> +	return DVS_GAMMA_ENABLE;
> +}
> +
>  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
>  			  const struct intel_plane_state *plane_state)
>  {
> @@ -1084,7 +1114,7 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
>  	u32 dvscntr;
>  
> -	dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
> +	dvscntr = DVS_ENABLE;
>  
>  	if (IS_GEN(dev_priv, 6))
>  		dvscntr |= DVS_TRICKLE_FEED_DISABLE;
> @@ -1140,7 +1170,6 @@ g4x_update_plane(struct intel_plane *plane,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
> -	u32 dvscntr = plane_state->ctl, dvsscale = 0;
>  	u32 dvssurf_offset = plane_state->color_plane[0].offset;
>  	u32 linear_offset;
>  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> @@ -1152,8 +1181,11 @@ g4x_update_plane(struct intel_plane *plane,
>  	uint32_t y = plane_state->color_plane[0].y;
>  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
>  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> +	u32 dvscntr, dvsscale = 0;
>  	unsigned long irqflags;
>  
> +	dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
> +
>  	/* Sizes are 0 based */
>  	src_w--;
>  	src_h--;
> -- 
> 2.19.2
>
On Fri, Jan 11, 2019 at 04:41:37PM -0800, Matt Roper wrote:
> On Fri, Jan 11, 2019 at 07:08:12PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On g4x+ the pipe gamma enable bit for the primary plane affects
> > the pipe bottom color as well. The same for the pipe csc enable
> > bit on ilk+. Thus we must configure those bits correctly even
> > when the primary plane is disabled.
> 
> This is only true for <gen9, right? Starting with gen9, we have
> dedicated bits that control this, so I don't think the primay plane's
> settings should have any impact when disabled.  I.e., we also need the
> bits set in this patch:
> 
>         https://patchwork.freedesktop.org/patch/271109/

Yes. I had the same bits (or something similar in a later patch in
the series). But we should probably just land your stuff first.

> 
> > To make the feasible let's split those settings from the
> > plane_ctl() function into a seprate funciton that we can
> > call from the ->disable_plane() hook as well.
> 
> Is calling it from ->disable_plane() enough?  If we just disable the
> primary plane, then those bits will remain set while the crtc remains
> active.  But if you then disable the whole crtc and re-enable it again
> later, won't we have lost the bits at that point?

Hmm. Yes, probably.

Just adding a needs_modeset() check into
intel_color_add_affected_planes() (introduced in a later patch)
could be one way. But that means the plane control reg won't be
updated before the pipe is already active. So we might need a
special case for that too.

That said, I kinda hate the pipe enable special cases for the
color management stuff. So I'm wondering if we could eliminate
it all and just rely on the normal pipe+plane update to do
things correctly. But to make that consistent we might have to
have another special case to disable gamma/etc. prior to
enabling the pipe so that it can all be enabled atomically
later. Hmm. I suppose that could be achieved by clearing all
relevant control bits in disable_plane() (or in crtc_disable()
for skl+) if the crtc is no longer active.

And I guess we could still keep the .load_luts() special case
since guaranteeing the atomicity for that isn't as easy.

It would mean the pipe alwasy comes up with gamma and csc
disabled. But since it's all some shade of black anyway I
guess it shouldn't be a big deal.

> 
> 
> Matt
> 
> > 
> > For consistency we'll do that on all the plane types. While
> > that has no real benefits at this time, it'll become useful
> > when we start to control the pipe gamma/csc enable bits
> > dynamically when we overhaul the color management code.
> > 
> > On pre-g4x there doesn't appear to be any way to gamma
> > correct the pipe bottom color, but sticking to the same
> > pattern doesn't hurt. And it'll still help us to do
> > crtc state readout correctly for the pipe gamma enable
> > bit for the color management overhaul.
> > 
> > An alternative apporach would be to still precompute these
> > bits into plane_state->ctl, but that would require that we
> > run through the plane check even when the plane isn't logically
> > enabled on any crtc. Currently that condition causes us to
> > short circuit the entire thing and not call ->check_plane().
> > There would also be some chicken and egg problems with
> > ->check_plane() vs. crtc color state check that would
> > requite splitting certain things into multiple steps.
> > So all in all this seems like the easier route.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
> >  drivers/gpu/drm/i915/intel_sprite.c  |  54 ++++++++---
> >  3 files changed, 139 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 5dc0de89c49e..a3871db4703b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3180,28 +3180,38 @@ i9xx_plane_max_stride(struct intel_plane *plane,
> >  	}
> >  }
> >  
> > +static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +	u32 dspcntr = 0;
> > +
> > +	dspcntr |= DISPPLANE_GAMMA_ENABLE;
> > +
> > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> > +
> > +	if (INTEL_GEN(dev_priv) < 5)
> > +		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> > +
> > +	return dspcntr;
> > +}
> > +
> >  static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
> >  			  const struct intel_plane_state *plane_state)
> >  {
> >  	struct drm_i915_private *dev_priv =
> >  		to_i915(plane_state->base.plane->dev);
> > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> >  	unsigned int rotation = plane_state->base.rotation;
> >  	u32 dspcntr;
> >  
> > -	dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
> > +	dspcntr = DISPLAY_PLANE_ENABLE;
> >  
> >  	if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||
> >  	    IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> >  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> >  
> > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > -		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> > -
> > -	if (INTEL_GEN(dev_priv) < 5)
> > -		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> > -
> >  	switch (fb->format->format) {
> >  	case DRM_FORMAT_C8:
> >  		dspcntr |= DISPPLANE_8BPP;
> > @@ -3329,11 +3339,13 @@ static void i9xx_update_plane(struct intel_plane *plane,
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> >  	u32 linear_offset;
> > -	u32 dspcntr = plane_state->ctl;
> >  	int x = plane_state->color_plane[0].x;
> >  	int y = plane_state->color_plane[0].y;
> >  	unsigned long irqflags;
> >  	u32 dspaddr_offset;
> > +	u32 dspcntr;
> > +
> > +	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
> >  
> >  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> >  
> > @@ -3393,10 +3405,23 @@ static void i9xx_disable_plane(struct intel_plane *plane,
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> >  	unsigned long irqflags;
> > +	u32 dspcntr;
> > +
> > +	/*
> > +	 * DSPCNTR pipe gamma enable on g4x+ and pipe csc
> > +	 * enable on ilk+ affect the pipe bottom color as
> > +	 * well, so we must configure them even if the plane
> > +	 * is disabled.
> > +	 *
> > +	 * On pre-g4x there is no way to gamma correct the
> > +	 * pipe bottom color but we'll keep on doing this
> > +	 * anyway.
> > +	 */
> > +	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
> >  
> >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >  
> > -	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
> > +	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
> >  	if (INTEL_GEN(dev_priv) >= 4)
> >  		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
> >  	else
> > @@ -3631,6 +3656,20 @@ static u32 cnl_plane_ctl_flip(unsigned int reflect)
> >  	return 0;
> >  }
> >  
> > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	u32 plane_ctl = 0;
> > +
> > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > +		return plane_ctl;
> > +
> > +	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> > +	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
> > +
> > +	return plane_ctl;
> > +}
> > +
> >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >  		  const struct intel_plane_state *plane_state)
> >  {
> > @@ -3645,10 +3684,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >  
> >  	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
> >  		plane_ctl |= skl_plane_ctl_alpha(plane_state);
> > -		plane_ctl |=
> > -			PLANE_CTL_PIPE_GAMMA_ENABLE |
> > -			PLANE_CTL_PIPE_CSC_ENABLE |
> > -			PLANE_CTL_PLANE_GAMMA_DISABLE;
> > +		plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >  
> >  		if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> >  			plane_ctl |= PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
> > @@ -3673,19 +3709,27 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >  	return plane_ctl;
> >  }
> >  
> > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	u32 plane_color_ctl = 0;
> > +
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		return plane_color_ctl;
> > +
> > +	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> > +	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> > +
> > +	return plane_color_ctl;
> > +}
> > +
> >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> >  			const struct intel_plane_state *plane_state)
> >  {
> > -	struct drm_i915_private *dev_priv =
> > -		to_i915(plane_state->base.plane->dev);
> >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> >  	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >  	u32 plane_color_ctl = 0;
> >  
> > -	if (INTEL_GEN(dev_priv) < 11) {
> > -		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> > -		plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> > -	}
> >  	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> >  	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> >  
> > @@ -9867,11 +9911,15 @@ i845_cursor_max_stride(struct intel_plane *plane,
> >  	return 2048;
> >  }
> >  
> > +static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	return CURSOR_GAMMA_ENABLE;
> > +}
> > +
> >  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> >  			   const struct intel_plane_state *plane_state)
> >  {
> >  	return CURSOR_ENABLE |
> > -		CURSOR_GAMMA_ENABLE |
> >  		CURSOR_FORMAT_ARGB |
> >  		CURSOR_STRIDE(plane_state->color_plane[0].stride);
> >  }
> > @@ -9941,7 +9989,9 @@ static void i845_update_cursor(struct intel_plane *plane,
> >  		unsigned int width = plane_state->base.crtc_w;
> >  		unsigned int height = plane_state->base.crtc_h;
> >  
> > -		cntl = plane_state->ctl;
> > +		cntl = plane_state->ctl |
> > +			i845_cursor_ctl_crtc(crtc_state);
> > +
> >  		size = (height << 12) | width;
> >  
> >  		base = intel_cursor_base(plane_state);
> > @@ -10006,27 +10056,36 @@ i9xx_cursor_max_stride(struct intel_plane *plane,
> >  	return plane->base.dev->mode_config.cursor_width * 4;
> >  }
> >  
> > -static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> > -			   const struct intel_plane_state *plane_state)
> > +static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> >  {
> > -	struct drm_i915_private *dev_priv =
> > -		to_i915(plane_state->base.plane->dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	u32 cntl = 0;
> >  
> > -	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > -		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> > +	if (INTEL_GEN(dev_priv) >= 11)
> > +		return cntl;
> >  
> > -	if (INTEL_GEN(dev_priv) <= 10) {
> > -		cntl |= MCURSOR_GAMMA_ENABLE;
> > +	cntl |= MCURSOR_GAMMA_ENABLE;
> >  
> > -		if (HAS_DDI(dev_priv))
> > -			cntl |= MCURSOR_PIPE_CSC_ENABLE;
> > -	}
> > +	if (HAS_DDI(dev_priv))
> > +		cntl |= MCURSOR_PIPE_CSC_ENABLE;
> >  
> >  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> >  		cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
> >  
> > +	return cntl;
> > +}
> > +
> > +static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> > +			   const struct intel_plane_state *plane_state)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(plane_state->base.plane->dev);
> > +	u32 cntl = 0;
> > +
> > +	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > +		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> > +
> >  	switch (plane_state->base.crtc_w) {
> >  	case 64:
> >  		cntl |= MCURSOR_MODE_64_ARGB_AX;
> > @@ -10151,7 +10210,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> >  	unsigned long irqflags;
> >  
> >  	if (plane_state && plane_state->base.visible) {
> > -		cntl = plane_state->ctl;
> > +		cntl = plane_state->ctl |
> > +			i9xx_cursor_ctl_crtc(crtc_state);
> >  
> >  		if (plane_state->base.crtc_h != plane_state->base.crtc_w)
> >  			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h - 1);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 3b051fdd0fce..88ac42b2d7ed 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1752,9 +1752,10 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> >  
> >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> >  			const struct intel_plane_state *plane_state);
> > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state);
> >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >  		  const struct intel_plane_state *plane_state);
> > -u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
> >  u32 skl_plane_stride(const struct intel_plane_state *plane_state,
> >  		     int plane);
> >  int skl_check_plane_surface(struct intel_plane_state *plane_state);
> > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > index 8f3982c03925..a45ef98b2f8d 100644
> > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > @@ -484,9 +484,16 @@ skl_program_plane(struct intel_plane *plane,
> >  	struct intel_plane *linked = plane_state->linked_plane;
> >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> >  	u8 alpha = plane_state->base.alpha >> 8;
> > +	u32 plane_color_ctl = 0;
> >  	unsigned long irqflags;
> >  	u32 keymsk, keymax;
> >  
> > +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > +
> > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > +		plane_color_ctl = plane_state->color_ctl |
> > +			glk_plane_color_ctl_crtc(crtc_state);
> > +
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> > @@ -533,8 +540,7 @@ skl_program_plane(struct intel_plane *plane,
> >  	}
> >  
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > -		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > -			      plane_state->color_ctl);
> > +		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id), plane_color_ctl);
> >  
> >  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> >  		icl_program_input_csc(plane, crtc_state, plane_state);
> > @@ -731,6 +737,11 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> >  		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> >  }
> >  
> > +static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	return SP_GAMMA_ENABLE;
> > +}
> > +
> >  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >  			  const struct intel_plane_state *plane_state)
> >  {
> > @@ -739,7 +750,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	u32 sprctl;
> >  
> > -	sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
> > +	sprctl = SP_ENABLE;
> >  
> >  	switch (fb->format->format) {
> >  	case DRM_FORMAT_YUYV:
> > @@ -806,7 +817,6 @@ vlv_update_plane(struct intel_plane *plane,
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum pipe pipe = plane->pipe;
> >  	enum plane_id plane_id = plane->id;
> > -	u32 sprctl = plane_state->ctl;
> >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> >  	u32 linear_offset;
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > @@ -817,6 +827,9 @@ vlv_update_plane(struct intel_plane *plane,
> >  	uint32_t x = plane_state->color_plane[0].x;
> >  	uint32_t y = plane_state->color_plane[0].y;
> >  	unsigned long irqflags;
> > +	u32 sprctl;
> > +
> > +	sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
> >  
> >  	/* Sizes are 0 based */
> >  	crtc_w--;
> > @@ -897,6 +910,19 @@ vlv_plane_get_hw_state(struct intel_plane *plane,
> >  	return ret;
> >  }
> >  
> > +static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > +	u32 sprctl = 0;
> > +
> > +	sprctl |= SPRITE_GAMMA_ENABLE;
> > +
> > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > +		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> > +
> > +	return sprctl;
> > +}
> > +
> >  static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >  			  const struct intel_plane_state *plane_state)
> >  {
> > @@ -907,14 +933,11 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	u32 sprctl;
> >  
> > -	sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
> > +	sprctl = SPRITE_ENABLE;
> >  
> >  	if (IS_IVYBRIDGE(dev_priv))
> >  		sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> >  
> > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > -		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> > -
> >  	switch (fb->format->format) {
> >  	case DRM_FORMAT_XBGR8888:
> >  		sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
> > @@ -966,7 +989,6 @@ ivb_update_plane(struct intel_plane *plane,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum pipe pipe = plane->pipe;
> > -	u32 sprctl = plane_state->ctl, sprscale = 0;
> >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> >  	u32 linear_offset;
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > @@ -978,8 +1000,11 @@ ivb_update_plane(struct intel_plane *plane,
> >  	uint32_t y = plane_state->color_plane[0].y;
> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> > +	u32 sprctl, sprscale = 0;
> >  	unsigned long irqflags;
> >  
> > +	sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> > +
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> > @@ -1074,6 +1099,11 @@ g4x_sprite_max_stride(struct intel_plane *plane,
> >  	return 16384;
> >  }
> >  
> > +static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > +{
> > +	return DVS_GAMMA_ENABLE;
> > +}
> > +
> >  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >  			  const struct intel_plane_state *plane_state)
> >  {
> > @@ -1084,7 +1114,7 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >  	u32 dvscntr;
> >  
> > -	dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
> > +	dvscntr = DVS_ENABLE;
> >  
> >  	if (IS_GEN(dev_priv, 6))
> >  		dvscntr |= DVS_TRICKLE_FEED_DISABLE;
> > @@ -1140,7 +1170,6 @@ g4x_update_plane(struct intel_plane *plane,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >  	enum pipe pipe = plane->pipe;
> > -	u32 dvscntr = plane_state->ctl, dvsscale = 0;
> >  	u32 dvssurf_offset = plane_state->color_plane[0].offset;
> >  	u32 linear_offset;
> >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > @@ -1152,8 +1181,11 @@ g4x_update_plane(struct intel_plane *plane,
> >  	uint32_t y = plane_state->color_plane[0].y;
> >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> > +	u32 dvscntr, dvsscale = 0;
> >  	unsigned long irqflags;
> >  
> > +	dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
> > +
> >  	/* Sizes are 0 based */
> >  	src_w--;
> >  	src_h--;
> > -- 
> > 2.19.2
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795
On Mon, Jan 14, 2019 at 07:11:10PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 11, 2019 at 04:41:37PM -0800, Matt Roper wrote:
> > On Fri, Jan 11, 2019 at 07:08:12PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On g4x+ the pipe gamma enable bit for the primary plane affects
> > > the pipe bottom color as well. The same for the pipe csc enable
> > > bit on ilk+. Thus we must configure those bits correctly even
> > > when the primary plane is disabled.
> > 
> > This is only true for <gen9, right? Starting with gen9, we have
> > dedicated bits that control this, so I don't think the primay plane's
> > settings should have any impact when disabled.  I.e., we also need the
> > bits set in this patch:
> > 
> >         https://patchwork.freedesktop.org/patch/271109/
> 
> Yes. I had the same bits (or something similar in a later patch in
> the series). But we should probably just land your stuff first.
> 
> > 
> > > To make the feasible let's split those settings from the
> > > plane_ctl() function into a seprate funciton that we can
> > > call from the ->disable_plane() hook as well.
> > 
> > Is calling it from ->disable_plane() enough?  If we just disable the
> > primary plane, then those bits will remain set while the crtc remains
> > active.  But if you then disable the whole crtc and re-enable it again
> > later, won't we have lost the bits at that point?
> 
> Hmm. Yes, probably.
> 
> Just adding a needs_modeset() check into
> intel_color_add_affected_planes() (introduced in a later patch)
> could be one way. But that means the plane control reg won't be
> updated before the pipe is already active. So we might need a
> special case for that too.
> 
> That said, I kinda hate the pipe enable special cases for the
> color management stuff. So I'm wondering if we could eliminate
> it all and just rely on the normal pipe+plane update to do
> things correctly. But to make that consistent we might have to
> have another special case to disable gamma/etc. prior to
> enabling the pipe so that it can all be enabled atomically
> later. Hmm. I suppose that could be achieved by clearing all
> relevant control bits in disable_plane() (or in crtc_disable()
> for skl+) if the crtc is no longer active.
> 
> And I guess we could still keep the .load_luts() special case
> since guaranteeing the atomicity for that isn't as easy.
> 
> It would mean the pipe alwasy comes up with gamma and csc
> disabled. But since it's all some shade of black anyway I
> guess it shouldn't be a big deal.

Bah. That won't actually work without more hacks for the
case where the crtc was enabled already. I guess I'll just
have to stick an explicit primary->disable_plane() call
into the crtc_enable() path :(

> 
> > 
> > 
> > Matt
> > 
> > > 
> > > For consistency we'll do that on all the plane types. While
> > > that has no real benefits at this time, it'll become useful
> > > when we start to control the pipe gamma/csc enable bits
> > > dynamically when we overhaul the color management code.
> > > 
> > > On pre-g4x there doesn't appear to be any way to gamma
> > > correct the pipe bottom color, but sticking to the same
> > > pattern doesn't hurt. And it'll still help us to do
> > > crtc state readout correctly for the pipe gamma enable
> > > bit for the color management overhaul.
> > > 
> > > An alternative apporach would be to still precompute these
> > > bits into plane_state->ctl, but that would require that we
> > > run through the plane check even when the plane isn't logically
> > > enabled on any crtc. Currently that condition causes us to
> > > short circuit the entire thing and not call ->check_plane().
> > > There would also be some chicken and egg problems with
> > > ->check_plane() vs. crtc color state check that would
> > > requite splitting certain things into multiple steps.
> > > So all in all this seems like the easier route.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------
> > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
> > >  drivers/gpu/drm/i915/intel_sprite.c  |  54 ++++++++---
> > >  3 files changed, 139 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 5dc0de89c49e..a3871db4703b 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3180,28 +3180,38 @@ i9xx_plane_max_stride(struct intel_plane *plane,
> > >  	}
> > >  }
> > >  
> > > +static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > > +	u32 dspcntr = 0;
> > > +
> > > +	dspcntr |= DISPPLANE_GAMMA_ENABLE;
> > > +
> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 5)
> > > +		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> > > +
> > > +	return dspcntr;
> > > +}
> > > +
> > >  static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  			  const struct intel_plane_state *plane_state)
> > >  {
> > >  	struct drm_i915_private *dev_priv =
> > >  		to_i915(plane_state->base.plane->dev);
> > > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > >  	unsigned int rotation = plane_state->base.rotation;
> > >  	u32 dspcntr;
> > >  
> > > -	dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
> > > +	dspcntr = DISPLAY_PLANE_ENABLE;
> > >  
> > >  	if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||
> > >  	    IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > >  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> > >  
> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > -		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> > > -
> > > -	if (INTEL_GEN(dev_priv) < 5)
> > > -		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> > > -
> > >  	switch (fb->format->format) {
> > >  	case DRM_FORMAT_C8:
> > >  		dspcntr |= DISPPLANE_8BPP;
> > > @@ -3329,11 +3339,13 @@ static void i9xx_update_plane(struct intel_plane *plane,
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> > >  	u32 linear_offset;
> > > -	u32 dspcntr = plane_state->ctl;
> > >  	int x = plane_state->color_plane[0].x;
> > >  	int y = plane_state->color_plane[0].y;
> > >  	unsigned long irqflags;
> > >  	u32 dspaddr_offset;
> > > +	u32 dspcntr;
> > > +
> > > +	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
> > >  
> > >  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> > >  
> > > @@ -3393,10 +3405,23 @@ static void i9xx_disable_plane(struct intel_plane *plane,
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> > >  	unsigned long irqflags;
> > > +	u32 dspcntr;
> > > +
> > > +	/*
> > > +	 * DSPCNTR pipe gamma enable on g4x+ and pipe csc
> > > +	 * enable on ilk+ affect the pipe bottom color as
> > > +	 * well, so we must configure them even if the plane
> > > +	 * is disabled.
> > > +	 *
> > > +	 * On pre-g4x there is no way to gamma correct the
> > > +	 * pipe bottom color but we'll keep on doing this
> > > +	 * anyway.
> > > +	 */
> > > +	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
> > >  
> > >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> > >  
> > > -	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
> > > +	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
> > >  	if (INTEL_GEN(dev_priv) >= 4)
> > >  		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
> > >  	else
> > > @@ -3631,6 +3656,20 @@ static u32 cnl_plane_ctl_flip(unsigned int reflect)
> > >  	return 0;
> > >  }
> > >  
> > > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > +	u32 plane_ctl = 0;
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > +		return plane_ctl;
> > > +
> > > +	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> > > +	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
> > > +
> > > +	return plane_ctl;
> > > +}
> > > +
> > >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  		  const struct intel_plane_state *plane_state)
> > >  {
> > > @@ -3645,10 +3684,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  
> > >  	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
> > >  		plane_ctl |= skl_plane_ctl_alpha(plane_state);
> > > -		plane_ctl |=
> > > -			PLANE_CTL_PIPE_GAMMA_ENABLE |
> > > -			PLANE_CTL_PIPE_CSC_ENABLE |
> > > -			PLANE_CTL_PLANE_GAMMA_DISABLE;
> > > +		plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> > >  
> > >  		if (plane_state->base.color_encoding == DRM_COLOR_YCBCR_BT709)
> > >  			plane_ctl |= PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
> > > @@ -3673,19 +3709,27 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  	return plane_ctl;
> > >  }
> > >  
> > > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > +	u32 plane_color_ctl = 0;
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > +		return plane_color_ctl;
> > > +
> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> > > +
> > > +	return plane_color_ctl;
> > > +}
> > > +
> > >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> > >  			const struct intel_plane_state *plane_state)
> > >  {
> > > -	struct drm_i915_private *dev_priv =
> > > -		to_i915(plane_state->base.plane->dev);
> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > >  	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> > >  	u32 plane_color_ctl = 0;
> > >  
> > > -	if (INTEL_GEN(dev_priv) < 11) {
> > > -		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> > > -		plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> > > -	}
> > >  	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > >  	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> > >  
> > > @@ -9867,11 +9911,15 @@ i845_cursor_max_stride(struct intel_plane *plane,
> > >  	return 2048;
> > >  }
> > >  
> > > +static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	return CURSOR_GAMMA_ENABLE;
> > > +}
> > > +
> > >  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> > >  			   const struct intel_plane_state *plane_state)
> > >  {
> > >  	return CURSOR_ENABLE |
> > > -		CURSOR_GAMMA_ENABLE |
> > >  		CURSOR_FORMAT_ARGB |
> > >  		CURSOR_STRIDE(plane_state->color_plane[0].stride);
> > >  }
> > > @@ -9941,7 +9989,9 @@ static void i845_update_cursor(struct intel_plane *plane,
> > >  		unsigned int width = plane_state->base.crtc_w;
> > >  		unsigned int height = plane_state->base.crtc_h;
> > >  
> > > -		cntl = plane_state->ctl;
> > > +		cntl = plane_state->ctl |
> > > +			i845_cursor_ctl_crtc(crtc_state);
> > > +
> > >  		size = (height << 12) | width;
> > >  
> > >  		base = intel_cursor_base(plane_state);
> > > @@ -10006,27 +10056,36 @@ i9xx_cursor_max_stride(struct intel_plane *plane,
> > >  	return plane->base.dev->mode_config.cursor_width * 4;
> > >  }
> > >  
> > > -static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> > > -			   const struct intel_plane_state *plane_state)
> > > +static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > >  {
> > > -	struct drm_i915_private *dev_priv =
> > > -		to_i915(plane_state->base.plane->dev);
> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > >  	u32 cntl = 0;
> > >  
> > > -	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > > -		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> > > +	if (INTEL_GEN(dev_priv) >= 11)
> > > +		return cntl;
> > >  
> > > -	if (INTEL_GEN(dev_priv) <= 10) {
> > > -		cntl |= MCURSOR_GAMMA_ENABLE;
> > > +	cntl |= MCURSOR_GAMMA_ENABLE;
> > >  
> > > -		if (HAS_DDI(dev_priv))
> > > -			cntl |= MCURSOR_PIPE_CSC_ENABLE;
> > > -	}
> > > +	if (HAS_DDI(dev_priv))
> > > +		cntl |= MCURSOR_PIPE_CSC_ENABLE;
> > >  
> > >  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> > >  		cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
> > >  
> > > +	return cntl;
> > > +}
> > > +
> > > +static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> > > +			   const struct intel_plane_state *plane_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv =
> > > +		to_i915(plane_state->base.plane->dev);
> > > +	u32 cntl = 0;
> > > +
> > > +	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> > > +		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> > > +
> > >  	switch (plane_state->base.crtc_w) {
> > >  	case 64:
> > >  		cntl |= MCURSOR_MODE_64_ARGB_AX;
> > > @@ -10151,7 +10210,8 @@ static void i9xx_update_cursor(struct intel_plane *plane,
> > >  	unsigned long irqflags;
> > >  
> > >  	if (plane_state && plane_state->base.visible) {
> > > -		cntl = plane_state->ctl;
> > > +		cntl = plane_state->ctl |
> > > +			i9xx_cursor_ctl_crtc(crtc_state);
> > >  
> > >  		if (plane_state->base.crtc_h != plane_state->base.crtc_w)
> > >  			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h - 1);
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 3b051fdd0fce..88ac42b2d7ed 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1752,9 +1752,10 @@ static inline u32 intel_plane_ggtt_offset(const struct intel_plane_state *state)
> > >  
> > >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> > >  			const struct intel_plane_state *plane_state);
> > > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state *crtc_state);
> > >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> > >  		  const struct intel_plane_state *plane_state);
> > > -u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> > > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state);
> > >  u32 skl_plane_stride(const struct intel_plane_state *plane_state,
> > >  		     int plane);
> > >  int skl_check_plane_surface(struct intel_plane_state *plane_state);
> > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> > > index 8f3982c03925..a45ef98b2f8d 100644
> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> > > @@ -484,9 +484,16 @@ skl_program_plane(struct intel_plane *plane,
> > >  	struct intel_plane *linked = plane_state->linked_plane;
> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> > >  	u8 alpha = plane_state->base.alpha >> 8;
> > > +	u32 plane_color_ctl = 0;
> > >  	unsigned long irqflags;
> > >  	u32 keymsk, keymax;
> > >  
> > > +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> > > +
> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > +		plane_color_ctl = plane_state->color_ctl |
> > > +			glk_plane_color_ctl_crtc(crtc_state);
> > > +
> > >  	/* Sizes are 0 based */
> > >  	src_w--;
> > >  	src_h--;
> > > @@ -533,8 +540,7 @@ skl_program_plane(struct intel_plane *plane,
> > >  	}
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > > -		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> > > -			      plane_state->color_ctl);
> > > +		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id), plane_color_ctl);
> > >  
> > >  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> > >  		icl_program_input_csc(plane, crtc_state, plane_state);
> > > @@ -731,6 +737,11 @@ vlv_update_clrc(const struct intel_plane_state *plane_state)
> > >  		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));
> > >  }
> > >  
> > > +static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	return SP_GAMMA_ENABLE;
> > > +}
> > > +
> > >  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > >  			  const struct intel_plane_state *plane_state)
> > >  {
> > > @@ -739,7 +750,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > >  	u32 sprctl;
> > >  
> > > -	sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
> > > +	sprctl = SP_ENABLE;
> > >  
> > >  	switch (fb->format->format) {
> > >  	case DRM_FORMAT_YUYV:
> > > @@ -806,7 +817,6 @@ vlv_update_plane(struct intel_plane *plane,
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > >  	enum pipe pipe = plane->pipe;
> > >  	enum plane_id plane_id = plane->id;
> > > -	u32 sprctl = plane_state->ctl;
> > >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> > >  	u32 linear_offset;
> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > @@ -817,6 +827,9 @@ vlv_update_plane(struct intel_plane *plane,
> > >  	uint32_t x = plane_state->color_plane[0].x;
> > >  	uint32_t y = plane_state->color_plane[0].y;
> > >  	unsigned long irqflags;
> > > +	u32 sprctl;
> > > +
> > > +	sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
> > >  
> > >  	/* Sizes are 0 based */
> > >  	crtc_w--;
> > > @@ -897,6 +910,19 @@ vlv_plane_get_hw_state(struct intel_plane *plane,
> > >  	return ret;
> > >  }
> > >  
> > > +static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> > > +	u32 sprctl = 0;
> > > +
> > > +	sprctl |= SPRITE_GAMMA_ENABLE;
> > > +
> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > +		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> > > +
> > > +	return sprctl;
> > > +}
> > > +
> > >  static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > >  			  const struct intel_plane_state *plane_state)
> > >  {
> > > @@ -907,14 +933,11 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > >  	u32 sprctl;
> > >  
> > > -	sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
> > > +	sprctl = SPRITE_ENABLE;
> > >  
> > >  	if (IS_IVYBRIDGE(dev_priv))
> > >  		sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> > >  
> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > -		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> > > -
> > >  	switch (fb->format->format) {
> > >  	case DRM_FORMAT_XBGR8888:
> > >  		sprctl |= SPRITE_FORMAT_RGBX888 | SPRITE_RGB_ORDER_RGBX;
> > > @@ -966,7 +989,6 @@ ivb_update_plane(struct intel_plane *plane,
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > >  	enum pipe pipe = plane->pipe;
> > > -	u32 sprctl = plane_state->ctl, sprscale = 0;
> > >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> > >  	u32 linear_offset;
> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > @@ -978,8 +1000,11 @@ ivb_update_plane(struct intel_plane *plane,
> > >  	uint32_t y = plane_state->color_plane[0].y;
> > >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> > >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> > > +	u32 sprctl, sprscale = 0;
> > >  	unsigned long irqflags;
> > >  
> > > +	sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> > > +
> > >  	/* Sizes are 0 based */
> > >  	src_w--;
> > >  	src_h--;
> > > @@ -1074,6 +1099,11 @@ g4x_sprite_max_stride(struct intel_plane *plane,
> > >  	return 16384;
> > >  }
> > >  
> > > +static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	return DVS_GAMMA_ENABLE;
> > > +}
> > > +
> > >  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > >  			  const struct intel_plane_state *plane_state)
> > >  {
> > > @@ -1084,7 +1114,7 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > >  	u32 dvscntr;
> > >  
> > > -	dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
> > > +	dvscntr = DVS_ENABLE;
> > >  
> > >  	if (IS_GEN(dev_priv, 6))
> > >  		dvscntr |= DVS_TRICKLE_FEED_DISABLE;
> > > @@ -1140,7 +1170,6 @@ g4x_update_plane(struct intel_plane *plane,
> > >  {
> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> > >  	enum pipe pipe = plane->pipe;
> > > -	u32 dvscntr = plane_state->ctl, dvsscale = 0;
> > >  	u32 dvssurf_offset = plane_state->color_plane[0].offset;
> > >  	u32 linear_offset;
> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> > > @@ -1152,8 +1181,11 @@ g4x_update_plane(struct intel_plane *plane,
> > >  	uint32_t y = plane_state->color_plane[0].y;
> > >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> > >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> > > +	u32 dvscntr, dvsscale = 0;
> > >  	unsigned long irqflags;
> > >  
> > > +	dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
> > > +
> > >  	/* Sizes are 0 based */
> > >  	src_w--;
> > >  	src_h--;
> > > -- 
> > > 2.19.2
> > > 
> > 
> > -- 
> > Matt Roper
> > Graphics Software Engineer
> > IoTG Platform Enabling & Development
> > Intel Corporation
> > (916) 356-2795
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>-----Original Message-----

>From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

>Ville Syrjälä

>Sent: Tuesday, January 15, 2019 12:42 AM

>To: Roper, Matthew D <matthew.d.roper@intel.com>

>Cc: intel-gfx@lists.freedesktop.org

>Subject: Re: [Intel-gfx] [PATCH 02/13] drm/i915: Split the gamma/csc enable bits

>from the plane_ctl() function

>

>On Mon, Jan 14, 2019 at 07:11:10PM +0200, Ville Syrjälä wrote:

>> On Fri, Jan 11, 2019 at 04:41:37PM -0800, Matt Roper wrote:

>> > On Fri, Jan 11, 2019 at 07:08:12PM +0200, Ville Syrjala wrote:

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

>> > >

>> > > On g4x+ the pipe gamma enable bit for the primary plane affects

>> > > the pipe bottom color as well. The same for the pipe csc enable

>> > > bit on ilk+. Thus we must configure those bits correctly even when

>> > > the primary plane is disabled.

>> >

>> > This is only true for <gen9, right? Starting with gen9, we have

>> > dedicated bits that control this, so I don't think the primay

>> > plane's settings should have any impact when disabled.  I.e., we

>> > also need the bits set in this patch:

>> >

>> >         https://patchwork.freedesktop.org/patch/271109/

>>

>> Yes. I had the same bits (or something similar in a later patch in the

>> series). But we should probably just land your stuff first.

>>

>> >

>> > > To make the feasible let's split those settings from the

>> > > plane_ctl() function into a seprate funciton that we can call from


Typo here.

>> > > the ->disable_plane() hook as well.

>> >

>> > Is calling it from ->disable_plane() enough?  If we just disable the

>> > primary plane, then those bits will remain set while the crtc

>> > remains active.  But if you then disable the whole crtc and

>> > re-enable it again later, won't we have lost the bits at that point?


Just curious, once crtc is re-enabled plane enable will eventually happen
where this can again be enabled. So is this the corner case where crtc gets 
enabled and primary plane is not enabled and it works with just overlays ?

If this is the case, why not just explicitly enable these separately in crtc enable
sequence and not rely on primary plane enabling path to set this for us. What
could be the potential problem if we do this ?

>>

>> Hmm. Yes, probably.

>>

>> Just adding a needs_modeset() check into

>> intel_color_add_affected_planes() (introduced in a later patch) could

>> be one way. But that means the plane control reg won't be updated

>> before the pipe is already active. So we might need a special case for

>> that too.

>>

>> That said, I kinda hate the pipe enable special cases for the color

>> management stuff. So I'm wondering if we could eliminate it all and

>> just rely on the normal pipe+plane update to do things correctly. But

>> to make that consistent we might have to have another special case to

>> disable gamma/etc. prior to enabling the pipe so that it can all be

>> enabled atomically later. Hmm. I suppose that could be achieved by

>> clearing all relevant control bits in disable_plane() (or in

>> crtc_disable() for skl+) if the crtc is no longer active.

>>

>> And I guess we could still keep the .load_luts() special case since

>> guaranteeing the atomicity for that isn't as easy.

>>

>> It would mean the pipe alwasy comes up with gamma and csc disabled.

>> But since it's all some shade of black anyway I guess it shouldn't be

>> a big deal.

>

>Bah. That won't actually work without more hacks for the case where the crtc

>was enabled already. I guess I'll just have to stick an explicit primary-

>>disable_plane() call into the crtc_enable() path :(


>>

>> >

>> >

>> > Matt

>> >

>> > >

>> > > For consistency we'll do that on all the plane types. While that

>> > > has no real benefits at this time, it'll become useful when we

>> > > start to control the pipe gamma/csc enable bits dynamically when

>> > > we overhaul the color management code.

>> > >

>> > > On pre-g4x there doesn't appear to be any way to gamma correct the

>> > > pipe bottom color, but sticking to the same pattern doesn't hurt.

>> > > And it'll still help us to do crtc state readout correctly for the

>> > > pipe gamma enable bit for the color management overhaul.

>> > >

>> > > An alternative apporach would be to still precompute these bits


Typo in approach.

>> > > into plane_state->ctl, but that would require that we run through

>> > > the plane check even when the plane isn't logically enabled on any

>> > > crtc. Currently that condition causes us to short circuit the

>> > > entire thing and not call ->check_plane().

>> > > There would also be some chicken and egg problems with

>> > > ->check_plane() vs. crtc color state check that would

>> > > requite splitting certain things into multiple steps.

>> > > So all in all this seems like the easier route.

>> > >

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

>> > > ---

>> > >  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------

>> > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +-

>> > >  drivers/gpu/drm/i915/intel_sprite.c  |  54 ++++++++---

>> > >  3 files changed, 139 insertions(+), 46 deletions(-)

>> > >

>> > > diff --git a/drivers/gpu/drm/i915/intel_display.c

>> > > b/drivers/gpu/drm/i915/intel_display.c

>> > > index 5dc0de89c49e..a3871db4703b 100644

>> > > --- a/drivers/gpu/drm/i915/intel_display.c

>> > > +++ b/drivers/gpu/drm/i915/intel_display.c

>> > > @@ -3180,28 +3180,38 @@ i9xx_plane_max_stride(struct intel_plane

>*plane,

>> > >  	}

>> > >  }

>> > >

>> > > +static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state) {

>> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);

>> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);

>> > > +	u32 dspcntr = 0;

>> > > +

>> > > +	dspcntr |= DISPPLANE_GAMMA_ENABLE;

>> > > +

>> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

>> > > +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;

>> > > +

>> > > +	if (INTEL_GEN(dev_priv) < 5)

>> > > +		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);

>> > > +

>> > > +	return dspcntr;

>> > > +}

>> > > +

>> > >  static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			  const struct intel_plane_state *plane_state)  {

>> > >  	struct drm_i915_private *dev_priv =

>> > >  		to_i915(plane_state->base.plane->dev);

>> > > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);

>> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;

>> > >  	unsigned int rotation = plane_state->base.rotation;

>> > >  	u32 dspcntr;

>> > >

>> > > -	dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;

>> > > +	dspcntr = DISPLAY_PLANE_ENABLE;

>> > >

>> > >  	if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||

>> > >  	    IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))

>> > >  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;

>> > >

>> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

>> > > -		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;

>> > > -

>> > > -	if (INTEL_GEN(dev_priv) < 5)

>> > > -		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);

>> > > -

>> > >  	switch (fb->format->format) {

>> > >  	case DRM_FORMAT_C8:

>> > >  		dspcntr |= DISPPLANE_8BPP;

>> > > @@ -3329,11 +3339,13 @@ static void i9xx_update_plane(struct

>intel_plane *plane,

>> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

>> > >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;

>> > >  	u32 linear_offset;

>> > > -	u32 dspcntr = plane_state->ctl;

>> > >  	int x = plane_state->color_plane[0].x;

>> > >  	int y = plane_state->color_plane[0].y;

>> > >  	unsigned long irqflags;

>> > >  	u32 dspaddr_offset;

>> > > +	u32 dspcntr;

>> > > +

>> > > +	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);

>> > >

>> > >  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);

>> > >

>> > > @@ -3393,10 +3405,23 @@ static void i9xx_disable_plane(struct

>intel_plane *plane,

>> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

>> > >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;

>> > >  	unsigned long irqflags;

>> > > +	u32 dspcntr;

>> > > +

>> > > +	/*

>> > > +	 * DSPCNTR pipe gamma enable on g4x+ and pipe csc

>> > > +	 * enable on ilk+ affect the pipe bottom color as

>> > > +	 * well, so we must configure them even if the plane

>> > > +	 * is disabled.

>> > > +	 *

>> > > +	 * On pre-g4x there is no way to gamma correct the

>> > > +	 * pipe bottom color but we'll keep on doing this

>> > > +	 * anyway.

>> > > +	 */

>> > > +	dspcntr = i9xx_plane_ctl_crtc(crtc_state);

>> > >

>> > >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);

>> > >

>> > > -	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);

>> > > +	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);

>> > >  	if (INTEL_GEN(dev_priv) >= 4)

>> > >  		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);

>> > >  	else

>> > > @@ -3631,6 +3656,20 @@ static u32 cnl_plane_ctl_flip(unsigned int

>reflect)

>> > >  	return 0;

>> > >  }

>> > >

>> > > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)

>> > > +{

>> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);

>> > > +	u32 plane_ctl = 0;

>> > > +

>> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

>> > > +		return plane_ctl;

>> > > +

>> > > +	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;

>> > > +	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;

>> > > +

>> > > +	return plane_ctl;

>> > > +}

>> > > +

>> > >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,

>> > >  		  const struct intel_plane_state *plane_state)  { @@ -3645,10

>> > > +3684,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state

>> > > *crtc_state,

>> > >

>> > >  	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {

>> > >  		plane_ctl |= skl_plane_ctl_alpha(plane_state);

>> > > -		plane_ctl |=

>> > > -			PLANE_CTL_PIPE_GAMMA_ENABLE |

>> > > -			PLANE_CTL_PIPE_CSC_ENABLE |

>> > > -			PLANE_CTL_PLANE_GAMMA_DISABLE;

>> > > +		plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;

>> > >

>> > >  		if (plane_state->base.color_encoding ==

>DRM_COLOR_YCBCR_BT709)

>> > >  			plane_ctl |=

>PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;

>> > > @@ -3673,19 +3709,27 @@ u32 skl_plane_ctl(const struct intel_crtc_state

>*crtc_state,

>> > >  	return plane_ctl;

>> > >  }

>> > >

>> > > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state) {

>> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);

>> > > +	u32 plane_color_ctl = 0;

>> > > +

>> > > +	if (INTEL_GEN(dev_priv) >= 11)

>> > > +		return plane_color_ctl;

>> > > +

>> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;

>> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;

>> > > +

>> > > +	return plane_color_ctl;

>> > > +}

>> > > +

>> > >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			const struct intel_plane_state *plane_state)  {

>> > > -	struct drm_i915_private *dev_priv =

>> > > -		to_i915(plane_state->base.plane->dev);

>> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;

>> > >  	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);

>> > >  	u32 plane_color_ctl = 0;

>> > >

>> > > -	if (INTEL_GEN(dev_priv) < 11) {

>> > > -		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;

>> > > -		plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;

>> > > -	}

>> > >  	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;

>> > >  	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);

>> > >

>> > > @@ -9867,11 +9911,15 @@ i845_cursor_max_stride(struct intel_plane

>*plane,

>> > >  	return 2048;

>> > >  }

>> > >

>> > > +static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state) {


May be we can make this inline function.

>> > > +	return CURSOR_GAMMA_ENABLE;

>> > > +}

>> > > +

>> > >  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			   const struct intel_plane_state *plane_state)  {

>> > >  	return CURSOR_ENABLE |

>> > > -		CURSOR_GAMMA_ENABLE |

>> > >  		CURSOR_FORMAT_ARGB |

>> > >  		CURSOR_STRIDE(plane_state->color_plane[0].stride);

>> > >  }

>> > > @@ -9941,7 +9989,9 @@ static void i845_update_cursor(struct intel_plane

>*plane,

>> > >  		unsigned int width = plane_state->base.crtc_w;

>> > >  		unsigned int height = plane_state->base.crtc_h;

>> > >

>> > > -		cntl = plane_state->ctl;

>> > > +		cntl = plane_state->ctl |

>> > > +			i845_cursor_ctl_crtc(crtc_state);

>> > > +

>> > >  		size = (height << 12) | width;

>> > >

>> > >  		base = intel_cursor_base(plane_state); @@ -10006,27

>+10056,36

>> > > @@ i9xx_cursor_max_stride(struct intel_plane *plane,

>> > >  	return plane->base.dev->mode_config.cursor_width * 4;  }

>> > >

>> > > -static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,

>> > > -			   const struct intel_plane_state *plane_state)

>> > > +static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state)

>> > >  {

>> > > -	struct drm_i915_private *dev_priv =

>> > > -		to_i915(plane_state->base.plane->dev);

>> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);

>> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);

>> > >  	u32 cntl = 0;

>> > >

>> > > -	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))

>> > > -		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;

>> > > +	if (INTEL_GEN(dev_priv) >= 11)

>> > > +		return cntl;

>> > >

>> > > -	if (INTEL_GEN(dev_priv) <= 10) {

>> > > -		cntl |= MCURSOR_GAMMA_ENABLE;

>> > > +	cntl |= MCURSOR_GAMMA_ENABLE;

>> > >

>> > > -		if (HAS_DDI(dev_priv))

>> > > -			cntl |= MCURSOR_PIPE_CSC_ENABLE;

>> > > -	}

>> > > +	if (HAS_DDI(dev_priv))

>> > > +		cntl |= MCURSOR_PIPE_CSC_ENABLE;

>> > >

>> > >  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))

>> > >  		cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);

>> > >

>> > > +	return cntl;

>> > > +}

>> > > +

>> > > +static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,

>> > > +			   const struct intel_plane_state *plane_state) {

>> > > +	struct drm_i915_private *dev_priv =

>> > > +		to_i915(plane_state->base.plane->dev);

>> > > +	u32 cntl = 0;

>> > > +

>> > > +	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))

>> > > +		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;

>> > > +

>> > >  	switch (plane_state->base.crtc_w) {

>> > >  	case 64:

>> > >  		cntl |= MCURSOR_MODE_64_ARGB_AX; @@ -10151,7 +10210,8

>@@ static

>> > > void i9xx_update_cursor(struct intel_plane *plane,

>> > >  	unsigned long irqflags;

>> > >

>> > >  	if (plane_state && plane_state->base.visible) {

>> > > -		cntl = plane_state->ctl;

>> > > +		cntl = plane_state->ctl |

>> > > +			i9xx_cursor_ctl_crtc(crtc_state);

>> > >

>> > >  		if (plane_state->base.crtc_h != plane_state->base.crtc_w)

>> > >  			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h

>- 1);

>> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h

>> > > b/drivers/gpu/drm/i915/intel_drv.h

>> > > index 3b051fdd0fce..88ac42b2d7ed 100644

>> > > --- a/drivers/gpu/drm/i915/intel_drv.h

>> > > +++ b/drivers/gpu/drm/i915/intel_drv.h

>> > > @@ -1752,9 +1752,10 @@ static inline u32

>> > > intel_plane_ggtt_offset(const struct intel_plane_state *state)

>> > >

>> > >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			const struct intel_plane_state *plane_state);

>> > > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state);

>> > >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,

>> > >  		  const struct intel_plane_state *plane_state);

>> > > -u32 glk_color_ctl(const struct intel_plane_state *plane_state);

>> > > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state);

>> > >  u32 skl_plane_stride(const struct intel_plane_state *plane_state,

>> > >  		     int plane);

>> > >  int skl_check_plane_surface(struct intel_plane_state

>> > > *plane_state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c

>> > > b/drivers/gpu/drm/i915/intel_sprite.c

>> > > index 8f3982c03925..a45ef98b2f8d 100644

>> > > --- a/drivers/gpu/drm/i915/intel_sprite.c

>> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c

>> > > @@ -484,9 +484,16 @@ skl_program_plane(struct intel_plane *plane,

>> > >  	struct intel_plane *linked = plane_state->linked_plane;

>> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;

>> > >  	u8 alpha = plane_state->base.alpha >> 8;

>> > > +	u32 plane_color_ctl = 0;

>> > >  	unsigned long irqflags;

>> > >  	u32 keymsk, keymax;

>> > >

>> > > +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);

>> > > +

>> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

>> > > +		plane_color_ctl = plane_state->color_ctl |

>> > > +			glk_plane_color_ctl_crtc(crtc_state);

>> > > +

>> > >  	/* Sizes are 0 based */

>> > >  	src_w--;

>> > >  	src_h--;

>> > > @@ -533,8 +540,7 @@ skl_program_plane(struct intel_plane *plane,

>> > >  	}

>> > >

>> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))

>> > > -		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),

>> > > -			      plane_state->color_ctl);

>> > > +		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),

>> > > +plane_color_ctl);

>> > >

>> > >  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))

>> > >  		icl_program_input_csc(plane, crtc_state, plane_state); @@

>> > > -731,6 +737,11 @@ vlv_update_clrc(const struct intel_plane_state

>*plane_state)

>> > >  		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));  }

>> > >

>> > > +static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state) {


This as well and other similar ones.

>> > > +	return SP_GAMMA_ENABLE;

>> > > +}

>> > > +

>> > >  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			  const struct intel_plane_state *plane_state)  { @@ -

>739,7

>> > > +750,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state

>*crtc_state,

>> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;

>> > >  	u32 sprctl;

>> > >

>> > > -	sprctl = SP_ENABLE | SP_GAMMA_ENABLE;

>> > > +	sprctl = SP_ENABLE;

>> > >

>> > >  	switch (fb->format->format) {

>> > >  	case DRM_FORMAT_YUYV:

>> > > @@ -806,7 +817,6 @@ vlv_update_plane(struct intel_plane *plane,

>> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

>> > >  	enum pipe pipe = plane->pipe;

>> > >  	enum plane_id plane_id = plane->id;

>> > > -	u32 sprctl = plane_state->ctl;

>> > >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;

>> > >  	u32 linear_offset;

>> > >  	const struct drm_intel_sprite_colorkey *key =

>> > > &plane_state->ckey; @@ -817,6 +827,9 @@ vlv_update_plane(struct

>intel_plane *plane,

>> > >  	uint32_t x = plane_state->color_plane[0].x;

>> > >  	uint32_t y = plane_state->color_plane[0].y;

>> > >  	unsigned long irqflags;

>> > > +	u32 sprctl;

>> > > +

>> > > +	sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);

>> > >

>> > >  	/* Sizes are 0 based */

>> > >  	crtc_w--;

>> > > @@ -897,6 +910,19 @@ vlv_plane_get_hw_state(struct intel_plane *plane,

>> > >  	return ret;

>> > >  }

>> > >

>> > > +static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state) {

>> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);

>> > > +	u32 sprctl = 0;

>> > > +

>> > > +	sprctl |= SPRITE_GAMMA_ENABLE;

>> > > +

>> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

>> > > +		sprctl |= SPRITE_PIPE_CSC_ENABLE;

>> > > +

>> > > +	return sprctl;

>> > > +}

>> > > +

>> > >  static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			  const struct intel_plane_state *plane_state)  { @@ -

>907,14

>> > > +933,11 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state

>*crtc_state,

>> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;

>> > >  	u32 sprctl;

>> > >

>> > > -	sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;

>> > > +	sprctl = SPRITE_ENABLE;

>> > >

>> > >  	if (IS_IVYBRIDGE(dev_priv))

>> > >  		sprctl |= SPRITE_TRICKLE_FEED_DISABLE;

>> > >

>> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

>> > > -		sprctl |= SPRITE_PIPE_CSC_ENABLE;

>> > > -

>> > >  	switch (fb->format->format) {

>> > >  	case DRM_FORMAT_XBGR8888:

>> > >  		sprctl |= SPRITE_FORMAT_RGBX888 |

>SPRITE_RGB_ORDER_RGBX; @@

>> > > -966,7 +989,6 @@ ivb_update_plane(struct intel_plane *plane,  {

>> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

>> > >  	enum pipe pipe = plane->pipe;

>> > > -	u32 sprctl = plane_state->ctl, sprscale = 0;

>> > >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;

>> > >  	u32 linear_offset;

>> > >  	const struct drm_intel_sprite_colorkey *key =

>> > > &plane_state->ckey; @@ -978,8 +1000,11 @@ ivb_update_plane(struct

>intel_plane *plane,

>> > >  	uint32_t y = plane_state->color_plane[0].y;

>> > >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;

>> > >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;

>> > > +	u32 sprctl, sprscale = 0;

>> > >  	unsigned long irqflags;

>> > >

>> > > +	sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);

>> > > +

>> > >  	/* Sizes are 0 based */

>> > >  	src_w--;

>> > >  	src_h--;

>> > > @@ -1074,6 +1099,11 @@ g4x_sprite_max_stride(struct intel_plane

>*plane,

>> > >  	return 16384;

>> > >  }

>> > >

>> > > +static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state

>> > > +*crtc_state) {

>> > > +	return DVS_GAMMA_ENABLE;

>> > > +}

>> > > +

>> > >  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,

>> > >  			  const struct intel_plane_state *plane_state)  { @@ -

>1084,7

>> > > +1114,7 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state

>*crtc_state,

>> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;

>> > >  	u32 dvscntr;

>> > >

>> > > -	dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;

>> > > +	dvscntr = DVS_ENABLE;

>> > >

>> > >  	if (IS_GEN(dev_priv, 6))

>> > >  		dvscntr |= DVS_TRICKLE_FEED_DISABLE; @@ -1140,7 +1170,6

>@@

>> > > g4x_update_plane(struct intel_plane *plane,  {

>> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);

>> > >  	enum pipe pipe = plane->pipe;

>> > > -	u32 dvscntr = plane_state->ctl, dvsscale = 0;

>> > >  	u32 dvssurf_offset = plane_state->color_plane[0].offset;

>> > >  	u32 linear_offset;

>> > >  	const struct drm_intel_sprite_colorkey *key =

>> > > &plane_state->ckey; @@ -1152,8 +1181,11 @@ g4x_update_plane(struct

>intel_plane *plane,

>> > >  	uint32_t y = plane_state->color_plane[0].y;

>> > >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;

>> > >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;

>> > > +	u32 dvscntr, dvsscale = 0;

>> > >  	unsigned long irqflags;

>> > >

>> > > +	dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);

>> > > +

>> > >  	/* Sizes are 0 based */

>> > >  	src_w--;

>> > >  	src_h--;

>> > > --

>> > > 2.19.2

>> > >

>> >

>> > --

>> > Matt Roper

>> > Graphics Software Engineer

>> > IoTG Platform Enabling & Development Intel Corporation

>> > (916) 356-2795

>>

>> --

>> Ville Syrjälä

>> Intel

>> _______________________________________________

>> Intel-gfx mailing list

>> Intel-gfx@lists.freedesktop.org

>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

>

>--

>Ville Syrjälä

>Intel

>_______________________________________________

>Intel-gfx mailing list

>Intel-gfx@lists.freedesktop.org

>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jan 16, 2019 at 05:11:26PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> >Ville Syrjälä
> >Sent: Tuesday, January 15, 2019 12:42 AM
> >To: Roper, Matthew D <matthew.d.roper@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org
> >Subject: Re: [Intel-gfx] [PATCH 02/13] drm/i915: Split the gamma/csc enable bits
> >from the plane_ctl() function
> >
> >On Mon, Jan 14, 2019 at 07:11:10PM +0200, Ville Syrjälä wrote:
> >> On Fri, Jan 11, 2019 at 04:41:37PM -0800, Matt Roper wrote:
> >> > On Fri, Jan 11, 2019 at 07:08:12PM +0200, Ville Syrjala wrote:
> >> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > >
> >> > > On g4x+ the pipe gamma enable bit for the primary plane affects
> >> > > the pipe bottom color as well. The same for the pipe csc enable
> >> > > bit on ilk+. Thus we must configure those bits correctly even when
> >> > > the primary plane is disabled.
> >> >
> >> > This is only true for <gen9, right? Starting with gen9, we have
> >> > dedicated bits that control this, so I don't think the primay
> >> > plane's settings should have any impact when disabled.  I.e., we
> >> > also need the bits set in this patch:
> >> >
> >> >         https://patchwork.freedesktop.org/patch/271109/
> >>
> >> Yes. I had the same bits (or something similar in a later patch in the
> >> series). But we should probably just land your stuff first.
> >>
> >> >
> >> > > To make the feasible let's split those settings from the
> >> > > plane_ctl() function into a seprate funciton that we can call from
> 
> Typo here.
> 
> >> > > the ->disable_plane() hook as well.
> >> >
> >> > Is calling it from ->disable_plane() enough?  If we just disable the
> >> > primary plane, then those bits will remain set while the crtc
> >> > remains active.  But if you then disable the whole crtc and
> >> > re-enable it again later, won't we have lost the bits at that point?
> 
> Just curious, once crtc is re-enabled plane enable will eventually happen
> where this can again be enabled. So is this the corner case where crtc gets 
> enabled and primary plane is not enabled and it works with just overlays ?

Yes.

> 
> If this is the case, why not just explicitly enable these separately in crtc enable
> sequence and not rely on primary plane enabling path to set this for us. What
> could be the potential problem if we do this ?

We have to configure them whenever gamma_enable/csc_enable change. Not
just crtc_enable. So we'd have to duplicate the DSPCNTR reg writes in
color_commit()/something which would also mean doing an ugly RMW since
we wouldn't have the plane state around. Much cleaner to just add the
plane to the state and let the normal codepath handle it.

> 
> >>
> >> Hmm. Yes, probably.
> >>
> >> Just adding a needs_modeset() check into
> >> intel_color_add_affected_planes() (introduced in a later patch) could
> >> be one way. But that means the plane control reg won't be updated
> >> before the pipe is already active. So we might need a special case for
> >> that too.
> >>
> >> That said, I kinda hate the pipe enable special cases for the color
> >> management stuff. So I'm wondering if we could eliminate it all and
> >> just rely on the normal pipe+plane update to do things correctly. But
> >> to make that consistent we might have to have another special case to
> >> disable gamma/etc. prior to enabling the pipe so that it can all be
> >> enabled atomically later. Hmm. I suppose that could be achieved by
> >> clearing all relevant control bits in disable_plane() (or in
> >> crtc_disable() for skl+) if the crtc is no longer active.
> >>
> >> And I guess we could still keep the .load_luts() special case since
> >> guaranteeing the atomicity for that isn't as easy.
> >>
> >> It would mean the pipe alwasy comes up with gamma and csc disabled.
> >> But since it's all some shade of black anyway I guess it shouldn't be
> >> a big deal.
> >
> >Bah. That won't actually work without more hacks for the case where the crtc
> >was enabled already. I guess I'll just have to stick an explicit primary-
> >>disable_plane() call into the crtc_enable() path :(
> 
> >>
> >> >
> >> >
> >> > Matt
> >> >
> >> > >
> >> > > For consistency we'll do that on all the plane types. While that
> >> > > has no real benefits at this time, it'll become useful when we
> >> > > start to control the pipe gamma/csc enable bits dynamically when
> >> > > we overhaul the color management code.
> >> > >
> >> > > On pre-g4x there doesn't appear to be any way to gamma correct the
> >> > > pipe bottom color, but sticking to the same pattern doesn't hurt.
> >> > > And it'll still help us to do crtc state readout correctly for the
> >> > > pipe gamma enable bit for the color management overhaul.
> >> > >
> >> > > An alternative apporach would be to still precompute these bits
> 
> Typo in approach.
> 
> >> > > into plane_state->ctl, but that would require that we run through
> >> > > the plane check even when the plane isn't logically enabled on any
> >> > > crtc. Currently that condition causes us to short circuit the
> >> > > entire thing and not call ->check_plane().
> >> > > There would also be some chicken and egg problems with
> >> > > ->check_plane() vs. crtc color state check that would
> >> > > requite splitting certain things into multiple steps.
> >> > > So all in all this seems like the easier route.
> >> > >
> >> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > > ---
> >> > >  drivers/gpu/drm/i915/intel_display.c | 128 ++++++++++++++++++++-------
> >> > >  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
> >> > >  drivers/gpu/drm/i915/intel_sprite.c  |  54 ++++++++---
> >> > >  3 files changed, 139 insertions(+), 46 deletions(-)
> >> > >
> >> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> > > b/drivers/gpu/drm/i915/intel_display.c
> >> > > index 5dc0de89c49e..a3871db4703b 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > > @@ -3180,28 +3180,38 @@ i9xx_plane_max_stride(struct intel_plane
> >*plane,
> >> > >  	}
> >> > >  }
> >> > >
> >> > > +static u32 i9xx_plane_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state) {
> >> > > +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > > +	u32 dspcntr = 0;
> >> > > +
> >> > > +	dspcntr |= DISPPLANE_GAMMA_ENABLE;
> >> > > +
> >> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >> > > +		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> >> > > +
> >> > > +	if (INTEL_GEN(dev_priv) < 5)
> >> > > +		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> >> > > +
> >> > > +	return dspcntr;
> >> > > +}
> >> > > +
> >> > >  static u32 i9xx_plane_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			  const struct intel_plane_state *plane_state)  {
> >> > >  	struct drm_i915_private *dev_priv =
> >> > >  		to_i915(plane_state->base.plane->dev);
> >> > > -	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> >> > >  	unsigned int rotation = plane_state->base.rotation;
> >> > >  	u32 dspcntr;
> >> > >
> >> > > -	dspcntr = DISPLAY_PLANE_ENABLE | DISPPLANE_GAMMA_ENABLE;
> >> > > +	dspcntr = DISPLAY_PLANE_ENABLE;
> >> > >
> >> > >  	if (IS_G4X(dev_priv) || IS_GEN(dev_priv, 5) ||
> >> > >  	    IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> >> > >  		dspcntr |= DISPPLANE_TRICKLE_FEED_DISABLE;
> >> > >
> >> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >> > > -		dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
> >> > > -
> >> > > -	if (INTEL_GEN(dev_priv) < 5)
> >> > > -		dspcntr |= DISPPLANE_SEL_PIPE(crtc->pipe);
> >> > > -
> >> > >  	switch (fb->format->format) {
> >> > >  	case DRM_FORMAT_C8:
> >> > >  		dspcntr |= DISPPLANE_8BPP;
> >> > > @@ -3329,11 +3339,13 @@ static void i9xx_update_plane(struct
> >intel_plane *plane,
> >> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >> > >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> >> > >  	u32 linear_offset;
> >> > > -	u32 dspcntr = plane_state->ctl;
> >> > >  	int x = plane_state->color_plane[0].x;
> >> > >  	int y = plane_state->color_plane[0].y;
> >> > >  	unsigned long irqflags;
> >> > >  	u32 dspaddr_offset;
> >> > > +	u32 dspcntr;
> >> > > +
> >> > > +	dspcntr = plane_state->ctl | i9xx_plane_ctl_crtc(crtc_state);
> >> > >
> >> > >  	linear_offset = intel_fb_xy_to_linear(x, y, plane_state, 0);
> >> > >
> >> > > @@ -3393,10 +3405,23 @@ static void i9xx_disable_plane(struct
> >intel_plane *plane,
> >> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >> > >  	enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> >> > >  	unsigned long irqflags;
> >> > > +	u32 dspcntr;
> >> > > +
> >> > > +	/*
> >> > > +	 * DSPCNTR pipe gamma enable on g4x+ and pipe csc
> >> > > +	 * enable on ilk+ affect the pipe bottom color as
> >> > > +	 * well, so we must configure them even if the plane
> >> > > +	 * is disabled.
> >> > > +	 *
> >> > > +	 * On pre-g4x there is no way to gamma correct the
> >> > > +	 * pipe bottom color but we'll keep on doing this
> >> > > +	 * anyway.
> >> > > +	 */
> >> > > +	dspcntr = i9xx_plane_ctl_crtc(crtc_state);
> >> > >
> >> > >  	spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> >> > >
> >> > > -	I915_WRITE_FW(DSPCNTR(i9xx_plane), 0);
> >> > > +	I915_WRITE_FW(DSPCNTR(i9xx_plane), dspcntr);
> >> > >  	if (INTEL_GEN(dev_priv) >= 4)
> >> > >  		I915_WRITE_FW(DSPSURF(i9xx_plane), 0);
> >> > >  	else
> >> > > @@ -3631,6 +3656,20 @@ static u32 cnl_plane_ctl_flip(unsigned int
> >reflect)
> >> > >  	return 0;
> >> > >  }
> >> > >
> >> > > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state *crtc_state)
> >> > > +{
> >> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >> > > +	u32 plane_ctl = 0;
> >> > > +
> >> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >> > > +		return plane_ctl;
> >> > > +
> >> > > +	plane_ctl |= PLANE_CTL_PIPE_GAMMA_ENABLE;
> >> > > +	plane_ctl |= PLANE_CTL_PIPE_CSC_ENABLE;
> >> > > +
> >> > > +	return plane_ctl;
> >> > > +}
> >> > > +
> >> > >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  		  const struct intel_plane_state *plane_state)  { @@ -3645,10
> >> > > +3684,7 @@ u32 skl_plane_ctl(const struct intel_crtc_state
> >> > > *crtc_state,
> >> > >
> >> > >  	if (INTEL_GEN(dev_priv) < 10 && !IS_GEMINILAKE(dev_priv)) {
> >> > >  		plane_ctl |= skl_plane_ctl_alpha(plane_state);
> >> > > -		plane_ctl |=
> >> > > -			PLANE_CTL_PIPE_GAMMA_ENABLE |
> >> > > -			PLANE_CTL_PIPE_CSC_ENABLE |
> >> > > -			PLANE_CTL_PLANE_GAMMA_DISABLE;
> >> > > +		plane_ctl |= PLANE_CTL_PLANE_GAMMA_DISABLE;
> >> > >
> >> > >  		if (plane_state->base.color_encoding ==
> >DRM_COLOR_YCBCR_BT709)
> >> > >  			plane_ctl |=
> >PLANE_CTL_YUV_TO_RGB_CSC_FORMAT_BT709;
> >> > > @@ -3673,19 +3709,27 @@ u32 skl_plane_ctl(const struct intel_crtc_state
> >*crtc_state,
> >> > >  	return plane_ctl;
> >> > >  }
> >> > >
> >> > > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state) {
> >> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >> > > +	u32 plane_color_ctl = 0;
> >> > > +
> >> > > +	if (INTEL_GEN(dev_priv) >= 11)
> >> > > +		return plane_color_ctl;
> >> > > +
> >> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> >> > > +	plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> >> > > +
> >> > > +	return plane_color_ctl;
> >> > > +}
> >> > > +
> >> > >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			const struct intel_plane_state *plane_state)  {
> >> > > -	struct drm_i915_private *dev_priv =
> >> > > -		to_i915(plane_state->base.plane->dev);
> >> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> >> > >  	struct intel_plane *plane = to_intel_plane(plane_state->base.plane);
> >> > >  	u32 plane_color_ctl = 0;
> >> > >
> >> > > -	if (INTEL_GEN(dev_priv) < 11) {
> >> > > -		plane_color_ctl |= PLANE_COLOR_PIPE_GAMMA_ENABLE;
> >> > > -		plane_color_ctl |= PLANE_COLOR_PIPE_CSC_ENABLE;
> >> > > -	}
> >> > >  	plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> >> > >  	plane_color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> >> > >
> >> > > @@ -9867,11 +9911,15 @@ i845_cursor_max_stride(struct intel_plane
> >*plane,
> >> > >  	return 2048;
> >> > >  }
> >> > >
> >> > > +static u32 i845_cursor_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state) {
> 
> May be we can make this inline function.

The compiler will inline if it deems it beneficial.

> 
> >> > > +	return CURSOR_GAMMA_ENABLE;
> >> > > +}
> >> > > +
> >> > >  static u32 i845_cursor_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			   const struct intel_plane_state *plane_state)  {
> >> > >  	return CURSOR_ENABLE |
> >> > > -		CURSOR_GAMMA_ENABLE |
> >> > >  		CURSOR_FORMAT_ARGB |
> >> > >  		CURSOR_STRIDE(plane_state->color_plane[0].stride);
> >> > >  }
> >> > > @@ -9941,7 +9989,9 @@ static void i845_update_cursor(struct intel_plane
> >*plane,
> >> > >  		unsigned int width = plane_state->base.crtc_w;
> >> > >  		unsigned int height = plane_state->base.crtc_h;
> >> > >
> >> > > -		cntl = plane_state->ctl;
> >> > > +		cntl = plane_state->ctl |
> >> > > +			i845_cursor_ctl_crtc(crtc_state);
> >> > > +
> >> > >  		size = (height << 12) | width;
> >> > >
> >> > >  		base = intel_cursor_base(plane_state); @@ -10006,27
> >+10056,36
> >> > > @@ i9xx_cursor_max_stride(struct intel_plane *plane,
> >> > >  	return plane->base.dev->mode_config.cursor_width * 4;  }
> >> > >
> >> > > -static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> >> > > -			   const struct intel_plane_state *plane_state)
> >> > > +static u32 i9xx_cursor_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state)
> >> > >  {
> >> > > -	struct drm_i915_private *dev_priv =
> >> > > -		to_i915(plane_state->base.plane->dev);
> >> > >  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> >> > > +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >> > >  	u32 cntl = 0;
> >> > >
> >> > > -	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> >> > > -		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> >> > > +	if (INTEL_GEN(dev_priv) >= 11)
> >> > > +		return cntl;
> >> > >
> >> > > -	if (INTEL_GEN(dev_priv) <= 10) {
> >> > > -		cntl |= MCURSOR_GAMMA_ENABLE;
> >> > > +	cntl |= MCURSOR_GAMMA_ENABLE;
> >> > >
> >> > > -		if (HAS_DDI(dev_priv))
> >> > > -			cntl |= MCURSOR_PIPE_CSC_ENABLE;
> >> > > -	}
> >> > > +	if (HAS_DDI(dev_priv))
> >> > > +		cntl |= MCURSOR_PIPE_CSC_ENABLE;
> >> > >
> >> > >  	if (INTEL_GEN(dev_priv) < 5 && !IS_G4X(dev_priv))
> >> > >  		cntl |= MCURSOR_PIPE_SELECT(crtc->pipe);
> >> > >
> >> > > +	return cntl;
> >> > > +}
> >> > > +
> >> > > +static u32 i9xx_cursor_ctl(const struct intel_crtc_state *crtc_state,
> >> > > +			   const struct intel_plane_state *plane_state) {
> >> > > +	struct drm_i915_private *dev_priv =
> >> > > +		to_i915(plane_state->base.plane->dev);
> >> > > +	u32 cntl = 0;
> >> > > +
> >> > > +	if (IS_GEN(dev_priv, 6) || IS_IVYBRIDGE(dev_priv))
> >> > > +		cntl |= MCURSOR_TRICKLE_FEED_DISABLE;
> >> > > +
> >> > >  	switch (plane_state->base.crtc_w) {
> >> > >  	case 64:
> >> > >  		cntl |= MCURSOR_MODE_64_ARGB_AX; @@ -10151,7 +10210,8
> >@@ static
> >> > > void i9xx_update_cursor(struct intel_plane *plane,
> >> > >  	unsigned long irqflags;
> >> > >
> >> > >  	if (plane_state && plane_state->base.visible) {
> >> > > -		cntl = plane_state->ctl;
> >> > > +		cntl = plane_state->ctl |
> >> > > +			i9xx_cursor_ctl_crtc(crtc_state);
> >> > >
> >> > >  		if (plane_state->base.crtc_h != plane_state->base.crtc_w)
> >> > >  			fbc_ctl = CUR_FBC_CTL_EN | (plane_state->base.crtc_h
> >- 1);
> >> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> > > b/drivers/gpu/drm/i915/intel_drv.h
> >> > > index 3b051fdd0fce..88ac42b2d7ed 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> > > @@ -1752,9 +1752,10 @@ static inline u32
> >> > > intel_plane_ggtt_offset(const struct intel_plane_state *state)
> >> > >
> >> > >  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			const struct intel_plane_state *plane_state);
> >> > > +u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state);
> >> > >  u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  		  const struct intel_plane_state *plane_state);
> >> > > -u32 glk_color_ctl(const struct intel_plane_state *plane_state);
> >> > > +u32 skl_plane_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state);
> >> > >  u32 skl_plane_stride(const struct intel_plane_state *plane_state,
> >> > >  		     int plane);
> >> > >  int skl_check_plane_surface(struct intel_plane_state
> >> > > *plane_state); diff --git a/drivers/gpu/drm/i915/intel_sprite.c
> >> > > b/drivers/gpu/drm/i915/intel_sprite.c
> >> > > index 8f3982c03925..a45ef98b2f8d 100644
> >> > > --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> > > +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> > > @@ -484,9 +484,16 @@ skl_program_plane(struct intel_plane *plane,
> >> > >  	struct intel_plane *linked = plane_state->linked_plane;
> >> > >  	const struct drm_framebuffer *fb = plane_state->base.fb;
> >> > >  	u8 alpha = plane_state->base.alpha >> 8;
> >> > > +	u32 plane_color_ctl = 0;
> >> > >  	unsigned long irqflags;
> >> > >  	u32 keymsk, keymax;
> >> > >
> >> > > +	plane_ctl |= skl_plane_ctl_crtc(crtc_state);
> >> > > +
> >> > > +	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >> > > +		plane_color_ctl = plane_state->color_ctl |
> >> > > +			glk_plane_color_ctl_crtc(crtc_state);
> >> > > +
> >> > >  	/* Sizes are 0 based */
> >> > >  	src_w--;
> >> > >  	src_h--;
> >> > > @@ -533,8 +540,7 @@ skl_program_plane(struct intel_plane *plane,
> >> > >  	}
> >> > >
> >> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >> > > -		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >> > > -			      plane_state->color_ctl);
> >> > > +		I915_WRITE_FW(PLANE_COLOR_CTL(pipe, plane_id),
> >> > > +plane_color_ctl);
> >> > >
> >> > >  	if (fb->format->is_yuv && icl_is_hdr_plane(plane))
> >> > >  		icl_program_input_csc(plane, crtc_state, plane_state); @@
> >> > > -731,6 +737,11 @@ vlv_update_clrc(const struct intel_plane_state
> >*plane_state)
> >> > >  		      SP_SH_SIN(sh_sin) | SP_SH_COS(sh_cos));  }
> >> > >
> >> > > +static u32 vlv_sprite_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state) {
> 
> This as well and other similar ones.
> 
> >> > > +	return SP_GAMMA_ENABLE;
> >> > > +}
> >> > > +
> >> > >  static u32 vlv_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			  const struct intel_plane_state *plane_state)  { @@ -
> >739,7
> >> > > +750,7 @@ static u32 vlv_sprite_ctl(const struct intel_crtc_state
> >*crtc_state,
> >> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >> > >  	u32 sprctl;
> >> > >
> >> > > -	sprctl = SP_ENABLE | SP_GAMMA_ENABLE;
> >> > > +	sprctl = SP_ENABLE;
> >> > >
> >> > >  	switch (fb->format->format) {
> >> > >  	case DRM_FORMAT_YUYV:
> >> > > @@ -806,7 +817,6 @@ vlv_update_plane(struct intel_plane *plane,
> >> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >> > >  	enum pipe pipe = plane->pipe;
> >> > >  	enum plane_id plane_id = plane->id;
> >> > > -	u32 sprctl = plane_state->ctl;
> >> > >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> >> > >  	u32 linear_offset;
> >> > >  	const struct drm_intel_sprite_colorkey *key =
> >> > > &plane_state->ckey; @@ -817,6 +827,9 @@ vlv_update_plane(struct
> >intel_plane *plane,
> >> > >  	uint32_t x = plane_state->color_plane[0].x;
> >> > >  	uint32_t y = plane_state->color_plane[0].y;
> >> > >  	unsigned long irqflags;
> >> > > +	u32 sprctl;
> >> > > +
> >> > > +	sprctl = plane_state->ctl | vlv_sprite_ctl_crtc(crtc_state);
> >> > >
> >> > >  	/* Sizes are 0 based */
> >> > >  	crtc_w--;
> >> > > @@ -897,6 +910,19 @@ vlv_plane_get_hw_state(struct intel_plane *plane,
> >> > >  	return ret;
> >> > >  }
> >> > >
> >> > > +static u32 ivb_sprite_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state) {
> >> > > +	struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev);
> >> > > +	u32 sprctl = 0;
> >> > > +
> >> > > +	sprctl |= SPRITE_GAMMA_ENABLE;
> >> > > +
> >> > > +	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >> > > +		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> >> > > +
> >> > > +	return sprctl;
> >> > > +}
> >> > > +
> >> > >  static u32 ivb_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			  const struct intel_plane_state *plane_state)  { @@ -
> >907,14
> >> > > +933,11 @@ static u32 ivb_sprite_ctl(const struct intel_crtc_state
> >*crtc_state,
> >> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >> > >  	u32 sprctl;
> >> > >
> >> > > -	sprctl = SPRITE_ENABLE | SPRITE_GAMMA_ENABLE;
> >> > > +	sprctl = SPRITE_ENABLE;
> >> > >
> >> > >  	if (IS_IVYBRIDGE(dev_priv))
> >> > >  		sprctl |= SPRITE_TRICKLE_FEED_DISABLE;
> >> > >
> >> > > -	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >> > > -		sprctl |= SPRITE_PIPE_CSC_ENABLE;
> >> > > -
> >> > >  	switch (fb->format->format) {
> >> > >  	case DRM_FORMAT_XBGR8888:
> >> > >  		sprctl |= SPRITE_FORMAT_RGBX888 |
> >SPRITE_RGB_ORDER_RGBX; @@
> >> > > -966,7 +989,6 @@ ivb_update_plane(struct intel_plane *plane,  {
> >> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >> > >  	enum pipe pipe = plane->pipe;
> >> > > -	u32 sprctl = plane_state->ctl, sprscale = 0;
> >> > >  	u32 sprsurf_offset = plane_state->color_plane[0].offset;
> >> > >  	u32 linear_offset;
> >> > >  	const struct drm_intel_sprite_colorkey *key =
> >> > > &plane_state->ckey; @@ -978,8 +1000,11 @@ ivb_update_plane(struct
> >intel_plane *plane,
> >> > >  	uint32_t y = plane_state->color_plane[0].y;
> >> > >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >> > >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >> > > +	u32 sprctl, sprscale = 0;
> >> > >  	unsigned long irqflags;
> >> > >
> >> > > +	sprctl = plane_state->ctl | ivb_sprite_ctl_crtc(crtc_state);
> >> > > +
> >> > >  	/* Sizes are 0 based */
> >> > >  	src_w--;
> >> > >  	src_h--;
> >> > > @@ -1074,6 +1099,11 @@ g4x_sprite_max_stride(struct intel_plane
> >*plane,
> >> > >  	return 16384;
> >> > >  }
> >> > >
> >> > > +static u32 g4x_sprite_ctl_crtc(const struct intel_crtc_state
> >> > > +*crtc_state) {
> >> > > +	return DVS_GAMMA_ENABLE;
> >> > > +}
> >> > > +
> >> > >  static u32 g4x_sprite_ctl(const struct intel_crtc_state *crtc_state,
> >> > >  			  const struct intel_plane_state *plane_state)  { @@ -
> >1084,7
> >> > > +1114,7 @@ static u32 g4x_sprite_ctl(const struct intel_crtc_state
> >*crtc_state,
> >> > >  	const struct drm_intel_sprite_colorkey *key = &plane_state->ckey;
> >> > >  	u32 dvscntr;
> >> > >
> >> > > -	dvscntr = DVS_ENABLE | DVS_GAMMA_ENABLE;
> >> > > +	dvscntr = DVS_ENABLE;
> >> > >
> >> > >  	if (IS_GEN(dev_priv, 6))
> >> > >  		dvscntr |= DVS_TRICKLE_FEED_DISABLE; @@ -1140,7 +1170,6
> >@@
> >> > > g4x_update_plane(struct intel_plane *plane,  {
> >> > >  	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
> >> > >  	enum pipe pipe = plane->pipe;
> >> > > -	u32 dvscntr = plane_state->ctl, dvsscale = 0;
> >> > >  	u32 dvssurf_offset = plane_state->color_plane[0].offset;
> >> > >  	u32 linear_offset;
> >> > >  	const struct drm_intel_sprite_colorkey *key =
> >> > > &plane_state->ckey; @@ -1152,8 +1181,11 @@ g4x_update_plane(struct
> >intel_plane *plane,
> >> > >  	uint32_t y = plane_state->color_plane[0].y;
> >> > >  	uint32_t src_w = drm_rect_width(&plane_state->base.src) >> 16;
> >> > >  	uint32_t src_h = drm_rect_height(&plane_state->base.src) >> 16;
> >> > > +	u32 dvscntr, dvsscale = 0;
> >> > >  	unsigned long irqflags;
> >> > >
> >> > > +	dvscntr = plane_state->ctl | g4x_sprite_ctl_crtc(crtc_state);
> >> > > +
> >> > >  	/* Sizes are 0 based */
> >> > >  	src_w--;
> >> > >  	src_h--;
> >> > > --
> >> > > 2.19.2
> >> > >
> >> >
> >> > --
> >> > Matt Roper
> >> > Graphics Software Engineer
> >> > IoTG Platform Enabling & Development Intel Corporation
> >> > (916) 356-2795
> >>
> >> --
> >> Ville Syrjälä
> >> Intel
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >--
> >Ville Syrjälä
> >Intel
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx