On Tue, Mar 31, 2015 at 04:50:10PM +0100, Tomas Elf wrote:
> On 19/03/2015 12:30, John.C.Harrison@xxxxxxxxx wrote:
> >From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> >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@xxxxxxxxx>
> >---
> > 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@xxxxxxxxx>
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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
