[Intel-gfx] [PATCH] drm/i915/guc: Skip suspend/resume GuC action on platforms w/o GuC submission

Fri Nov 15 23:10:15 UTC 2019

On 11/15/2019 2:22 PM, Hiatt, Don wrote:
>
>> From: Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>
>> Sent: Friday, November 15, 2019 2:02 PM
>> To: Hiatt, Don <don.hiatt at intel.com>; intel-gfx at lists.freedesktop.org
>> Cc: Ewins, Jon <jon.ewins at intel.com>; KiteStramuort
>> <kitestramuort at autistici.org>; S . Zharkoff <s.zharkoff at gmail.com>;
>> Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Summers, Stuart
>> <stuart.summers at intel.com>; Chris Wilson <chris at chris-wilson.co.uk>; Tomas
>> Janousek <tomi at nomi.cz>
>> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
>> platforms w/o GuC submission
>>
>>
>>
>> On 11/15/19 10:20 AM, don.hiatt at intel.com wrote:
>>> From: Don Hiatt <don.hiatt at intel.com>
>>>
>>> On some platforms (e.g. KBL) that do not support GuC submission, but
>>> the user enabled the GuC communication (e.g for HuC authentication)
>>> calling the GuC EXIT_S_STATE action results in lose of ability to
>>> enter RC6. We can remove the GuC suspend/resume entirely as we do
>>> not need to save the GuC submission status.
>>>
>>> Add intel_guc_submission_is_enabled() function to determine if
>>> GuC submission is active.
>>>
>>> v2: Do not suspend/resume the GuC on platforms that do not support
>>>       Guc Submission.
>>> v3: Fix typo, move suspend logic to remove goto.
>>> v4: Use intel_guc_submission_is_enabled() to check GuC submission
>>>       status.
>>> v5: No need to look at engine to determine if submission is enabled.
>>>       Squash fix + intel_guc_submission_is_enabled() patch into one.
>>>
>>> Reported-by: KiteStramuort <kitestramuort at autistici.org>
>>> Reported-by: S. Zharkoff <s.zharkoff at gmail.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111594
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111623
>>> Fixes: f774f0964919 ("drm/i915/guc: Turn on GuC/HuC auto mode")
>> I don't think this Fixes tag is appropriate since we already reverted
>> that patch.
>>
> Can you suggest an appropriate tag? Chris suggest "guc/huc enabling by default" patch
> but I couldn't find it and this one seemed similar.

Do we want to backport this even if guc is off by default and taints on 
enabling? if we do, then we probably need to go all the way back to when 
we introduced the new binaries that show the problem ("drm/i915/guc: 
Updates for GuC 32.0.3 firmware").

Daniele

>
>>> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Daniele Ceralo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Stuart Summers <stuart.summers at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> Tested-by: Tomas Janousek <tomi at nomi.cz>
>>> Signed-off-by: Don Hiatt <don.hiatt at intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/uc/intel_guc.c | 7 +++++++
>>>    drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 8 ++++++++
>>>    drivers/gpu/drm/i915/i915_drv.h        | 6 ++++++
>>>    3 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> index 019ae6486e8d..92d9305c0d73 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.c
>>> @@ -553,6 +553,13 @@ int intel_guc_suspend(struct intel_guc *guc)
>>>    		GUC_POWER_D1, /* any value greater than GUC_POWER_D0
>> */
>>>    	};
>>>
>>> +	/*
>>> +	 * If GuC communication is enabled but submission is not supported,
>>> +	 * we do not need to suspend the GuC.
>>> +	 */
>>> +	if (!intel_guc_submission_is_enabled(guc))
>>> +		return 0;
>>> +
>>>    	/*
>>>    	 * The ENTER_S_STATE action queues the save/restore operation in GuC
>> FW
>>>    	 * and then returns, so waiting on the H2G is not enough to guarantee
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index 629b19377a29..4dd43b99a334 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -605,6 +605,14 @@ static int __uc_resume(struct intel_uc *uc, bool
>> enable_communication)
>>>    	if (enable_communication)
>>>    		guc_enable_communication(guc);
>>>
>>> +	/*
>>> +	 * If GuC communication is enabled but submission is not supported,
>>> +	 * we do not need to resume the GuC but we do need to enable the
>>> +	 * GuC communication on resume (above).
>>> +	 */
>>> +	if (!intel_guc_submission_is_enabled(guc))
>>> +		return 0;
>>> +
>> I would put this check inside intel_guc_resume(), for symmetry with the
>> one you put inside intel_guc_suspend().
>>
> Will do.
>
>> Daniele
>>
>>>    	err = intel_guc_resume(guc);
>>>    	if (err) {
>>>    		DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index 7bc1d8c102b2..2b879472e760 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2044,4 +2044,10 @@ i915_coherent_map_type(struct drm_i915_private
>> *i915)
>>>    	return HAS_LLC(i915) ? I915_MAP_WB : I915_MAP_WC;
>>>    }
>>>
>>> +static inline bool intel_guc_submission_is_enabled(struct intel_guc *guc)
>>> +{
>>> +	return intel_guc_is_submission_supported(guc) &&
>>> +		intel_guc_is_running(guc);
>>> +}
>>> +
>>>    #endif
>>>