On 21/12/15 11:01, Chris Wilson wrote:
On Mon, Dec 21, 2015 at 11:48:35AM +0100, Daniel Vetter wrote:
On Wed, Dec 16, 2015 at 06:36:48PM +0000, Dave Gordon wrote:
We set up engines in forwards order, so some things (notably the
default context) are "owned" by engine 0 (the render engine, aka "RCS").
For symmetry and to make sure such shared objects don't disappear too
early, we should generally run teardown loops in the reverse order,
so that engine 0 is processed last.
This patch changes i915_gem_context_fini() to do that, and clarifies the
comments in i915_gem_context_{init,fini}() about the refcounting of the
default {struct intel_)context: the refcount is just ONE, no matter how
many rings exist or are active, and this refcount is nominally ascribed
to the render ring (RCS), which is set up first and now torn down last.
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 900ffd0..e143ea5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev)
for (i = 0; i < I915_NUM_RINGS; i++) {
struct intel_engine_cs *ring = &dev_priv->ring[i];
- /* NB: RCS will hold a ref for all rings */
+ /*
+ * Although each engine has a pointer to the global default
+ * context, they don't contribute to the refcount on the
+ * context. We consider that RCS (which is set up first and
+ * torn down last) holds this reference on behalf of all the
+ * other engines
+ */
Instead of piles of comments, can't we just reference-count this pointer
properly? Pointers to reference-counted objects which don't hold a full
reference are just fraught with peril, and doing that should imo only be
done when there's really clear performance data justifying the
atomic_inc/dec overhead. Init/teardown code isn't such a place.
This misdesign goes back to the original execlist merge, which expanded
the default context from RCS to all engines.
Or do I miss something and we can't do this?
We can. It's actually even easier to just do dev_priv->kernel_context.
-Chris
I'm sure it can be done, but Nick & I tried changing this section so
that each engine held a reference, and it broke something else (because
the last unreference then occured at a different point), hence my
comment about the "the delicate and fragile load/unload dance".
This is why we're trying to tidy up and clarify, bit by bit, without
breaking too many of the existing assumptions at once.
But ... I like Chris' idea of moving the pointer to the default context
from the engine structure to dev_priv; that will simplify several of the
complicated ...->engine[RCS].default_context constructs, and mean that
we're change the pointers to match the lifecycle rather than vice versa.
I'll try that ...
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
