[Intel-gfx] [PATCH] drm/i915: Fix context/engine cleanup order

[Dave Gordon]

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.