op 22-01-14 13:11, Thomas Hellstrom schreef: > On 01/22/2014 11:58 AM, Maarten Lankhorst wrote: >> op 22-01-14 11:27, Thomas Hellstrom schreef: >>> 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. >> Yeah you could, but what exactly are you waiting on then? If it's some >> specific existing rendering, >> I would argue that you should create an android userspace fence during >> command submission, >> or provide your own api to block on a specfic fence in userspace. >> >> If you don't then I think taking a reservation is not unreasonable. In >> the most common case the buffer is >> idle and not reserved, so it isn't contested. The actual waiting >> itself can be done without reservation held, >> by taking a reference on the fence. > Yeah, here is where we disagree. I'm afraid people will start getting > sloppy with reservations and use them to protect more stuff, and after a > while they start wondering why the GPU command queue drains... > Also the reservation_object's lock is a normal lock, so you should be able to pull info about it when enabling CONFIG_LOCKSTAT.
~Maarten
