On Fri, Mar 21, 2014 at 12:41:53PM +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.
>
> v2: Carefully read the two registers to catch wraparound between
> the reads.
> v3: Use a WARN_ON rather than loop indefinitely on an unstable
> register read.
>
I truly feel bad for saying this at v3, but we can probably simplify
this. Unless I am missing something, we only actually care about the
value of acthd in:
if (ring->hangcheck.acthd != acthd)
return HANGCHECK_ACTIVE;
I think therefore it would be safe to make hangcheck.acthd a u64.
Compare the lower dword. If they're not equal, then we're done. If they
are equal, compare the UDW. If UDW doesn't match, we're done. If UDW
does match, do another read of the LDW and call it good:
ring_stuck(u32 acthd)
{
if (lower_32_bits(ring->hangcheck.acthd) != acthd)
return HANGCHECK_ACTIVE;
else if (upper_32_bits(ring->hangcheck.acthd) !=
I915_READ(ACTHD_UDW))
return HANGCHECK_ACTIVE
else if (acthd != I915_READ(RING_ACTHD))
return HANGCHECK_ACTIVE;
}
After writing that, I'm not really sure how much less complex it is, but I
think it gets the same job done. Potentially there is some MMIO savings,
but I'd guess it to be negligible.
Jesse, please request ACTHD is expanded to a proper 64b register for
gen100000000.
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
> Cc: Timo Aaltonen <tjaalton@xxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 13 ++++++++++++-
> 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 | 22 ++++++++++++++++------
> drivers/gpu/drm/i915/intel_ringbuffer.h | 6 +++---
> 6 files changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5418253..4c09fb2 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 */
> @@ -2786,6 +2786,17 @@ void vlv_force_wake_put(struct drm_i915_private *dev_priv, int fw_engine);
> #define I915_WRITE64(reg, val) dev_priv->uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true)
> #define I915_READ64(reg) dev_priv->uncore.funcs.mmio_readq(dev_priv, (reg), true)
>
> +#define I915_READ64_2x32(lower_reg, upper_reg) ({ \
> + u32 upper = I915_READ(upper_reg); \
> + u32 lower = I915_READ(lower_reg); \
> + u32 tmp = I915_READ(upper_reg); \
> + if (upper != tmp) { \
> + upper = tmp; \
> + lower = I915_READ(lower_reg); \
> + WARN_ON(I915_READ(upper_reg) != upper); \
> + } \
> + (u64)upper << 32 | lower; })
> +
> #define POSTING_READ(reg) (void)I915_READ_NOTRACE(reg)
> #define POSTING_READ16(reg) (void)I915_READ16_NOTRACE(reg)
>
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index b153a16..9519aa2 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);
> 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 77dbef6..9caae98 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 f010ff7..3c464d3 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 7a01911..ce8ddee 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -417,13 +417,20 @@ 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;
> + u64 acthd;
>
> - return I915_READ(acthd_reg);
> + if (INTEL_INFO(ring->dev)->gen >= 8)
> + acthd = I915_READ64_2x32(RING_ACTHD(ring->mmio_base),
> + RING_ACTHD_UDW(ring->mmio_base));
> + else if (INTEL_INFO(ring->dev)->gen >= 4)
> + acthd = I915_READ(RING_ACTHD(ring->mmio_base));
> + else
> + acthd = I915_READ(ACTHD);
> +
> + return acthd;
> }
>
> static void ring_setup_phys_status_page(struct intel_ring_buffer *ring)
> @@ -820,8 +827,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 69b9086..e2872c6 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)
> --
> 1.7.9.5
>
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
