| Message ID | 1506504639-25631-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 27 Sep 2017 11:30:31 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> 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) > > 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 | 22 +++++++++++++--------- > drivers/gpu/drm/i915/i915_drv.h | 5 +++-- > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- > 3 files changed, 32 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c > index 7056bb2..d0a710d 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"); btw, code above/below uses DRM_ERROR() > i915_restore_state(dev_priv); > intel_pps_unlock_regs_wa(dev_priv); > @@ -2495,7 +2498,11 @@ 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) { > + enable_rpm_wakeref_asserts(dev_priv); > + return ret; as this is second place where we exit, maybe it's time for if (ret) goto fail; fail: enable_rpm_wakeref_asserts(dev_priv); return ret; } > + } > intel_guc_suspend(dev_priv); > @@ -2515,6 +2522,8 @@ 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); > return ret; > @@ -2596,13 +2605,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 +2617,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)
On 9/27/2017 9:11 PM, Michal Wajdeczko wrote: > On Wed, 27 Sep 2017 11:30:31 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> 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) >> >> 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 | 22 +++++++++++++--------- >> drivers/gpu/drm/i915/i915_drv.h | 5 +++-- >> drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- >> 3 files changed, 32 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c >> b/drivers/gpu/drm/i915/i915_drv.c >> index 7056bb2..d0a710d 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"); > > btw, code above/below uses DRM_ERROR() This was in symmetry with i915_gem_suspend return handling. > >> i915_restore_state(dev_priv); >> intel_pps_unlock_regs_wa(dev_priv); >> @@ -2495,7 +2498,11 @@ 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) { >> + enable_rpm_wakeref_asserts(dev_priv); >> + return ret; > > as this is second place where we exit, maybe it's time for > > if (ret) > goto fail; > fail: > enable_rpm_wakeref_asserts(dev_priv); > return ret; > } > Yes. Will update. >> + } >> intel_guc_suspend(dev_priv); >> @@ -2515,6 +2522,8 @@ 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); >> return ret; >> @@ -2596,13 +2605,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 +2617,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)
Quoting Sagar Arun Kamble (2017-09-27 10:30:31) > 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) > > 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 | 22 +++++++++++++--------- > drivers/gpu/drm/i915/i915_drv.h | 5 +++-- > drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- > 3 files changed, 32 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 7056bb2..d0a710d 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"); I am still uneasy about this. Later on in the resume we try to reinit the hw under the assumption that this earlier step succeeded. Previously we have tried to make sure that if GEM fails, we do not leave the display in an unusable state. Is that still true? > > i915_restore_state(dev_priv); > intel_pps_unlock_regs_wa(dev_priv); > @@ -2495,7 +2498,11 @@ 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) { > + enable_rpm_wakeref_asserts(dev_priv); > + return ret; > + } > > intel_guc_suspend(dev_priv); > > @@ -2515,6 +2522,8 @@ 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); > > return ret; > @@ -2596,13 +2605,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). > - */ This comment shouldn't be attached to the following gem init operations as they cannot fail. If they could, we should be throwing a warning here as this may result in a change of swizzling/fencing state as seen by userspace and therefore graphical corruption. -Chris
On 9/28/2017 5:07 PM, Chris Wilson wrote: > Quoting Sagar Arun Kamble (2017-09-27 10:30:31) >> 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) >> >> 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 | 22 +++++++++++++--------- >> drivers/gpu/drm/i915/i915_drv.h | 5 +++-- >> drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- >> 3 files changed, 32 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 7056bb2..d0a710d 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"); > I am still uneasy about this. Later on in the resume we try to reinit > the hw under the assumption that this earlier step succeeded. > > Previously we have tried to make sure that if GEM fails, we do not leave > the display in an unusable state. Is that still true? As part of gem_resume we are resuming GuC and if that fails we are declaring gem wedged. Will that be okay or we ignore the GuC resume failure and go ahead with reinit? >> >> i915_restore_state(dev_priv); >> intel_pps_unlock_regs_wa(dev_priv); >> @@ -2495,7 +2498,11 @@ 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) { >> + enable_rpm_wakeref_asserts(dev_priv); >> + return ret; >> + } >> >> intel_guc_suspend(dev_priv); >> >> @@ -2515,6 +2522,8 @@ 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); >> >> return ret; >> @@ -2596,13 +2605,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). >> - */ > This comment shouldn't be attached to the following gem init operations > as they cannot fail. If they could, we should be throwing a warning here > as this may result in a change of swizzling/fencing state as seen by > userspace and therefore graphical corruption. > -Chris Ok. Will remove this comment.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 7056bb2..d0a710d 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,11 @@ 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) { + enable_rpm_wakeref_asserts(dev_priv); + return ret; + } intel_guc_suspend(dev_priv); @@ -2515,6 +2522,8 @@ 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); return ret; @@ -2596,13 +2605,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 +2617,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) 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 | 22 +++++++++++++--------- drivers/gpu/drm/i915/i915_drv.h | 5 +++-- drivers/gpu/drm/i915/i915_gem.c | 18 ++++++++++++++++-- 3 files changed, 32 insertions(+), 13 deletions(-)