drm/i915/guc: Skip suspend/resume GuC action on platforms w/o GuC submission

Commit Message

Hiatt, Don Nov. 15, 2019, 6:20 p.m. UTC

From: Don Hiatt <[e-mail]>

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 <[e-mail]>
Reported-by: S. Zharkoff <[e-mail]>
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")
Cc: Michal Wajdeczko <[e-mail]>
Cc: Daniele Ceralo Spurio <[e-mail]>
Cc: Stuart Summers <[e-mail]>
Cc: Chris Wilson <[e-mail]>
Tested-by: Tomas Janousek <[e-mail]>
Signed-off-by: Don Hiatt <[e-mail]>
---
 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(+)

Comments

Daniele Ceraolo Spurio Nov. 15, 2019, 10:02 p.m. UTC | #1

On 11/15/19 10:20 AM, [e-mail] wrote:
> From: Don Hiatt <[e-mail]>
> 
> 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 <[e-mail]>
> Reported-by: S. Zharkoff <[e-mail]>
> 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.

> Cc: Michal Wajdeczko <[e-mail]>
> Cc: Daniele Ceralo Spurio <[e-mail]>
> Cc: Stuart Summers <[e-mail]>
> Cc: Chris Wilson <[e-mail]>
> Tested-by: Tomas Janousek <[e-mail]>
> Signed-off-by: Don Hiatt <[e-mail]>
> ---
>   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().

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
>

Hiatt, Don Nov. 15, 2019, 10:22 p.m. UTC | #2

> From: Ceraolo Spurio, Daniele <[e-mail]>
> Sent: Friday, November 15, 2019 2:02 PM
> To: Hiatt, Don <[e-mail]>; [e-mail]
> Cc: Ewins, Jon <[e-mail]>; KiteStramuort
> <[e-mail]>; S . Zharkoff <[e-mail]>;
> Wajdeczko, Michal <[e-mail]>; Summers, Stuart
> <[e-mail]>; Chris Wilson <[e-mail]>; Tomas
> Janousek <[e-mail]>
> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
> platforms w/o GuC submission
> 
> 
> 
> On 11/15/19 10:20 AM, [e-mail] wrote:
> > From: Don Hiatt <[e-mail]>
> >
> > 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 <[e-mail]>
> > Reported-by: S. Zharkoff <[e-mail]>
> > 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.


> > Cc: Michal Wajdeczko <[e-mail]>
> > Cc: Daniele Ceralo Spurio <[e-mail]>
> > Cc: Stuart Summers <[e-mail]>
> > Cc: Chris Wilson <[e-mail]>
> > Tested-by: Tomas Janousek <[e-mail]>
> > Signed-off-by: Don Hiatt <[e-mail]>
> > ---
> >   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
> >

Daniele Ceraolo Spurio Nov. 15, 2019, 11:10 p.m. UTC | #3

On 11/15/2019 2:22 PM, Hiatt, Don wrote:
>
>> From: Ceraolo Spurio, Daniele <[e-mail]>
>> Sent: Friday, November 15, 2019 2:02 PM
>> To: Hiatt, Don <[e-mail]>; [e-mail]
>> Cc: Ewins, Jon <[e-mail]>; KiteStramuort
>> <[e-mail]>; S . Zharkoff <[e-mail]>;
>> Wajdeczko, Michal <[e-mail]>; Summers, Stuart
>> <[e-mail]>; Chris Wilson <[e-mail]>; Tomas
>> Janousek <[e-mail]>
>> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
>> platforms w/o GuC submission
>>
>>
>>
>> On 11/15/19 10:20 AM, [e-mail] wrote:
>>> From: Don Hiatt <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Reported-by: S. Zharkoff <[e-mail]>
>>> 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 <[e-mail]>
>>> Cc: Daniele Ceralo Spurio <[e-mail]>
>>> Cc: Stuart Summers <[e-mail]>
>>> Cc: Chris Wilson <[e-mail]>
>>> Tested-by: Tomas Janousek <[e-mail]>
>>> Signed-off-by: Don Hiatt <[e-mail]>
>>> ---
>>>    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
>>>

Hiatt, Don Nov. 15, 2019, 11:15 p.m. UTC | #4

> From: Ceraolo Spurio, Daniele <[e-mail]>
> Sent: Friday, November 15, 2019 3:10 PM
> To: Hiatt, Don <[e-mail]>; [e-mail]
> Cc: Ewins, Jon <[e-mail]>; KiteStramuort
> <[e-mail]>; S . Zharkoff <[e-mail]>;
> Wajdeczko, Michal <[e-mail]>; Summers, Stuart
> <[e-mail]>; Chris Wilson <[e-mail]>; Tomas
> Janousek <[e-mail]>
> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
> platforms w/o GuC submission
> 
> 
> 
> On 11/15/2019 2:22 PM, Hiatt, Don wrote:
> >
> >> From: Ceraolo Spurio, Daniele <[e-mail]>
> >> Sent: Friday, November 15, 2019 2:02 PM
> >> To: Hiatt, Don <[e-mail]>; [e-mail]
> >> Cc: Ewins, Jon <[e-mail]>; KiteStramuort
> >> <[e-mail]>; S . Zharkoff <[e-mail]>;
> >> Wajdeczko, Michal <[e-mail]>; Summers, Stuart
> >> <[e-mail]>; Chris Wilson <[e-mail]>;
> Tomas
> >> Janousek <[e-mail]>
> >> Subject: Re: [PATCH] drm/i915/guc: Skip suspend/resume GuC action on
> >> platforms w/o GuC submission
> >>
> >>
> >>
> >> On 11/15/19 10:20 AM, [e-mail] wrote:
> >>> From: Don Hiatt <[e-mail]>
> >>>
> >>> 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 <[e-mail]>
> >>> Reported-by: S. Zharkoff <[e-mail]>
> >>> 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").
> 
Yes, we do as the original reports was with the user specifically enabling
with the driver parameters.

Thanks a lot! The update patch will be posted in a second.

> Daniele
> 
> >
> >>> Cc: Michal Wajdeczko <[e-mail]>
> >>> Cc: Daniele Ceralo Spurio <[e-mail]>
> >>> Cc: Stuart Summers <[e-mail]>
> >>> Cc: Chris Wilson <[e-mail]>
> >>> Tested-by: Tomas Janousek <[e-mail]>
> >>> Signed-off-by: Don Hiatt <[e-mail]>
> >>> ---
> >>>    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
> >>>

Patch

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;
+
 	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