On Wed, Nov 27, 2024 at 1:11 AM Ville Syrjala
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Cc: Vidya Srinivas <vidya.srinivas@xxxxxxxxx>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> .../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)) {
I don't fully understand this part. Why can we safely assume we can do
__i915_vma_evict(), shouldn't we want to __i915_vma_unbind() in the
case where it was bound? Because of the unconditional evict_all we
might be unbinding a bound vma, no? Is that safe? Please forgive my
ignorance if this question doesn't make sense.
> 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
>
