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?
Thanks, Daniel
> ring->default_context = ctx;
> }
>
> @@ -431,14 +437,21 @@ 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)
> + if (ring->last_context) {
> i915_gem_context_unreference(ring->last_context);
> + ring->last_context = NULL;
> + }
>
> + /*
> + * These default_context pointers don't contribute to the
> + * refcount on the context. We consider that RCS holds its
> + * reference on behalf of all the other engines, so there's
> + * just a single unreference() call below.
> + */
> ring->default_context = NULL;
> - ring->last_context = NULL;
> }
>
> i915_gem_context_unreference(dctx);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
