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@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
