On 14/12/15 22:17, Chris Wilson wrote:
On Mon, Dec 14, 2015 at 05:13:43PM +0000, Chris Wilson wrote:
On Mon, Dec 14, 2015 at 04:30:04PM +0000, Nick Hoath wrote:
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 900ffd0..7df3c7a 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -431,17 +431,22 @@ void i915_gem_context_fini(struct drm_device *dev)
i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state);
}
- for (i = 0; i < I915_NUM_RINGS; i++) {
+ for (i = I915_NUM_RINGS; --i >= 0;) {
struct intel_engine_cs *ring = &dev_priv->ring[i];
if (ring->last_context)
i915_gem_context_unreference(ring->last_context);
- ring->default_context = NULL;
ring->last_context = NULL;
}
i915_gem_context_unreference(dctx);
+
+ for (i = I915_NUM_RINGS; --i >= 0;) {
+ struct intel_engine_cs *ring = &dev_priv->ring[i];
+
+ ring->default_context = NULL;
+ }
}
Why?
Ok, as you say intel_lrc needs to iterate over all rings to look at the
default context in context_free. Feels odd, and breaks the onion of
context_init / context_fini. The pecularity is in lrc and I would push
the oddness back there.
That engine->default_context is pinned and mapped is only because of the
HWS and for no other reason does it exist (seqno writes are to address
not relative the per-context HWS index). You could simply allocate a
page for each engine and use that as the global HWS irrespective of
contexts.
-Chris
The default (render) context has to be pinned anyway (nowadays), because
the GuC can access it asynchronously if it decides to reset an engine.
Also we use it when we need to give the GuC a valid LRCA for things that
aren't requests, e.g. suspend/resume et al.
Having a default context that's created and owned by the driver is an
old (pre-execlist) decision, and it allows us to switch away from all
user-visible contexts for idling. Leaving it pinned at all times means
we don't risk being unable to find space when we're trying to idle in
order to reclaim space!
Seqno writes are relative to the global HWSP independent of which
context is doing them. User code could be using the PPHWSP for its own
purposes.
I agree that it might be nicer to have a separate object for the global
HWSP, as we do in legacy mode. That would cost an extra page per engine,
but meh.
However a simpler solution for now might be to put a flag inside the
default context, rather than the LRC code deducing that that it's the
default one because the engine structure points back at it. That removes
the dependency of the LRC-specific code on knowing what the ring
(engine!) management code is doing :)
I'll discuss with Nick tomorrow, and I think we'll find a neater patch :)
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
