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

[Chris Wilson]

On Thu, Jul 28, 2016 at 10:14:36PM +0200, Daniel Vetter wrote:
> On Thu, Jul 28, 2016 at 01:45:44PM +0100, Chris Wilson wrote:
> > On Thu, Jul 28, 2016 at 02:28:10PM +0200, Daniel Vetter wrote:
> > > On Thu, Jul 28, 2016 at 01:17:39PM +0100, Chris Wilson wrote:
> > > > On Thu, Jul 28, 2016 at 01:59:45PM +0200, Daniel Vetter wrote:
> > > > > 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.
> > > > 
> > > > But then explodes. Look at the lockdep. Clearly the kerneldoc is wrong.
> > > > Good job I was reading the code :-p
> > > 
> > > Hm, that'd be a bug in the ww_mutex_lock, and we make plenty use uf a NULL
> > > ctx in drm_modeset_lock.c. How exactly does this blow up? Can you attach
> > > the splat please?
> > 
> > &ctx->dep_map with a NULL pointer != 0 which blows up when it gets
> > deferenced inside lockdep, __lockdep_acquire():
> >         if (nest_lock && !__lock_is_held(nest_lock))
> 
> In my current tree I have:
> 
> void __sched ww_mutex_unlock(struct ww_mutex *lock)
> {
> 	/*
> 	 * The unlocking fastpath is the 0->1 transition from 'locked'
> 	 * into 'unlocked' state:
> 	 */
> 	if (lock->ctx) {
> #ifdef CONFIG_DEBUG_MUTEXES
> 		DEBUG_LOCKS_WARN_ON(!lock->ctx->acquired);
> #endif
> 		if (lock->ctx->acquired > 0)
> 			lock->ctx->acquired--;
> 		lock->ctx = NULL;
> 	}
> 
> #ifndef CONFIG_DEBUG_MUTEXES
> 	/*
> 	 * When debugging is enabled we must not clear the owner before time,
> 	 * the slow path will always be taken, and that clears the owner field
> 	 * after verifying that it was indeed current.
> 	 */
> 	mutex_clear_owner(&lock->base);
> #endif
> 	__mutex_fastpath_unlock(&lock->base.count, __mutex_unlock_slowpath);
> }
> 
> and
> 
> static inline int ww_mutex_lock(struct ww_mutex *lock, struct ww_acquire_ctx *ctx)
> {
> 	if (ctx)
> 		return __ww_mutex_lock(lock, ctx);
> 
> 	mutex_lock(&lock->base);
> 	return 0;
> }
> 
> I really don't see where we can blow up on NULL ctx when using
> ww_mutex_lock/unlock. And if you look at some of the get* drm ioctls, you
> can chase a drm_modeset_lock(obj, NULL) down to exactly such a
> ww_mutex_lock(lock, NULL) call, and evidently X doesn't crash.
> 
> In short I still can't find how you managed to blow up on the nest_lock
> being NULL anywhere, at least in ww_mutex code.

When I first tried ww_mutex_lock, I got an oops. However, I've just
tried with ww_mutex_lock(NULL) with KASAN and lockdep/RCU, and it
worked.

Pebkac.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre