| Message ID | 1434128948-9221-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. */ 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
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.
On Fri, Jun 12, 2015 at 08:55:09PM +0100, Dave Gordon wrote: > > 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. I find the opposite, since we compute how much space we want I think it is counter intuitive to suggest otherwise. You then need to assert that the computed space is enough. By saying if we wait until this request, we must have at least this space available and only using that space there is no implicit knowlege. > * 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. Because there is a cost associated with every retirement. See above for why being explicit is clearer. > * 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. Eh. The code is still strictly correct. The biggest issue is that we have multiple locations that decide how to interpret the amount of space generated by completing the request. However, we want to keep request retirement very simple since it is a hot function. -Chris
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); return 0; } diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index b70d25b..a3406b2 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -66,7 +66,8 @@ void intel_ring_update_space(struct intel_ringbuffer *ringbuf) } ringbuf->space = __intel_ring_space(ringbuf->head & HEAD_ADDR, - ringbuf->tail, ringbuf->size); + ringbuf->tail, + ringbuf->effective_size); } int intel_ring_space(struct intel_ringbuffer *ringbuf) @@ -2117,8 +2118,9 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n) return 0; list_for_each_entry(request, &ring->request_list, list) { + /* Would completion of this request free enough space? */ space = __intel_ring_space(request->postfix, ringbuf->tail, - ringbuf->size); + ringbuf->effective_size); if (space >= n) break; } @@ -2130,7 +2132,8 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n) if (ret) return ret; - ringbuf->space = space; + /* Update ring space after wait+retire */ + intel_ring_update_space(ringbuf); return 0; }
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(-)