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@xxxxxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
