Re: [PATCH v10 1/9] drm/i915: Create GEM runtime resume helper and handle GEM suspend/resume errors

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

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@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > ---
> >  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(&reg->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)

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