Re: [PATCH 2/5] drm/nouveau: add reservation to nouveau_gem_ioctl_cpu_prep

[Thomas Hellstrom <thomas@shipmail.org>]

On 01/22/2014 10:55 AM, Maarten Lankhorst wrote:
> op 22-01-14 10:40, Thomas Hellstrom schreef:
>> On 01/22/2014 09:19 AM, Maarten Lankhorst wrote:
>>> op 21-01-14 18:44, Thomas Hellstrom schreef:
>>>> On 01/21/2014 04:29 PM, Maarten Lankhorst wrote:
>>>>> Hey,
>>>>>
>>>>> op 21-01-14 16:17, Thomas Hellstrom schreef:
>>>>>> Maarten, for this and the other patches in this series,
>>>>>>
>>>>>> I seem to recall we have this discussion before?
>>>>>> IIRC I stated that reservation was a too heavy-weight lock to
>>>>>> hold to
>>>>>> determine whether a buffer was idle? It's a pretty nasty thing to
>>>>>> build in.
>>>>>>
>>>>> I've sent this patch after determining that this already didn't
>>>>> end up
>>>>> being heavyweight.
>>>>> Most places were already using the fence_lock and reservation, I just
>>>>> fixed up the few
>>>>> places that didn't hold a reservation while waiting. Converting the
>>>>> few places that didn't
>>>>> ended up being trivial, so I thought I'd submit it.
>>>> Actually the only *valid* reason for holding a reservation when
>>>> waiting
>>>> for idle is
>>>> 1) You want to block further command submission on the buffer.
>>>> 2) You want to switch GPU engine and don't have access to gpu
>>>> semaphores
>>>> / barriers.
>>>>
>>>> Reservation has the nasty side effect that it blocks command
>>>> submission
>>>> and pins the buffer (in addition now makes the evict list traversals
>>>> skip the buffer) which in general is *not* necessary for most wait
>>>> cases, so we should instead actually convert the wait cases that don't
>>>> fulfill 1) and 2) above in the other direction if we have performance
>>>> and latency-reduction in mind. I can't see how a spinlock protecting a
>>>> fence pointer or fence list is stopping you from using RW fences as
>>>> long
>>>> as the spinlock is held while manipulating the fence list?
>>>>
>>> You wish. Fine I'll enumerate all cases of ttm_bo_wait (with the
>>> patchset, though) and enumerate if they can be changed to work without
>>> reservation or not.
>>>
>>> ttm/ttm_bo.c
>>> ttm_bo_cleanup_refs_or_queue: needs reservation and ttm_bo_wait to
>>> finish for the direct destroy fastpath, if either fails it needs to be
>>> queued. Cannot work without reservation.
>> Doesn't block and no significant reservation contention expected.
>>
>>> ttm_bo_cleanup_refs_and_unlock: already drops reservation to wait,
>>> doesn't need to re-acquire. Simply reordering ttm_bo_wait until after
>>> re-reserve is enough.
>> Currently follows the above rules.
>>
>>> ttm_bo_evict: already has the reservation, cannot be dropped since
>>> only trylock is allowed. Dropping reservation would cause badness,
>>> cannot be converted.
>> Follows rule 2 above. We're about to move the buffer and if that can't
>> be pipelined using the GPU (which TTM currently doesn't allow), we need
>> to wait. Although eviction should be low priority compared to new
>> command submission, so I can't really see why we couldn't wait before
>> trying to reserve here?
>>
>>> ttm_bo_move_buffer: called from ttm_bo_validate, cannot drop
>>> reservation for same reason as ttm_bo_evict. It might be part of a
>>> ticketed reservation so really don't drop lock here.
>> Part of command submission and as such follows rule 2 above. If we can
>> pipeline the move with the GPU, no need to wait (but needs to be
>> implemented, of course).
>>
>>> ttm_bo_synccpu_write_grab: the wait could be converted to be done
>>> afterwards, without  fence_lock. But in this case reservation could
>>> take the role of fence_lock too,
>>>
>>> so no separate fence_lock would be needed.
>> With the exception that reservation is more likely to be contended.
> True but rule 1.
>>> ttm_bo_swapout: see ttm_bo_evict.
>>>
>>> ttm/ttm_bo_util.c:
>>> ttm_bo_move_accel_cleanup: calls ttm_bo_wait, cannot drop lock, see
>>> ttm_bo_move_buffer, can be called from that function.
>> Rule 2.
>>
>>> ttm/ttm_bo_vm.c
>>> ttm_bo_vm_fault_idle: I guess you COULD drop the reservation here, but
>>> you already had the reservation, so a similar optimization to
>>> ttm_bo_synccpu_write_grab could be done without requiring fence_lock.
>>> If you would write it like that, you would end up with a patch similar
>>> to drm/nouveau: add reservation to nouveau_gem_ioctl_cpu_prep. I think
>>> we should do this, an
>>>
>>> Ok, so the core does NOT need fence_lock because we can never drop
>>> reservations except in synccpu_write_grab and maybe
>>> ttm_bo_vm_fault_idle, but even in those cases reservation is done. So
>>> that could be used instead of fence_lock.
>>>
>>> nouveau_gem_ioctl_cpu_prep:
>>> Either block on a global spinlock or a local reservation lock. Doesn't
>>> matter much which, I don't need the need to keep a global lock for
>>> this function...
>>> 2 cases can happen in the trylock reservation failure case: buffer is
>>> not reserved, so it's not in the process of being evicted. buffer is
>>> reserved, which means it's being used in command submission right now,
>>> or in one of the functions described above (eg not idle).
>>>
>>> nouveau_gem_pushbuf_reloc_apply:
>>> has to call ttm_bo_wait with reservation, cannot be dropped.
>>>
>>> So for core ttm and nouveau the fence_lock is never needed, radeon has
>>> only 1 function that calls ttm_bo_wait which uses a reservation too.
>>> It doesn't need the fence_lock either.
>> And vmwgfx now also has a syccpu IOCTL (see drm-next).
>>
>> So assuming that we converted the functions that can be converted to
>> wait outside of reservation, the same way you have done with Nouveau,
>> leaving the ones that fall under 1) and 2) above, I would still argue
>> that a spinlock should be used because taking a reservation may
>> implicitly mean wait for gpu, and could give bad performance- and
>> latency charateristics. You shouldn't need to wait for gpu to check for
>> buffer idle.
> Except that without reservation you can't tell if the buffer is really
> idle, or is currently being
> used as part of some command submission/eviction before the fence
> pointer is set.
>

Yes, but when that matters, you're either in case 1 or case 2 again.
Otherwise, when you release the reservation, you still don't know.
A typical example of this is the vmwgfx synccpu ioctl, where you can
either choose to block command submission (not used currently)
or not (user-space inter-process synchronization). The former is a case
1 wait and holds reservation while waiting for idle and then ups
cpu_writers. The latter waits without reservation for previously
submitted rendering to finish.

/Thomas


> ~Maarten
>