[Intel-gfx] [PATCH 22/22] drm/i915: Export our request as a dma-buf fence on the reservation object

[Daniel Vetter]

On Thu, Jul 28, 2016 at 11:40:29AM +0100, Chris Wilson wrote:
> On Thu, Jul 28, 2016 at 12:32:42PM +0200, Daniel Vetter wrote:
> > On Wed, Jul 27, 2016 at 12:15:00PM +0100, Chris Wilson wrote:
> > > If the GEM objects being rendered with in this request have been
> > > exported via dma-buf to a third party, hook ourselves into the dma-buf
> > > reservation object so that the third party can serialise with our
> > > rendering via the dma-buf fences.
> > > 
> > > Testcase: igt/prime_busy
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > 
> > Style nit: I prefer ww_mutex_lock(&resv->lock, NULL); over
> > mutex_lock(&resv->lock.base). The former makes it clear it's a ww mutex,
> > but we don't bother with the multi-lock dance. The latter needles around
> > in implemenation details, which it really shouldn't. Please change.
> 
> Passing NULL as ww_acquite_ctx is illegal.

Hm, where exactly do you see that? kerneldoc for ww_mutex_lock clearly
says that it can be NULL, and the static inline has a check for it and
calls mutex_lock in the else path. Which means it /should/ boil down to
the exact same code after gcc has pondered it enough.

> > The other wonky bit is that changing reservations on multiple objects
> > without the full ww mutex dance is deadlock-risky. But only when you both
> > add and wait/stall on fences.
> 
> Note that it is only so when trying to lock multiple objects simultaneously,
> which we do not require.

With deadlocks I didn't mean locking deadlocks, but loops in the fences.
Sure we eventually recover when you have a good gpu driver with hang
check, but other drivers might be less fortunate. And if you lock objects
individually and update their fences individually it's fairly simple to
race command submission (on different drivers) against each another with
objects listed in reverse and end up with a ABBA fence depency. The fix
for that is to:
1. grab all the reservation locks
2. assemeble the full list of fences you need to stall on
3. update the reservation with the new fence for the current rendering job
4. only then release all locks

Without that drama ensues ;-) And the current patch is still pretty far
away from the above sequence (but it does roughly match to our overall
execbuf logic, with s/lock/pin/).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch