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.