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 at 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? >>> 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 > >
