Re: [PATCH 04/10] drm/ttm: change fence_lock to inner lock, v3

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

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.

~Maarten