On Mon, Mar 24, 2014 at 07:41:17PM -0700, Ben Widawsky wrote:
> 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.
Right after I sent that, I realized that doesn't provide for potentially
corrupting ring->hangcheck.acthd. So I am back to thinking this method
is better.
>
>
> > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > Cc: Ben Widawsky <benjamin.widaw...@intel.com>
> > Cc: Timo Aaltonen <tjaal...@ubuntu.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursu...@linux.intel.com>
> > ---
> > 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
