Re: [PATCH] drm/ttm: remove fence_lock

[Thomas Hellstrom <thomas@shipmail.org>]

On 10/18/2012 01:38 PM, Maarten Lankhorst wrote:
> Op 18-10-12 13:02, Thomas Hellstrom schreef:
>> On 10/18/2012 10:37 AM, Maarten Lankhorst wrote:
>>> Hey,
>>>
>>> Op 18-10-12 09:59, Thomas Hellstrom schreef:
>>>>
>>>> On 10/18/2012 09:28 AM, Thomas Hellstrom wrote:
>>>>> Hi, Maarten,
>>>>>
>>>>> As you know I have been having my doubts about this change.
>>>>> To me it seems insane to be forced to read the fence pointer under a
>>>>> reserved lock, simply because when you take the reserve lock, another
>>>>> process may have it and there is a substantial chance that that process
>>>>> will also be waiting for idle while holding the reserve lock.
>>> I think it makes perfect sense, the only times you want to read the fence
>>> is when you want to change the members protected by the reservation.
>> No, that's not true. A typical case (or the only case)
>> is where you want to do a map with no_wait semantics. You will want
>> to be able to access a buffer's results even if the eviction code
>> is about to schedule an unbind from the GPU, and have the buffer
>> reserved?
> Well either block on reserve or return -EBUSY if reserved, presumably the
> former..
>
> ttm_bo_vm_fault does the latter already, anyway

ttm_bo_vm_fault only trylocks to avoid a deadlock with mmap_sem, it's really
a waiting reserve but for different reasons. Typically a user-space app 
will keep
asynchronous maps to TTM during a buffer lifetime, and the buffer 
lifetime may
be long as user-space apps keep caches.
That's not the same as accessing a buffer after the GPU is done with it.

>
> You don't need to hold the reservation while performing the wait itself though,
> you could check if ttm_bo_wait(no_wait_gpu = true) returns -EBUSY, and if so
> take a ref to the sync_obj member and then wait after unreserving. You won't
> reset sync_obj member to NULL in that case, but that should be harmless.
> This will allow you to keep the reservations fast and short. Maybe a helper
> would be appropriate for this since radeon and nouveau both seem to do this.
>

The problem is that as long as other users are waiting for idle with 
reservation
held, for example to switch GPU engine or to delete a GPU bind, waiting
for reserve will in many case mean wait for GPU.

/Thomas