public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Always apply cursor width changes
@ 2014-04-12 15:20 Chris Wilson
  2014-04-12 17:39 ` Daniel Vetter
  0 siblings, 1 reply; 2+ messages in thread
From: Chris Wilson @ 2014-04-12 15:20 UTC (permalink / raw)
  To: intel-gfx

It is possible for userspace to create a big object large enough for a
256x256, and then switch over to using it as a 64x64 cursor. This
requires the cursor update routines to check for a change in width on
every update, rather than just when the cursor is originally enabled.

This also fixes an issue with 845g/865g which cannot change the base
address of the cursor whilst it is active.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 106 +++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |   3 +-
 3 files changed, 53 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4c5de60e5757..7799b1394741 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2450,7 +2450,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
 
 			active = cursor_position(dev, crtc->pipe, &x, &y);
 			seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
-				   yesno(crtc->cursor_visible),
+				   yesno(crtc->cursor_base),
 				   x, y, crtc->cursor_addr,
 				   yesno(active));
 		}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5e9cb9383f05..92d152694464 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7564,29 +7564,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	bool visible = base != 0;
-	u32 cntl;
+	uint32_t cntl;
 
-	if (intel_crtc->cursor_visible == visible)
-		return;
-
-	cntl = I915_READ(_CURACNTR);
-	if (visible) {
+	if (base != intel_crtc->cursor_base) {
 		/* On these chipsets we can only modify the base whilst
 		 * the cursor is disabled.
 		 */
+		if (intel_crtc->cursor_cntl) {
+			I915_WRITE(_CURACNTR, 0);
+			POSTING_READ(_CURACNTR);
+			intel_crtc->cursor_cntl = 0;
+		}
+
 		I915_WRITE(_CURABASE, base);
+		POSTING_READ(_CURABASE);
+	}
 
-		cntl &= ~(CURSOR_FORMAT_MASK);
-		/* XXX width must be 64, stride 256 => 0x00 << 28 */
-		cntl |= CURSOR_ENABLE |
+	/* XXX width must be 64, stride 256 => 0x00 << 28 */
+	cntl = 0;
+	if (base)
+		cntl = (CURSOR_ENABLE |
 			CURSOR_GAMMA_ENABLE |
-			CURSOR_FORMAT_ARGB;
-	} else
-		cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
-	I915_WRITE(_CURACNTR, cntl);
-
-	intel_crtc->cursor_visible = visible;
+			CURSOR_FORMAT_ARGB);
+	if (intel_crtc->cursor_cntl != cntl) {
+		I915_WRITE(_CURACNTR, cntl);
+		POSTING_READ(_CURACNTR);
+		intel_crtc->cursor_cntl = cntl;
+	}
 }
 
 static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
@@ -7595,16 +7599,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
-	bool visible = base != 0;
-
-	if (intel_crtc->cursor_visible != visible) {
-		int16_t width = intel_crtc->cursor_width;
-		uint32_t cntl = I915_READ(CURCNTR(pipe));
-		if (base) {
-			cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
-			cntl |= MCURSOR_GAMMA_ENABLE;
+	uint32_t cntl;
 
-			switch (width) {
+	cntl = 0;
+	if (base) {
+		cntl = MCURSOR_GAMMA_ENABLE;
+		switch (intel_crtc->cursor_width) {
 			case 64:
 				cntl |= CURSOR_MODE_64_ARGB_AX;
 				break;
@@ -7617,18 +7617,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
 			default:
 				WARN_ON(1);
 				return;
-			}
-			cntl |= pipe << 28; /* Connect to correct pipe */
-		} else {
-			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
-			cntl |= CURSOR_MODE_DISABLE;
 		}
+		cntl |= pipe << 28; /* Connect to correct pipe */
+	}
+	if (intel_crtc->cursor_cntl != cntl) {
 		I915_WRITE(CURCNTR(pipe), cntl);
-
-		intel_crtc->cursor_visible = visible;
+		POSTING_READ(CURCNTR(pipe));
+		intel_crtc->cursor_cntl = cntl;
 	}
+
 	/* and commit changes on next vblank */
-	POSTING_READ(CURCNTR(pipe));
 	I915_WRITE(CURBASE(pipe), base);
 	POSTING_READ(CURBASE(pipe));
 }
@@ -7639,15 +7637,12 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
-	bool visible = base != 0;
-
-	if (intel_crtc->cursor_visible != visible) {
-		int16_t width = intel_crtc->cursor_width;
-		uint32_t cntl = I915_READ(CURCNTR_IVB(pipe));
-		if (base) {
-			cntl &= ~CURSOR_MODE;
-			cntl |= MCURSOR_GAMMA_ENABLE;
-			switch (width) {
+	uint32_t cntl;
+
+	cntl = 0;
+	if (base) {
+		cntl = MCURSOR_GAMMA_ENABLE;
+		switch (intel_crtc->cursor_width) {
 			case 64:
 				cntl |= CURSOR_MODE_64_ARGB_AX;
 				break;
@@ -7660,21 +7655,18 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
 			default:
 				WARN_ON(1);
 				return;
-			}
-		} else {
-			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
-			cntl |= CURSOR_MODE_DISABLE;
-		}
-		if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
-			cntl |= CURSOR_PIPE_CSC_ENABLE;
-			cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
 		}
-		I915_WRITE(CURCNTR_IVB(pipe), cntl);
+	}
+	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+		cntl |= CURSOR_PIPE_CSC_ENABLE;
 
-		intel_crtc->cursor_visible = visible;
+	if (intel_crtc->cursor_cntl != cntl) {
+		I915_WRITE(CURCNTR_IVB(pipe), cntl);
+		POSTING_READ(CURCNTR_IVB(pipe));
+		intel_crtc->cursor_cntl = cntl;
 	}
+
 	/* and commit changes on next vblank */
-	POSTING_READ(CURCNTR_IVB(pipe));
 	I915_WRITE(CURBASE_IVB(pipe), base);
 	POSTING_READ(CURBASE_IVB(pipe));
 }
@@ -7690,7 +7682,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	int x = intel_crtc->cursor_x;
 	int y = intel_crtc->cursor_y;
 	u32 base = 0, pos = 0;
-	bool visible;
 
 	if (on)
 		base = intel_crtc->cursor_addr;
@@ -7719,8 +7710,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 	}
 	pos |= y << CURSOR_Y_SHIFT;
 
-	visible = base != 0;
-	if (!visible && !intel_crtc->cursor_visible)
+	if (base == 0 && intel_crtc->cursor_base == 0)
 		return;
 
 	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
@@ -7733,6 +7723,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
 		else
 			i9xx_update_cursor(crtc, base);
 	}
+	intel_crtc->cursor_base = base;
 }
 
 static int intel_crtc_cursor_set(struct drm_crtc *crtc,
@@ -10681,6 +10672,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 		intel_crtc->plane = !pipe;
 	}
 
+	intel_crtc->cursor_base = ~0;
+	intel_crtc->cursor_cntl = ~0;
+
 	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
 	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
 	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2da2efcfbd47..c6ab4ac4063a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -379,7 +379,8 @@ struct intel_crtc {
 	uint32_t cursor_addr;
 	int16_t cursor_x, cursor_y;
 	int16_t cursor_width, cursor_height;
-	bool cursor_visible;
+	uint32_t cursor_cntl;
+	uint32_t cursor_base;
 
 	struct intel_plane_config plane_config;
 	struct intel_crtc_config config;
-- 
1.9.2

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] drm/i915: Always apply cursor width changes
  2014-04-12 15:20 [PATCH] drm/i915: Always apply cursor width changes Chris Wilson
@ 2014-04-12 17:39 ` Daniel Vetter
  0 siblings, 0 replies; 2+ messages in thread
From: Daniel Vetter @ 2014-04-12 17:39 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Sat, Apr 12, 2014 at 04:20:49PM +0100, Chris Wilson wrote:
> It is possible for userspace to create a big object large enough for a
> 256x256, and then switch over to using it as a 64x64 cursor. This
> requires the cursor update routines to check for a change in width on
> every update, rather than just when the cursor is originally enabled.
> 
> This also fixes an issue with 845g/865g which cannot change the base
> address of the cursor whilst it is active.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Hm, can we have a testcase for this please? I'm thinking of a black cursor
on black fb at full size, then using the same cursor at smaller size and
filling out the oversize with white. If the CRCs don't match we know that
we scan out a bit of white.
-Daniel



> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 106 +++++++++++++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |   3 +-
>  3 files changed, 53 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4c5de60e5757..7799b1394741 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2450,7 +2450,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  
>  			active = cursor_position(dev, crtc->pipe, &x, &y);
>  			seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
> -				   yesno(crtc->cursor_visible),
> +				   yesno(crtc->cursor_base),
>  				   x, y, crtc->cursor_addr,
>  				   yesno(active));
>  		}
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5e9cb9383f05..92d152694464 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7564,29 +7564,33 @@ static void i845_update_cursor(struct drm_crtc *crtc, u32 base)
>  	struct drm_device *dev = crtc->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	bool visible = base != 0;
> -	u32 cntl;
> +	uint32_t cntl;
>  
> -	if (intel_crtc->cursor_visible == visible)
> -		return;
> -
> -	cntl = I915_READ(_CURACNTR);
> -	if (visible) {
> +	if (base != intel_crtc->cursor_base) {
>  		/* On these chipsets we can only modify the base whilst
>  		 * the cursor is disabled.
>  		 */
> +		if (intel_crtc->cursor_cntl) {
> +			I915_WRITE(_CURACNTR, 0);
> +			POSTING_READ(_CURACNTR);
> +			intel_crtc->cursor_cntl = 0;
> +		}
> +
>  		I915_WRITE(_CURABASE, base);
> +		POSTING_READ(_CURABASE);
> +	}
>  
> -		cntl &= ~(CURSOR_FORMAT_MASK);
> -		/* XXX width must be 64, stride 256 => 0x00 << 28 */
> -		cntl |= CURSOR_ENABLE |
> +	/* XXX width must be 64, stride 256 => 0x00 << 28 */
> +	cntl = 0;
> +	if (base)
> +		cntl = (CURSOR_ENABLE |
>  			CURSOR_GAMMA_ENABLE |
> -			CURSOR_FORMAT_ARGB;
> -	} else
> -		cntl &= ~(CURSOR_ENABLE | CURSOR_GAMMA_ENABLE);
> -	I915_WRITE(_CURACNTR, cntl);
> -
> -	intel_crtc->cursor_visible = visible;
> +			CURSOR_FORMAT_ARGB);
> +	if (intel_crtc->cursor_cntl != cntl) {
> +		I915_WRITE(_CURACNTR, cntl);
> +		POSTING_READ(_CURACNTR);
> +		intel_crtc->cursor_cntl = cntl;
> +	}
>  }
>  
>  static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
> @@ -7595,16 +7599,12 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	bool visible = base != 0;
> -
> -	if (intel_crtc->cursor_visible != visible) {
> -		int16_t width = intel_crtc->cursor_width;
> -		uint32_t cntl = I915_READ(CURCNTR(pipe));
> -		if (base) {
> -			cntl &= ~(CURSOR_MODE | MCURSOR_PIPE_SELECT);
> -			cntl |= MCURSOR_GAMMA_ENABLE;
> +	uint32_t cntl;
>  
> -			switch (width) {
> +	cntl = 0;
> +	if (base) {
> +		cntl = MCURSOR_GAMMA_ENABLE;
> +		switch (intel_crtc->cursor_width) {
>  			case 64:
>  				cntl |= CURSOR_MODE_64_ARGB_AX;
>  				break;
> @@ -7617,18 +7617,16 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  			default:
>  				WARN_ON(1);
>  				return;
> -			}
> -			cntl |= pipe << 28; /* Connect to correct pipe */
> -		} else {
> -			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> -			cntl |= CURSOR_MODE_DISABLE;
>  		}
> +		cntl |= pipe << 28; /* Connect to correct pipe */
> +	}
> +	if (intel_crtc->cursor_cntl != cntl) {
>  		I915_WRITE(CURCNTR(pipe), cntl);
> -
> -		intel_crtc->cursor_visible = visible;
> +		POSTING_READ(CURCNTR(pipe));
> +		intel_crtc->cursor_cntl = cntl;
>  	}
> +
>  	/* and commit changes on next vblank */
> -	POSTING_READ(CURCNTR(pipe));
>  	I915_WRITE(CURBASE(pipe), base);
>  	POSTING_READ(CURBASE(pipe));
>  }
> @@ -7639,15 +7637,12 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	int pipe = intel_crtc->pipe;
> -	bool visible = base != 0;
> -
> -	if (intel_crtc->cursor_visible != visible) {
> -		int16_t width = intel_crtc->cursor_width;
> -		uint32_t cntl = I915_READ(CURCNTR_IVB(pipe));
> -		if (base) {
> -			cntl &= ~CURSOR_MODE;
> -			cntl |= MCURSOR_GAMMA_ENABLE;
> -			switch (width) {
> +	uint32_t cntl;
> +
> +	cntl = 0;
> +	if (base) {
> +		cntl = MCURSOR_GAMMA_ENABLE;
> +		switch (intel_crtc->cursor_width) {
>  			case 64:
>  				cntl |= CURSOR_MODE_64_ARGB_AX;
>  				break;
> @@ -7660,21 +7655,18 @@ static void ivb_update_cursor(struct drm_crtc *crtc, u32 base)
>  			default:
>  				WARN_ON(1);
>  				return;
> -			}
> -		} else {
> -			cntl &= ~(CURSOR_MODE | MCURSOR_GAMMA_ENABLE);
> -			cntl |= CURSOR_MODE_DISABLE;
> -		}
> -		if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> -			cntl |= CURSOR_PIPE_CSC_ENABLE;
> -			cntl &= ~CURSOR_TRICKLE_FEED_DISABLE;
>  		}
> -		I915_WRITE(CURCNTR_IVB(pipe), cntl);
> +	}
> +	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> +		cntl |= CURSOR_PIPE_CSC_ENABLE;
>  
> -		intel_crtc->cursor_visible = visible;
> +	if (intel_crtc->cursor_cntl != cntl) {
> +		I915_WRITE(CURCNTR_IVB(pipe), cntl);
> +		POSTING_READ(CURCNTR_IVB(pipe));
> +		intel_crtc->cursor_cntl = cntl;
>  	}
> +
>  	/* and commit changes on next vblank */
> -	POSTING_READ(CURCNTR_IVB(pipe));
>  	I915_WRITE(CURBASE_IVB(pipe), base);
>  	POSTING_READ(CURBASE_IVB(pipe));
>  }
> @@ -7690,7 +7682,6 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	int x = intel_crtc->cursor_x;
>  	int y = intel_crtc->cursor_y;
>  	u32 base = 0, pos = 0;
> -	bool visible;
>  
>  	if (on)
>  		base = intel_crtc->cursor_addr;
> @@ -7719,8 +7710,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  	}
>  	pos |= y << CURSOR_Y_SHIFT;
>  
> -	visible = base != 0;
> -	if (!visible && !intel_crtc->cursor_visible)
> +	if (base == 0 && intel_crtc->cursor_base == 0)
>  		return;
>  
>  	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> @@ -7733,6 +7723,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  		else
>  			i9xx_update_cursor(crtc, base);
>  	}
> +	intel_crtc->cursor_base = base;
>  }
>  
>  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -10681,6 +10672,9 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
>  		intel_crtc->plane = !pipe;
>  	}
>  
> +	intel_crtc->cursor_base = ~0;
> +	intel_crtc->cursor_cntl = ~0;
> +
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
>  	       dev_priv->plane_to_crtc_mapping[intel_crtc->plane] != NULL);
>  	dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2da2efcfbd47..c6ab4ac4063a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -379,7 +379,8 @@ struct intel_crtc {
>  	uint32_t cursor_addr;
>  	int16_t cursor_x, cursor_y;
>  	int16_t cursor_width, cursor_height;
> -	bool cursor_visible;
> +	uint32_t cursor_cntl;
> +	uint32_t cursor_base;
>  
>  	struct intel_plane_config plane_config;
>  	struct intel_crtc_config config;
> -- 
> 1.9.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2014-04-12 17:39 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-12 15:20 [PATCH] drm/i915: Always apply cursor width changes Chris Wilson
2014-04-12 17:39 ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox