On 25/06/15 19:38, Tomas Elf wrote:
> On 24/06/2015 18:03, John.C.Harrison@xxxxxxxxx wrote:
>> From: John Harrison <John.C.Harrison@xxxxxxxxx>
>>
>> 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@xxxxxxxx>
>> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
