| Message ID | 20241127061117.25622-4-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/dpt: Try to make DPT shrinkable again | expand |
> -----Original Message----- > From: Ville Syrjala <ville.syrjala@linux.intel.com> > Sent: 27 November 2024 11:41 > To: intel-gfx@lists.freedesktop.org > Cc: Brian Geffon <bgeffon@google.com>; Srinivas, Vidya > <vidya.srinivas@intel.com> > Subject: [PATCH 3/4] drm/i915/dpt: Evict all DPT VMAs on suspend > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Currently intel_dpt_resume() tries to blindly rewrite all the PTEs for currently > bound DPT VMAs. That is problematic because the CPU mapping for the DPT is > only really guaranteed to exist while the DPT object has been pinned. In the > past we worked around this issue by making DPT objects unshrinkable, but that > is undesirable as it'll waste physical RAM. > > Let's instead forcefully evict all the DPT VMAs on suspend, thus guaranteeing > that intel_dpt_resume() has nothing to do. > To guarantee that all the DPT VMAs are evictable by > intel_dpt_suspend() we need to flush the cleanup workqueue after the display > output has been shut down. > > And for good measure throw in a few extra WARNs to catch any mistakes. > > Cc: Brian Geffon <bgeffon@google.com> > Cc: Vidya Srinivas <vidya.srinivas@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > .../drm/i915/display/intel_display_driver.c | 3 +++ > drivers/gpu/drm/i915/display/intel_dpt.c | 4 ++-- > drivers/gpu/drm/i915/gt/intel_ggtt.c | 19 ++++++++++++++----- > drivers/gpu/drm/i915/gt/intel_gtt.h | 4 ++-- > 4 files changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c > b/drivers/gpu/drm/i915/display/intel_display_driver.c > index 286d6f893afa..973bee43b554 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_driver.c > +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c > @@ -680,6 +680,9 @@ int intel_display_driver_suspend(struct > drm_i915_private *i915) > else > i915->display.restore.modeset_state = state; > > + /* ensure all DPT VMAs have been unpinned for intel_dpt_suspend() > */ > + flush_workqueue(i915->display.wq.cleanup); > + > intel_dp_mst_suspend(i915); > > return ret; > diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c > b/drivers/gpu/drm/i915/display/intel_dpt.c > index ce8c76e44e6a..8b1f0e92a11c 100644 > --- a/drivers/gpu/drm/i915/display/intel_dpt.c > +++ b/drivers/gpu/drm/i915/display/intel_dpt.c > @@ -205,7 +205,7 @@ void intel_dpt_resume(struct drm_i915_private *i915) > struct intel_framebuffer *fb = to_intel_framebuffer(drm_fb); > > if (fb->dpt_vm) > - i915_ggtt_resume_vm(fb->dpt_vm); > + i915_ggtt_resume_vm(fb->dpt_vm, true); > } > mutex_unlock(&i915->drm.mode_config.fb_lock); > } > @@ -233,7 +233,7 @@ void intel_dpt_suspend(struct drm_i915_private *i915) > struct intel_framebuffer *fb = to_intel_framebuffer(drm_fb); > > if (fb->dpt_vm) > - i915_ggtt_suspend_vm(fb->dpt_vm); > + i915_ggtt_suspend_vm(fb->dpt_vm, true); > } > > mutex_unlock(&i915->drm.mode_config.fb_lock); > diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c > b/drivers/gpu/drm/i915/gt/intel_ggtt.c > index d60a6ca0cae5..f6c59f20832f 100644 > --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c > +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c > @@ -107,11 +107,12 @@ int i915_ggtt_init_hw(struct drm_i915_private *i915) > /** > * i915_ggtt_suspend_vm - Suspend the memory mappings for a GGTT or DPT > VM > * @vm: The VM to suspend the mappings for > + * @evict_all: Evict all VMAs > * > * Suspend the memory mappings for all objects mapped to HW via the GGTT > or a > * DPT page table. > */ > -void i915_ggtt_suspend_vm(struct i915_address_space *vm) > +void i915_ggtt_suspend_vm(struct i915_address_space *vm, bool > +evict_all) > { > struct i915_vma *vma, *vn; > int save_skip_rewrite; > @@ -157,7 +158,7 @@ void i915_ggtt_suspend_vm(struct i915_address_space > *vm) > goto retry; > } > > - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { > + if (evict_all || !i915_vma_is_bound(vma, > I915_VMA_GLOBAL_BIND)) { Hello Ville, many thanks for the patch series. We tested it on MTL chromebook and we are not seeing any issues (yet). The older suspend/resume issue is not reproduced with this series. We had attempted something similar in https://patchwork.freedesktop.org/patch/625967/ Thank you so much again for the help and fix. Regards Vidya > i915_vma_wait_for_bind(vma); > > __i915_vma_evict(vma, false); > @@ -172,13 +173,15 @@ void i915_ggtt_suspend_vm(struct > i915_address_space *vm) > vm->skip_pte_rewrite = save_skip_rewrite; > > mutex_unlock(&vm->mutex); > + > + drm_WARN_ON(&vm->i915->drm, evict_all && > +!list_empty(&vm->bound_list)); > } > > void i915_ggtt_suspend(struct i915_ggtt *ggtt) { > struct intel_gt *gt; > > - i915_ggtt_suspend_vm(&ggtt->vm); > + i915_ggtt_suspend_vm(&ggtt->vm, false); > ggtt->invalidate(ggtt); > > list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) @@ -1545,6 +1548,7 > @@ int i915_ggtt_enable_hw(struct drm_i915_private *i915) > /** > * i915_ggtt_resume_vm - Restore the memory mappings for a GGTT or DPT > VM > * @vm: The VM to restore the mappings for > + * @all_evicted: Were all VMAs expected to be evicted on suspend? > * > * Restore the memory mappings for all objects mapped to HW via the GGTT or > a > * DPT page table. > @@ -1552,13 +1556,18 @@ int i915_ggtt_enable_hw(struct drm_i915_private > *i915) > * Returns %true if restoring the mapping for any object that was in a write > * domain before suspend. > */ > -bool i915_ggtt_resume_vm(struct i915_address_space *vm) > +bool i915_ggtt_resume_vm(struct i915_address_space *vm, bool > +all_evicted) > { > struct i915_vma *vma; > bool write_domain_objs = false; > > drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); > > + if (all_evicted) { > + drm_WARN_ON(&vm->i915->drm, !list_empty(&vm- > >bound_list)); > + return false; > + } > + > /* First fill our portion of the GTT with scratch pages */ > vm->clear_range(vm, 0, vm->total); > > @@ -1598,7 +1607,7 @@ void i915_ggtt_resume(struct i915_ggtt *ggtt) > list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) > intel_gt_check_and_clear_faults(gt); > > - flush = i915_ggtt_resume_vm(&ggtt->vm); > + flush = i915_ggtt_resume_vm(&ggtt->vm, false); > > if (drm_mm_node_allocated(&ggtt->error_capture)) > ggtt->vm.scratch_range(&ggtt->vm, ggtt->error_capture.start, > diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h > b/drivers/gpu/drm/i915/gt/intel_gtt.h > index 6b85222ee3ea..0a36ea751b63 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gtt.h > +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h > @@ -608,8 +608,8 @@ int i915_ppgtt_init_hw(struct intel_gt *gt); struct > i915_ppgtt *i915_ppgtt_create(struct intel_gt *gt, > unsigned long lmem_pt_obj_flags); > > -void i915_ggtt_suspend_vm(struct i915_address_space *vm); -bool > i915_ggtt_resume_vm(struct i915_address_space *vm); > +void i915_ggtt_suspend_vm(struct i915_address_space *vm, bool > +evict_all); bool i915_ggtt_resume_vm(struct i915_address_space *vm, > +bool all_evicted); > void i915_ggtt_suspend(struct i915_ggtt *gtt); void i915_ggtt_resume(struct > i915_ggtt *ggtt); > > -- > 2.45.2
diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c index 286d6f893afa..973bee43b554 100644 --- a/drivers/gpu/drm/i915/display/intel_display_driver.c +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c @@ -680,6 +680,9 @@ int intel_display_driver_suspend(struct drm_i915_private *i915) else i915->display.restore.modeset_state = state; + /* ensure all DPT VMAs have been unpinned for intel_dpt_suspend() */ + flush_workqueue(i915->display.wq.cleanup); + intel_dp_mst_suspend(i915); return ret; diff --git a/drivers/gpu/drm/i915/display/intel_dpt.c b/drivers/gpu/drm/i915/display/intel_dpt.c index ce8c76e44e6a..8b1f0e92a11c 100644 --- a/drivers/gpu/drm/i915/display/intel_dpt.c +++ b/drivers/gpu/drm/i915/display/intel_dpt.c @@ -205,7 +205,7 @@ void intel_dpt_resume(struct drm_i915_private *i915) struct intel_framebuffer *fb = to_intel_framebuffer(drm_fb); if (fb->dpt_vm) - i915_ggtt_resume_vm(fb->dpt_vm); + i915_ggtt_resume_vm(fb->dpt_vm, true); } mutex_unlock(&i915->drm.mode_config.fb_lock); } @@ -233,7 +233,7 @@ void intel_dpt_suspend(struct drm_i915_private *i915) struct intel_framebuffer *fb = to_intel_framebuffer(drm_fb); if (fb->dpt_vm) - i915_ggtt_suspend_vm(fb->dpt_vm); + i915_ggtt_suspend_vm(fb->dpt_vm, true); } mutex_unlock(&i915->drm.mode_config.fb_lock); diff --git a/drivers/gpu/drm/i915/gt/intel_ggtt.c b/drivers/gpu/drm/i915/gt/intel_ggtt.c index d60a6ca0cae5..f6c59f20832f 100644 --- a/drivers/gpu/drm/i915/gt/intel_ggtt.c +++ b/drivers/gpu/drm/i915/gt/intel_ggtt.c @@ -107,11 +107,12 @@ int i915_ggtt_init_hw(struct drm_i915_private *i915) /** * i915_ggtt_suspend_vm - Suspend the memory mappings for a GGTT or DPT VM * @vm: The VM to suspend the mappings for + * @evict_all: Evict all VMAs * * Suspend the memory mappings for all objects mapped to HW via the GGTT or a * DPT page table. */ -void i915_ggtt_suspend_vm(struct i915_address_space *vm) +void i915_ggtt_suspend_vm(struct i915_address_space *vm, bool evict_all) { struct i915_vma *vma, *vn; int save_skip_rewrite; @@ -157,7 +158,7 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) goto retry; } - if (!i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { + if (evict_all || !i915_vma_is_bound(vma, I915_VMA_GLOBAL_BIND)) { i915_vma_wait_for_bind(vma); __i915_vma_evict(vma, false); @@ -172,13 +173,15 @@ void i915_ggtt_suspend_vm(struct i915_address_space *vm) vm->skip_pte_rewrite = save_skip_rewrite; mutex_unlock(&vm->mutex); + + drm_WARN_ON(&vm->i915->drm, evict_all && !list_empty(&vm->bound_list)); } void i915_ggtt_suspend(struct i915_ggtt *ggtt) { struct intel_gt *gt; - i915_ggtt_suspend_vm(&ggtt->vm); + i915_ggtt_suspend_vm(&ggtt->vm, false); ggtt->invalidate(ggtt); list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) @@ -1545,6 +1548,7 @@ int i915_ggtt_enable_hw(struct drm_i915_private *i915) /** * i915_ggtt_resume_vm - Restore the memory mappings for a GGTT or DPT VM * @vm: The VM to restore the mappings for + * @all_evicted: Were all VMAs expected to be evicted on suspend? * * Restore the memory mappings for all objects mapped to HW via the GGTT or a * DPT page table. @@ -1552,13 +1556,18 @@ int i915_ggtt_enable_hw(struct drm_i915_private *i915) * Returns %true if restoring the mapping for any object that was in a write * domain before suspend. */ -bool i915_ggtt_resume_vm(struct i915_address_space *vm) +bool i915_ggtt_resume_vm(struct i915_address_space *vm, bool all_evicted) { struct i915_vma *vma; bool write_domain_objs = false; drm_WARN_ON(&vm->i915->drm, !vm->is_ggtt && !vm->is_dpt); + if (all_evicted) { + drm_WARN_ON(&vm->i915->drm, !list_empty(&vm->bound_list)); + return false; + } + /* First fill our portion of the GTT with scratch pages */ vm->clear_range(vm, 0, vm->total); @@ -1598,7 +1607,7 @@ void i915_ggtt_resume(struct i915_ggtt *ggtt) list_for_each_entry(gt, &ggtt->gt_list, ggtt_link) intel_gt_check_and_clear_faults(gt); - flush = i915_ggtt_resume_vm(&ggtt->vm); + flush = i915_ggtt_resume_vm(&ggtt->vm, false); if (drm_mm_node_allocated(&ggtt->error_capture)) ggtt->vm.scratch_range(&ggtt->vm, ggtt->error_capture.start, diff --git a/drivers/gpu/drm/i915/gt/intel_gtt.h b/drivers/gpu/drm/i915/gt/intel_gtt.h index 6b85222ee3ea..0a36ea751b63 100644 --- a/drivers/gpu/drm/i915/gt/intel_gtt.h +++ b/drivers/gpu/drm/i915/gt/intel_gtt.h @@ -608,8 +608,8 @@ int i915_ppgtt_init_hw(struct intel_gt *gt); struct i915_ppgtt *i915_ppgtt_create(struct intel_gt *gt, unsigned long lmem_pt_obj_flags); -void i915_ggtt_suspend_vm(struct i915_address_space *vm); -bool i915_ggtt_resume_vm(struct i915_address_space *vm); +void i915_ggtt_suspend_vm(struct i915_address_space *vm, bool evict_all); +bool i915_ggtt_resume_vm(struct i915_address_space *vm, bool all_evicted); void i915_ggtt_suspend(struct i915_ggtt *gtt); void i915_ggtt_resume(struct i915_ggtt *ggtt);