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

Am 22.07.2014 17:17, schrieb Daniel Vetter:
> On Tue, Jul 22, 2014 at 3:45 PM, Christian K?nig
> <deathsimple at vodafone.de> wrote:
>>> Would that be something you can agree to?
>>
>> No, the whole enable_signaling stuff should go away. No callback from
the
>> driver into the fence code, only the other way around.
>>
>> fence->signaled as well as fence->wait should become mandatory
and only
>> called from process context without holding any locks, neither atomic
nor
>> any mutex/semaphore (rcu might be ok).
> So for the enable_signaling, that's optional already. It's only for
> drivers that don't want to keep interrupts enabled all the time. You
> can opt out of that easily.
>
> Wrt holding no locks at all while calling into any fence functions,
> that's just not going to work out. The point here is to make different
> drivers work together and we can rework all the ttm and i915 code to
> work locklessly in all cases where they need to wait for someone to
> complete rendering. Or at least I don't think that's feasible. So
if
> you insist that no one might call into radeon code then we simply need
> to exclude radeon from participating in any shared fencing. But that's
> a bit pointless.
>
>>> Like I've said I think restricting the insanity other people
are willing
>>> to live with just because you don't like it isn't right.
But it is
>>> certainly right for you to insist on not being forced into any such
>>> design. I think the above would achieve this.
>>
>> I don't think so. If it's just me I would say that I'm just
to cautious and
>> the idea is still save to apply to the whole kernel.
>>
>> But since Dave, Jerome and Ben seems to have similar concerns I think
we
>> need to agree to a minimum and save interface for all drivers.
> Well I haven't yet seen a proposal that actually works.
How about this:

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.

Christian.
>   From an intel
> pov I don't care that much since we don't care about desktop prime,
so
> if radeon/nouveau don't want to do that, meh. Imo the design as-is is
> fairly sound, and as simple as it can get given the requirements. I
> haven't heard an  argument convincing me otherwise, so I guess we
> won't have prime support on linux that actually works, ever.
> -Daniel

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()?
> Locking
> correctness is enforced with some extremely nasty lockdep annotations
> + additional debugging infrastructure enabled with
> CONFIG_DEBUG_WW_MUTEX_SLOWPATH. We really need to be able to hold
> dma-buf ww_mutexes while updating fences or waiting for them. And
> obviously for ->wait we need non-atomic context, not just
> non-interrupt.
Sounds mostly reasonable, but for holding the dma-buf ww_mutex, wouldn't 
be an RCU be more appropriate here? E.g. aren't we just interested that 
the current assigned fence at some point is signaled?

Something like grab ww_mutexes, grab a reference to the current fence 
object, release ww_mutex, wait for fence, release reference to the fence 
object.

> 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.
> 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.

Christian.
> -Daniel