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

[John Harrison <John.C.Harrison@Intel.com>]

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.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx