Re: [PATCH] drm/i915: Reserve space improvements

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

On 25/06/15 19:38, Tomas Elf wrote:
> On 24/06/2015 18:03, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> An earlier patch was added to reserve space in the ring buffer for the
>> commands issued during 'add_request()'. The initial version was
>> pessimistic in the way it handled buffer wrapping and would cause
>> premature wraps and thus waste ring space.
>>
>> This patch updates the code to better handle the wrap case. It no
>> longer enforces that the space being asked for and the reserved space
>> are a single contiguous block. Instead, it allows the reserve to be on
>> the far end of a wrap operation. It still guarantees that the space is
>> available so when the wrap occurs, no wait will happen. Thus the wrap
>> cannot fail which is the whole point of the exercise.
>>
>> Also fixed a merge failure with some comments from the original patch.
>>
>> For: VIZ-5115
>> CC: Daniel Vetter <daniel@ffwll.ch>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_lrc.c        | 54 +++++++++++++-----------
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 73
>> +++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  4 +-
>>   3 files changed, 79 insertions(+), 52 deletions(-)

[snip]

>> @@ -748,31 +749,36 @@ static int logical_ring_wrap_buffer(struct
>> drm_i915_gem_request *req)
>>   static int logical_ring_prepare(struct drm_i915_gem_request *req,
>> int bytes)
>>   {
>>       struct intel_ringbuffer *ringbuf = req->ringbuf;
>> -    int ret;
>> -
>> -    /*
>> -     * Add on the reserved size to the request to make sure that after
>> -     * the intended commands have been emitted, there is guaranteed to
>> -     * still be enough free space to send them to the hardware.
>> -     */
>> -    if (!ringbuf->reserved_in_use)
>> -        bytes += ringbuf->reserved_size;
>> +    int ret, max_bytes;
>>
> 
> It would be helpful if we could flesh out the flow through the
> ring_prepare functions and be more explicit about what is actually going
> on. Largely this is because there is a distinct lack of documentation
> for the entire ring buffer management code on top of a quite
> counter-intuitive legacy design, so this is not due to your changes.
> However, your changes make things even more complex and hard to
> understand. So I've suggested a few comments below. Feel free to reword
> or do whatever with them. It would be nice if we could be slightly more
> clear about what is going on here, though. The same comments apply to
> both legacy and execlist function implementations obviously.

It would be simpler to understand if the unnecessary complication of
unlikely(wait-for-space-and-wrap) followed by unlikely(wait-for-space)
were simplified into a single process of "precalculate space required,
allowing for wrapping where necessary; wait for that much space; finally
fill the tail of the ring if we determined that wrapping was necessary".

This change can be applied before any of the complications of "reserving
space". See
http://lists.freedesktop.org/archives/intel-gfx/2015-June/068545.html

Secondly, a simpler way to implement "reserved space" is just to have
the "calculate remaining space" function intel_ring_space() deduct the
amount-reserved-for-add-request from its result UNLESS the
"use-reserved-space" flag has been set. It already deducts
I915_RING_FREE_SPACE, which represents the h/w limitation of the minimum
space required between tail and head, so conditionally deducting the
extra amount would be easy; then all other code could continue to use
the value it returns when checking for enough space. Nothing else would
have to know about "reserved space" at all.

.Dave.

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