| Message ID | 1505929104-28823-2-git-send-email-sagar.a.kamble@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 20, 2017 at 11:08:16PM +0530, Sagar Arun Kamble wrote: > Prepared generic helpers intel_uc_suspend, intel_uc_resume, > intel_uc_runtime_suspend, intel_uc_runtime_resume. Added > error handling to all the calls for suspend/resume. I find the error handling done this way quite surprising... More below. > > v2: Rebase w.r.t removal of GuC code restructuring. > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++++++++++++++--- > drivers/gpu/drm/i915/i915_gem.c | 7 ++++++- > drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++ > drivers/gpu/drm/i915/intel_uc.h | 4 ++++ > 4 files changed, 50 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 5c111ea..8635f40 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1691,7 +1691,15 @@ static int i915_drm_resume(struct drm_device *dev) > } > mutex_unlock(&dev->struct_mutex); > > - intel_guc_resume(dev_priv); > + /* > + * NB: Full gem reinitialization is being done above, hence > + * intel_uc_resume will be of no use. Currently intel_uc_resume > + * is nop. If full reinitialization is removed, will need to put > + * functionality to resume from sleep in intel_uc_resume. > + */ > + ret = intel_uc_resume(dev_priv); > + if (ret) > + DRM_ERROR("failed to resume uc\n"); > > intel_modeset_init_hw(dev); > > @@ -2493,7 +2501,12 @@ static int intel_runtime_suspend(struct device *kdev) > */ > i915_gem_runtime_suspend(dev_priv); > > - intel_guc_suspend(dev_priv); > + ret = intel_uc_runtime_suspend(dev_priv); > + if (ret) { > + DRM_ERROR("uc runtime suspend failed, disabling it(%d)\n", ret); > + enable_rpm_wakeref_asserts(dev_priv); > + return ret; Early exit? > + } > > intel_runtime_pm_disable_interrupts(dev_priv); > > @@ -2578,7 +2591,11 @@ 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); > + ret = intel_uc_runtime_resume(dev_priv); > + if (ret) { > + DRM_ERROR("uc runtime resume failed (%d)\n", ret); > + return ret; Same here. > + } > > if (IS_GEN9_LP(dev_priv)) { > bxt_disable_dc9(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index c4bf348..dd56d45 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4539,7 +4539,11 @@ 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); > + ret = intel_uc_suspend(dev_priv); > + if (ret) { > + DRM_ERROR("uc suspend failed (%d)\n", ret); > + goto out; Should we really exit early if GuC sleep action fails? In all of those cases - is this really something critical? Shouldn't we go through the rest of the suspend/resume dance even if we fail to talk with GuC? -Michał > + } > > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > cancel_delayed_work_sync(&dev_priv->gt.retire_work); > @@ -4583,6 +4587,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) > > err_unlock: > mutex_unlock(&dev->struct_mutex); > +out: > intel_runtime_pm_put(dev_priv); > return ret; > } > diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c > index 0178ba4..8e4d8b0 100644 > --- a/drivers/gpu/drm/i915/intel_uc.c > +++ b/drivers/gpu/drm/i915/intel_uc.c > @@ -537,3 +537,23 @@ int intel_guc_sample_forcewake(struct intel_guc *guc) > > return intel_guc_send(guc, action, ARRAY_SIZE(action)); > } > + > +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv) > +{ > + return intel_guc_suspend(dev_priv); > +} > + > +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv) > +{ > + return intel_guc_resume(dev_priv); > +} > + > +int intel_uc_suspend(struct drm_i915_private *dev_priv) > +{ > + return intel_guc_suspend(dev_priv); > +} > + > +int intel_uc_resume(struct drm_i915_private *dev_priv) > +{ > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h > index 7703c9a..3d33a51 100644 > --- a/drivers/gpu/drm/i915/intel_uc.h > +++ b/drivers/gpu/drm/i915/intel_uc.h > @@ -208,6 +208,10 @@ struct intel_huc { > void intel_uc_fini_fw(struct drm_i915_private *dev_priv); > int intel_uc_init_hw(struct drm_i915_private *dev_priv); > void intel_uc_fini_hw(struct drm_i915_private *dev_priv); > +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv); > +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv); > +int intel_uc_suspend(struct drm_i915_private *dev_priv); > +int intel_uc_resume(struct drm_i915_private *dev_priv); > int intel_guc_sample_forcewake(struct intel_guc *guc); > int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); > int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len); > -- > 1.9.1 >
On 9/21/2017 10:46 PM, Michał Winiarski wrote: > On Wed, Sep 20, 2017 at 11:08:16PM +0530, Sagar Arun Kamble wrote: >> Prepared generic helpers intel_uc_suspend, intel_uc_resume, >> intel_uc_runtime_suspend, intel_uc_runtime_resume. Added >> error handling to all the calls for suspend/resume. > I find the error handling done this way quite surprising... > More below. > >> v2: Rebase w.r.t removal of GuC code restructuring. >> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> >> Cc: Michał Winiarski <michal.winiarski@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++++++++++++++--- >> drivers/gpu/drm/i915/i915_gem.c | 7 ++++++- >> drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++ >> drivers/gpu/drm/i915/intel_uc.h | 4 ++++ >> 4 files changed, 50 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c >> index 5c111ea..8635f40 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -1691,7 +1691,15 @@ static int i915_drm_resume(struct drm_device *dev) >> } >> mutex_unlock(&dev->struct_mutex); >> >> - intel_guc_resume(dev_priv); >> + /* >> + * NB: Full gem reinitialization is being done above, hence >> + * intel_uc_resume will be of no use. Currently intel_uc_resume >> + * is nop. If full reinitialization is removed, will need to put >> + * functionality to resume from sleep in intel_uc_resume. >> + */ >> + ret = intel_uc_resume(dev_priv); >> + if (ret) >> + DRM_ERROR("failed to resume uc\n"); >> >> intel_modeset_init_hw(dev); >> >> @@ -2493,7 +2501,12 @@ static int intel_runtime_suspend(struct device *kdev) >> */ >> i915_gem_runtime_suspend(dev_priv); >> >> - intel_guc_suspend(dev_priv); >> + ret = intel_uc_runtime_suspend(dev_priv); >> + if (ret) { >> + DRM_ERROR("uc runtime suspend failed, disabling it(%d)\n", ret); >> + enable_rpm_wakeref_asserts(dev_priv); >> + return ret; > Early exit? > >> + } >> >> intel_runtime_pm_disable_interrupts(dev_priv); >> >> @@ -2578,7 +2591,11 @@ 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); >> + ret = intel_uc_runtime_resume(dev_priv); >> + if (ret) { >> + DRM_ERROR("uc runtime resume failed (%d)\n", ret); >> + return ret; > Same here. > >> + } >> >> if (IS_GEN9_LP(dev_priv)) { >> bxt_disable_dc9(dev_priv); >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index c4bf348..dd56d45 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -4539,7 +4539,11 @@ 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); >> + ret = intel_uc_suspend(dev_priv); >> + if (ret) { >> + DRM_ERROR("uc suspend failed (%d)\n", ret); >> + goto out; > Should we really exit early if GuC sleep action fails? > > In all of those cases - is this really something critical? Shouldn't we go > through the rest of the suspend/resume dance even if we fail to talk with GuC? > > -Michał Yes. Failure in GuC suspend/resume has to be critical. Essentially while going into RPM suspend we do want to ensure GuC is paused and dont want to go ahead and set PCI device state to D3 even if we knew GuC is active. Similarly for resume. If we want to resume with i915 execlists when GuC resume fails that would be different code change. > >> + } >> >> cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); >> cancel_delayed_work_sync(&dev_priv->gt.retire_work); >> @@ -4583,6 +4587,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) >> >> err_unlock: >> mutex_unlock(&dev->struct_mutex); >> +out: >> intel_runtime_pm_put(dev_priv); >> return ret; >> } >> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c >> index 0178ba4..8e4d8b0 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.c >> +++ b/drivers/gpu/drm/i915/intel_uc.c >> @@ -537,3 +537,23 @@ int intel_guc_sample_forcewake(struct intel_guc *guc) >> >> return intel_guc_send(guc, action, ARRAY_SIZE(action)); >> } >> + >> +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv) >> +{ >> + return intel_guc_suspend(dev_priv); >> +} >> + >> +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv) >> +{ >> + return intel_guc_resume(dev_priv); >> +} >> + >> +int intel_uc_suspend(struct drm_i915_private *dev_priv) >> +{ >> + return intel_guc_suspend(dev_priv); >> +} >> + >> +int intel_uc_resume(struct drm_i915_private *dev_priv) >> +{ >> + return 0; >> +} >> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h >> index 7703c9a..3d33a51 100644 >> --- a/drivers/gpu/drm/i915/intel_uc.h >> +++ b/drivers/gpu/drm/i915/intel_uc.h >> @@ -208,6 +208,10 @@ struct intel_huc { >> void intel_uc_fini_fw(struct drm_i915_private *dev_priv); >> int intel_uc_init_hw(struct drm_i915_private *dev_priv); >> void intel_uc_fini_hw(struct drm_i915_private *dev_priv); >> +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv); >> +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv); >> +int intel_uc_suspend(struct drm_i915_private *dev_priv); >> +int intel_uc_resume(struct drm_i915_private *dev_priv); >> int intel_guc_sample_forcewake(struct intel_guc *guc); >> int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); >> int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len); >> -- >> 1.9.1 >>
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 5c111ea..8635f40 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1691,7 +1691,15 @@ static int i915_drm_resume(struct drm_device *dev) } mutex_unlock(&dev->struct_mutex); - intel_guc_resume(dev_priv); + /* + * NB: Full gem reinitialization is being done above, hence + * intel_uc_resume will be of no use. Currently intel_uc_resume + * is nop. If full reinitialization is removed, will need to put + * functionality to resume from sleep in intel_uc_resume. + */ + ret = intel_uc_resume(dev_priv); + if (ret) + DRM_ERROR("failed to resume uc\n"); intel_modeset_init_hw(dev); @@ -2493,7 +2501,12 @@ static int intel_runtime_suspend(struct device *kdev) */ i915_gem_runtime_suspend(dev_priv); - intel_guc_suspend(dev_priv); + ret = intel_uc_runtime_suspend(dev_priv); + if (ret) { + DRM_ERROR("uc runtime suspend failed, disabling it(%d)\n", ret); + enable_rpm_wakeref_asserts(dev_priv); + return ret; + } intel_runtime_pm_disable_interrupts(dev_priv); @@ -2578,7 +2591,11 @@ 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); + ret = intel_uc_runtime_resume(dev_priv); + if (ret) { + DRM_ERROR("uc runtime resume failed (%d)\n", ret); + return ret; + } if (IS_GEN9_LP(dev_priv)) { bxt_disable_dc9(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index c4bf348..dd56d45 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4539,7 +4539,11 @@ 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); + ret = intel_uc_suspend(dev_priv); + if (ret) { + DRM_ERROR("uc suspend failed (%d)\n", ret); + goto out; + } cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); cancel_delayed_work_sync(&dev_priv->gt.retire_work); @@ -4583,6 +4587,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv) err_unlock: mutex_unlock(&dev->struct_mutex); +out: intel_runtime_pm_put(dev_priv); return ret; } diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c index 0178ba4..8e4d8b0 100644 --- a/drivers/gpu/drm/i915/intel_uc.c +++ b/drivers/gpu/drm/i915/intel_uc.c @@ -537,3 +537,23 @@ int intel_guc_sample_forcewake(struct intel_guc *guc) return intel_guc_send(guc, action, ARRAY_SIZE(action)); } + +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv) +{ + return intel_guc_suspend(dev_priv); +} + +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv) +{ + return intel_guc_resume(dev_priv); +} + +int intel_uc_suspend(struct drm_i915_private *dev_priv) +{ + return intel_guc_suspend(dev_priv); +} + +int intel_uc_resume(struct drm_i915_private *dev_priv) +{ + return 0; +} diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h index 7703c9a..3d33a51 100644 --- a/drivers/gpu/drm/i915/intel_uc.h +++ b/drivers/gpu/drm/i915/intel_uc.h @@ -208,6 +208,10 @@ struct intel_huc { void intel_uc_fini_fw(struct drm_i915_private *dev_priv); int intel_uc_init_hw(struct drm_i915_private *dev_priv); void intel_uc_fini_hw(struct drm_i915_private *dev_priv); +int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv); +int intel_uc_runtime_resume(struct drm_i915_private *dev_priv); +int intel_uc_suspend(struct drm_i915_private *dev_priv); +int intel_uc_resume(struct drm_i915_private *dev_priv); int intel_guc_sample_forcewake(struct intel_guc *guc); int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len); int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);
Prepared generic helpers intel_uc_suspend, intel_uc_resume, intel_uc_runtime_suspend, intel_uc_runtime_resume. Added error handling to all the calls for suspend/resume. v2: Rebase w.r.t removal of GuC code restructuring. Cc: Michal Wajdeczko <michal.wajdeczko@intel.com> Cc: Michał Winiarski <michal.winiarski@intel.com> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 23 ++++++++++++++++++++--- drivers/gpu/drm/i915/i915_gem.c | 7 ++++++- drivers/gpu/drm/i915/intel_uc.c | 20 ++++++++++++++++++++ drivers/gpu/drm/i915/intel_uc.h | 4 ++++ 4 files changed, 50 insertions(+), 4 deletions(-)