diff mbox series

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

Message ID 20191115011112.25249-2-don.hiatt@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915/guc: Add GuC method to determine if submission is active. | expand

Commit Message

Hiatt, Don Nov. 15, 2019, 1:11 a.m. UTC 
From: Don Hiatt <don.hiatt@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.

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.

Signed-off-by: Don Hiatt <don.hiatt@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc.c | 7 +++++++
 drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 8 ++++++++
 2 files changed, 15 insertions(+)

Comments

Summers, Stuart Nov. 15, 2019, 5:12 p.m. UTC | #1 
On Thu, 2019-11-14 at 17:11 -0800, don.hiatt@intel.com wrote:
> From: Don Hiatt <don.hiatt@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.
> 
> 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.
> 
> Signed-off-by: Don Hiatt <don.hiatt@intel.com>

Any reason not to just combine both of these into a single patch?

Thanks,
Stuart

> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc.c | 7 +++++++
>  drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 8 ++++++++
>  2 files changed, 15 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;
> +
>  	err = intel_guc_resume(guc);
>  	if (err) {
>  		DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err);
Chris Wilson Nov. 15, 2019, 5:19 p.m. UTC | #2 
Quoting Summers, Stuart (2019-11-15 17:12:58)
> On Thu, 2019-11-14 at 17:11 -0800, don.hiatt@intel.com wrote:
> > From: Don Hiatt <don.hiatt@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.
> > 
> > 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.
> > 
> > Signed-off-by: Don Hiatt <don.hiatt@intel.com>
> 
> Any reason not to just combine both of these into a single patch?

Also please remember to include the bugzilla link, ask if the reporter
wants to be credited, and most important of all a Fixes: so we can
backport it correctly. If there is no singular cause, point at the
"guc/huc enabling by default" patch.

Last but not least, think about how did we miss in this CI and provide
a Testcase: to verify that it is fixed and stays fixed.
-Chris
Hiatt, Don Nov. 15, 2019, 5:22 p.m. UTC | #3 
> From: Summers, Stuart <stuart.summers@intel.com>
> Sent: Friday, November 15, 2019 9:13 AM
> 
> On Thu, 2019-11-14 at 17:11 -0800, don.hiatt@intel.com wrote:
> > From: Don Hiatt <don.hiatt@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.
> >
> > 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.
> >
> > Signed-off-by: Don Hiatt <don.hiatt@intel.com>
> 
> Any reason not to just combine both of these into a single patch?
> 
> Thanks,
> Stuart
> 

I didn't combine them for two reasons:
1) I wasn't sure if there'd be an ask to use intel_guc_submission_is_enabled() in
    other places.
2) The git log entry highlights the introduction of a new function available for use.

I'll combine them if that's the consensus. 

don

> > ---
> >  drivers/gpu/drm/i915/gt/uc/intel_guc.c | 7 +++++++
> >  drivers/gpu/drm/i915/gt/uc/intel_uc.c  | 8 ++++++++
> >  2 files changed, 15 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;
> > +
> >  	err = intel_guc_resume(guc);
> >  	if (err) {
> >  		DRM_DEBUG_DRIVER("Failed to resume GuC, err=%d", err);
Hiatt, Don Nov. 15, 2019, 5:26 p.m. UTC | #4 
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Friday, November 15, 2019 9:20 AM
> To: Hiatt, Don <don.hiatt@intel.com>; Summers, Stuart
> <stuart.summers@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Skip suspend/resume GuC
> action on platforms w/o GuC submission
> 
> Quoting Summers, Stuart (2019-11-15 17:12:58)
> > On Thu, 2019-11-14 at 17:11 -0800, don.hiatt@intel.com wrote:
> > > From: Don Hiatt <don.hiatt@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.
> > >
> > > 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.
> > >
> > > Signed-off-by: Don Hiatt <don.hiatt@intel.com>
> >
> > Any reason not to just combine both of these into a single patch?
> 
> Also please remember to include the bugzilla link, ask if the reporter
> wants to be credited, and most important of all a Fixes: so we can
> backport it correctly. If there is no singular cause, point at the
> "guc/huc enabling by default" patch.
> 
> Last but not least, think about how did we miss in this CI and provide
> a Testcase: to verify that it is fixed and stays fixed.
> -Chris

Will do, thanks Chris.

don
Tomas Janousek Nov. 15, 2019, 5:29 p.m. UTC | #5 
Don, Chris,

On Fri, Nov 15, 2019 at 05:19:59PM +0000, Chris Wilson wrote:
> Quoting Summers, Stuart (2019-11-15 17:12:58)
> > On Thu, 2019-11-14 at 17:11 -0800, don.hiatt@intel.com wrote:
> > > From: Don Hiatt <don.hiatt@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.
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Don Hiatt <don.hiatt@intel.com>
> > 
> > Any reason not to just combine both of these into a single patch?
> 
> Also please remember to include the bugzilla link, ask if the reporter
> wants to be credited, and most important of all a Fixes: so we can
> backport it correctly. If there is no singular cause, point at the
> "guc/huc enabling by default" patch.

This comment of mine can help with the Fixes: line:
https://bugs.freedesktop.org/show_bug.cgi?id=111623#c20

> I bisected this to [cc80b2ef24898dc62242e194270200b01ca758d1] drm/i915/guc:
> Change platform default GuC mode -- I've been running with enable_guc=-1. If
> I had been running with enable_guc=3, I guess it would've bisected to
> [a2904ade3dc28cf1a1b7deded41f4369f75e664c] drm/i915/guc: Don't allow GuC
> submission.
>
> Anyway, it seems that on this Kaby Lake system, loading HuC without GuC
> submission leads to the GPU not entering rc6 and thus keeping the package
> out of C-states. Obviously then it doesn't matter what GuC version it is, be
> it 9.39 or 32.0.3 or whatever.

Also, as mentioned in another comment:

Tested-by: Tomas Janousek <tomi@nomi.cz>

(Do note that I'm running a backport to 5.3,
https://bugs.freedesktop.org/attachment.cgi?id=145969, not drm-tip.)
Hiatt, Don Nov. 15, 2019, 5:31 p.m. UTC | #6 
> From: Tomas Janousek <tomi@nomi.cz>
> Sent: Friday, November 15, 2019 9:29 AM
> To: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Hiatt, Don <don.hiatt@intel.com>; Summers, Stuart
> <stuart.summers@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 2/2] drm/i915/guc: Skip suspend/resume GuC action on
> platforms w/o GuC submission
> 
> Don, Chris,
> 
> Also, as mentioned in another comment:
> 
> Tested-by: Tomas Janousek <tomi@nomi.cz>
> 

Thanks, Tomas. I replied to your comment earlier today. I'll include your
'Tested-by'.

Thanks,

don


> (Do note that I'm running a backport to 5.3,
> https://bugs.freedesktop.org/attachment.cgi?id=145969, not drm-tip.)
> 
> --
> Tomáš Janoušek, a.k.a. Pivník, a.k.a. Liskni_si, http://work.lisk.in/
Hiatt, Don Nov. 15, 2019, 5:35 p.m. UTC | #7 
Hi Stuart/Chris,

> From: Chris Wilson <chris@chris-wilson.co.uk>
> Sent: Friday, November 15, 2019 9:20 AM
> To: Hiatt, Don <don.hiatt@intel.com>; Summers, Stuart
> <stuart.summers@intel.com>; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Skip suspend/resume GuC
> action on platforms w/o GuC submission
> 
> Quoting Summers, Stuart (2019-11-15 17:12:58)
> > On Thu, 2019-11-14 at 17:11 -0800, don.hiatt@intel.com wrote:
> > > From: Don Hiatt <don.hiatt@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.
> > >
> > > 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.
> > >
> > > Signed-off-by: Don Hiatt <don.hiatt@intel.com>
> >
> > Any reason not to just combine both of these into a single patch?
> 
> Also please remember to include the bugzilla link, ask if the reporter
> wants to be credited, and most important of all a Fixes: so we can
> backport it correctly. If there is no singular cause, point at the
> "guc/huc enabling by default" patch.
> 
> Last but not least, think about how did we miss in this CI and provide
> a Testcase: to verify that it is fixed and stays fixed.
> -Chris


Hi Stuart/Chris.

Actually, I'll just combine them now. I think your point is valid and being a single
patch will make it easier to backport and track wrt to Chris's comments.

Thanks,

don
diff mbox series

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