Re: [PATCH 2/5] drm/i915/guc: Release all client doorbells on suspend and acquire on resume

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

On 11/6/2017 5:43 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-11-05 13:39:40)
>> Before GT device suspend, i915 should release GuC client doorbells by
>> stopping doorbell controller snooping and doorbell deallocation through
>> GuC. They need to be acquired again on resume. This will also resolve
>> the driver unload issue with GuC, where doorbell deallocation was being
>> done post GEM suspend.
>> Release function is called from guc_suspend prior to sending sleep action
>> during runtime and drm suspend. Acquiral is different in runtime and drm
>> resume paths as on drm resume we are currently doing full reinit. Release
>> should be done in synchronization with acquire hence GuC suspend/resume
>> along with doorbell release/acquire should be under struct_mutex. Upcoming
>> suspend and resume restructuring for GuC will update these changes.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c            |  3 +++
>>   drivers/gpu/drm/i915/i915_gem.c            |  5 +++--
>>   drivers/gpu/drm/i915/i915_guc_submission.c | 20 ++++++++++++++++----
>>   drivers/gpu/drm/i915/i915_guc_submission.h |  2 ++
>>   drivers/gpu/drm/i915/intel_guc.c           |  2 ++
>>   5 files changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index e7e9e06..3df8a7d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -47,6 +47,7 @@
>>   #include <drm/i915_drm.h>
>>   
>>   #include "i915_drv.h"
>> +#include "i915_guc_submission.h"
>>   #include "i915_trace.h"
>>   #include "i915_vgpu.h"
>>   #include "intel_drv.h"
>> @@ -2615,6 +2616,8 @@ static int intel_runtime_resume(struct device *kdev)
>>   
>>          intel_guc_resume(dev_priv);
>>   
>> +       i915_guc_clients_acquire_doorbells(&dev_priv->guc);
> intel_guc_acquire_doorbells();
Prefixed "i915_guc_clients" since this modifies submission state 
(clients/doorbells). Should have kept dev_priv as parameter.
what should be the correct option here: intel_guc*(guc) or 
i915_guc*(dev_priv)
>
> Though, if we need to specialise between resume and runtime_resume, I
> suggest adding intel_guc_resume() and intel_guc_runtime_resume() entry
> points. (We probably should find a better way to distinguish those two
> paths, system_resume vs runtime_resume?)
Yes. these will be added in the upcoming GuC suspend/resume 
restructuring series.
Functionality along runtime/system suspend path will be same for GuC. 
Will defer in
the resume path since we are doing full gem reinit on resume from system 
suspend.
> -Chris

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