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@xxxxxxxxx>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> ---
> 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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
