Re: [PATCH] drm/i915: bdw expands ACTHD to 64bit

[Ben Widawsky <ben@bwidawsk.net>]

On Wed, Mar 19, 2014 at 09:54:48PM +0000, Chris Wilson wrote:
> As Broadwell has an increased virtual address size, it requires more
> than 32 bits to store offsets into its address space. This includes the
> debug registers to track the current HEAD of the individual rings, which
> may be anywhere within the per-process address spaces. In order to find
> the full location, we need to read the high bits from a second register.
> We then also need to expand our storage to keep track of the larger
> address.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <benjamin.widawsky@intel.com>
> Cc: Timo Aaltonen <tjaalton@ubuntu.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h         |  2 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c   |  2 +-
>  drivers/gpu/drm/i915/i915_irq.c         |  8 +++++---
>  drivers/gpu/drm/i915/i915_reg.h         |  1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 21 +++++++++++++++------
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  6 +++---
>  6 files changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ed67b4abf9e3..ee913b63a945 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -354,12 +354,12 @@ struct drm_i915_error_state {
>  		u32 ipeir;
>  		u32 ipehr;
>  		u32 instdone;
> -		u32 acthd;
>  		u32 bbstate;
>  		u32 instpm;
>  		u32 instps;
>  		u32 seqno;
>  		u64 bbaddr;
> +		u64 acthd;
>  		u32 fault_reg;
>  		u32 faddr;
>  		u32 rc_psmi; /* sleep state */
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index b153a16ead0a..9519aa240614 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -248,7 +248,7 @@ static void i915_ring_error_state(struct drm_i915_error_state_buf *m,
>  	err_printf(m, "  TAIL: 0x%08x\n", ring->tail);
>  	err_printf(m, "  CTL: 0x%08x\n", ring->ctl);
>  	err_printf(m, "  HWS: 0x%08x\n", ring->hws);
> -	err_printf(m, "  ACTHD: 0x%08x\n", ring->acthd);
> +	err_printf(m, "  ACTHD: 0x%08llx\n", ring->acthd);

%016x?

if (gen8)
	%016x
?

>  	err_printf(m, "  IPEIR: 0x%08x\n", ring->ipeir);
>  	err_printf(m, "  IPEHR: 0x%08x\n", ring->ipehr);
>  	err_printf(m, "  INSTDONE: 0x%08x\n", ring->instdone);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1dd9d3628919..b79792317f39 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2507,7 +2507,8 @@ static struct intel_ring_buffer *
>  semaphore_waits_for(struct intel_ring_buffer *ring, u32 *seqno)
>  {
>  	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -	u32 cmd, ipehr, acthd, acthd_min;
> +	u64 acthd, acthd_min;
> +	u32 cmd, ipehr;
>  
>  	ipehr = I915_READ(RING_IPEHR(ring->mmio_base));
>  	if ((ipehr & ~(0x3 << 16)) !=
> @@ -2563,7 +2564,7 @@ static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv)
>  }
>  
>  static enum intel_ring_hangcheck_action
> -ring_stuck(struct intel_ring_buffer *ring, u32 acthd)
> +ring_stuck(struct intel_ring_buffer *ring, u64 acthd)
>  {
>  	struct drm_device *dev = ring->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -2631,7 +2632,8 @@ static void i915_hangcheck_elapsed(unsigned long data)
>  		return;
>  
>  	for_each_ring(ring, dev_priv, i) {
> -		u32 seqno, acthd;
> +		u64 acthd;
> +		u32 seqno;
>  		bool busy = true;
>  
>  		semaphore_clear_deadlocks(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f010ff7e7e2a..3c464d307a2b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -708,6 +708,7 @@ enum punit_power_well {
>  #define BLT_HWS_PGA_GEN7	(0x04280)
>  #define VEBOX_HWS_PGA_GEN7	(0x04380)
>  #define RING_ACTHD(base)	((base)+0x74)
> +#define RING_ACTHD_UDW(base)	((base)+0x5c)
>  #define RING_NOPID(base)	((base)+0x94)
>  #define RING_IMR(base)		((base)+0xa8)
>  #define RING_TIMESTAMP(base)	((base)+0x358)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 7a01911c16f8..a6ceb2c6f36d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -417,13 +417,19 @@ static void ring_write_tail(struct intel_ring_buffer *ring,
>  	I915_WRITE_TAIL(ring, value);
>  }
>  
> -u32 intel_ring_get_active_head(struct intel_ring_buffer *ring)
> +u64 intel_ring_get_active_head(struct intel_ring_buffer *ring)
>  {
>  	drm_i915_private_t *dev_priv = ring->dev->dev_private;
> -	u32 acthd_reg = INTEL_INFO(ring->dev)->gen >= 4 ?
> -			RING_ACTHD(ring->mmio_base) : ACTHD;
>  
> -	return I915_READ(acthd_reg);
> +	u32 reg = (INTEL_INFO(ring->dev)->gen >= 4 ?
> +		   RING_ACTHD(ring->mmio_base) : ACTHD);
> +	u64 acthd;
> +
> +	acthd = I915_READ(reg);
> +	if (INTEL_INFO(ring->dev)->gen >= 8)
> +		acthd |= (u64)I915_READ(RING_ACTHD_UDW(ring->mmio_base)) << 32;

I had to double check we didn't need to explicitly 0 the upper bits :-)

> +
> +	return acthd;
>  }
>  
>  static void ring_setup_phys_status_page(struct intel_ring_buffer *ring)
> @@ -820,8 +826,11 @@ gen6_ring_get_seqno(struct intel_ring_buffer *ring, bool lazy_coherency)
>  	/* Workaround to force correct ordering between irq and seqno writes on
>  	 * ivb (and maybe also on snb) by reading from a CS register (like
>  	 * ACTHD) before reading the status page. */
> -	if (!lazy_coherency)
> -		intel_ring_get_active_head(ring);
> +	if (!lazy_coherency) {
> +		struct drm_i915_private *dev_priv = ring->dev->dev_private;
> +		POSTING_READ(RING_ACTHD(ring->mmio_base));
> +	}
> +
>  	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 69b908622e14..e2872c6b522b 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -46,11 +46,11 @@ enum intel_ring_hangcheck_action {
>  #define HANGCHECK_SCORE_RING_HUNG 31
>  
>  struct intel_ring_hangcheck {
> -	bool deadlock;
> +	u64 acthd;
>  	u32 seqno;
> -	u32 acthd;
>  	int score;
>  	enum intel_ring_hangcheck_action action;
> +	bool deadlock;
>  };
>  
>  struct  intel_ring_buffer {
> @@ -294,7 +294,7 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev);
>  int intel_init_blt_ring_buffer(struct drm_device *dev);
>  int intel_init_vebox_ring_buffer(struct drm_device *dev);
>  
> -u32 intel_ring_get_active_head(struct intel_ring_buffer *ring);
> +u64 intel_ring_get_active_head(struct intel_ring_buffer *ring);
>  void intel_ring_setup_status_page(struct intel_ring_buffer *ring);
>  
>  static inline u32 intel_ring_get_tail(struct intel_ring_buffer *ring)

With the 016 pad fix:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

-- 
Ben Widawsky, Intel Open Source Technology Center