Re: [PATCH v6 1/7] drm/i915: Create uc runtime and system suspend/resume helpers

[Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>]

On 9/22/2017 12:41 PM, Michal Wajdeczko wrote:
> On Fri, 22 Sep 2017 06:49:12 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@xxxxxxxxx> 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.
> >
> > 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;
> > +    }
> >     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);
>
> Is it ok that we suspend uc in 'gem_suspend' but resume in 'drm_resume' ?
>
> Michal
Currently it is fine as we are doing full reinit. But in future, it is 
better to have it in i915_gem_resume.
Will move it there. Thanks.
> > +    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);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx