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

Chris Wilson chris at chris-wilson.co.uk
Sat Oct 24 01:52:52 PDT 2015

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

Chris Wilson, Intel Open Source Technology Centre

More information about the Intel-gfx mailing list