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

[Chris Wilson]

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 at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Michał Winiarski <michal.winiarski at 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