| Message ID | 1426768264-16996-5-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > John.C.Harrison@Intel.com > Sent: Thursday, March 19, 2015 12:30 PM > To: Intel-GFX@Lists.FreeDesktop.Org > Subject: [Intel-gfx] [PATCH 04/59] drm/i915: Fix for ringbuf space wait in LRC > mode > > From: John Harrison <John.C.Harrison@Intel.com> > > The legacy and LRC code paths have an almost identical procedure for waiting > for > space in the ring buffer. They both search for a request in the free list that > will advance the tail to a point where sufficient space is available. They then > wait for that request, retire it and recalculate the free space value. > > Unfortunately, a bug in the LRC side meant that the resulting free space might > not be as large as expected and indeed, might not be sufficient. This is because > it was testing against the value of request->tail not request->postfix. Whereas, > when a request is retired, ringbuf->tail is updated to req->postfix not > req->tail. > > Another significant difference between the two is that the LRC one did not trust > the wait for request to work! It redid the is there enough space available test > and would fail the call if insufficient. Whereas, the legacy version just said > 'return 0' - it assumed the preceeding code works. This difference meant that > the LRC version still worked even with the bug - it just fell back to the > polling wait path. > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++---- > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++---- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 6504689..1c3834fc 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -634,7 +634,7 @@ static int logical_ring_wait_request(struct > intel_ringbuffer *ringbuf, > { > struct intel_engine_cs *ring = ringbuf->ring; > struct drm_i915_gem_request *request; > - int ret; > + int ret, new_space; > > if (intel_ring_space(ringbuf) >= bytes) > return 0; > @@ -650,10 +650,10 @@ static int logical_ring_wait_request(struct > intel_ringbuffer *ringbuf, > continue; > > /* Would completion of this request free enough space? */ > - if (__intel_ring_space(request->tail, ringbuf->tail, > - ringbuf->size) >= bytes) { > + new_space = __intel_ring_space(request->postfix, ringbuf->tail, > + ringbuf->size); > + if (new_space >= bytes) > break; > - } > } > > if (&request->list == &ring->request_list) > @@ -665,6 +665,8 @@ static int logical_ring_wait_request(struct > intel_ringbuffer *ringbuf, > > i915_gem_retire_requests_ring(ring); > > + WARN_ON(intel_ring_space(ringbuf) < new_space); > + > return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC; > } > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 99fb2f0..a26bdf8 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2059,16 +2059,16 @@ static int intel_ring_wait_request(struct > intel_engine_cs *ring, int n) > { > struct intel_ringbuffer *ringbuf = ring->buffer; > struct drm_i915_gem_request *request; > - int ret; > + int ret, new_space; > > if (intel_ring_space(ringbuf) >= n) > return 0; > > list_for_each_entry(request, &ring->request_list, list) { > - if (__intel_ring_space(request->postfix, ringbuf->tail, > - ringbuf->size) >= n) { > + new_space = __intel_ring_space(request->postfix, ringbuf->tail, > + ringbuf->size); > + if (new_space >= n) > break; > - } > } > > if (&request->list == &ring->request_list) > @@ -2080,6 +2080,8 @@ static int intel_ring_wait_request(struct > intel_engine_cs *ring, int n) > > i915_gem_retire_requests_ring(ring); > > + WARN_ON(intel_ring_space(ringbuf) < new_space); > + > return 0; > } > > -- > 1.7.9.5 Reviewed-by: Thomas Daniel <thomas.daniel@intel.com>
On 19/03/2015 12:30, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The legacy and LRC code paths have an almost identical procedure for waiting for > space in the ring buffer. They both search for a request in the free list that > will advance the tail to a point where sufficient space is available. They then > wait for that request, retire it and recalculate the free space value. > > Unfortunately, a bug in the LRC side meant that the resulting free space might > not be as large as expected and indeed, might not be sufficient. This is because > it was testing against the value of request->tail not request->postfix. Whereas, > when a request is retired, ringbuf->tail is updated to req->postfix not > req->tail. > > Another significant difference between the two is that the LRC one did not trust > the wait for request to work! It redid the is there enough space available test > and would fail the call if insufficient. Whereas, the legacy version just said > 'return 0' - it assumed the preceeding code works. This difference meant that > the LRC version still worked even with the bug - it just fell back to the > polling wait path. > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++---- > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++---- > 2 files changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 6504689..1c3834fc 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -634,7 +634,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, > { > struct intel_engine_cs *ring = ringbuf->ring; > struct drm_i915_gem_request *request; > - int ret; > + int ret, new_space; > > if (intel_ring_space(ringbuf) >= bytes) > return 0; > @@ -650,10 +650,10 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, > continue; > > /* Would completion of this request free enough space? */ > - if (__intel_ring_space(request->tail, ringbuf->tail, > - ringbuf->size) >= bytes) { > + new_space = __intel_ring_space(request->postfix, ringbuf->tail, > + ringbuf->size); > + if (new_space >= bytes) > break; > - } > } > > if (&request->list == &ring->request_list) > @@ -665,6 +665,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, > > i915_gem_retire_requests_ring(ring); > > + WARN_ON(intel_ring_space(ringbuf) < new_space); > + > return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC; > } > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 99fb2f0..a26bdf8 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -2059,16 +2059,16 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) > { > struct intel_ringbuffer *ringbuf = ring->buffer; > struct drm_i915_gem_request *request; > - int ret; > + int ret, new_space; > > if (intel_ring_space(ringbuf) >= n) > return 0; > > list_for_each_entry(request, &ring->request_list, list) { > - if (__intel_ring_space(request->postfix, ringbuf->tail, > - ringbuf->size) >= n) { > + new_space = __intel_ring_space(request->postfix, ringbuf->tail, > + ringbuf->size); > + if (new_space >= n) > break; > - } > } > > if (&request->list == &ring->request_list) > @@ -2080,6 +2080,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) > > i915_gem_retire_requests_ring(ring); > > + WARN_ON(intel_ring_space(ringbuf) < new_space); > + > return 0; > } > > Reviewed-by: Tomas Elf <tomas.elf@intel.com> Thanks, Tomas
On Tue, Mar 31, 2015 at 04:50:10PM +0100, Tomas Elf wrote: > On 19/03/2015 12:30, John.C.Harrison@Intel.com wrote: > >From: John Harrison <John.C.Harrison@Intel.com> > > > >The legacy and LRC code paths have an almost identical procedure for waiting for > >space in the ring buffer. They both search for a request in the free list that > >will advance the tail to a point where sufficient space is available. They then > >wait for that request, retire it and recalculate the free space value. > > > >Unfortunately, a bug in the LRC side meant that the resulting free space might > >not be as large as expected and indeed, might not be sufficient. This is because > >it was testing against the value of request->tail not request->postfix. Whereas, > >when a request is retired, ringbuf->tail is updated to req->postfix not > >req->tail. > > > >Another significant difference between the two is that the LRC one did not trust > >the wait for request to work! It redid the is there enough space available test > >and would fail the call if insufficient. Whereas, the legacy version just said > >'return 0' - it assumed the preceeding code works. This difference meant that > >the LRC version still worked even with the bug - it just fell back to the > >polling wait path. > > > >For: VIZ-5115 > >Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > >--- > > drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++---- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++---- > > 2 files changed, 12 insertions(+), 8 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >index 6504689..1c3834fc 100644 > >--- a/drivers/gpu/drm/i915/intel_lrc.c > >+++ b/drivers/gpu/drm/i915/intel_lrc.c > >@@ -634,7 +634,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, > > { > > struct intel_engine_cs *ring = ringbuf->ring; > > struct drm_i915_gem_request *request; > >- int ret; > >+ int ret, new_space; > > > > if (intel_ring_space(ringbuf) >= bytes) > > return 0; > >@@ -650,10 +650,10 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, > > continue; > > > > /* Would completion of this request free enough space? */ > >- if (__intel_ring_space(request->tail, ringbuf->tail, > >- ringbuf->size) >= bytes) { > >+ new_space = __intel_ring_space(request->postfix, ringbuf->tail, > >+ ringbuf->size); > >+ if (new_space >= bytes) > > break; > >- } > > } > > > > if (&request->list == &ring->request_list) > >@@ -665,6 +665,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, > > > > i915_gem_retire_requests_ring(ring); > > > >+ WARN_ON(intel_ring_space(ringbuf) < new_space); > >+ > > return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC; > > } > > > >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >index 99fb2f0..a26bdf8 100644 > >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >@@ -2059,16 +2059,16 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) > > { > > struct intel_ringbuffer *ringbuf = ring->buffer; > > struct drm_i915_gem_request *request; > >- int ret; > >+ int ret, new_space; > > > > if (intel_ring_space(ringbuf) >= n) > > return 0; > > > > list_for_each_entry(request, &ring->request_list, list) { > >- if (__intel_ring_space(request->postfix, ringbuf->tail, > >- ringbuf->size) >= n) { > >+ new_space = __intel_ring_space(request->postfix, ringbuf->tail, > >+ ringbuf->size); > >+ if (new_space >= n) > > break; > >- } > > } > > > > if (&request->list == &ring->request_list) > >@@ -2080,6 +2080,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) > > > > i915_gem_retire_requests_ring(ring); > > > >+ WARN_ON(intel_ring_space(ringbuf) < new_space); > >+ > > return 0; > > } > > > > > > Reviewed-by: Tomas Elf <tomas.elf@intel.com> Merged up to this one here, thanks for patches&review. I still like to get to the bottom of the reservation discussion before proceeding (totally forgot that we still have an open question there). -Daniel
On Tue, Mar 31, 2015 at 04:50:10PM +0100, Tomas Elf wrote: > On 19/03/2015 12:30, John.C.Harrison@Intel.com wrote: > >From: John Harrison <John.C.Harrison@Intel.com> > > > >The legacy and LRC code paths have an almost identical procedure for waiting for > >space in the ring buffer. They both search for a request in the free list that > >will advance the tail to a point where sufficient space is available. They then > >wait for that request, retire it and recalculate the free space value. > > > >Unfortunately, a bug in the LRC side meant that the resulting free space might > >not be as large as expected and indeed, might not be sufficient. This is because > >it was testing against the value of request->tail not request->postfix. Whereas, > >when a request is retired, ringbuf->tail is updated to req->postfix not > >req->tail. req->postfix is garbage, please fix. -Chris
On 01/04/2015 06:56, Daniel Vetter wrote: > On Tue, Mar 31, 2015 at 04:50:10PM +0100, Tomas Elf wrote: >> On 19/03/2015 12:30, John.C.Harrison@Intel.com wrote: >>> From: John Harrison <John.C.Harrison@Intel.com> >>> >>> The legacy and LRC code paths have an almost identical procedure for waiting for >>> space in the ring buffer. They both search for a request in the free list that >>> will advance the tail to a point where sufficient space is available. They then >>> wait for that request, retire it and recalculate the free space value. >>> >>> Unfortunately, a bug in the LRC side meant that the resulting free space might >>> not be as large as expected and indeed, might not be sufficient. This is because >>> it was testing against the value of request->tail not request->postfix. Whereas, >>> when a request is retired, ringbuf->tail is updated to req->postfix not >>> req->tail. >>> >>> Another significant difference between the two is that the LRC one did not trust >>> the wait for request to work! It redid the is there enough space available test >>> and would fail the call if insufficient. Whereas, the legacy version just said >>> 'return 0' - it assumed the preceeding code works. This difference meant that >>> the LRC version still worked even with the bug - it just fell back to the >>> polling wait path. >>> >>> For: VIZ-5115 >>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_lrc.c | 10 ++++++---- >>> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++++++---- >>> 2 files changed, 12 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index 6504689..1c3834fc 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -634,7 +634,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, >>> { >>> struct intel_engine_cs *ring = ringbuf->ring; >>> struct drm_i915_gem_request *request; >>> - int ret; >>> + int ret, new_space; >>> >>> if (intel_ring_space(ringbuf) >= bytes) >>> return 0; >>> @@ -650,10 +650,10 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, >>> continue; >>> >>> /* Would completion of this request free enough space? */ >>> - if (__intel_ring_space(request->tail, ringbuf->tail, >>> - ringbuf->size) >= bytes) { >>> + new_space = __intel_ring_space(request->postfix, ringbuf->tail, >>> + ringbuf->size); >>> + if (new_space >= bytes) >>> break; >>> - } >>> } >>> >>> if (&request->list == &ring->request_list) >>> @@ -665,6 +665,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, >>> >>> i915_gem_retire_requests_ring(ring); >>> >>> + WARN_ON(intel_ring_space(ringbuf) < new_space); >>> + >>> return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC; >>> } >>> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >>> index 99fb2f0..a26bdf8 100644 >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >>> @@ -2059,16 +2059,16 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) >>> { >>> struct intel_ringbuffer *ringbuf = ring->buffer; >>> struct drm_i915_gem_request *request; >>> - int ret; >>> + int ret, new_space; >>> >>> if (intel_ring_space(ringbuf) >= n) >>> return 0; >>> >>> list_for_each_entry(request, &ring->request_list, list) { >>> - if (__intel_ring_space(request->postfix, ringbuf->tail, >>> - ringbuf->size) >= n) { >>> + new_space = __intel_ring_space(request->postfix, ringbuf->tail, >>> + ringbuf->size); >>> + if (new_space >= n) >>> break; >>> - } >>> } >>> >>> if (&request->list == &ring->request_list) >>> @@ -2080,6 +2080,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) >>> >>> i915_gem_retire_requests_ring(ring); >>> >>> + WARN_ON(intel_ring_space(ringbuf) < new_space); >>> + >>> return 0; >>> } >>> >>> >> Reviewed-by: Tomas Elf <tomas.elf@intel.com> > Merged up to this one here, thanks for patches&review. I still like to get > to the bottom of the reservation discussion before proceeding (totally > forgot that we still have an open question there). > -Daniel Yeah, I've been busy with Android issues lately and haven't had chance to look at this yet. John.
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 6504689..1c3834fc 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -634,7 +634,7 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, { struct intel_engine_cs *ring = ringbuf->ring; struct drm_i915_gem_request *request; - int ret; + int ret, new_space; if (intel_ring_space(ringbuf) >= bytes) return 0; @@ -650,10 +650,10 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, continue; /* Would completion of this request free enough space? */ - if (__intel_ring_space(request->tail, ringbuf->tail, - ringbuf->size) >= bytes) { + new_space = __intel_ring_space(request->postfix, ringbuf->tail, + ringbuf->size); + if (new_space >= bytes) break; - } } if (&request->list == &ring->request_list) @@ -665,6 +665,8 @@ static int logical_ring_wait_request(struct intel_ringbuffer *ringbuf, i915_gem_retire_requests_ring(ring); + WARN_ON(intel_ring_space(ringbuf) < new_space); + return intel_ring_space(ringbuf) >= bytes ? 0 : -ENOSPC; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 99fb2f0..a26bdf8 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -2059,16 +2059,16 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) { struct intel_ringbuffer *ringbuf = ring->buffer; struct drm_i915_gem_request *request; - int ret; + int ret, new_space; if (intel_ring_space(ringbuf) >= n) return 0; list_for_each_entry(request, &ring->request_list, list) { - if (__intel_ring_space(request->postfix, ringbuf->tail, - ringbuf->size) >= n) { + new_space = __intel_ring_space(request->postfix, ringbuf->tail, + ringbuf->size); + if (new_space >= n) break; - } } if (&request->list == &ring->request_list) @@ -2080,6 +2080,8 @@ static int intel_ring_wait_request(struct intel_engine_cs *ring, int n) i915_gem_retire_requests_ring(ring); + WARN_ON(intel_ring_space(ringbuf) < new_space); + return 0; }