[PATCH 3/4] drm/i915/dpt: Evict all DPT VMAs on suspend

[Ville Syrjälä]

On Tue, Dec 03, 2024 at 10:20:23AM +0200, Ville Syrjälä wrote:
> 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 at linux.intel.com> wrote:
> > >
> > > From: Ville Syrjälä <ville.syrjala at 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 at google.com>
> > > Cc: Vidya Srinivas <vidya.srinivas at intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at 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)) {
> > 
> > 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.

Ah, we do drm_mm_remove_node(&vma->node) manually anyway here.
That explains why it doesn't blow up later in vma_insert().
So yeah, this kinda just looks like a hand rolled vma_unbind()
more or less. Why it doesn't just call the whole thing I 
don't know.

-- 
Ville Syrjälä
Intel