Hey, Op 20-11-12 16:08, Thomas Hellstrom schreef: > On 11/20/2012 02:13 PM, Maarten Lankhorst wrote: >> Op 20-11-12 13:03, Thomas Hellstrom schreef: >>> On 11/20/2012 12:33 PM, Maarten Lankhorst wrote: >>>> Op 20-11-12 08:48, Thomas Hellstrom schreef: >>>>> On 11/19/2012 04:33 PM, Maarten Lankhorst wrote: >>>>>> Op 19-11-12 16:04, Thomas Hellstrom schreef: >>>>>>> On 11/19/2012 03:17 PM, Thomas Hellstrom wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> This patch looks mostly good, although I think ttm_bo_cleanup_refs becomes overly complicated: >>>>>>>> Could this do, or am I missing something? >>>>>>>> >>>>>>> Actually, my version is bad, because ttm_bo_wait() is called with the lru lock held. >>>>>>> >>>>>>> /Thomas >>>>>> Oh digging through it made me remember why I had to release the reservation early and >>>>>> had to allow move_notify to be called without reservation. >>>>>> >>>>>> Fortunately move_notify has a NULL parameter, which is the only time that happens, >>>>>> so you can still check do BUG_ON(mem != NULL && !ttm_bo_reserved(bo)); in your >>>>>> move_notify handler. >>>>>> >>>>>> 05/10 removed the loop and assumed no new fence could be attached after the driver has >>>>>> declared the bo dead. >>>>>> >>>>>> However, at that point it may no longer hold a reservation to confirm this, that's why >>>>>> I moved the cleanup to be done in the release_list handler. It could still be done in >>>>>> ttm_bo_release, but we no longer have a reservation after we waited. Getting >>>>>> a reservation can fail if the bo is imported for example. >>>>>> >>>>>> While it would be true that in that case a new fence may be attached as well, that >>>>>> would be less harmful since that operation wouldn't involve this device, so the >>>>>> ttm bo can still be removed in that case. When that time comes I should probably >>>>>> fix up that WARN_ON(ret) in ttm_bo_cleanup_refs. :-) >>>>>> >>>>>> I did add a WARN_ON(!atomic_read(&bo->kref.refcount)); to >>>>>> ttm_bo_reserve and ttm_eu_reserve_buffers to be sure nothing is done on the device >>>>>> itself. If that is too paranoid, those WARN_ON's could be dropped. I prefer to leave them >>>>>> in for a kernel release or 2. But according to the rules that would be the only time you >>>>>> could attach a new fence and trigger the WARN_ON for now.. >>>>> Hmm, I'd appreciate if you could group patches with functional changes that depend on eachother togeteher, >>>>> and "this is done because ...", which makes it much easier to review, (and to follow the commit history in case >>>>> something goes terribly wrong and we need to revert). >>>>> >>>>> Meanwhile I'll take a look at the final ttm_bo.c and see if I can spot any culprits. >>>>> >>>>> In general, as long as a bo is on a LRU list, we must be able to attach fences because of accelerated eviction. >>>> I thought it was deliberately designed in such a way that it was kept on the lru list, >>>> but since it's also on the ddestroy list it won't start accelerated eviction, >>>> since it branches into cleanup_refs early, and lru_lock still protects all the list entries. >>> I used bad wording. I meant that unbinding might be accelerated, but currently (quite inefficiently) >>> do synchronized unbinding, assuming that only the CPU can do that. When we start to support >>> unsynchronized moves, we need to be able to attach fences at least at the last move_notify(bo, NULL); >> Would you need to wait in that case on fence_wait being completed before calling move_notify? >> >> If not, you would still only need to perform one wait, but you'd have to make sure move_notify only gets >> called by 1 thread before checking the fence pointer and performing a wait. At that point you still hold the >> lru_lock though, so it shouldn't be too hard to make something safe. > > I think typically a driver that wants to implement asynchronous moves don't want to wait before calling > move_notify, but may wait in move_notify or move. Typically (upcoming vmwgfx) it would invalidate the buffer in move_notify(bo, NULL), attach a fence and then use the normal delayed destroy to wait on that fence before destroying the buffer. > > Otherwise, since binds / unbinds are handled in the GPU command stream there's never any need to wait for moves except when there's a CPU > access. Well, nouveau actually needs fence_wait to finish first, since vm changes are out of band. But I guess it should be possible to attach it as work to the fence when it's signaled, and I may want to do something like that already for performance reasons in a different place, so I guess it doesn't matter. Is calling move_notify(bo, NULL) legal and a noop the second time? That would save a flag in the bo to check if it's called already, although I suppose we could always define a TTM_BO_PRIV_FLAG_* for it otherwise. move_notify might end up being called with the lru_lock held, but that shouldn't be a problem. ~Maarten _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
