Re: [Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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 > > --- > > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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 > > --- > > 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? Just copypaste, and not wanting to have to answer too many questions. > 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? That would require doing that portion of display shutdown earlier. The problem is in starting GPU activity from within the async suspend is error prone. For overlay, it's not much of an issue, we could just submit the command and wait on it synchronously, as those systems don't have the fiddly things like contexts and rc6 to contend with ;) I can't think of a different way of slicing gem suspend, as it's core operation is to switch the GPU to the kernel context, and that involves submitting a GPU command. After gem suspend, we shouldn't tolerate any more requests. I am splitting the reset at the end of gem suspend into suspend_late though (mainly so that we don't do it for SUSPEND_TEST_DEVICES as it conflicts nastily with reset failure testing). -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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 > --- > 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? > + > + if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) > + return 0; > + > + return i915_drm_prepare(dev); > +} > + > static int i915_pm_suspend(struct device *kdev) > { > struct pci_dev *pdev = to_pci_dev(kdev); > @@ -2731,6 +2751,7 @@ const struct dev_pm_ops i915_pm_ops = { >* S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, >* PMSG_RESUME] >*/ > + .prepare = i915_pm_prepare, > .suspend = i915_pm_suspend, > .suspend_late = i915_pm_suspend_late, > .resume_early = i915_pm_resume_early, > -- > 2.17.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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 --- 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; + } + + if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + + return i915_drm_prepare(dev); +} + static int i915_pm_suspend(struct device *kdev) { struct pci_dev *pdev = to_pci_dev(kdev); @@ -2731,6 +2751,7 @@ const struct dev_pm_ops i915_pm_ops = { * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, * PMSG_RESUME] */ + .prepare = i915_pm_prepare, .suspend = i915_pm_suspend, .suspend_late = i915_pm_suspend_late, .resume_early = i915_pm_resume_early, -- 2.17.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106640 Testcase: igt/drv_suspend after igt/gem_tiled_swapping Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.c | 47 ++--- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9c449b8d8eab..a0beabd49f62 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1553,12 +1553,30 @@ 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; + + disable_rpm_wakeref_asserts(i915); + + err = i915_gem_suspend(i915); + if (err) { + dev_err(&i915->drm.pdev->dev, + "GEM idle failed, suspend/resume might fail\n"); + goto out; + } +out: + enable_rpm_wakeref_asserts(i915); + + 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 +1593,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 +1620,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 +2091,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; + } + + if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + + return i915_drm_prepare(dev); +} + static int i915_pm_suspend(struct device *kdev) { struct pci_dev *pdev = to_pci_dev(kdev); @@ -2731,6 +2757,7 @@ const struct dev_pm_ops i915_pm_ops = { * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, * PMSG_RESUME] */ + .prepare = i915_pm_prepare, .suspend = i915_pm_suspend, .suspend_late = i915_pm_suspend_late, .resume_early = i915_pm_resume_early, -- 2.17.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
Hi Chris, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on v4.17-rc6 next-20180517] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Prepare-GEM-for-suspend-earlier/20180524-231951 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-randconfig-x000-201820 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/gpu//drm/i915/i915_drv.c: In function 'i915_drm_suspend': >> drivers/gpu//drm/i915/i915_drv.c:1624:1: error: label 'out' defined but not >> used [-Werror=unused-label] out: ^~~ Cyclomatic Complexity 5 include/linux/compiler.h:__read_once_size Cyclomatic Complexity 1 include/linux/kasan-checks.h:kasan_check_read Cyclomatic Complexity 1 include/linux/kasan-checks.h:kasan_check_write Cyclomatic Complexity 2 arch/x86/include/asm/bitops.h:clear_bit Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:constant_test_bit Cyclomatic Complexity 1 arch/x86/include/asm/bitops.h:fls Cyclomatic Complexity 1 arch/x86/include/asm/arch_hweight.h:__arch_hweight32 Cyclomatic Complexity 1 arch/x86/include/asm/arch_hweight.h:__arch_hweight8 Cyclomatic Complexity 1 include/linux/log2.h:__ilog2_u32 Cyclomatic Complexity 1 include/linux/list.h:list_empty Cyclomatic Complexity 1 arch/x86/include/asm/mem_encrypt.h:sme_active Cyclomatic Complexity 1 include/linux/mem_encrypt.h:sme_get_me_mask Cyclomatic Complexity 1 include/asm-generic/getorder.h:__get_order Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:arch_atomic_read Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:arch_atomic_inc Cyclomatic Complexity 1 arch/x86/include/asm/atomic.h:arch_atomic_dec Cyclomatic Complexity 1 include/asm-generic/atomic-instrumented.h:atomic_read Cyclomatic Complexity 1 include/asm-generic/atomic-instrumented.h:atomic_inc Cyclomatic Complexity 1 include/asm-generic/atomic-instrumented.h:atomic_dec Cyclomatic Complexity 1 include/linux/err.h:ERR_PTR Cyclomatic Complexity 1 include/linux/err.h:PTR_ERR Cyclomatic Complexity 3 include/linux/err.h:IS_ERR_OR_NULL Cyclomatic Complexity 1 include/linux/lockdep.h:lock_is_held Cyclomatic Complexity 1 include/linux/spinlock.h:spinlock_check Cyclomatic Complexity 1 include/linux/spinlock.h:spin_lock_irq Cyclomatic Complexity 1 include/linux/spinlock.h:spin_unlock_irq Cyclomatic Complexity 1 include/linux/jiffies.h:_msecs_to_jiffies Cyclomatic Complexity 3 include/linux/jiffies.h:msecs_to_jiffies Cyclomatic Complexity 3 include/linux/ktime.h:ktime_compare Cyclomatic Complexity 1 include/linux/ktime.h:ktime_after Cyclomatic Complexity 1 include/linux/workqueue.h:queue_delayed_work Cyclomatic Complexity 67 include/linux/slab.h:kmalloc_large Cyclomatic Complexity 3 include/linux/slab.h:kmalloc Cyclomatic Complexity 1 include/linux/slab.h:kzalloc Cyclomatic Complexity 1 include/linux/device.h:dev_get_drvdata Cyclomatic Complexity 1 include/linux/device.h:dev_set_drvdata Cyclomatic Complexity 1 include/linux/device.h:dev_pm_set_driver_flags Cyclomatic Complexity 1 include/linux/pci.h:pci_disable_msi Cyclomatic Complexity 1 include/linux/pci.h:pci_enable_msi Cyclomatic Complexity 1 arch/x86/include/asm/pci.h:pci_domain_nr Cyclomatic Complexity 1 include/linux/pci.h:pci_get_drvdata Cyclomatic Complexity 1 include/linux/pci.h:pci_set_drvdata Cyclomatic Complexity 1 arch/x86/include/asm/dma-mapping.h:get_arch_dma_ops Cyclomatic Complexity 4 include/linux/dma-mapping.h:get_dma_ops Cyclomatic Complexity 3 include/linux/dma-mapping.h:dma_check_mask Cyclomatic Complexity 4 include/linux/dma-mapping.h:dma_supported Cyclomatic Complexity 2 include/linux/dma-mapping.h:dma_set_coherent_mask Cyclomatic Complexity 1 include/linux/vgaarb.h:vga_client_register Cyclomatic Complexity 2 include/linux/fb.h:alloc_apertures Cyclomatic Complexity 1 include/linux/vga_switcheroo.h:vga_switcheroo_unregister_client Cyclomatic Complexity 1 include/linux/vga_switcheroo.h:vga_switcheroo_register_client Cyclomatic Complexity 1 include/linux/vga_switcheroo.h:vga_switcheroo_process_delayed_switch Cyclomatic Complexity 1 include/drm/drm_print.h:drm_debug_printer Cyclomatic Complexity 2 drivers/gpu//drm/i915/i915_utils.h:onoff Cyclomatic Complexity 3 drivers/gpu//drm/i915/intel_device_info.h:sseu_subslice_total Cyclomatic Complexity 1 drivers/gpu//drm/i915/intel_ringbuffer.h:intel_engine_flag Cyclomatic Complexity 1 drivers/gpu//drm/i915/intel_uncore.h:intel_wait_for_register Cyclomatic Complexity 1 drivers/gpu//drm/i915/i915_gpu_error.h:i915_reset_error_state Cyclomatic C
Re: [Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
Hi Chris, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on v4.17-rc6 next-20180517] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Chris-Wilson/drm-i915-Prepare-GEM-for-suspend-earlier/20180524-231951 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-x015-201820 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): drivers/gpu//drm/i915/i915_drv.c: In function 'i915_drm_suspend': drivers/gpu//drm/i915/i915_drv.c:1624:1: warning: label 'out' defined but not used [-Wunused-label] out: ^~~ >> drivers/gpu//drm/i915/i915_drv.c:1627:9: warning: 'error' is used >> uninitialized in this function [-Wuninitialized] return error; ^ vim +/error +1627 drivers/gpu//drm/i915/i915_drv.c 156987f3e drivers/gpu/drm/i915/i915_drv.c Chris Wilson 2018-05-22 1574 5e365c391 drivers/gpu/drm/i915/i915_drv.c Imre Deak 2014-10-23 1575 static int i915_drm_suspend(struct drm_device *dev) ba8bbcf6f drivers/char/drm/i915_drv.c Jesse Barnes 2007-11-22 1576 { fac5e23e3 drivers/gpu/drm/i915/i915_drv.c Chris Wilson 2016-07-04 1577 struct drm_i915_private *dev_priv = to_i915(dev); 52a05c302 drivers/gpu/drm/i915/i915_drv.c David Weinehall 2016-08-22 1578 struct pci_dev *pdev = dev_priv->drm.pdev; e5747e3ad drivers/gpu/drm/i915/i915_drv.c Jesse Barnes 2014-06-12 1579 pci_power_t opregion_target_state; d58189385 drivers/gpu/drm/i915/i915_drv.c Daniel Vetter 2015-02-23 1580 int error; 61caf87cb drivers/gpu/drm/i915/i915_drv.c Rafael J. Wysocki 2010-02-18 1581 b8efb17b3 drivers/gpu/drm/i915/i915_drv.c Zhang Rui 2013-02-05 1582 /* ignore lid events during suspend */ b8efb17b3 drivers/gpu/drm/i915/i915_drv.c Zhang Rui 2013-02-05 1583 mutex_lock(&dev_priv->modeset_restore_lock); b8efb17b3 drivers/gpu/drm/i915/i915_drv.c Zhang Rui 2013-02-05 1584 dev_priv->modeset_restore = MODESET_SUSPENDED; b8efb17b3 drivers/gpu/drm/i915/i915_drv.c Zhang Rui 2013-02-05 1585 mutex_unlock(&dev_priv->modeset_restore_lock); b8efb17b3 drivers/gpu/drm/i915/i915_drv.c Zhang Rui 2013-02-05 1586 1f814daca drivers/gpu/drm/i915/i915_drv.c Imre Deak 2015-12-16 1587 disable_rpm_wakeref_asserts(dev_priv); 1f814daca drivers/gpu/drm/i915/i915_drv.c Imre Deak 2015-12-16 1588 c67a470b1 drivers/gpu/drm/i915/i915_drv.c Paulo Zanoni 2013-08-19 1589 /* We do a lot of poking in a lot of registers, make sure they work c67a470b1 drivers/gpu/drm/i915/i915_drv.c Paulo Zanoni 2013-08-19 1590 * properly. */ da7e29bd5 drivers/gpu/drm/i915/i915_drv.c Imre Deak 2014-02-18 1591 intel_display_set_init_power(dev_priv, true); cb10799c1 drivers/gpu/drm/i915/i915_drv.c Paulo Zanoni 2013-01-25 1592 5bcf719b7 drivers/gpu/drm/i915/i915_drv.c Dave Airlie 2010-12-07 1593 drm_kms_helper_poll_disable(dev); 5bcf719b7 drivers/gpu/drm/i915/i915_drv.c Dave Airlie 2010-12-07 1594 52a05c302 drivers/gpu/drm/i915/i915_drv.c David Weinehall 2016-08-22 1595 pci_save_state(pdev); ba8bbcf6f drivers/char/drm/i915_drv.c Jesse Barnes 2007-11-22 1596 6b72d4862 drivers/gpu/drm/i915/i915_drv.c Maarten Lankhorst 2015-06-01 1597 intel_display_suspend(dev); 7d708ee40 drivers/gpu/drm/i915/i915_drv.c Imre Deak 2013-04-17 1598 0e32b39ce drivers/gpu/drm/i915/i915_drv.c Dave Airlie 2014-05-02 1599 intel_dp_mst_suspend(dev); 09b64267c drivers/gpu/drm/i915/i915_drv.c Dave Airlie 2014-07-23 1600 b963291cf drivers/gpu/drm/i915/i915_drv.c Daniel Vetter 2014-09-30 1601 intel_runtime_pm_disable_interrupts(dev_priv); 1d0d343ab drivers/gpu/drm/i915/i915_drv.c Imre Deak 2014-08-18 1602 intel_hpd_cancel_work(dev_priv); 0e32b39ce drivers/gpu/drm/i915/i915_drv.c Dave Airlie 2014-05-02 1603 07f9cd0b3 drivers/gpu/drm/i915/i915_drv.c Imre Deak 2014-08-18 1604 intel_suspend_encoders(dev_priv); 07f9cd0b3 drivers/gpu/drm/i915/i915_drv.c Imre Deak 2014-08-18 1605 712bf3644 drivers/gpu/drm/i915/i915_drv.c Ville Syrjälä 2016-10-31 1606 intel_suspend_hw(dev_priv); 5669fcacc drivers/gpu/drm/i915/i915_drv.c Jesse Barnes 2009-02-17 1607 275a991c0 drivers/gpu/drm/i915/i915_drv.c Tvrtko Ursulin2016-11-16 1608 i915_gem_suspend_gtt_mappings(dev_priv); 828c79087 drivers/gpu/drm/i915/i915_drv.c Ben Widawsky 2013-10-16 1609 af6dc7425 drivers/gpu/drm/i915/i915_drv.c Tvrtko Ursulin2016-12-01 1610 i915_save_state(dev_priv); 9e06dd39f drivers/gpu/drm/i915/i915_drv.c Jesse Barnes 2009-06-22 1611
Re: [Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
Quoting Chris Wilson (2018-05-22 16:49:02) > 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: Add manual calls to i915_drm_prepare() before i915_drm_suspend(). Revision 2 passed, but revision 3 fails. I don't understand... :( -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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: Add manual calls to i915_drm_prepare() before i915_drm_suspend(). Testcase: igt/drv_suspend after igt/gem_tiled_swapping Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.c | 55 +++-- 1 file changed, 45 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9c449b8d8eab..8642f9a1aff2 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1553,12 +1553,30 @@ 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; + + disable_rpm_wakeref_asserts(i915); + + err = i915_gem_suspend(i915); + if (err) { + dev_err(&i915->drm.pdev->dev, + "GEM idle failed, suspend/resume might fail\n"); + goto out; + } +out: + enable_rpm_wakeref_asserts(i915); + + 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 +1593,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 +1620,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) @@ -1695,6 +1705,10 @@ static int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state) if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) return 0; + error = i915_drm_prepare(dev); + if (error) + return error; + error = i915_drm_suspend(dev); if (error) return error; @@ -2081,6 +2095,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; + } + + if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + + return i915_drm_prepare(dev); +} + static int i915_pm_suspend(struct device *kdev) { struct pci_dev *pdev = to_pci_dev(kdev); @@ -2153,6 +2183,10 @@ static int i915_pm_freeze(struct device *kdev) int ret; if (dev->switch_power_state != DRM_SWITCH_POWER_OFF) { + ret = i915_drm_prepare(dev); + if (ret) + return ret; + ret = i915_drm_suspend(dev); if (ret) return ret; @@ -2731,6 +2765,7 @@ const struct dev_pm_ops i915_pm_ops = { * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, * PMSG_RESUME] */ + .prepare = i915_pm_prepare, .suspend = i915_pm_suspend, .suspend_late = i915_pm_suspend_late, .resume_early = i915_pm_resume_early, -- 2.17.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Prepare GEM for suspend earlier
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(). Testcase: igt/drv_suspend after igt/gem_tiled_swapping Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/i915_drv.c | 43 +++-- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9c449b8d8eab..256642fb8315 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1553,6 +1553,25 @@ 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; + + disable_rpm_wakeref_asserts(i915); + + err = i915_gem_suspend(i915); + if (err) { + dev_err(&i915->drm.pdev->dev, + "GEM idle failed, suspend/resume might fail\n"); + goto out; + } +out: + enable_rpm_wakeref_asserts(i915); + + return err; +} + static int i915_drm_suspend(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); @@ -1575,13 +1594,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); @@ -2081,6 +2093,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; + } + + if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) + return 0; + + return i915_drm_prepare(dev); +} + static int i915_pm_suspend(struct device *kdev) { struct pci_dev *pdev = to_pci_dev(kdev); @@ -2731,6 +2759,7 @@ const struct dev_pm_ops i915_pm_ops = { * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND, * PMSG_RESUME] */ + .prepare = i915_pm_prepare, .suspend = i915_pm_suspend, .suspend_late = i915_pm_suspend_late, .resume_early = i915_pm_resume_early, -- 2.17.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx