[Intel-gfx] [PATCH] drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup

Thu May 23 10:21:41 UTC 2019

Quoting Tvrtko Ursulin (2019-05-23 11:19:30)
> 
> On 23/05/2019 11:14, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-05-23 11:07:50)
> >> Quoting Tvrtko Ursulin (2019-05-23 11:01:09)
> >>>
> >>> On 23/05/2019 10:21, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 23/05/2019 07:49, Chris Wilson wrote:
> >>>>> We rearranged the vm_destroy_ioctl to avoid taking struct_mutex, little
> >>>>> realising that buried underneath the gen6 ppgtt release path was a
> >>>>> struct_mutex requirement (to remove its GGTT vma). Until that
> >>>>> struct_mutex is vanquished, take a detour in gen6_ppgtt_cleanup to do
> >>>>> the i915_vma_destroy from inside a worker under the struct_mutex.
> >>>>>
> >>>>> <4> [257.740160] WARN_ON(debug_locks &&
> >>>>> !lock_is_held(&(&vma->vm->i915->drm.struct_mutex)->dep_map))
> >>>>> <4> [257.740213] WARNING: CPU: 3 PID: 1507 at
> >>>>> drivers/gpu/drm/i915/i915_vma.c:841 i915_vma_destroy+0x1ae/0x3a0 [i915]
> >>>>> <4> [257.740214] Modules linked in: snd_hda_codec_hdmi i915
> >>>>> x86_pkg_temp_thermal mei_hdcp coretemp crct10dif_pclmul crc32_pclmul
> >>>>> ghash_clmulni_intel snd_hda_codec_realtek snd_hda_codec_generic
> >>>>> snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core r8169 realtek
> >>>>> snd_pcm mei_me mei prime_numbers lpc_ich
> >>>>> <4> [257.740224] CPU: 3 PID: 1507 Comm: gem_vm_create Tainted: G
> >>>>> U            5.2.0-rc1-CI-CI_DRM_6118+ #1
> >>>>> <4> [257.740225] Hardware name: MSI MS-7924/Z97M-G43(MS-7924), BIOS
> >>>>> V1.12 02/15/2016
> >>>>> <4> [257.740249] RIP: 0010:i915_vma_destroy+0x1ae/0x3a0 [i915]
> >>>>> <4> [257.740250] Code: 00 00 00 48 81 c7 c8 00 00 00 e8 ed 08 f0 e0 85
> >>>>> c0 0f 85 78 fe ff ff 48 c7 c6 e8 ec 30 a0 48 c7 c7 da 55 33 a0 e8 42
> >>>>> 8c e9 e0 <0f> 0b 8b 83 40 01 00 00 85 c0 0f 84 63 fe ff ff 48 c7 c1 c1
> >>>>> 58 33
> >>>>> <4> [257.740251] RSP: 0018:ffffc90000aafc68 EFLAGS: 00010282
> >>>>> <4> [257.740252] RAX: 0000000000000000 RBX: ffff8883f7957840 RCX:
> >>>>> 0000000000000003
> >>>>> <4> [257.740253] RDX: 0000000000000046 RSI: 0000000000000006 RDI:
> >>>>> ffffffff8212d1b9
> >>>>> <4> [257.740254] RBP: ffffc90000aafcc8 R08: 0000000000000000 R09:
> >>>>> 0000000000000000
> >>>>> <4> [257.740255] R10: 0000000000000000 R11: 0000000000000000 R12:
> >>>>> ffff8883f4d5c2a8
> >>>>> <4> [257.740256] R13: ffff8883f4d5d680 R14: ffff8883f4d5c668 R15:
> >>>>> ffff8883f4d5c2f0
> >>>>> <4> [257.740257] FS:  00007f777fa8fe40(0000) GS:ffff88840f780000(0000)
> >>>>> knlGS:0000000000000000
> >>>>> <4> [257.740258] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >>>>> <4> [257.740259] CR2: 00007f777f6522b0 CR3: 00000003c612a006 CR4:
> >>>>> 00000000001606e0
> >>>>> <4> [257.740260] Call Trace:
> >>>>> <4> [257.740283]  gen6_ppgtt_cleanup+0x25/0x60 [i915]
> >>>>> <4> [257.740306]  i915_ppgtt_release+0x102/0x290 [i915]
> >>>>> <4> [257.740330]  i915_gem_vm_destroy_ioctl+0x7c/0xa0 [i915]
> >>>>> <4> [257.740376]  ? i915_gem_vm_create_ioctl+0x160/0x160 [i915]
> >>>>> <4> [257.740379]  drm_ioctl_kernel+0x83/0xf0
> >>>>> <4> [257.740382]  drm_ioctl+0x2f3/0x3b0
> >>>>> <4> [257.740422]  ? i915_gem_vm_create_ioctl+0x160/0x160 [i915]
> >>>>> <4> [257.740426]  ? _raw_spin_unlock_irqrestore+0x39/0x60
> >>>>> <4> [257.740430]  do_vfs_ioctl+0xa0/0x6e0
> >>>>> <4> [257.740433]  ? lock_acquire+0xa6/0x1c0
> >>>>> <4> [257.740436]  ? __task_pid_nr_ns+0xb9/0x1f0
> >>>>> <4> [257.740439]  ksys_ioctl+0x35/0x60
> >>>>> <4> [257.740441]  __x64_sys_ioctl+0x11/0x20
> >>>>> <4> [257.740443]  do_syscall_64+0x55/0x1c0
> >>>>> <4> [257.740445]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >>>>>
> >>>>> References: e0695db7298e ("drm/i915: Create/destroy VM (ppGTT) for use
> >>>>> with contexts")
> >>>>> Fixes: 7f3f317a66ca ("drm/i915: Restore control over ppgtt for context
> >>>>> creation ABI")
> >>>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>>>> ---
> >>>>>    drivers/gpu/drm/i915/i915_gem_gtt.c | 32 +++++++++++++++++++++++++++--
> >>>>>    drivers/gpu/drm/i915/i915_gem_gtt.h |  2 ++
> >>>>>    2 files changed, 32 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> index 9ed41aefb456..5a350251766a 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >>>>> @@ -1828,11 +1828,33 @@ static void gen6_ppgtt_free_pd(struct
> >>>>> gen6_hw_ppgtt *ppgtt)
> >>>>>                free_pt(&ppgtt->base.vm, pt);
> >>>>>    }
> >>>>> +struct gen6_ppgtt_cleanup_work {
> >>>>> +    struct work_struct base;
> >>>>> +    struct i915_vma *vma;
> >>>>> +};
> >>>>> +
> >>>>> +static void gen6_ppgtt_cleanup_work(struct work_struct *wrk)
> >>>>> +{
> >>>>> +    struct gen6_ppgtt_cleanup_work *work =
> >>>>> +        container_of(wrk, typeof(*work), base);
> >>>>> +    struct drm_i915_private *i915 = work->vma->vm->i915;
> >>>>
> >>>> Does the sequence need an extra reference on ppgtt for dereferencing the
> >>>> vm here? Alternatively storing i915 in the work struct could work around
> >>>> that.
> >>>
> >>> Nope - vma has a pointer to vm as well which it will dereference. So if
> >>> reference is needed it's needed - it looks like it is to me, since only
> >>> contexts take them, which vma's still rely on, right?
> >>
> >> Nah, vm is decidedly dead at this point, we just need to stuff the i915
> >> pointer into the cleanup_work.
> > 
> > Argh. vma is in ggtt, so vma->vm is alive and kicking -- not the vm we
> > just destroyed.
> 
> Why is this an argh? :) Doesn't it mean you were right - just storing a 
> pointer to i915 in work struct should work. I missed the fact they are 
> two separate VMs.

We don't need to store an extra pointer as vma->vm is valid as
i915->ggtt isn't destroyed until after the worker is flushed.
-Chris