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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com> wrote:
> >
> > 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)) {
> 
> 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