| Message ID | 20191206105527.1130413-4-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] drm/i915/gem: Flush the pwrite through the chipset before signaling | expand |
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
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
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,
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(-)