From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup
Date: Thu, 23 May 2019 10:21:17 +0100 [thread overview]
Message-ID: <d482e1a9-a485-d513-bb7d-cb2e39f7bb24@linux.intel.com> (raw)
In-Reply-To: <20190523064933.23604-1-chris@chris-wilson.co.uk>
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@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
Regards,
Tvrtko
> +
> + mutex_lock(&i915->drm.struct_mutex);
> + i915_vma_destroy(work->vma);
> + mutex_unlock(&i915->drm.struct_mutex);
> +
> + kfree(work);
> +}
> +
> static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> {
> struct gen6_hw_ppgtt *ppgtt = to_gen6_ppgtt(i915_vm_to_ppgtt(vm));
> + struct gen6_ppgtt_cleanup_work *work = ppgtt->work;
>
> - i915_vma_destroy(ppgtt->vma);
> + /* FIXME remove the struct_mutex to bring the locking under control */
> + INIT_WORK(&work->base, gen6_ppgtt_cleanup_work);
> + work->vma = ppgtt->vma;
> + schedule_work(&work->base);
>
> gen6_ppgtt_free_pd(ppgtt);
> gen6_ppgtt_free_scratch(vm);
> @@ -2011,9 +2033,13 @@ static struct i915_hw_ppgtt *gen6_ppgtt_create(struct drm_i915_private *i915)
>
> ppgtt->base.vm.pte_encode = ggtt->vm.pte_encode;
>
> + ppgtt->work = kmalloc(sizeof(*ppgtt->work), GFP_KERNEL);
> + if (!ppgtt->work)
> + goto err_free;
> +
> err = gen6_ppgtt_init_scratch(ppgtt);
> if (err)
> - goto err_free;
> + goto err_work;
>
> ppgtt->vma = pd_vma_create(ppgtt, GEN6_PD_SIZE);
> if (IS_ERR(ppgtt->vma)) {
> @@ -2025,6 +2051,8 @@ static struct i915_hw_ppgtt *gen6_ppgtt_create(struct drm_i915_private *i915)
>
> err_scratch:
> gen6_ppgtt_free_scratch(&ppgtt->base.vm);
> +err_work:
> + kfree(ppgtt->work);
> err_free:
> kfree(ppgtt);
> return ERR_PTR(err);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 98fc71053f7c..38496039456b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -424,6 +424,8 @@ struct gen6_hw_ppgtt {
>
> unsigned int pin_count;
> bool scan_for_unused_pt;
> +
> + struct gen6_ppgtt_cleanup_work *work;
> };
>
> #define __to_gen6_ppgtt(base) container_of(base, struct gen6_hw_ppgtt, base)
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2019-05-23 9:21 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-23 6:49 [PATCH] drm/i915/gtt: Always acquire struct_mutex for gen6_ppgtt_cleanup Chris Wilson
2019-05-23 9:21 ` Tvrtko Ursulin [this message]
2019-05-23 10:01 ` Tvrtko Ursulin
2019-05-23 10:07 ` Chris Wilson
2019-05-23 10:14 ` Chris Wilson
2019-05-23 10:19 ` Tvrtko Ursulin
2019-05-23 10:20 ` Tvrtko Ursulin
2019-05-23 10:21 ` Chris Wilson
2019-05-23 14:28 ` ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2019-05-23 14:58 ` ✓ Fi.CI.BAT: success " Patchwork
2019-05-24 21:48 ` ✓ Fi.CI.IGT: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d482e1a9-a485-d513-bb7d-cb2e39f7bb24@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.