| Message ID | 1395266088-24536-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
On Wed, Mar 19, 2014 at 04:06:38PM -0700, Ben Widawsky wrote: > 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 > ? I wasn't sure either, but I thought since we didn't do anything special for BBADDR, to leave ACTHD alone. I wonder if it would help splitting it up, having to count 8 extra leading zeros is going to be a nightmare (especially as the decoder doesn't pad it right either...) For reading, I think I would prefer err_printf(m, " ACTHD: 0x%016llx [0x%08x %08x]\n", ring->acthd, (u32)(ring->acthd>>32), (u32)(ring->acthd)); or maybe just err_printf(m, " ACTHD: 0x%08x %08x\n", (u32)(ring->acthd>>32), (u32)(ring->acthd)); -Chris
On Thu, Mar 20, 2014 at 07:54:19AM +0000, Chris Wilson wrote: > On Wed, Mar 19, 2014 at 04:06:38PM -0700, Ben Widawsky wrote: > > 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 > > ? > > I wasn't sure either, but I thought since we didn't do anything special > for BBADDR, to leave ACTHD alone. > > I wonder if it would help splitting it up, having to count 8 extra > leading zeros is going to be a nightmare (especially as the decoder > doesn't pad it right either...) > > For reading, I think I would prefer > err_printf(m, " ACTHD: 0x%016llx [0x%08x %08x]\n", > ring->acthd, (u32)(ring->acthd>>32), (u32)(ring->acthd)); > > or maybe just > err_printf(m, " ACTHD: 0x%08x %08x\n", > (u32)(ring->acthd>>32), (u32)(ring->acthd)); I expect this to lead to a day-long wtf session once we have 64b ppgtt enabled and a leaking X hung ;-) I use g* a lot in vim when reading error states, so dropping information is dangerous. -Daniel
On Thu, Mar 20, 2014 at 04:17:26PM +0100, Daniel Vetter wrote: > On Thu, Mar 20, 2014 at 07:54:19AM +0000, Chris Wilson wrote: > > On Wed, Mar 19, 2014 at 04:06:38PM -0700, Ben Widawsky wrote: > > > 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 > > > ? > > > > I wasn't sure either, but I thought since we didn't do anything special > > for BBADDR, to leave ACTHD alone. > > > > I wonder if it would help splitting it up, having to count 8 extra > > leading zeros is going to be a nightmare (especially as the decoder > > doesn't pad it right either...) > > > > For reading, I think I would prefer > > err_printf(m, " ACTHD: 0x%016llx [0x%08x %08x]\n", > > ring->acthd, (u32)(ring->acthd>>32), (u32)(ring->acthd)); > > > > or maybe just > > err_printf(m, " ACTHD: 0x%08x %08x\n", > > (u32)(ring->acthd>>32), (u32)(ring->acthd)); > > I expect this to lead to a day-long wtf session once we have 64b ppgtt > enabled and a leaking X hung ;-) I use g* a lot in vim when reading error > states, so dropping information is dangerous. I'm not following, where is the information loss? If you haven't already guessed, we are seeing ACTHD above 4GiB already. -Chris
On Thu, Mar 20, 2014 at 5:28 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > I wasn't sure either, but I thought since we didn't do anything special >> > for BBADDR, to leave ACTHD alone. >> > >> > I wonder if it would help splitting it up, having to count 8 extra >> > leading zeros is going to be a nightmare (especially as the decoder >> > doesn't pad it right either...) >> > >> > For reading, I think I would prefer >> > err_printf(m, " ACTHD: 0x%016llx [0x%08x %08x]\n", >> > ring->acthd, (u32)(ring->acthd>>32), (u32)(ring->acthd)); >> > >> > or maybe just >> > err_printf(m, " ACTHD: 0x%08x %08x\n", >> > (u32)(ring->acthd>>32), (u32)(ring->acthd)); >> >> I expect this to lead to a day-long wtf session once we have 64b ppgtt >> enabled and a leaking X hung ;-) I use g* a lot in vim when reading error >> states, so dropping information is dangerous. > > I'm not following, where is the information loss? If you haven't already > guessed, we are seeing ACTHD above 4GiB already. Utter failure to read your diff. I agree that some form of split would look nice. /me hides in shame -Daniel
On 03/19/2014 09:54 PM, Chris Wilson wrote: > 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; > + > + return acthd; > } Can it happen, and does anyone care, for a low dword to wrap so instead of say, 0x00010000, this function falsely returns 0x0001ffff ? Tvrtko
On Thu, Mar 20, 2014 at 04:56:57PM +0000, Tvrtko Ursulin wrote: > > On 03/19/2014 09:54 PM, Chris Wilson wrote: > >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; > >+ > >+ return acthd; > > } > > Can it happen, and does anyone care, for a low dword to wrap so > instead of say, 0x00010000, this function falsely returns 0x0001ffff > ? Yeah, it could happen. This is used in error capture where the other rings may still be running, and for hangcheck where it would not matter as it would still differ on the next pass. And only when multiple passes stalled would the hangcheck fire. Anyway, it is probably better to handle this correctly in case we do use in a sensitive check later. Thanks, -Chris
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); 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; + + 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)
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(-)