[PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Sagar Arun Kamble]

i915 is currently doing Full GPU reset at the end of suspend followed by
GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
no longer match the GuC's expectations and its suspend will not be
successful. With this change, i915 suspends GuC after suspending GEM and
before doing Full GPU reset.

v2: Commit message update. (Chris)

Cc: Jeff McGee <jeff.mcgee@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 --
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c616b4e..7b4fa84 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1469,8 +1469,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 		goto out;
 	}
 
-	intel_guc_suspend(dev_priv);
-
 	intel_display_suspend(dev);
 
 	intel_dp_mst_suspend(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bbc6f1c..9234334 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4456,6 +4456,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	i915_gem_context_lost(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
+	intel_guc_suspend(dev_priv);
+
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
 
-- 
1.9.1

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

✓ Fi.CI.BAT: success for series starting with [v2,1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend
URL   : https://patchwork.freedesktop.org/series/22512/
State : success

== Summary ==

Series 22512v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/22512/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 435s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 425s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 573s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 505s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 541s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 479s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 475s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 405s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 404s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 423s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 483s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 465s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 451s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 570s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 462s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 467s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 499s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 432s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 533s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 398s

bdea8c440d1dba4f8f8145cf86852011f396282f drm-tip: 2017y-04m-05d-09h-08m-21s UTC integration manifest
080b177 drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4406/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Chris Wilson]

On Wed, Apr 05, 2017 at 03:51:50PM +0530, Sagar Arun Kamble wrote:
> i915 is currently doing Full GPU reset at the end of suspend followed by
> GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
> suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
> no longer match the GuC's expectations and its suspend will not be
> successful. With this change, i915 suspends GuC after suspending GEM and
> before doing Full GPU reset.

I'll massage the commit message slightly (much easier when proofreading
someone else's writing). It makes sense to me, any takers from those who
know guc more intimately or at least have observed and tested this patch?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Daniele Ceraolo Spurio]

On 05/04/17 05:24, Chris Wilson wrote:
> On Wed, Apr 05, 2017 at 03:51:50PM +0530, Sagar Arun Kamble wrote:
>> i915 is currently doing Full GPU reset at the end of suspend followed by
>> GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
>> suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
>> no longer match the GuC's expectations and its suspend will not be
>> successful. With this change, i915 suspends GuC after suspending GEM and

Here I would say here that GuC suspend fails because we're resetting GuC 
as part of the GPU reset and not because the expectation doesn't match.

>> before doing Full GPU reset.
>
> I'll massage the commit message slightly (much easier when proofreading
> someone else's writing). It makes sense to me, any takers from those who
> know guc more intimately or at least have observed and tested this patch?
> -Chris
>

Change looks good to me:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

On a related note, we only allocate the pages for saving the GuC state 
if i915.enable_guc_submission=1 (they're part of the ADS), but from what 
I can see we call intel_guc_suspend/resume unconditionally.
Since the ADS are not designed to be submission-only structs I think we 
should move the ADS allocation to always happen when the GuC is loaded 
without looking at i915.enable_guc_submission.

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

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Chris Wilson]

On Wed, Apr 05, 2017 at 09:38:49AM -0700, Daniele Ceraolo Spurio wrote:
> 
> 
> On 05/04/17 05:24, Chris Wilson wrote:
> >On Wed, Apr 05, 2017 at 03:51:50PM +0530, Sagar Arun Kamble wrote:
> >>i915 is currently doing Full GPU reset at the end of suspend followed by
> >>GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
> >>suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
> >>no longer match the GuC's expectations and its suspend will not be
> >>successful. With this change, i915 suspends GuC after suspending GEM and
> 
> Here I would say here that GuC suspend fails because we're resetting
> GuC as part of the GPU reset and not because the expectation doesn't
> match.
> 
> >>before doing Full GPU reset.
> >
> >I'll massage the commit message slightly (much easier when proofreading
> >someone else's writing). It makes sense to me, any takers from those who
> >know guc more intimately or at least have observed and tested this patch?
> >-Chris
> >
> 
> Change looks good to me:
> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>

Amended commit as suggested and pushed. Thanks for the patch and review,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Joonas Lahtinen]

On ke, 2017-04-05 at 15:51 +0530, Sagar Arun Kamble wrote:
> i915 is currently doing Full GPU reset at the end of suspend followed by
> GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
> suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
> no longer match the GuC's expectations and its suspend will not be
> successful. With this change, i915 suspends GuC after suspending GEM and
> before doing Full GPU reset.

+ David, Oscar and Michel

My understanding is that reloading GuC firmware after each resume is a
major bottleneck in resume time, and we instead should be telling GuC
to suspend and not reset the GPU, at most only reset the engines.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Chris Wilson]

On Wed, Apr 05, 2017 at 03:54:27PM +0300, Joonas Lahtinen wrote:
> On ke, 2017-04-05 at 15:51 +0530, Sagar Arun Kamble wrote:
> > i915 is currently doing Full GPU reset at the end of suspend followed by
> > GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
> > suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
> > no longer match the GuC's expectations and its suspend will not be
> > successful. With this change, i915 suspends GuC after suspending GEM and
> > before doing Full GPU reset.
> 
> + David, Oscar and Michel
> 
> My understanding is that reloading GuC firmware after each resume is a
> major bottleneck in resume time, and we instead should be telling GuC
> to suspend and not reset the GPU, at most only reset the engines.

resetting the gpu is for robustness, to make sure that the gpu is truly
inactive after we lose control. Now you might argue to move that to only
the hibernate paths, but paranoia says to do it before the bios is
invovled on suspend.

We do not need to wait for firmware to load, on init or resume, along
with most things we can do it asynchronously. We could even start
userspace with execlists and then switch to guc, if firmware loading was
that slow (or just the firmware was non-existent e.g. builtin with no
initrd).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[David Weinehall]

On 2017-04-05 15:54, Joonas Lahtinen wrote:
> On ke, 2017-04-05 at 15:51 +0530, Sagar Arun Kamble wrote:
>> i915 is currently doing Full GPU reset at the end of suspend followed by
>> GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
>> suspend before we do a direct intel_gpu_reset, Otherwise the gpu state will
>> no longer match the GuC's expectations and its suspend will not be
>> successful. With this change, i915 suspends GuC after suspending GEM and
>> before doing Full GPU reset.
> + David, Oscar and Michel
>
> My understanding is that reloading GuC firmware after each resume is a
> major bottleneck in resume time, and we instead should be telling GuC
> to suspend and not reset the GPU, at most only reset the engines.
>
> Regards, Joonas

If this is possible, then yes, it'd definitely be preferable. If not,
then doing the GuC + HuC loading asynchronously on resume would be 
preferable.
Anusha mentioned working on asynchronous loading, hence added to CC.


Kind regards, David
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[Kamble, Sagar A]

On 4/5/2017 6:32 PM, David Weinehall wrote:
> On 2017-04-05 15:54, Joonas Lahtinen wrote:
>> On ke, 2017-04-05 at 15:51 +0530, Sagar Arun Kamble wrote:
>>> i915 is currently doing Full GPU reset at the end of suspend 
>>> followed by
>>> GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
>>> suspend before we do a direct intel_gpu_reset, Otherwise the gpu 
>>> state will
>>> no longer match the GuC's expectations and its suspend will not be
>>> successful. With this change, i915 suspends GuC after suspending GEM 
>>> and
>>> before doing Full GPU reset.
>> + David, Oscar and Michel
>>
>> My understanding is that reloading GuC firmware after each resume is a
>> major bottleneck in resume time, and we instead should be telling GuC
>> to suspend and not reset the GPU, at most only reset the engines.
>>
>> Regards, Joonas
>
> If this is possible, then yes, it'd definitely be preferable. If not,
> then doing the GuC + HuC loading asynchronously on resume would be 
> preferable.
> Anusha mentioned working on asynchronous loading, hence added to CC.
>
>
> Kind regards, David
Data points about GuC load times for reference that I collected today on 
SKL and APL.
At Rpn(lowest frequency) it loads/becomes ready in 3-4 jiffies. Running 
at >=RPe it becomes ready in a jiffy.
Are these times in tolerable limits?
Another concern Daniele highlighted was rare chance of WOPCM persisting 
post suspend/resume and hence needing reload.
I believe the fetch from disk must be time consuming during boot time 
load and for that asynchronous
load can be pursued.

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

Re: [PATCH v2 1/1] drm/i915: Suspend GuC prior to GPU Reset during GEM suspend

[David Weinehall]

On 2017-04-05 18:46, Kamble, Sagar A wrote:
> On 4/5/2017 6:32 PM, David Weinehall wrote:
>> On 2017-04-05 15:54, Joonas Lahtinen wrote:
>>> On ke, 2017-04-05 at 15:51 +0530, Sagar Arun Kamble wrote:
>>>> i915 is currently doing Full GPU reset at the end of suspend 
>>>> followed by
>>>> GuC suspend. This reset bypasses the GuC. We need to tell the GuC to
>>>> suspend before we do a direct intel_gpu_reset, Otherwise the gpu 
>>>> state will
>>>> no longer match the GuC's expectations and its suspend will not be
>>>> successful. With this change, i915 suspends GuC after suspending 
>>>> GEM and
>>>> before doing Full GPU reset.
>>> + David, Oscar and Michel
>>>
>>> My understanding is that reloading GuC firmware after each resume is a
>>> major bottleneck in resume time, and we instead should be telling GuC
>>> to suspend and not reset the GPU, at most only reset the engines.
>>>
>>> Regards, Joonas
>>
>> If this is possible, then yes, it'd definitely be preferable. If not,
>> then doing the GuC + HuC loading asynchronously on resume would be 
>> preferable.
>> Anusha mentioned working on asynchronous loading, hence added to CC.
>>
>>
>> Kind regards, David
> Data points about GuC load times for reference that I collected today 
> on SKL and APL.
> At Rpn(lowest frequency) it loads/becomes ready in 3-4 jiffies. 
> Running at >=RPe it becomes ready in a jiffy.
> Are these times in tolerable limits?
> Another concern Daniele highlighted was rare chance of WOPCM 
> persisting post suspend/resume and hence needing reload.
> I believe the fetch from disk must be time consuming during boot time 
> load and for that asynchronous
> load can be pursued.
>
I've seen times on the order of 30-35ms. It should be noted though that 
this included HuC (and HuC verifying the integrity of HuC or whatever it 
does).

I can re-run the tests once I'm done with various other tests I'm doing 
right now though;
my GuC-related tests are quite old, since it's not loaded by default 
(which is probably a good
thing, considering the stability issues we're seeing with GuC).


Kind regards, David
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx