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

[Sagar Arun Kamble]

On 9/28/2017 5:07 PM, Chris Wilson wrote:
> 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?
As part of gem_resume we are resuming GuC and if that fails we are 
declaring gem wedged.
Will that be okay or we ignore the GuC resume failure and go ahead with 
reinit?
>>   
>>          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
Ok. Will remove this comment.