[Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier

[Chris Wilson]

Quoting Ville Syrjälä (2018-05-25 11:51:13)
> On Fri, May 25, 2018 at 10:26:29AM +0100, Chris Wilson wrote:
> > In order to prepare the GPU for sleeping, we may want to submit commands
> > to it. This is a complicated process that may even require some swapping
> > in from shmemfs, if the GPU was in the wrong state. As such, we need to
> > do this preparation step synchronously before the rest of the system has
> > started to turn off (e.g. swapin fails if scsi is suspended).
> > Fortunately, we are provided with a such a hook, pm_ops.prepare().
> > 
> > v2: Compile cleanup
> > v3: Fewer asserts, fewer problems?
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106640
> > Testcase: igt/drv_suspend after igt/gem_tiled_swapping
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 41 +++++++++++++++++++++++++--------
> >  1 file changed, 31 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 9c449b8d8eab..9d6ac7f44812 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1553,12 +1553,24 @@ static bool suspend_to_idle(struct drm_i915_private *dev_priv)
> >       return false;
> >  }
> >  
> > +static int i915_drm_prepare(struct drm_device *dev)
> > +{
> > +     struct drm_i915_private *i915 = to_i915(dev);
> > +     int err;
> > +
> > +     err = i915_gem_suspend(i915);
> > +     if (err)
> > +             dev_err(&i915->drm.pdev->dev,
> > +                     "GEM idle failed, suspend/resume might fail\n");
> > +
> > +     return err;
> > +}
> > +
> >  static int i915_drm_suspend(struct drm_device *dev)
> >  {
> >       struct drm_i915_private *dev_priv = to_i915(dev);
> >       struct pci_dev *pdev = dev_priv->drm.pdev;
> >       pci_power_t opregion_target_state;
> > -     int error;
> >  
> >       /* ignore lid events during suspend */
> >       mutex_lock(&dev_priv->modeset_restore_lock);
> > @@ -1575,13 +1587,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  
> >       pci_save_state(pdev);
> >  
> > -     error = i915_gem_suspend(dev_priv);
> > -     if (error) {
> > -             dev_err(&pdev->dev,
> > -                     "GEM idle failed, resume might fail\n");
> > -             goto out;
> > -     }
> > -
> >       intel_display_suspend(dev);
> >  
> >       intel_dp_mst_suspend(dev);
> > @@ -1609,10 +1614,9 @@ static int i915_drm_suspend(struct drm_device *dev)
> >  
> >       intel_csr_ucode_suspend(dev_priv);
> >  
> > -out:
> >       enable_rpm_wakeref_asserts(dev_priv);
> >  
> > -     return error;
> > +     return 0;
> >  }
> >  
> >  static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
> > @@ -2081,6 +2085,22 @@ int i915_reset_engine(struct intel_engine_cs *engine, const char *msg)
> >       return ret;
> >  }
> >  
> > +static int i915_pm_prepare(struct device *kdev)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(kdev);
> > +     struct drm_device *dev = pci_get_drvdata(pdev);
> > +
> > +     if (!dev) {
> > +             dev_err(kdev, "DRM not initialized, aborting suspend.\n");
> > +             return -ENODEV;
> > +     }
> 
> How can this happen?
> 
> IIRC I actually wrote a patch once to move the gem suspend to happen
> after display suspend. The idea being that shutting down the display(s)
> may require gem services (MI_OVERLAY_OFF being the prime example I
> had in mind at the time). Just wondering if we can split the gem suspend
> somehow to allow that, or would we need to just move display suspend
> earlier as well?

Ville accepted that this didn't really change the status quo (on irc)
and so was ok with postponing such fixes until later. I added
+       /*
+        * NB intel_display_suspend() may issue new requests after we've
+        * ostensibly marked the GPU as ready-to-sleep here. We need to
+        * split out that work and pull it forward so that after point,
+        * the GPU is not woken again.
+        */
to record the issue so that hopefully we might fix it before any one
notices.

I pulled in Mika's review from a later thread and pushed so I can close
the bug.

Thanks for the review,
-Chris