public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2] drm/i915: Always apply cursor width changes
Date: Mon, 2 Jun 2014 10:49:56 +0200	[thread overview]
Message-ID: <20140602084956.GP19050@phenom.ffwll.local> (raw)
In-Reply-To: <1401456926-2398-1-git-send-email-chris@chris-wilson.co.uk>

On Fri, May 30, 2014 at 04:35:26PM +0300, 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>
> [Antti:rebased, adjusted macro names and moved some lines, no functional
> changes]
> Reviewed-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> Tested-by: Antti Koskipaa <antti.koskipaa@linux.intel.com>

Still missing the igt testcase. Is Antti still working on that one? And I
guess now we also need an Cc: stable@vger.kernel.org on this one here.
-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 2e5f76a..04d0b7c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2353,7 +2353,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 731cd01..955f92d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7872,29 +7872,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)
> @@ -7903,16 +7907,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;
> @@ -7925,18 +7925,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));
>  }
> @@ -7947,15 +7945,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(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;
> @@ -7968,21 +7963,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(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(pipe), cntl);
> +		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));
>  }
> @@ -7998,7 +7990,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;
> @@ -8027,8 +8018,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;
>  
>  	I915_WRITE(CURPOS(pipe), pos);
> @@ -8039,6 +8029,7 @@ static void intel_crtc_update_cursor(struct drm_crtc *crtc,
>  		i845_update_cursor(crtc, base);
>  	else
>  		i9xx_update_cursor(crtc, base);
> +	intel_crtc->cursor_base = base;
>  }
>  
>  static int intel_crtc_cursor_set(struct drm_crtc *crtc,
> @@ -10970,6 +10961,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;
> +
>  	init_waitqueue_head(&intel_crtc->vbl_wait);
>  
>  	BUG_ON(pipe >= ARRAY_SIZE(dev_priv->plane_to_crtc_mapping) ||
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9bb70dc..9d1ff7b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -386,7 +386,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.8.3.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

  reply	other threads:[~2014-06-02  8:50 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-05-30 13:35 [PATCH v2] drm/i915: Always apply cursor width changes Chris Wilson
2014-06-02  8:49 ` Daniel Vetter [this message]
2014-06-02 10:46   ` Antti Koskipää
2014-06-02 15:34     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140602084956.GP19050@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox