Re: [RFC PATCH v1 08/16] drm/radeon: use common fence implementation for fences

["Christian König" <deathsimple@vodafone.de>]

Am 19.05.2014 15:35, schrieb Maarten Lankhorst:
> op 19-05-14 14:30, Christian König schreef:
>> Am 19.05.2014 12:10, schrieb Maarten Lankhorst:
>>> op 19-05-14 10:27, Christian König schreef:
>>>> Am 19.05.2014 10:00, schrieb Maarten Lankhorst:
>>>> [SNIP]
>>>> The problem here is that the whole approach collides with the way 
>>>> we do reset handling from a conceptual point of view. Every IOCTL 
>>>> or other call chain into the driver is protected by the read side 
>>>> of the exclusive_lock semaphore. So in the case of a GPU lockup we 
>>>> can take the write side of the semaphore and so make sure that we 
>>>> have nobody else accessing the hardware or internal driver 
>>>> structures only changed at init time.
>>>>
>>>> Leaking a drivers IRQ context into another driver as well as 
>>>> calling into a driver in atomic context is just a quite uncommon 
>>>> approach and should be considered very carefully.
>>>>
>>>> I would rather vote for a completely synchronous interface only 
>>>> allowing blocking waits and checks if a fence is signaled from not 
>>>> atomic context.
>>>>
>>>> If a driver needs to avoid blocking it should just use a workqueue 
>>>> and checking a fence outside your own driver is probably be better 
>>>> done in a bottom halve handler anyway.
>>>
>>> Except that you might want to do something like
>>> fence_is_signaled() in another driver to check whether you need to
>>> defer, or can submit the batch buffer immediately, saving a bunch of
>>> context switches. Running the is_signaled atomic is really useful here
>>> because it means you can't do too many scary things in your is_signaled
>>> handler.
>>
>> This is indeed a nice optimization, but nothing more. If you want to 
>> provide a is_signalled interface for atomic context then this should 
>> be optional, not mandatory.
> See below.
>>> In case of enable_signaling it was the only sane solution, because
>>> fence_signal can be called from irq context, and any calls after 
>>> that to
>>> fence_add_callback and fence_wait aren't allowed to do anything, so
>>> fence_enable_sw_signaling and the default wait implementation must be
>>> atomic. fence_wait itself doesn't have to be, so it's easy to grab
>>> exclusive_lock there.
>>
>> I don't think you understood my point here: Completely drop 
>> enable_signaling, it's unnecessary and only complicates the interface.
>>
>> We purposely avoided exactly this paradigm in the past and I haven't 
>> seen any good argument to start with it now.
>
> In the common case a lot more fences will be emitted than will be 
> waited on.
> This means it makes sense to delay signaling a fence with fence_signal 
> for
> as long as possible. But when a fence user wants to work with a fence
> some way is needed to ensure that the fence will complete. This is the 
> idea
> behind .enable_signaling, it tells the fence driver to call 
> fence_signal on
> the fence 'soon' because there are now waiters for it.
>
> The atomic .signaled is optional, and can be set to NULL, but there is
> no guarantee that fence_is_signaled will ever return true in that case,
> unless fence_enable_sw_signaling is called (which calls 
> .enable_signaling).
>
> Providing a custom wait function is optional in the interface, if the 
> default wait
> function is used all waiters are signaled when fence_signal is called.
>
> Removing enable_signaling would only make sense if fence_signal was 
> removed too,
> but that would mean that fence_is_signaled could no longer exist in 
> the core fence
> code, and would mean completely rewriting the interface.
>
And this is what I'm suggesting here.

We have avoided quite hard adding any form of those callbacks in the 
past and I don't really see a reason why that would have changed. For 
example see the discussion here: 
http://lists.freedesktop.org/archives/dri-devel/2012-May/022388.html

Jerome and Dave rejected my approach for handling the sub allocator 
through a callback for exactly the same reason. And that was even for 
call chains inside the same driver, you're suggesting this for cross 
driver synchronization.

Christian.