Message ID | 1425044418-2964-1-git-send-email-michel.thierry@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, Feb 27, 2015 at 01:40:18PM +0000, Michel Thierry wrote: > From: Ben Widawsky <benjamin.widawsky@intel.com> > > The problem is we're going to switch to a new context, which could be > the default context. The plan was to use restore inhibit, which would be > fine, except if we are using dynamic page tables (which we will). If we > use dynamic page tables and we don't load new page tables, the previous > page tables might go away, and future operations will fault. > > CTXA runs. > switch to default, restore inhibit > CTXA dies and has its address space taken away. > Run CTXB, tries to save using the context A's address space - this > fails. > > The general solution is to make sure every context has it's own state, > and its own address space. For cases when we must restore inhibit, first > thing we do is load a valid address space. I thought this would be > enough, but apparently there are references within the context itself > which will refer to the old address space - therefore, we also must > reinitialize. > > It was tricky to track this down as we don't have much insight into what > happens in a context save. > > This is required for the next patch which enables dynamic page tables. This sneaks in a major change that is not mentioned at all above. Namely: > @@ -744,7 +747,7 @@ static int do_switch(struct intel_engine_cs *ring, > i915_gem_context_unreference(from); > } > > - uninitialized = !to->legacy_hw_ctx.initialized && from == NULL; > + uninitialized = !to->legacy_hw_ctx.initialized; > to->legacy_hw_ctx.initialized = true; That has nothing to do with VM spaces, but with w/a that need to be applied to the context image. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 8b288a8..1ff86f1 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -600,13 +600,6 @@ needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to) (ring != &dev_priv->ring[RCS])) && to->ppgtt; } -static bool -needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to) -{ - return IS_GEN8(ring->dev) && - (to->ppgtt || &to->ppgtt->pd_dirty_rings); -} - static int do_switch(struct intel_engine_cs *ring, struct intel_context *to) { @@ -685,16 +678,26 @@ static int do_switch(struct intel_engine_cs *ring, goto unpin_out; } - if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) + /* GEN8 does *not* require an explicit reload if the PDPs have been + * setup, and we do not wish to move them. + */ + if (!to->legacy_hw_ctx.initialized) { hw_flags |= MI_RESTORE_INHIBIT; - else if (to->ppgtt && test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings)) + /* NB: If we inhibit the restore, the context is not allowed to + * die because future work may end up depending on valid address + * space. This means we must enforce that a page table load + * occur when this occurs. */ + } else if (to->ppgtt && test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings)) hw_flags |= MI_FORCE_RESTORE; ret = mi_set_context(ring, to, hw_flags); if (ret) goto unpin_out; - if (needs_pd_load_post(ring, to)) { + if (IS_GEN8(ring->dev) && to->ppgtt && (hw_flags & MI_RESTORE_INHIBIT)) { + /* We have a valid page directory (scratch) to switch to. This + * allows the old VM to be freed. Note that if anything occurs + * between the set context, and here, we are f*cked */ ret = to->ppgtt->switch_mm(to->ppgtt, ring); /* The hardware context switch is emitted, but we haven't * actually changed the state - so it's probably safe to bail @@ -744,7 +747,7 @@ static int do_switch(struct intel_engine_cs *ring, i915_gem_context_unreference(from); } - uninitialized = !to->legacy_hw_ctx.initialized && from == NULL; + uninitialized = !to->legacy_hw_ctx.initialized; to->legacy_hw_ctx.initialized = true; done: