Quoting Janusz Krzysztofik (2020-05-18 20:17:19)
> GGTT including its scratch page is not cleaned up until driver release.
> Since DMA mappings still exist before scratch page cleanup, unmapping
> them on last device close after the driver has been already removed may
> be judged by intel-iommu code as a bug and result in kernel panic.
>
> Since we abort requests and block user access to hardware on device
> removal, there seems not much sense in still keeping GGTT. Clean it up
> on driver remove. However, skip GGTT address space cleanup as its
> mutext may still be needed if there are objects to be freed. That
> cleanup is always called on address space release after all.
>
> [ 81.290284] ------------[ cut here ]------------
> [ 81.291602] kernel BUG at drivers/iommu/intel-iommu.c:3591!
> [ 81.293558] invalid opcode: 0000 [#1] PREEMPT SMP
> [ 81.294695] CPU: 0 PID: 207 Comm: core_hotunplug Tainted: G U 5.4.17 #2
> [ 81.296579] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2007
> [ 81.297959] RIP: 0010:intel_unmap+0x200/0x230
> [ 81.299015] Code: 00 e8 e4 45 c5 ff 85 c0 74 09 80 3d 2b 84 c0 00 00 74 19 65 ff 0d 78 9a b2 7e 0f 85 fa fe ff ff e8 95 57 b1 ff e9 f0 fe ff ff <0f> 0b e8 19 4c c5 ff 85 c0 75 de 48 c7 c2 48 d2 e1 81 be 57 00 00
> [ 81.303425] RSP: 0018:ffffc9000013fda0 EFLAGS: 00010246
> [ 81.304683] RAX: 0000000000000000 RBX: ffff8882228dd0b0 RCX: 0000000000000000
> [ 81.306384] RDX: 0000000000001000 RSI: 00000000af801000 RDI: ffff8882228dd0b0
> [ 81.308086] RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> [ 81.309788] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000af801000
> [ 81.311489] R13: ffff888223a00000 R14: 0000000000001000 R15: ffff888223a0a2e8
> [ 81.313191] FS: 00007f5408e3c940(0000) GS:ffff888228600000(0000) knlGS:0000000000000000
> [ 81.315116] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 81.316495] CR2: 0000000001fc0048 CR3: 000000022464a000 CR4: 00000000000006b0
> [ 81.318196] Call Trace:
> [ 81.318967] cleanup_scratch_page+0x44/0x80 [i915]
> [ 81.320281] i915_ggtt_driver_release+0x15b/0x220 [i915]
> [ 81.321717] i915_driver_release+0x33/0x90 [i915]
> [ 81.322856] drm_release+0xbc/0xd0
> [ 81.323691] __fput+0xcd/0x260
> [ 81.324447] task_work_run+0x90/0xc0
> [ 81.325323] do_syscall_64+0x3da/0x560
> [ 81.326240] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 81.327457] RIP: 0033:0x7f54096ecba3
> [ 81.328332] Code: 00 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 45 c3 0f 1f 40 00 48 83 ec 18 89 7c 24 0c e8
> [ 81.332741] RSP: 002b:00007ffcc5165698 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
> [ 81.334546] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00007f54096ecba3
> [ 81.336247] RDX: 00000000005cc5d0 RSI: 0000000000000005 RDI: 0000000000000004
> [ 81.337949] RBP: 0000000000000003 R08: 00000000005b8014 R09: 0000000000000004
> [ 81.339650] R10: 00000000005cc650 R11: 0000000000000246 R12: 00000000004022f0
> [ 81.341352] R13: 00007ffcc5165850 R14: 0000000000000000 R15: 0000000000000000
> [ 81.343059] Modules linked in: i915 mfd_core intel_gtt prime_numbers
> [ 81.345015] ---[ end trace 010aae55e56f8998 ]---
>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
>
> drm/i915: Defer GGTT vm address space fini to vm release
Hum?
>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/gt/intel_ggtt.c | 13 +++++++++----
> drivers/gpu/drm/i915/gt/intel_gtt.h | 1 +
> drivers/gpu/drm/i915/i915_drv.c | 2 ++
> 3 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> index 66165b10256e..ff2b4f74149a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c
> @@ -701,7 +701,6 @@ static void ggtt_cleanup_hw(struct i915_ggtt *ggtt)
> ggtt->vm.cleanup(&ggtt->vm);
>
> mutex_unlock(&ggtt->vm.mutex);
> - i915_address_space_fini(&ggtt->vm);
Ok, so this was defered to release. Where are we going to drop the final ref?
And also - I can see that we have a:
GEM_BUG_ON(i915_is_ggtt(vm));
in i915_vm_release().
Which means that we probably don't drop the final ref and don't ever call
i915_address_space_fini for ggtt.
-Michał
>
> arch_phys_wc_del(ggtt->mtrr);
>
> @@ -709,6 +708,15 @@ static void ggtt_cleanup_hw(struct i915_ggtt *ggtt)
> io_mapping_fini(&ggtt->iomap);
> }
>
> +void i915_ggtt_driver_remove(struct drm_i915_private *i915)
> +{
> + struct i915_ggtt *ggtt = &i915->ggtt;
> +
> + fini_aliasing_ppgtt(ggtt);
> +
> + ggtt_cleanup_hw(ggtt);
> +}
> +
> /**
> * i915_ggtt_driver_release - Clean up GGTT hardware initialization
> * @i915: i915 device
> @@ -718,10 +726,7 @@ void i915_ggtt_driver_release(struct drm_i915_private *i915)
> struct i915_ggtt *ggtt = &i915->ggtt;
> struct pagevec *pvec;
>
> - fini_aliasing_ppgtt(ggtt);
> -
> intel_ggtt_fini_fences(ggtt);
> - ggtt_cleanup_hw(ggtt);
>
> pvec = &i915->mm.wc_stash.pvec;
> if (pvec->nr) {
> diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h
> index d93ebdf3fa0e..f140ce5c171a 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gtt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h
> @@ -501,6 +501,7 @@ int i915_ggtt_enable_hw(struct drm_i915_private *i915);
> void i915_ggtt_enable_guc(struct i915_ggtt *ggtt);
> void i915_ggtt_disable_guc(struct i915_ggtt *ggtt);
> int i915_init_ggtt(struct drm_i915_private *i915);
> +void i915_ggtt_driver_remove(struct drm_i915_private *i915);
> void i915_ggtt_driver_release(struct drm_i915_private *i915);
>
> static inline bool i915_ggtt_has_aperture(const struct i915_ggtt *ggtt)
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 34ee12f3f02d..e4d9f0f6f183 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -752,6 +752,7 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
> err_mem_regions:
> intel_memory_regions_driver_release(dev_priv);
> err_ggtt:
> + i915_ggtt_driver_remove(dev_priv);
> i915_ggtt_driver_release(dev_priv);
> err_perf:
> i915_perf_fini(dev_priv);
> @@ -768,6 +769,7 @@ static void i915_driver_hw_remove(struct drm_i915_private *dev_priv)
>
> i915_perf_fini(dev_priv);
>
> + i915_ggtt_driver_remove(dev_priv);
> if (pdev->msi_enabled)
> pci_disable_msi(pdev);
>
> --
> 2.21.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
