On Wed, 2020-05-27 at 21:12 +0200, Michał Winiarski wrote:
> 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?
That's a patch squashing left-over I apparently missed, sorry.
>
> > 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.
Uh, that renders my solution invalid. Let me take another approach.
Thanks,
Janusz
>
> -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
