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

[Dave Gordon]

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 at 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.