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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
