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.gor...@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
