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@xxxxxxxxxxxxxxxxxx> > > --- > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
