[Intel-gfx] [PATCH] drm/i915/guc: Fix a false alert of memory leak when free LRC

[Chris Wilson]

On Fri, Oct 23, 2015 at 10:40:11PM +0100, Dave Gordon wrote:
> >@@ -732,6 +727,11 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
> >  	if (intel_ring_stopped(ring))
> >  		return;
> >
> >+	if (request->ctx != ring->default_context)
> >+		intel_lr_context_pin(request);
> >+
> >+	i915_gem_request_reference(request);
> >+
> >  	if (dev_priv->guc.execbuf_client)
> >  		i915_guc_submit(dev_priv->guc.execbuf_client, request);
> >  	else
> 
> Not entirely happy about this. If an extra ref is needed for both
> execlist and GuC mode (of which I'm not convinced) what about legacy
> ringbuffer mode? I thought execlists needed the extra ref only
> because it added an extra queue to hold work not yet submitted to
> hardware.
> That queue isn't needed in ringbuffer OR GuC mode. OTOH if an extra
> ref is needed in ALL modes it should be in common core code, not
> replicated into all submission paths :)

Indeed that extra ref here is bogus.
 
> You've got intel_logical_ring_advance_and_submit() taking the extra
> reference, but each of the execlist and GuC paths adding the request
> to the "execlist_queue" :( And then two separate but very similar
> functions reversing these two operations :(
> 
> Surely the VMA should not be freed while there are any outstanding
> (not-yet-retired) requests referring to it? Perhaps
> i915_gem_context_clean() is called in the wrong place (too early?).
> Shouldn't we have waited for all requests to complete and be retired
> BEFORE freeing the context they're using?

The issue is that the context_clean() is being called whilst the
bookkeeping is in an inconsistent state. My preferred solution here is
to revert that bogus patch as it doesn't fix the issue it was purported
to.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre