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

[Sagar Arun Kamble]

On 11/7/2017 3:57 PM, Michal Wajdeczko wrote:
> On Tue, 07 Nov 2017 10:21:17 +0100, Chris Wilson 
> <chris at chris-wilson.co.uk> wrote:
>
>> Quoting Sagar Arun Kamble (2017-11-07 06:05:01)
>>>
>>>
>>> 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 at intel.com>
>>> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> >> Cc: Michał Winiarski <michal.winiarski at intel.com>
>>> >> Cc: Michel Thierry <michel.thierry at intel.com>
>>> >> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> >> Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
>>> >> Cc: Joonas Lahtinen <joonas.lahtinen at 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)
>>
>> GuC submission is not i915. It is not part of the user facing api.
>> Operate on intel_guc as you were.
>
> So if GuC submission is not i915 than maybe to avoid confusion we should
> rename i915_guc_submission.c to intel_guc_submission.c ?
Yes. Will need to update functions that are i915_guc* as well. (spotted 
some in guc_log.c as well.)
>
> Michal