Re: [PATCH 1/2] drm/i915: use effective_size for ringbuffer calculations

[Dave Gordon <david.s.gordon@intel.com>]

On 12/06/15 19:12, Chris Wilson wrote:
> On Fri, Jun 12, 2015 at 06:09:07PM +0100, Dave Gordon wrote:
>> When calculating the available space in a ringbuffer, we should
>> use the effective_size rather than the true size of the ring.
>>
>> v2: rebase to latest drm-intel-nightly
>> v3: rebase to latest drm-intel-nightly
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_lrc.c        |    5 +++--
>>  drivers/gpu/drm/i915/intel_ringbuffer.c |    9 ++++++---
>>  2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 9b74ffa..454e836 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -699,7 +699,7 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>>  
>>  		/* Would completion of this request free enough space? */
>>  		space = __intel_ring_space(request->postfix, ringbuf->tail,
>> -					   ringbuf->size);
>> +					   ringbuf->effective_size);
>>  		if (space >= bytes)
>>  			break;
>>  	}
>> @@ -711,7 +711,8 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
>>  	if (ret)
>>  		return ret;
>>  
>> -	ringbuf->space = space;
>> +	/* Update ring space after wait+retire */
>> +	intel_ring_update_space(ringbuf);
> 
> Does the function not do what it says on the tin? At least make it seem
> like you are explaining your reasoning, not documenting the following
> function.
> 
> /*
>  * Having waited for the request, query the HEAD of most recent retired
>  * request and use that for our space calcuations.
>  */

That's what the comment means; the important bit is mentioning "retire",
since it's not explicitly called from here but only via wait_request().
We could say,

	/*
	 * After waiting, at least one request must have completed
	 * and been retired, so make sure that the ringbuffer's
	 * space calculations are up to date
	 */
	intel_ring_update_space(ringbuf);
	BUG_ON(ringbuf->space < bytes);

> However, that makes an incorrect assumption about the waiter. Given that
> the current code is written such that ringbuf->last_retired_head =
> request->postfix and that space is identical to the repeated
> calculation, what is your intention exactly?
> -Chris

Three factors:

* firstly, a preference: I find it logically simpler that there should
be one and only one piece of code that writes into ringbuf->space. One
doesn't then have to reason about whether two different calculations are
in fact equivalent.

* secondly, for future proofing: although wait_request() now retires
only up to the waited-for request, that wasn't always the case, nor is
there any reason why it could not in future opportunistically retire
additional requests that have completed while it was waiting.

* thirdly, for correctness: using the function has the additional effect
of consuming the last_retired_head value set by retire_request. If this
is not done, a later call to intel_ring_space() may become confused,
with the result that 'head' (and therefore 'space') will be incorrectly
updated.

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