[Intel-gfx] [PATCH 04/59] drm/i915: Fix for ringbuf space wait in LRC mode

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> John.C.Harrison at Intel.com
> Sent: Thursday, March 19, 2015 12:30 PM
> To: Intel-GFX at 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 at 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 at 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 at intel.com>