* [PATCH] drm/i915: specify the address of the semaphore register for comparison
@ 2011-09-21 11:42 Chris Wilson
2011-09-21 12:46 ` Daniel Vetter
2011-09-21 14:25 ` Ben Widawsky
0 siblings, 2 replies; 6+ messages in thread
From: Chris Wilson @ 2011-09-21 11:42 UTC (permalink / raw)
To: intel-gfx
In addition to telling the GPU that we are comparing with a named MMIO
register, we also apparently have to tell the GPU the address of that
register.
The lack of giving the GPU the duplicate information it seems to desire
leads to such fun confusion as:
Blitter command stream:
seqno: 0x00001c6c
Render command stream:
seqno: 0x00001c7a
Active batch on BLT (seqno 1c6e):
0x03b55000: 0x30222221: UNKNOWN
0x03b55004: 0xffb7b4b1: UNKNOWN
0x03b55008: 0xffb7b4b1: UNKNOWN
0x03b5500c: 0xffb7b4b1: UNKNOWN
0x03b55010: 0xffb7b4b1: UNKNOWN
0x03b55014: 0xffb7b4b1: UNKNOWN
0x03b55018: 0xffb7b4b1: UNKNOWN
0x03b5501c: 0xffb7b4b1: UNKNOWN
0x03b55020: 0xffb7b4b1: UNKNOWN
0x03b55024: 0xffb7b4b1: UNKNOWN
0x03b55028: 0xffb7b4b1: UNKNOWN
0x03b5502c: 0xffb7b4b1: UNKNOWN
... [an antialiased rgba box]
Render ringbuffer:
...
0x00007e70: 0x0b160001: MI_SEMAPHORE_MBOX
0x00007e74: 0x00001c6e: dword 1
0x00007e78: 0x00000000: dword 2
0x00007e7c: 0x00000000: MI_NOOP
0x00007e80: 0x18800180: MI_BATCH_BUFFER_START
0x00007e84: 0x03b5b000: dword 1
0x00007e88: 0x02000004: MI_FLUSH
0x00007e8c: 0x00000000: MI_NOOP
0x00007e90: 0x0b240001: MI_SEMAPHORE_MBOX
0x00007e94: 0x00001c70: dword 1
0x00007e98: 0x00022040: dword 2
0x00007e9c: 0x0b240001: MI_SEMAPHORE_MBOX
0x00007ea0: 0x00001c70: dword 1
0x00007ea4: 0x00012044: dword 2
0x00007ea8: 0x10800001: MI_STORE_DATA_INDEX
0x00007eac: 0x00000080: dword 1
0x00007eb0: 0x00001c70: dword 2
0x00007eb4: 0x01000000: MI_USER_INTERRUPT
0x00007eb8: 0x02000000: MI_FLUSH
... [wait on results of batch 1cde from BLT, then execute 1c70 on RENDER]
As the RENDER ring has already completed batch 1c7a before the BLT began
processing 1c6e, it is obvious then that the GPU did not heed the
semaphore wait instruction. Adding the symmetric register address from
the signal instruction to the compare instruction prevents the hang and
also prevents visual corruption whilst running KDE.
Reported-by: Andrew Lutomirski <luto@mit.edu>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Andrew Lutomirski <luto@mit.edu>
---
drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5fa3d99..6b9790e 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -359,7 +359,8 @@ intel_ring_sync(struct intel_ring_buffer *ring,
intel_ring_sync_index(ring, to) << 17 |
MI_SEMAPHORE_COMPARE);
intel_ring_emit(ring, seqno);
- intel_ring_emit(ring, 0);
+ intel_ring_emit(ring,
+ RING_SYNC_0(to->mmio_base) + 4*intel_ring_sync_index(ring,to));
intel_ring_emit(ring, MI_NOOP);
intel_ring_advance(ring);
--
1.7.6.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: specify the address of the semaphore register for comparison
2011-09-21 11:42 [PATCH] drm/i915: specify the address of the semaphore register for comparison Chris Wilson
@ 2011-09-21 12:46 ` Daniel Vetter
2011-09-21 13:16 ` Chris Wilson
2011-09-21 14:25 ` Ben Widawsky
1 sibling, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2011-09-21 12:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Wed, Sep 21, 2011 at 12:42:22PM +0100, Chris Wilson wrote:
> In addition to telling the GPU that we are comparing with a named MMIO
> register, we also apparently have to tell the GPU the address of that
> register.
Unfortunately this still doesn't fix the hard hangs on my kde on snb :(
But at least we now seem to have two devs who can reproduce this.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: specify the address of the semaphore register for comparison
2011-09-21 12:46 ` Daniel Vetter
@ 2011-09-21 13:16 ` Chris Wilson
0 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2011-09-21 13:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Wed, 21 Sep 2011 14:46:47 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Sep 21, 2011 at 12:42:22PM +0100, Chris Wilson wrote:
> > In addition to telling the GPU that we are comparing with a named MMIO
> > register, we also apparently have to tell the GPU the address of that
> > register.
>
> Unfortunately this still doesn't fix the hard hangs on my kde on snb :(
> But at least we now seem to have two devs who can reproduce this.
You're on your own again, Daniel. :-p
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: specify the address of the semaphore register for comparison
2011-09-21 11:42 [PATCH] drm/i915: specify the address of the semaphore register for comparison Chris Wilson
2011-09-21 12:46 ` Daniel Vetter
@ 2011-09-21 14:25 ` Ben Widawsky
2011-09-21 14:49 ` Chris Wilson
1 sibling, 1 reply; 6+ messages in thread
From: Ben Widawsky @ 2011-09-21 14:25 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Wed, 21 Sep 2011 12:42:22 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> In addition to telling the GPU that we are comparing with a named MMIO
> register, we also apparently have to tell the GPU the address of that
> register.
>
> The lack of giving the GPU the duplicate information it seems to
> desire leads to such fun confusion as:
>
> Blitter command stream:
> seqno: 0x00001c6c
> Render command stream:
> seqno: 0x00001c7a
>
> Active batch on BLT (seqno 1c6e):
> 0x03b55000: 0x30222221: UNKNOWN
> 0x03b55004: 0xffb7b4b1: UNKNOWN
> 0x03b55008: 0xffb7b4b1: UNKNOWN
> 0x03b5500c: 0xffb7b4b1: UNKNOWN
> 0x03b55010: 0xffb7b4b1: UNKNOWN
> 0x03b55014: 0xffb7b4b1: UNKNOWN
> 0x03b55018: 0xffb7b4b1: UNKNOWN
> 0x03b5501c: 0xffb7b4b1: UNKNOWN
> 0x03b55020: 0xffb7b4b1: UNKNOWN
> 0x03b55024: 0xffb7b4b1: UNKNOWN
> 0x03b55028: 0xffb7b4b1: UNKNOWN
> 0x03b5502c: 0xffb7b4b1: UNKNOWN
> ... [an antialiased rgba box]
>
> Render ringbuffer:
> ...
> 0x00007e70: 0x0b160001: MI_SEMAPHORE_MBOX
> 0x00007e74: 0x00001c6e: dword 1
> 0x00007e78: 0x00000000: dword 2
> 0x00007e7c: 0x00000000: MI_NOOP
> 0x00007e80: 0x18800180: MI_BATCH_BUFFER_START
> 0x00007e84: 0x03b5b000: dword 1
> 0x00007e88: 0x02000004: MI_FLUSH
> 0x00007e8c: 0x00000000: MI_NOOP
> 0x00007e90: 0x0b240001: MI_SEMAPHORE_MBOX
> 0x00007e94: 0x00001c70: dword 1
> 0x00007e98: 0x00022040: dword 2
> 0x00007e9c: 0x0b240001: MI_SEMAPHORE_MBOX
> 0x00007ea0: 0x00001c70: dword 1
> 0x00007ea4: 0x00012044: dword 2
> 0x00007ea8: 0x10800001: MI_STORE_DATA_INDEX
> 0x00007eac: 0x00000080: dword 1
> 0x00007eb0: 0x00001c70: dword 2
> 0x00007eb4: 0x01000000: MI_USER_INTERRUPT
> 0x00007eb8: 0x02000000: MI_FLUSH
> ... [wait on results of batch 1cde from BLT, then execute 1c70 on
> RENDER]
>
> As the RENDER ring has already completed batch 1c7a before the BLT
> began processing 1c6e, it is obvious then that the GPU did not heed
> the semaphore wait instruction. Adding the symmetric register address
> from the signal instruction to the compare instruction prevents the
> hang and also prevents visual corruption whilst running KDE.
>
> Reported-by: Andrew Lutomirski <luto@mit.edu>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Andrew Lutomirski <luto@mit.edu>
> ---
> drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5fa3d99..6b9790e
> 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -359,7 +359,8 @@ intel_ring_sync(struct intel_ring_buffer *ring,
> intel_ring_sync_index(ring, to) << 17 |
> MI_SEMAPHORE_COMPARE);
> intel_ring_emit(ring, seqno);
> - intel_ring_emit(ring, 0);
> + intel_ring_emit(ring,
> + RING_SYNC_0(to->mmio_base) +
> 4*intel_ring_sync_index(ring,to)); intel_ring_emit(ring, MI_NOOP);
> intel_ring_advance(ring);
>
I'm sort of afraid this is just papering over the issue, ie. the mmio
access is just adding delay that happens to make it work. I think we
should follow up with devs on this one.
Ben
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: specify the address of the semaphore register for comparison
2011-09-21 14:25 ` Ben Widawsky
@ 2011-09-21 14:49 ` Chris Wilson
2011-09-21 16:07 ` Ben Widawsky
0 siblings, 1 reply; 6+ messages in thread
From: Chris Wilson @ 2011-09-21 14:49 UTC (permalink / raw)
To: Ben Widawsky; +Cc: intel-gfx
On Wed, 21 Sep 2011 07:25:03 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Wed, 21 Sep 2011 12:42:22 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > ---
> > drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ++-
> > 1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5fa3d99..6b9790e
> > 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -359,7 +359,8 @@ intel_ring_sync(struct intel_ring_buffer *ring,
> > intel_ring_sync_index(ring, to) << 17 |
> > MI_SEMAPHORE_COMPARE);
> > intel_ring_emit(ring, seqno);
> > - intel_ring_emit(ring, 0);
> > + intel_ring_emit(ring,
> > + RING_SYNC_0(to->mmio_base) +
> > 4*intel_ring_sync_index(ring,to)); intel_ring_emit(ring, MI_NOOP);
> > intel_ring_advance(ring);
> >
>
> I'm sort of afraid this is just papering over the issue, ie. the mmio
> access is just adding delay that happens to make it work. I think we
> should follow up with devs on this one.
In the broken English from the spec in front of me:
PointerBitFieldName/MMIO Register Address
Project: All
Address: GraphicsVirtualAddress[31:2]
Surface Type: Semaphore
if Compare Register bit[18] is set, this field if the Graphics Memory Address of the 32 bit value for the semaphore.
If Compare Register bit[18] is cleared, this field is the MMIO address of the register for the semaphore.
I'm embarrassed to have only set the field for Update and not Compare.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: specify the address of the semaphore register for comparison
2011-09-21 14:49 ` Chris Wilson
@ 2011-09-21 16:07 ` Ben Widawsky
0 siblings, 0 replies; 6+ messages in thread
From: Ben Widawsky @ 2011-09-21 16:07 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Wed, 21 Sep 2011 15:49:07 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, 21 Sep 2011 07:25:03 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > On Wed, 21 Sep 2011 12:42:22 +0100
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > ---
> > > drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ++-
> > > 1 files changed, 2 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > b/drivers/gpu/drm/i915/intel_ringbuffer.c index 5fa3d99..6b9790e
> > > 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > > @@ -359,7 +359,8 @@ intel_ring_sync(struct intel_ring_buffer
> > > *ring, intel_ring_sync_index(ring, to) << 17 |
> > > MI_SEMAPHORE_COMPARE);
> > > intel_ring_emit(ring, seqno);
> > > - intel_ring_emit(ring, 0);
> > > + intel_ring_emit(ring,
> > > + RING_SYNC_0(to->mmio_base) +
> > > 4*intel_ring_sync_index(ring,to)); intel_ring_emit(ring, MI_NOOP);
> > > intel_ring_advance(ring);
> > >
> >
> > I'm sort of afraid this is just papering over the issue, ie. the
> > mmio access is just adding delay that happens to make it work. I
> > think we should follow up with devs on this one.
>
> In the broken English from the spec in front of me:
>
> PointerBitFieldName/MMIO Register Address
> Project: All
> Address: GraphicsVirtualAddress[31:2]
> Surface Type: Semaphore
> if Compare Register bit[18] is set, this field if the Graphics Memory
> Address of the 32 bit value for the semaphore. If Compare Register
> bit[18] is cleared, this field is the MMIO address of the register
> for the semaphore.
>
> I'm embarrassed to have only set the field for Update and not Compare.
> -Chris
>
Reading the simulator code, that third dword has no effect. I don't
think that means that is how the HW behaves, but food for though...
Ben
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-21 16:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-21 11:42 [PATCH] drm/i915: specify the address of the semaphore register for comparison Chris Wilson
2011-09-21 12:46 ` Daniel Vetter
2011-09-21 13:16 ` Chris Wilson
2011-09-21 14:25 ` Ben Widawsky
2011-09-21 14:49 ` Chris Wilson
2011-09-21 16:07 ` Ben Widawsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.