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

[Tomas Elf]

On 19/03/2015 12:30, John.C.Harrison at Intel.com wrote:
> 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;
>   }
>
>

Reviewed-by: Tomas Elf <tomas.elf at intel.com>

Thanks,
Tomas