Re: [Nouveau] [PATCH 09/17] drm/radeon: use common fence implementation for fences

[Rob Clark <robdclark@gmail.com>]

On Wed, Jul 23, 2014 at 2:52 AM, Christian König
<christian.koenig@amd.com> wrote:
> Am 23.07.2014 08:40, schrieb Maarten Lankhorst:
>
>> op 22-07-14 17:59, Christian König schreef:
>>>
>>> Am 22.07.2014 17:42, schrieb Daniel Vetter:
>>>>
>>>> On Tue, Jul 22, 2014 at 5:35 PM, Christian König
>>>> <christian.koenig@amd.com> wrote:
>>>>>
>>>>> Drivers exporting fences need to provide a fence->signaled and a
>>>>> fence->wait
>>>>> function, everything else like fence->enable_signaling or calling
>>>>> fence_signaled() from the driver is optional.
>>>>>
>>>>> Drivers wanting to use exported fences don't call fence->signaled or
>>>>> fence->wait in atomic or interrupt context, and not with holding any
>>>>> global
>>>>> locking primitives (like mmap_sem etc...). Holding locking primitives
>>>>> local
>>>>> to the driver is ok, as long as they don't conflict with anything
>>>>> possible
>>>>> used by their own fence implementation.
>>>>
>>>> Well that's almost what we have right now with the exception that
>>>> drivers are allowed (actually must for correctness when updating
>>>> fences) the ww_mutexes for dma-bufs (or other buffer objects).
>>>
>>> In this case sorry for so much noise. I really haven't looked in so much
>>> detail into anything but Maarten's Radeon patches.
>>>
>>> But how does that then work right now? My impression was that it's
>>> mandatory for drivers to call fence_signaled()?
>>
>> It's only mandatory to call fence_signal() if the .enable_signaling
>> callback has been called, else you can get away with never calling signaling
>> a fence at all before dropping the last refcount to it.
>> This allows you to keep interrupts disabled when you don't need them.
>
>
> Can we somehow avoid the need to call fence_signal() at all? The interrupts
> at least on radeon are way to unreliable for such a thing. Can
> enable_signalling fail? What's the reason for fence_signaled() in the first
> place?
>

the device you are sharing with may not be able to do hw<->hw
signalling.. think about buffer sharing w/ camera, for example.

You probably want your ->enable_signalling() to enable whatever
workaround periodic-polling you need to do to catch missed irq's (and
then call fence->signal() once you detect the fence has passed.

fwiw, I haven't had a chance to read this whole thread yet, but I
expect that a lot of the SoC devices, especially ones with separate
kms-only display and gpu drivers, will want callback from gpu's irq to
bang a few display controller registers.  I agree in general callbacks
from atomic ctx is probably something you want to avoid, but in this
particular case I think it is worth the extra complexity.  Nothing is
stopping a driver from using a callback that just chucks something on
a workqueue, whereas the inverse is not possible.

BR,
-R

>
>>>> Agreed that any shared locks are out of the way (especially stuff like
>>>> dev->struct_mutex or other non-strictly driver-private stuff, i915 is
>>>> really bad here still).
>>>
>>> Yeah that's also an point I've wanted to note on Maartens patch. Radeon
>>> grabs the read side of it's exclusive semaphore while waiting for fences
>>> (because it assumes that the fence it waits for is a Radeon fence).
>>>
>>> Assuming that we need to wait in both directions with Prime (e.g. Intel
>>> driver needs to wait for Radeon to finish rendering and Radeon needs to wait
>>> for Intel to finish displaying), this might become a perfect example of
>>> locking inversion.
>>
>> In the preliminary patches where I can sync radeon with other GPU's I've
>> been very careful in all the places that call into fences, to make sure that
>> radeon wouldn't try to handle lockups for a different (possibly also radeon)
>> card.
>
>
> That's actually not such a good idea.
>
> In case of a lockup we need to handle the lockup cause otherwise it could
> happen that radeon waits for the lockup to be resolved and the lockup
> handling needs to wait for a fence that's never signaled because of the
> lockup.
>
> Christian.
>
>
>>
>> This is also why fence_is_signaled should never block, and why it trylocks
>> the exclusive_lock. :-) I think lockdep would complain if I grabbed
>> exclusive_lock while blocking in is_signaled.
>>
>>>> So from the core fence framework I think we already have exactly this,
>>>> and we only need to adjust the radeon implementation a bit to make it
>>>> less risky and invasive to the radeon driver logic.
>>>
>>> Agree. Well the biggest problem I see is that exclusive semaphore I need
>>> to take when anything calls into the driver. For the fence code I need to
>>> move that down into the fence->signaled handler, cause that now can be
>>> called from outside the driver.
>>>
>>> Maarten solved this by telling the driver in the lockup handler (where we
>>> grab the write side of the exclusive lock) that all interrupts are already
>>> enabled, so that fence->signaled hopefully wouldn't mess with the hardware
>>> at all. While this probably works, it just leaves me with a feeling that we
>>> are doing something wrong here.
>>
>> There is unfortunately no global mechanism to say 'this card is locked up,
>> please don't call into any of my fences', and I don't associate fences with
>> devices, and radeon doesn't keep a global list of fences.
>> If all of that existed, it would complicate the interface and its callers
>> a lot, while I like to keep things simple.
>> So I did the best I could, and simply prevented the fence calls from
>> fiddling with the hardware. Fortunately gpu lockup is not a common
>> operation. :-)
>>
>> ~Maarten
>>
>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel