[PATCH] drm/i915: Park the GPU on module load

[PATCH] drm/i915: Park the GPU on module load

[Chris Wilson]

Once we have flushed the first request through the system to both load a
context and record the default state; tell the GPU to park and idle
itself, putting itself immediately (hopefully at least) into a
powersaving state, and allowing ourselves to start from known state
after setting up all our bookkeeping.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 89834ce19acd..be9d012d851b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 
 	assert_kernel_context_is_current(i915);
 
+	/*
+	 * Immediately park the GPU so that we enable powersaving and
+	 * treat it as idle. The next time we issue a request, we will
+	 * unpark and start using the engine->pinned_default_state, otherwise
+	 * it is in limbo and an early reset may fail.
+	 */
+	__i915_gem_park(i915);
+
 	for_each_engine(engine, i915, id) {
 		struct i915_vma *state;
 
-- 
2.19.0

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

Re: [PATCH] drm/i915: Park the GPU on module load

[Tvrtko Ursulin]

On 14/09/2018 09:34, Chris Wilson wrote:
> Once we have flushed the first request through the system to both load a
> context and record the default state; tell the GPU to park and idle
> itself, putting itself immediately (hopefully at least) into a
> powersaving state, and allowing ourselves to start from known state
> after setting up all our bookkeeping.

Otherwise this would happen after a second or so, when the idle work 
handler does it. Crucial thing to mention is I think the park/unpark 
cycle which sets up the pinned default state.

Alternative to patch could be to extract 
__intel_engine_pin_default_state to be called from here and from 
intel_engines_unpark.

But I guess there is some value in going powersave as soon as possible 
so okay.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 89834ce19acd..be9d012d851b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   
>   	assert_kernel_context_is_current(i915);
>   
> +	/*
> +	 * Immediately park the GPU so that we enable powersaving and
> +	 * treat it as idle. The next time we issue a request, we will
> +	 * unpark and start using the engine->pinned_default_state, otherwise
> +	 * it is in limbo and an early reset may fail.
> +	 */
> +	__i915_gem_park(i915);

Why not after we have grabbed the default state?

I was worried this might access a sleeping device but apparently at 
least vma unbind takes the rpm ref. Still, just this I think makes it 
more logical to park after we have grabbed the default state, instead o 
putting the device to sleep and then immediately waking up below.

> +
>   	for_each_engine(engine, i915, id) {
>   		struct i915_vma *state;
>   
> 

Regards,

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

Re: [PATCH] drm/i915: Park the GPU on module load

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-09-14 10:40:43)
> 
> On 14/09/2018 09:34, Chris Wilson wrote:
> > Once we have flushed the first request through the system to both load a
> > context and record the default state; tell the GPU to park and idle
> > itself, putting itself immediately (hopefully at least) into a
> > powersaving state, and allowing ourselves to start from known state
> > after setting up all our bookkeeping.
> 
> Otherwise this would happen after a second or so, when the idle work 
> handler does it. Crucial thing to mention is I think the park/unpark 
> cycle which sets up the pinned default state.

Hmm, I didn't think it was worth highlight it here as one assumes that
all the bookkeeping is equally required :) I pulled out for special
mention in the comment, because I thought that was relevant to the
function where we are acquiring the default_state.
 
> Alternative to patch could be to extract 
> __intel_engine_pin_default_state to be called from here and from 
> intel_engines_unpark.
> 
> But I guess there is some value in going powersave as soon as possible 
> so okay.

Yeah. Later on (as in patches that have been on the ml for several
months with feature requests depending on them...) we acquire a
load_powersaving_context() function, which should make this a little more
interesting.

> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 89834ce19acd..be9d012d851b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >   
> >       assert_kernel_context_is_current(i915);
> >   
> > +     /*
> > +      * Immediately park the GPU so that we enable powersaving and
> > +      * treat it as idle. The next time we issue a request, we will
> > +      * unpark and start using the engine->pinned_default_state, otherwise
> > +      * it is in limbo and an early reset may fail.
> > +      */
> > +     __i915_gem_park(i915);
> 
> Why not after we have grabbed the default state?

I was just wanting to tie it to the switch to kernel-context + idle.
I was thinking this was the closed point to the normal park sequence.
 
> I was worried this might access a sleeping device but apparently at 
> least vma unbind takes the rpm ref. Still, just this I think makes it 
> more logical to park after we have grabbed the default state, instead o 
> putting the device to sleep and then immediately waking up below.

Oh, we have wakerefs aplenty on module load, we are not assuming gt.awake
here.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Park the GPU on module load

[Tvrtko Ursulin]

On 14/09/2018 10:47, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-09-14 10:40:43)
>>
>> On 14/09/2018 09:34, Chris Wilson wrote:
>>> Once we have flushed the first request through the system to both load a
>>> context and record the default state; tell the GPU to park and idle
>>> itself, putting itself immediately (hopefully at least) into a
>>> powersaving state, and allowing ourselves to start from known state
>>> after setting up all our bookkeeping.
>>
>> Otherwise this would happen after a second or so, when the idle work
>> handler does it. Crucial thing to mention is I think the park/unpark
>> cycle which sets up the pinned default state.
> 
> Hmm, I didn't think it was worth highlight it here as one assumes that
> all the bookkeeping is equally required :) I pulled out for special
> mention in the comment, because I thought that was relevant to the
> function where we are acquiring the default_state.
>   
>> Alternative to patch could be to extract
>> __intel_engine_pin_default_state to be called from here and from
>> intel_engines_unpark.
>>
>> But I guess there is some value in going powersave as soon as possible
>> so okay.
> 
> Yeah. Later on (as in patches that have been on the ml for several
> months with feature requests depending on them...) we acquire a
> load_powersaving_context() function, which should make this a little more
> interesting.
> 
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
>>>    1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 89834ce19acd..be9d012d851b 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>>>    
>>>        assert_kernel_context_is_current(i915);
>>>    
>>> +     /*
>>> +      * Immediately park the GPU so that we enable powersaving and
>>> +      * treat it as idle. The next time we issue a request, we will
>>> +      * unpark and start using the engine->pinned_default_state, otherwise
>>> +      * it is in limbo and an early reset may fail.
>>> +      */
>>> +     __i915_gem_park(i915);
>>
>> Why not after we have grabbed the default state?
> 
> I was just wanting to tie it to the switch to kernel-context + idle.
> I was thinking this was the closed point to the normal park sequence.
>   
>> I was worried this might access a sleeping device but apparently at
>> least vma unbind takes the rpm ref. Still, just this I think makes it
>> more logical to park after we have grabbed the default state, instead o
>> putting the device to sleep and then immediately waking up below.
> 
> Oh, we have wakerefs aplenty on module load, we are not assuming gt.awake
> here.

Now I am having doubts if it is a business of 
__intel_engines_record_defaults to park the engines, or the caller 
(i915_gem_init) would be a better design..

Regards,

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

Re: [PATCH] drm/i915: Park the GPU on module load

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-09-14 11:03:03)
> 
> On 14/09/2018 10:47, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-09-14 10:40:43)
> >>
> >> On 14/09/2018 09:34, Chris Wilson wrote:
> >>> Once we have flushed the first request through the system to both load a
> >>> context and record the default state; tell the GPU to park and idle
> >>> itself, putting itself immediately (hopefully at least) into a
> >>> powersaving state, and allowing ourselves to start from known state
> >>> after setting up all our bookkeeping.
> >>
> >> Otherwise this would happen after a second or so, when the idle work
> >> handler does it. Crucial thing to mention is I think the park/unpark
> >> cycle which sets up the pinned default state.
> > 
> > Hmm, I didn't think it was worth highlight it here as one assumes that
> > all the bookkeeping is equally required :) I pulled out for special
> > mention in the comment, because I thought that was relevant to the
> > function where we are acquiring the default_state.
> >   
> >> Alternative to patch could be to extract
> >> __intel_engine_pin_default_state to be called from here and from
> >> intel_engines_unpark.
> >>
> >> But I guess there is some value in going powersave as soon as possible
> >> so okay.
> > 
> > Yeah. Later on (as in patches that have been on the ml for several
> > months with feature requests depending on them...) we acquire a
> > load_powersaving_context() function, which should make this a little more
> > interesting.
> > 
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
> >>>    1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>> index 89834ce19acd..be9d012d851b 100644
> >>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>> @@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> >>>    
> >>>        assert_kernel_context_is_current(i915);
> >>>    
> >>> +     /*
> >>> +      * Immediately park the GPU so that we enable powersaving and
> >>> +      * treat it as idle. The next time we issue a request, we will
> >>> +      * unpark and start using the engine->pinned_default_state, otherwise
> >>> +      * it is in limbo and an early reset may fail.
> >>> +      */
> >>> +     __i915_gem_park(i915);
> >>
> >> Why not after we have grabbed the default state?
> > 
> > I was just wanting to tie it to the switch to kernel-context + idle.
> > I was thinking this was the closed point to the normal park sequence.
> >   
> >> I was worried this might access a sleeping device but apparently at
> >> least vma unbind takes the rpm ref. Still, just this I think makes it
> >> more logical to park after we have grabbed the default state, instead o
> >> putting the device to sleep and then immediately waking up below.
> > 
> > Oh, we have wakerefs aplenty on module load, we are not assuming gt.awake
> > here.
> 
> Now I am having doubts if it is a business of 
> __intel_engines_record_defaults to park the engines, or the caller 
> (i915_gem_init) would be a better design..

Hmm, nah, you're just reading too much specificity into record_defaults
:-p
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Park the GPU on module load

[Patchwork]

== Series Details ==

Series: drm/i915: Park the GPU on module load
URL   : https://patchwork.freedesktop.org/series/49693/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4824 -> Patchwork_10188 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_10188 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10188, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/49693/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10188:

  === IGT changes ===

    ==== Possible regressions ====

    igt@drv_selftest@live_coherency:
      fi-bdw-5557u:       PASS -> DMESG-WARN +3
      fi-skl-6260u:       PASS -> DMESG-WARN +3
      fi-snb-2600:        PASS -> DMESG-WARN +2

    igt@drv_selftest@live_contexts:
      fi-snb-2520m:       NOTRUN -> DMESG-WARN +1
      fi-pnv-d510:        PASS -> DMESG-WARN +1
      fi-glk-dsi:         PASS -> DMESG-WARN +3
      fi-gdg-551:         PASS -> DMESG-WARN +1
      fi-byt-clapper:     PASS -> DMESG-WARN +1
      fi-ivb-3770:        PASS -> DMESG-WARN +2

    igt@drv_selftest@live_execlists:
      fi-skl-6700k2:      PASS -> DMESG-WARN +3
      fi-skl-6600u:       PASS -> DMESG-WARN +2

    igt@drv_selftest@live_requests:
      fi-whl-u:           PASS -> DMESG-WARN +3
      fi-skl-gvtdvm:      PASS -> DMESG-WARN +3
      fi-ilk-650:         PASS -> DMESG-WARN +2
      fi-ivb-3520m:       PASS -> DMESG-WARN
      fi-bdw-gvtdvm:      PASS -> DMESG-WARN +3
      fi-bxt-j4205:       PASS -> DMESG-WARN +4
      fi-cfl-guc:         PASS -> DMESG-WARN +4
      fi-skl-iommu:       PASS -> DMESG-WARN +3
      fi-skl-guc:         PASS -> DMESG-WARN +3
      fi-kbl-7567u:       PASS -> DMESG-WARN +3
      fi-glk-j4005:       PASS -> DMESG-WARN +4
      fi-hsw-peppy:       PASS -> DMESG-WARN +1
      fi-icl-u:           PASS -> DMESG-WARN +3
      fi-hsw-4770r:       NOTRUN -> DMESG-WARN +2
      fi-bwr-2160:        PASS -> DMESG-WARN +1
      fi-skl-6770hq:      PASS -> DMESG-WARN +2
      fi-byt-n2820:       PASS -> DMESG-WARN +1
      fi-byt-j1900:       PASS -> DMESG-WARN +1
      fi-kbl-7560u:       PASS -> DMESG-WARN +2
      fi-hsw-4770:        PASS -> DMESG-WARN +2
      fi-blb-e6850:       NOTRUN -> DMESG-WARN +1

    igt@drv_selftest@live_workarounds:
      fi-cfl-8700k:       PASS -> DMESG-WARN +3
      fi-kbl-r:           PASS -> DMESG-WARN +3
      fi-cfl-8109u:       PASS -> DMESG-WARN +2
      fi-kbl-guc:         PASS -> DMESG-WARN +4
      fi-bsw-kefka:       PASS -> DMESG-WARN +3
      fi-cfl-s3:          PASS -> DMESG-WARN +3
      fi-bsw-n3050:       PASS -> DMESG-WARN +3
      fi-bxt-dsi:         PASS -> DMESG-WARN +1
      fi-kbl-8809g:       PASS -> DMESG-WARN +2
      fi-kbl-7500u:       PASS -> DMESG-WARN +2
      fi-kbl-x1275:       PASS -> DMESG-WARN +3

    
== Known issues ==

  Here are the changes found in Patchwork_10188 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@kms_psr@primary_page_flip:
      fi-kbl-r:           PASS -> FAIL (fdo#107336)

    
    ==== Possible fixes ====

    igt@drv_getparams_basic@basic-subslice-total:
      fi-snb-2520m:       DMESG-WARN (fdo#103713) -> PASS +9

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#107336 https://bugs.freedesktop.org/show_bug.cgi?id=107336
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718


== Participating hosts (47 -> 44) ==

  Additional (1): fi-hsw-4770r 
  Missing    (4): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4824 -> Patchwork_10188

  CI_DRM_4824: 110556c298bfe6ce81b453869e60dedbd4f0f182 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4640: 9a8da36e708f9ed15b20689dfe305e41f9a19008 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10188: a70adb0b4d9dc379bf6470e1f5312a955f35261e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

a70adb0b4d9d drm/i915: Park the GPU on module load

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10188/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Park the GPU on module load

[Chris Wilson]

Quoting Chris Wilson (2018-09-14 10:47:36)
> Quoting Tvrtko Ursulin (2018-09-14 10:40:43)
> > 
> > On 14/09/2018 09:34, Chris Wilson wrote:
> > > Once we have flushed the first request through the system to both load a
> > > context and record the default state; tell the GPU to park and idle
> > > itself, putting itself immediately (hopefully at least) into a
> > > powersaving state, and allowing ourselves to start from known state
> > > after setting up all our bookkeeping.
> > 
> > Otherwise this would happen after a second or so, when the idle work 
> > handler does it. Crucial thing to mention is I think the park/unpark 
> > cycle which sets up the pinned default state.
> 
> Hmm, I didn't think it was worth highlight it here as one assumes that
> all the bookkeeping is equally required :) I pulled out for special
> mention in the comment, because I thought that was relevant to the
> function where we are acquiring the default_state.
>  
> > Alternative to patch could be to extract 
> > __intel_engine_pin_default_state to be called from here and from 
> > intel_engines_unpark.
> > 
> > But I guess there is some value in going powersave as soon as possible 
> > so okay.
> 
> Yeah. Later on (as in patches that have been on the ml for several
> months with feature requests depending on them...) we acquire a
> load_powersaving_context() function, which should make this a little more
> interesting.
> 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_gem.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 89834ce19acd..be9d012d851b 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -5414,6 +5414,14 @@ static int __intel_engines_record_defaults(struct drm_i915_private *i915)
> > >   
> > >       assert_kernel_context_is_current(i915);
> > >   
> > > +     /*
> > > +      * Immediately park the GPU so that we enable powersaving and
> > > +      * treat it as idle. The next time we issue a request, we will
> > > +      * unpark and start using the engine->pinned_default_state, otherwise
> > > +      * it is in limbo and an early reset may fail.
> > > +      */
> > > +     __i915_gem_park(i915);
> > 
> > Why not after we have grabbed the default state?
> 
> I was just wanting to tie it to the switch to kernel-context + idle.
> I was thinking this was the closed point to the normal park sequence.
>  
> > I was worried this might access a sleeping device but apparently at 
> > least vma unbind takes the rpm ref. Still, just this I think makes it 
> > more logical to park after we have grabbed the default state, instead o 
> > putting the device to sleep and then immediately waking up below.
> 
> Oh, we have wakerefs aplenty on module load, we are not assuming gt.awake
> here.

I was very much mistaken!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx