Re: [PATCH 1/4] drm/i915: teardown default context in reverse, update comments

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/4] drm/i915: teardown default context in reverse, update comments
Date: Mon, 21 Dec 2015 11:38:06 +0000	[thread overview]
Message-ID: <5677E49E.1030902@intel.com> (raw)
In-Reply-To: <20151221110155.GH28538@nuc-i3427.alporthouse.com>

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@intel.com>
>>> ---
>>>   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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx