| Message ID | 1506504639-25631-3-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 27 Sep 2017 11:30:32 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > This patch moves GuC suspend/resume handlers to corresponding GEM > handlers > and orders them properly in the runtime and system suspend/resume flows. > i915_gem_restore_fences is GEM resumption task hence it is moved to > i915_gem_resume from i915_restore_state. > > v2: Removed documentation of suspend/resume handlers as those are not > interfaces and are just hooks. (Chris) > > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 7 ------- > drivers/gpu/drm/i915/i915_gem.c | 11 ++++++++--- > drivers/gpu/drm/i915/i915_suspend.c | 2 -- > 3 files changed, 8 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index d0a710d..174a7c5 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1698,8 +1698,6 @@ static int i915_drm_resume(struct drm_device *dev) > } > mutex_unlock(&dev->struct_mutex); > - intel_guc_resume(dev_priv); > - > intel_modeset_init_hw(dev); > spin_lock_irq(&dev_priv->irq_lock); > @@ -2504,8 +2502,6 @@ static int intel_runtime_suspend(struct device > *kdev) > return ret; > } > - intel_guc_suspend(dev_priv); > - > intel_runtime_pm_disable_interrupts(dev_priv); > ret = 0; > @@ -2522,7 +2518,6 @@ static int intel_runtime_suspend(struct device > *kdev) > DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); > intel_runtime_pm_enable_interrupts(dev_priv); > - intel_guc_resume(dev_priv); > i915_gem_runtime_resume(dev_priv); > enable_rpm_wakeref_asserts(dev_priv); > @@ -2591,8 +2586,6 @@ static int intel_runtime_resume(struct device > *kdev) > if (intel_uncore_unclaimed_mmio(dev_priv)) > DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n"); > - intel_guc_resume(dev_priv); > - > if (IS_GEN9_LP(dev_priv)) { > bxt_disable_dc9(dev_priv); > bxt_display_core_init(dev_priv, true); > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 59a88f2..11922af 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2027,6 +2027,8 @@ int i915_gem_runtime_suspend(struct > drm_i915_private *dev_priv) > struct drm_i915_gem_object *obj, *on; > int i; > + intel_guc_suspend(dev_priv); > + > /* > * Only called during RPM suspend. All users of the userfault_list > * must be holding an RPM wakeref to ensure that this can not > @@ -2077,6 +2079,8 @@ void i915_gem_runtime_resume(struct > drm_i915_private *dev_priv) > */ > i915_gem_init_swizzling(dev_priv); > i915_gem_restore_fences(dev_priv); > + > + intel_guc_resume(dev_priv); > } > static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object > *obj) > @@ -4551,8 +4555,6 @@ int i915_gem_suspend(struct drm_i915_private > *dev_priv) > i915_gem_contexts_lost(dev_priv); > mutex_unlock(&dev->struct_mutex); > - intel_guc_suspend(dev_priv); > - > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > cancel_delayed_work_sync(&dev_priv->gt.retire_work); > @@ -4569,6 +4571,8 @@ int i915_gem_suspend(struct drm_i915_private > *dev_priv) > if (WARN_ON(!intel_engines_are_idle(dev_priv))) > i915_gem_set_wedged(dev_priv); /* no hope, discard everything */ > + intel_guc_suspend(dev_priv); > + > /* > * Neither the BIOS, ourselves or any other kernel > * expects the system to be in execlists mode on startup, > @@ -4607,13 +4611,14 @@ int i915_gem_resume(struct drm_i915_private > *dev_priv) > mutex_lock(&dev->struct_mutex); > i915_gem_restore_gtt_mappings(dev_priv); > + i915_gem_restore_fences(dev_priv); > /* As we didn't flush the kernel context before suspend, we cannot > * guarantee that the context image is complete. So let's just reset > * it and start again. > */ > dev_priv->gt.resume(dev_priv); > - I would leave that empty line > + intel_guc_resume(dev_priv); > mutex_unlock(&dev->struct_mutex); > return 0; > diff --git a/drivers/gpu/drm/i915/i915_suspend.c > b/drivers/gpu/drm/i915/i915_suspend.c > index 5c86925a..8f3aa4d 100644 > --- a/drivers/gpu/drm/i915/i915_suspend.c > +++ b/drivers/gpu/drm/i915/i915_suspend.c > @@ -108,8 +108,6 @@ int i915_restore_state(struct drm_i915_private > *dev_priv) > mutex_lock(&dev_priv->drm.struct_mutex); > - i915_gem_restore_fences(dev_priv); > - And this move of i915_gem_restore_fences() can likely be in separate patch Michal > if (IS_GEN4(dev_priv)) > pci_write_config_word(pdev, GCDGMBUS, > dev_priv->regfile.saveGCDGMBUS);
On 9/27/2017 9:17 PM, Michal Wajdeczko wrote: > On Wed, 27 Sep 2017 11:30:32 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> This patch moves GuC suspend/resume handlers to corresponding GEM >> handlers >> and orders them properly in the runtime and system suspend/resume flows. >> i915_gem_restore_fences is GEM resumption task hence it is moved to >> i915_gem_resume from i915_restore_state. >> >> v2: Removed documentation of suspend/resume handlers as those are not >> interfaces and are just hooks. (Chris) >> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 7 ------- >> drivers/gpu/drm/i915/i915_gem.c | 11 ++++++++--- >> drivers/gpu/drm/i915/i915_suspend.c | 2 -- >> 3 files changed, 8 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index d0a710d..174a7c5 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -1698,8 +1698,6 @@ static int i915_drm_resume(struct drm_device *dev) >> } >> mutex_unlock(&dev->struct_mutex); >> - intel_guc_resume(dev_priv); >> - >> intel_modeset_init_hw(dev); >> spin_lock_irq(&dev_priv->irq_lock); >> @@ -2504,8 +2502,6 @@ static int intel_runtime_suspend(struct device >> *kdev) >> return ret; >> } >> - intel_guc_suspend(dev_priv); >> - >> intel_runtime_pm_disable_interrupts(dev_priv); >> ret = 0; >> @@ -2522,7 +2518,6 @@ static int intel_runtime_suspend(struct device >> *kdev) >> DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); >> intel_runtime_pm_enable_interrupts(dev_priv); >> - intel_guc_resume(dev_priv); >> i915_gem_runtime_resume(dev_priv); >> enable_rpm_wakeref_asserts(dev_priv); >> @@ -2591,8 +2586,6 @@ static int intel_runtime_resume(struct device >> *kdev) >> if (intel_uncore_unclaimed_mmio(dev_priv)) >> DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n"); >> - intel_guc_resume(dev_priv); >> - >> if (IS_GEN9_LP(dev_priv)) { >> bxt_disable_dc9(dev_priv); >> bxt_display_core_init(dev_priv, true); >> diff --git a/drivers/gpu/drm/i915/i915_gem.c >> b/drivers/gpu/drm/i915/i915_gem.c >> index 59a88f2..11922af 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2027,6 +2027,8 @@ int i915_gem_runtime_suspend(struct >> drm_i915_private *dev_priv) >> struct drm_i915_gem_object *obj, *on; >> int i; >> + intel_guc_suspend(dev_priv); >> + >> /* >> * Only called during RPM suspend. All users of the userfault_list >> * must be holding an RPM wakeref to ensure that this can not >> @@ -2077,6 +2079,8 @@ void i915_gem_runtime_resume(struct >> drm_i915_private *dev_priv) >> */ >> i915_gem_init_swizzling(dev_priv); >> i915_gem_restore_fences(dev_priv); >> + >> + intel_guc_resume(dev_priv); >> } >> static int i915_gem_object_create_mmap_offset(struct >> drm_i915_gem_object *obj) >> @@ -4551,8 +4555,6 @@ int i915_gem_suspend(struct drm_i915_private >> *dev_priv) >> i915_gem_contexts_lost(dev_priv); >> mutex_unlock(&dev->struct_mutex); >> - intel_guc_suspend(dev_priv); >> - >> cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); >> cancel_delayed_work_sync(&dev_priv->gt.retire_work); >> @@ -4569,6 +4571,8 @@ int i915_gem_suspend(struct drm_i915_private >> *dev_priv) >> if (WARN_ON(!intel_engines_are_idle(dev_priv))) >> i915_gem_set_wedged(dev_priv); /* no hope, discard >> everything */ >> + intel_guc_suspend(dev_priv); >> + >> /* >> * Neither the BIOS, ourselves or any other kernel >> * expects the system to be in execlists mode on startup, >> @@ -4607,13 +4611,14 @@ int i915_gem_resume(struct drm_i915_private >> *dev_priv) >> mutex_lock(&dev->struct_mutex); >> i915_gem_restore_gtt_mappings(dev_priv); >> + i915_gem_restore_fences(dev_priv); >> /* As we didn't flush the kernel context before suspend, we cannot >> * guarantee that the context image is complete. So let's just >> reset >> * it and start again. >> */ >> dev_priv->gt.resume(dev_priv); >> - > > I would leave that empty line Sure. Will update. > >> + intel_guc_resume(dev_priv); >> mutex_unlock(&dev->struct_mutex); >> return 0; >> diff --git a/drivers/gpu/drm/i915/i915_suspend.c >> b/drivers/gpu/drm/i915/i915_suspend.c >> index 5c86925a..8f3aa4d 100644 >> --- a/drivers/gpu/drm/i915/i915_suspend.c >> +++ b/drivers/gpu/drm/i915/i915_suspend.c >> @@ -108,8 +108,6 @@ int i915_restore_state(struct drm_i915_private >> *dev_priv) >> mutex_lock(&dev_priv->drm.struct_mutex); >> - i915_gem_restore_fences(dev_priv); >> - > > And this move of i915_gem_restore_fences() can likely be in separate > patch > > Michal Ok. Will create separate patch. > >> if (IS_GEN4(dev_priv)) >> pci_write_config_word(pdev, GCDGMBUS, >> dev_priv->regfile.saveGCDGMBUS);
Quoting Sagar Arun Kamble (2017-09-27 10:30:32) > @@ -4607,13 +4611,14 @@ int i915_gem_resume(struct drm_i915_private *dev_priv) > > mutex_lock(&dev->struct_mutex); > i915_gem_restore_gtt_mappings(dev_priv); > + i915_gem_restore_fences(dev_priv); Seconded Michal's suggestion, otherwise it looks very clean. -Chris
On 9/28/2017 5:10 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2017-09-27 10:30:32) >> @@ -4607,13 +4611,14 @@ int i915_gem_resume(struct drm_i915_private *dev_priv) >> >> mutex_lock(&dev->struct_mutex); >> i915_gem_restore_gtt_mappings(dev_priv); >> + i915_gem_restore_fences(dev_priv); > Seconded Michal's suggestion, otherwise it looks very clean. > -Chris This has been updated in the following patch in the latest revision of series: https://patchwork.freedesktop.org/patch/179403/
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d0a710d..174a7c5 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1698,8 +1698,6 @@ static int i915_drm_resume(struct drm_device *dev) } mutex_unlock(&dev->struct_mutex); - intel_guc_resume(dev_priv); - intel_modeset_init_hw(dev); spin_lock_irq(&dev_priv->irq_lock); @@ -2504,8 +2502,6 @@ static int intel_runtime_suspend(struct device *kdev) return ret; } - intel_guc_suspend(dev_priv); - intel_runtime_pm_disable_interrupts(dev_priv); ret = 0; @@ -2522,7 +2518,6 @@ static int intel_runtime_suspend(struct device *kdev) DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); intel_runtime_pm_enable_interrupts(dev_priv); - intel_guc_resume(dev_priv); i915_gem_runtime_resume(dev_priv); enable_rpm_wakeref_asserts(dev_priv); @@ -2591,8 +2586,6 @@ static int intel_runtime_resume(struct device *kdev) if (intel_uncore_unclaimed_mmio(dev_priv)) DRM_DEBUG_DRIVER("Unclaimed access during suspend, bios?\n"); - intel_guc_resume(dev_priv); - if (IS_GEN9_LP(dev_priv)) { bxt_disable_dc9(dev_priv); bxt_display_core_init(dev_priv, true); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 59a88f2..11922af 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2027,6 +2027,8 @@ int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) struct drm_i915_gem_object *obj, *on; int i; + intel_guc_suspend(dev_priv); + /* * Only called during RPM suspend. All users of the userfault_list * must be holding an RPM wakeref to ensure that this can not @@ -2077,6 +2079,8 @@ void i915_gem_runtime_resume(struct drm_i915_private *dev_priv) */ i915_gem_init_swizzling(dev_priv); i915_gem_restore_fences(dev_priv); + + intel_guc_resume(dev_priv); } static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) @@ -4551,8 +4555,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) i915_gem_contexts_lost(dev_priv); mutex_unlock(&dev->struct_mutex); - intel_guc_suspend(dev_priv); - cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); cancel_delayed_work_sync(&dev_priv->gt.retire_work); @@ -4569,6 +4571,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) if (WARN_ON(!intel_engines_are_idle(dev_priv))) i915_gem_set_wedged(dev_priv); /* no hope, discard everything */ + intel_guc_suspend(dev_priv); + /* * Neither the BIOS, ourselves or any other kernel * expects the system to be in execlists mode on startup, @@ -4607,13 +4611,14 @@ int i915_gem_resume(struct drm_i915_private *dev_priv) mutex_lock(&dev->struct_mutex); i915_gem_restore_gtt_mappings(dev_priv); + i915_gem_restore_fences(dev_priv); /* As we didn't flush the kernel context before suspend, we cannot * guarantee that the context image is complete. So let's just reset * it and start again. */ dev_priv->gt.resume(dev_priv); - + intel_guc_resume(dev_priv); mutex_unlock(&dev->struct_mutex); return 0; diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c index 5c86925a..8f3aa4d 100644 --- a/drivers/gpu/drm/i915/i915_suspend.c +++ b/drivers/gpu/drm/i915/i915_suspend.c @@ -108,8 +108,6 @@ int i915_restore_state(struct drm_i915_private *dev_priv) mutex_lock(&dev_priv->drm.struct_mutex); - i915_gem_restore_fences(dev_priv); - if (IS_GEN4(dev_priv)) pci_write_config_word(pdev, GCDGMBUS, dev_priv->regfile.saveGCDGMBUS);
This patch moves GuC suspend/resume handlers to corresponding GEM handlers and orders them properly in the runtime and system suspend/resume flows. i915_gem_restore_fences is GEM resumption task hence it is moved to i915_gem_resume from i915_restore_state. v2: Removed documentation of suspend/resume handlers as those are not interfaces and are just hooks. (Chris) Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Imre Deak <imre.deak@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 7 ------- drivers/gpu/drm/i915/i915_gem.c | 11 ++++++++--- drivers/gpu/drm/i915/i915_suspend.c | 2 -- 3 files changed, 8 insertions(+), 12 deletions(-)