[4/5] drm/i915/gem: Pin gen6_ppgtt prior to constructing the request
diff mbox series

Message ID 20191206105527.1130413-4-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [1/5] drm/i915/gem: Flush the pwrite through the chipset before signaling
Related show

Commit Message

Chris Wilson Dec. 6, 2019, 10:55 a.m. UTC
All pinning must be done prior to i915_request_create, to avoid
timeline->mutex inversions.

Here we slightly abuse the context_barrier_task stages to utilise the
'skip' decision as an opportunity to acquire the pin on the new ppgtt.
Consider it s/skip/prepare/. At the moment, we only have on user of
context_barrier_task, so it might be worth breaking it down for the
specific task of set-vm and refactor it later if we find a second
purpose.

<4> [402.377487] WARNING: possible circular locking dependency detected
<4> [402.377493] 5.4.0-rc8-CI-CI_DRM_7491+ #1 Tainted: G     U
<4> [402.377497] ------------------------------------------------------
<4> [402.377502] gem_exec_parall/2506 is trying to acquire lock:
<4> [402.377507] ffff888403cdac70 (&kernel#2){+.+.}, at: i915_request_create+0x16/0x1c0 [i915]
<4> [402.377593]
but task is already holding lock:
<4> [402.377597] ffff88835efad550 (&ppgtt->pin_mutex){+.+.}, at: gen6_ppgtt_pin+0x4d/0x110 [i915]
<4> [402.377660]
which lock already depends on the new lock.

<4> [402.377664]
the existing dependency chain (in reverse order) is:
<4> [402.377668]
-> #1 (&ppgtt->pin_mutex){+.+.}:
<4> [402.377674]        __mutex_lock+0x9a/0x9d0
<4> [402.377713]        gen6_ppgtt_pin+0x4d/0x110 [i915]
<4> [402.377756]        emit_ppgtt_update+0x1dc/0x370 [i915]
<4> [402.377801]        context_barrier_task+0x176/0x310 [i915]
<4> [402.377844]        ctx_setparam+0x400/0xb10 [i915]
<4> [402.377886]        i915_gem_context_setparam_ioctl+0xc8/0x160 [i915]
<4> [402.377891]        drm_ioctl_kernel+0xa7/0xf0
<4> [402.377895]        drm_ioctl+0x2e1/0x390
<4> [402.377899]        do_vfs_ioctl+0xa0/0x6f0
<4> [402.377903]        ksys_ioctl+0x35/0x60
<4> [402.377906]        __x64_sys_ioctl+0x11/0x20
<4> [402.377910]        do_syscall_64+0x4f/0x210
<4> [402.377914]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
<4> [402.377917]
-> #0 (&kernel#2){+.+.}:
<4> [402.377923]        __lock_acquire+0x1328/0x15d0
<4> [402.377926]        lock_acquire+0xa7/0x1c0
<4> [402.377930]        __mutex_lock+0x9a/0x9d0
<4> [402.377977]        i915_request_create+0x16/0x1c0 [i915]
<4> [402.378013]        intel_engine_flush_barriers+0x4c/0x100 [i915]
<4> [402.378062]        i915_ggtt_pin+0x7d/0x130 [i915]
<4> [402.378108]        gen6_ppgtt_pin+0x9c/0x110 [i915]
<4> [402.378148]        ring_context_pin+0x2e/0xc0 [i915]
<4> [402.378183]        __intel_context_do_pin+0x6b/0x190 [i915]
<4> [402.378226]        i915_gem_do_execbuffer+0x180c/0x26b0 [i915]
<4> [402.378268]        i915_gem_execbuffer2_ioctl+0x11b/0x460 [i915]
<4> [402.378272]        drm_ioctl_kernel+0xa7/0xf0
<4> [402.378275]        drm_ioctl+0x2e1/0x390
<4> [402.378279]        do_vfs_ioctl+0xa0/0x6f0
<4> [402.378282]        ksys_ioctl+0x35/0x60
<4> [402.378286]        __x64_sys_ioctl+0x11/0x20
<4> [402.378289]        do_syscall_64+0x4f/0x210
<4> [402.378292]        entry_SYSCALL_64_after_hwframe+0x49/0xbe
<4> [402.378295]
other info that might help us debug this:

<4> [402.378299]  Possible unsafe locking scenario:

<4> [402.378302]        CPU0                    CPU1
<4> [402.378305]        ----                    ----
<4> [402.378307]   lock(&ppgtt->pin_mutex);
<4> [402.378310]                                lock(&kernel#2);
<4> [402.378314]                                lock(&ppgtt->pin_mutex);
<4> [402.378317]   lock(&kernel#2);
<4> [402.378320]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Andi Shyti Dec. 6, 2019, 11:31 p.m. UTC | #1
Hi Chris,

> All pinning must be done prior to i915_request_create, to avoid
> timeline->mutex inversions.
> 
> Here we slightly abuse the context_barrier_task stages to utilise the
> 'skip' decision as an opportunity to acquire the pin on the new ppgtt.
> Consider it s/skip/prepare/. At the moment, we only have on user of
> context_barrier_task, so it might be worth breaking it down for the
> specific task of set-vm and refactor it later if we find a second
> purpose.

[...]

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 9f1dc96b10a6..9d8d75765ee4 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1141,8 +1141,6 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>  		*cs++ = MI_NOOP;
>  		intel_ring_advance(rq, cs);
>  	} else {
> -		/* ppGTT is not part of the legacy context image */
> -		gen6_ppgtt_pin(i915_vm_to_ppgtt(vm));
>  	}

mh? Am I not seeing something obvious? Can we remove the else?

>  
>  	return 0;
> @@ -1150,10 +1148,20 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
>  
>  static bool skip_ppgtt_update(struct intel_context *ce, void *data)
>  {
> +	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
> +		return true;
> +
>  	if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> -		return !ce->state;
> -	else
> -		return !atomic_read(&ce->pin_count);
> +		return false;
> +
> +	if (!atomic_read(&ce->pin_count))
> +		return true;
> +
> +	/* ppGTT is not part of the legacy context image */
> +	if (gen6_ppgtt_pin(i915_vm_to_ppgtt(ce->vm)))
> +		return true;
> +
> +	return false;

looks correct, a bit tricky, but I don't see any issue.

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Andi
Chris Wilson Dec. 6, 2019, 11:35 p.m. UTC | #2
Quoting Andi Shyti (2019-12-06 23:31:26)
> Hi Chris,
> 
> > All pinning must be done prior to i915_request_create, to avoid
> > timeline->mutex inversions.
> > 
> > Here we slightly abuse the context_barrier_task stages to utilise the
> > 'skip' decision as an opportunity to acquire the pin on the new ppgtt.
> > Consider it s/skip/prepare/. At the moment, we only have on user of
> > context_barrier_task, so it might be worth breaking it down for the
> > specific task of set-vm and refactor it later if we find a second
> > purpose.
> 
> [...]
> 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > index 9f1dc96b10a6..9d8d75765ee4 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > @@ -1141,8 +1141,6 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >               *cs++ = MI_NOOP;
> >               intel_ring_advance(rq, cs);
> >       } else {
> > -             /* ppGTT is not part of the legacy context image */
> > -             gen6_ppgtt_pin(i915_vm_to_ppgtt(vm));
> >       }
> 
> mh? Am I not seeing something obvious? Can we remove the else?

Sure, I just have this thing about if() else if() that feels unbalanced
Just feels odd not to have something there.  :)

> 
> >  
> >       return 0;
> > @@ -1150,10 +1148,20 @@ static int emit_ppgtt_update(struct i915_request *rq, void *data)
> >  
> >  static bool skip_ppgtt_update(struct intel_context *ce, void *data)
> >  {
> > +     if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
> > +             return true;
> > +
> >       if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
> > -             return !ce->state;
> > -     else
> > -             return !atomic_read(&ce->pin_count);
> > +             return false;
> > +
> > +     if (!atomic_read(&ce->pin_count))
> > +             return true;
> > +
> > +     /* ppGTT is not part of the legacy context image */
> > +     if (gen6_ppgtt_pin(i915_vm_to_ppgtt(ce->vm)))
> > +             return true;
> > +
> > +     return false;
> 
> looks correct, a bit tricky, but I don't see any issue.

The issue is the code is creaking beyond its design tolerances. :)
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 9f1dc96b10a6..9d8d75765ee4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1141,8 +1141,6 @@  static int emit_ppgtt_update(struct i915_request *rq, void *data)
 		*cs++ = MI_NOOP;
 		intel_ring_advance(rq, cs);
 	} else {
-		/* ppGTT is not part of the legacy context image */
-		gen6_ppgtt_pin(i915_vm_to_ppgtt(vm));
 	}
 
 	return 0;
@@ -1150,10 +1148,20 @@  static int emit_ppgtt_update(struct i915_request *rq, void *data)
 
 static bool skip_ppgtt_update(struct intel_context *ce, void *data)
 {
+	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags))
+		return true;
+
 	if (HAS_LOGICAL_RING_CONTEXTS(ce->engine->i915))
-		return !ce->state;
-	else
-		return !atomic_read(&ce->pin_count);
+		return false;
+
+	if (!atomic_read(&ce->pin_count))
+		return true;
+
+	/* ppGTT is not part of the legacy context image */
+	if (gen6_ppgtt_pin(i915_vm_to_ppgtt(ce->vm)))
+		return true;
+
+	return false;
 }
 
 static int set_ppgtt(struct drm_i915_file_private *file_priv,