Re: [RFC PATCH 3/4] drm/i915: Move GGTT cleanup from driver_release to _remove

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux