| Message ID | 1506581329-29720-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: > These changes are preparation to handle GuC suspend/resume. Prepared > helper i915_gem_runtime_resume to reinitialize suspended gem setup. > Returning status from i915_gem_runtime_suspend and i915_gem_resume. > This will be placeholder for handling any errors from uC suspend/resume > in upcoming patches. Restructured the suspend/resume routines w.r.t setup > creation and rollback order. > This also fixes issue of ordering of i915_gem_runtime_resume with > intel_runtime_pm_enable_interrupts. > > v2: Fixed return from intel_runtime_resume. (Michał Winiarski) > > v3: Not returning status from gem_runtime_resume. (Chris) > > v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko) > > 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> <SNIP> > @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev) > > intel_csr_ucode_resume(dev_priv); > > - i915_gem_resume(dev_priv); > + ret = i915_gem_resume(dev_priv); > + if (ret) > + dev_err(&pdev->dev, "GEM resume failed\n"); Not DRM_ERROR like other paths? Also, this might be worth a WARN(). I'm open for discussion. > @@ -2560,6 +2561,15 @@ static int intel_runtime_suspend(struct device *kdev) > > DRM_DEBUG_KMS("Device suspended\n"); > return 0; > + > +err_suspend_complete: err_disable: > + intel_runtime_pm_enable_interrupts(dev_priv); > + intel_guc_resume(dev_priv); > + i915_gem_runtime_resume(dev_priv); > + > +err_gem_suspend: err_gem: We are in "suspend" function, after all. > + enable_rpm_wakeref_asserts(dev_priv); > + return ret; > } Regards, Joonas
Quoting Joonas Lahtinen (2017-09-29 12:43:48) > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: > > These changes are preparation to handle GuC suspend/resume. Prepared > > helper i915_gem_runtime_resume to reinitialize suspended gem setup. > > Returning status from i915_gem_runtime_suspend and i915_gem_resume. > > This will be placeholder for handling any errors from uC suspend/resume > > in upcoming patches. Restructured the suspend/resume routines w.r.t setup > > creation and rollback order. > > This also fixes issue of ordering of i915_gem_runtime_resume with > > intel_runtime_pm_enable_interrupts. > > > > v2: Fixed return from intel_runtime_resume. (Michał Winiarski) > > > > v3: Not returning status from gem_runtime_resume. (Chris) > > > > v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko) > > > > 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> > > <SNIP> > > > @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev) > > > > intel_csr_ucode_resume(dev_priv); > > > > - i915_gem_resume(dev_priv); > > + ret = i915_gem_resume(dev_priv); > > + if (ret) > > + dev_err(&pdev->dev, "GEM resume failed\n"); > > Not DRM_ERROR like other paths? Bah, we really need to migrate to dev_err() like all the other cool drivers. DRM_DEV_ERROR() The uppercase is an eyesore, but for an error message, acceptable. Similarly any user output like DRM_DEV_INFO() could reasonably be shouty so that it is not missed. So maybe the uppercase is always justified. -Chris
On Fri, 2017-09-29 at 12:49 +0100, Chris Wilson wrote: > Quoting Joonas Lahtinen (2017-09-29 12:43:48) > > On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: > > > These changes are preparation to handle GuC suspend/resume. Prepared > > > helper i915_gem_runtime_resume to reinitialize suspended gem setup. > > > Returning status from i915_gem_runtime_suspend and i915_gem_resume. > > > This will be placeholder for handling any errors from uC suspend/resume > > > in upcoming patches. Restructured the suspend/resume routines w.r.t setup > > > creation and rollback order. > > > This also fixes issue of ordering of i915_gem_runtime_resume with > > > intel_runtime_pm_enable_interrupts. > > > > > > v2: Fixed return from intel_runtime_resume. (Michał Winiarski) > > > > > > v3: Not returning status from gem_runtime_resume. (Chris) > > > > > > v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko) > > > > > > 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> > > > > <SNIP> > > > > > @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev) > > > > > > intel_csr_ucode_resume(dev_priv); > > > > > > - i915_gem_resume(dev_priv); > > > + ret = i915_gem_resume(dev_priv); > > > + if (ret) > > > + dev_err(&pdev->dev, "GEM resume failed\n"); > > > > Not DRM_ERROR like other paths? > > Bah, we really need to migrate to dev_err() like all the other cool > drivers. > > DRM_DEV_ERROR() +1 on that. Regards, Joonas
On 9/29/2017 6:46 PM, Joonas Lahtinen wrote: > On Fri, 2017-09-29 at 12:49 +0100, Chris Wilson wrote: >> Quoting Joonas Lahtinen (2017-09-29 12:43:48) >>> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote: >>>> These changes are preparation to handle GuC suspend/resume. Prepared >>>> helper i915_gem_runtime_resume to reinitialize suspended gem setup. >>>> Returning status from i915_gem_runtime_suspend and i915_gem_resume. >>>> This will be placeholder for handling any errors from uC suspend/resume >>>> in upcoming patches. Restructured the suspend/resume routines w.r.t setup >>>> creation and rollback order. >>>> This also fixes issue of ordering of i915_gem_runtime_resume with >>>> intel_runtime_pm_enable_interrupts. >>>> >>>> v2: Fixed return from intel_runtime_resume. (Michał Winiarski) >>>> >>>> v3: Not returning status from gem_runtime_resume. (Chris) >>>> >>>> v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko) >>>> >>>> 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> >>> <SNIP> >>> >>>> @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev) >>>> >>>> intel_csr_ucode_resume(dev_priv); >>>> >>>> - i915_gem_resume(dev_priv); >>>> + ret = i915_gem_resume(dev_priv); >>>> + if (ret) >>>> + dev_err(&pdev->dev, "GEM resume failed\n"); >>> Not DRM_ERROR like other paths? >> Bah, we really need to migrate to dev_err() like all the other cool >> drivers. >> >> DRM_DEV_ERROR() > +1 on that. > > Regards, Joonas Sure. Will update. Thanks for review.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 7056bb2..502ae13 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1655,6 +1655,7 @@ static int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state) static int i915_drm_resume(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); + struct pci_dev *pdev = dev_priv->drm.pdev; int ret; disable_rpm_wakeref_asserts(dev_priv); @@ -1666,7 +1667,9 @@ static int i915_drm_resume(struct drm_device *dev) intel_csr_ucode_resume(dev_priv); - i915_gem_resume(dev_priv); + ret = i915_gem_resume(dev_priv); + if (ret) + dev_err(&pdev->dev, "GEM resume failed\n"); i915_restore_state(dev_priv); intel_pps_unlock_regs_wa(dev_priv); @@ -2495,7 +2498,9 @@ static int intel_runtime_suspend(struct device *kdev) * We are safe here against re-faults, since the fault handler takes * an RPM reference. */ - i915_gem_runtime_suspend(dev_priv); + ret = i915_gem_runtime_suspend(dev_priv); + if (ret) + goto err_gem_suspend; intel_guc_suspend(dev_priv); @@ -2513,11 +2518,7 @@ static int intel_runtime_suspend(struct device *kdev) if (ret) { DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret); - intel_runtime_pm_enable_interrupts(dev_priv); - - enable_rpm_wakeref_asserts(dev_priv); - - return ret; + goto err_suspend_complete; } intel_uncore_suspend(dev_priv); @@ -2560,6 +2561,15 @@ static int intel_runtime_suspend(struct device *kdev) DRM_DEBUG_KMS("Device suspended\n"); return 0; + +err_suspend_complete: + intel_runtime_pm_enable_interrupts(dev_priv); + intel_guc_resume(dev_priv); + i915_gem_runtime_resume(dev_priv); + +err_gem_suspend: + enable_rpm_wakeref_asserts(dev_priv); + return ret; } static int intel_runtime_resume(struct device *kdev) @@ -2596,13 +2606,6 @@ static int intel_runtime_resume(struct device *kdev) ret = vlv_resume_prepare(dev_priv, true); } - /* - * No point of rolling back things in case of an error, as the best - * we can do is to hope that things will still work (and disable RPM). - */ - i915_gem_init_swizzling(dev_priv); - i915_gem_restore_fences(dev_priv); - intel_runtime_pm_enable_interrupts(dev_priv); /* @@ -2615,6 +2618,8 @@ static int intel_runtime_resume(struct device *kdev) intel_enable_ipc(dev_priv); + i915_gem_runtime_resume(dev_priv); + enable_rpm_wakeref_asserts(dev_priv); if (ret) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b7cba89..42f3d89 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3479,7 +3479,8 @@ struct i915_vma * __must_check int i915_gem_object_unbind(struct drm_i915_gem_object *obj); void i915_gem_release_mmap(struct drm_i915_gem_object *obj); -void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv); +int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv); +void i915_gem_runtime_resume(struct drm_i915_private *dev_priv); static inline int __sg_page_count(const struct scatterlist *sg) { @@ -3682,7 +3683,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine, int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv, unsigned int flags); int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv); -void i915_gem_resume(struct drm_i915_private *dev_priv); +int i915_gem_resume(struct drm_i915_private *dev_priv); int i915_gem_fault(struct vm_fault *vmf); int i915_gem_object_wait(struct drm_i915_gem_object *obj, unsigned int flags, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 73eeb6b..59a88f2 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2022,7 +2022,7 @@ int i915_gem_fault(struct vm_fault *vmf) intel_runtime_pm_put(i915); } -void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) +int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) { struct drm_i915_gem_object *obj, *on; int i; @@ -2065,6 +2065,18 @@ void i915_gem_runtime_suspend(struct drm_i915_private *dev_priv) GEM_BUG_ON(!list_empty(®->vma->obj->userfault_link)); reg->dirty = true; } + + return 0; +} + +void i915_gem_runtime_resume(struct drm_i915_private *dev_priv) +{ + /* + * No point of rolling back things in case of an error, as the best + * we can do is to hope that things will still work (and disable RPM). + */ + i915_gem_init_swizzling(dev_priv); + i915_gem_restore_fences(dev_priv); } static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj) @@ -4587,7 +4599,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) return ret; } -void i915_gem_resume(struct drm_i915_private *dev_priv) +int i915_gem_resume(struct drm_i915_private *dev_priv) { struct drm_device *dev = &dev_priv->drm; @@ -4603,6 +4615,8 @@ void i915_gem_resume(struct drm_i915_private *dev_priv) dev_priv->gt.resume(dev_priv); mutex_unlock(&dev->struct_mutex); + + return 0; } void i915_gem_init_swizzling(struct drm_i915_private *dev_priv)
These changes are preparation to handle GuC suspend/resume. Prepared helper i915_gem_runtime_resume to reinitialize suspended gem setup. Returning status from i915_gem_runtime_suspend and i915_gem_resume. This will be placeholder for handling any errors from uC suspend/resume in upcoming patches. Restructured the suspend/resume routines w.r.t setup creation and rollback order. This also fixes issue of ordering of i915_gem_runtime_resume with intel_runtime_pm_enable_interrupts. v2: Fixed return from intel_runtime_resume. (Michał Winiarski) v3: Not returning status from gem_runtime_resume. (Chris) v4: Refined return from i915_gem_runtime_suspend. (Michal Wajdeczko) 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 | 33 +++++++++++++++++++-------------- drivers/gpu/drm/i915/i915_drv.h | 5 +++-- drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- 3 files changed, 38 insertions(+), 18 deletions(-)