On Mon, Dec 02, 2024 at 10:40:36AM -0500, Brian Geffon wrote:
> 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.
It looked to me like __i915_vma_unbind() pretty much just calls
__i915_vma_evict() anyway, and the sync stuff shouldn't matter
here.
Hmm, I suppose there is that vma->node handling that might screw
us over somehow. I'll need to check what that actually does.
--
Ville Syrjälä
Intel
