[Intel-gfx] [PATCH v2 12/24] drm/i915: Track page table reload need
From: Ben Widawsky This patch was formerly known as, "Force pd restore when PDEs change, gen6-7." I had to change the name because it is needed for GEN8 too. The real issue this is trying to solve is when a new object is mapped into the current address space. The GPU does not snoop the new mapping so we must do the gen specific action to reload the page tables. GEN8 and GEN7 do differ in the way they load page tables for the RCS. GEN8 does so with the context restore, while GEN7 requires the proper load commands in the command streamer. Non-render is similar for both. Caveat for GEN7 The docs say you cannot change the PDEs of a currently running context. We never map new PDEs of a running context, and expect them to be present - so I think this is okay. (We can unmap, but this should also be okay since we only unmap unreferenced objects that the GPU shouldn't be tryingto va->pa xlate.) The MI_SET_CONTEXT command does have a flag to signal that even if the context is the same, force a reload. It's unclear exactly what this does, but I have a hunch it's the right thing to do. The logic assumes that we always emit a context switch after mapping new PDEs, and before we submit a batch. This is the case today, and has been the case since the inception of hardware contexts. A note in the comment let's the user know. Signed-off-by: Ben Widawsky squash! drm/i915: Force pd restore when PDEs change, gen6-7 It's not just for gen8. If the current context has mappings change, we need a context reload to switch v2: Rebased after ppgtt clean up patches. Split the warning for aliasing and true ppgtt options. And do not break aliasing ppgtt, where to->ppgtt is always null. v3: Invalidate PPGTT TLBs inside alloc_va_range and teardown_va_range. Signed-off-by: Michel Thierry (v2+) --- drivers/gpu/drm/i915/i915_gem_context.c| 27 ++- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++ drivers/gpu/drm/i915/i915_gem_gtt.c| 12 drivers/gpu/drm/i915/i915_gem_gtt.h| 2 ++ 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 7b20bd4..fa9d4a1 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -567,8 +567,18 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring, struct intel_context *from, struct intel_context *to) { - if (from == to && !to->remap_slice) - return true; + struct drm_i915_private *dev_priv = ring->dev->dev_private; + + if (to->remap_slice) + return false; + + if (to->ppgtt) { + if (from == to && !test_bit(ring->id, &to->ppgtt->base.pd_reload_mask)) + return true; + } else { + if (from == to && !test_bit(ring->id, &dev_priv->mm.aliasing_ppgtt->base.pd_reload_mask)) + return true; + } return false; } @@ -585,9 +595,8 @@ needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to) static bool needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to) { - return (!to->legacy_hw_ctx.initialized || - i915_gem_context_is_default(to)) && - to->ppgtt && IS_GEN8(ring->dev); + return IS_GEN8(ring->dev) && + (to->ppgtt || &to->ppgtt->base.pd_reload_mask); } static int do_switch(struct intel_engine_cs *ring, @@ -632,6 +641,12 @@ static int do_switch(struct intel_engine_cs *ring, ret = to->ppgtt->switch_mm(to->ppgtt, ring); if (ret) goto unpin_out; + + /* Doing a PD load always reloads the page dirs */ + if (to->ppgtt) + clear_bit(ring->id, &to->ppgtt->base.pd_reload_mask); + else + clear_bit(ring->id, &dev_priv->mm.aliasing_ppgtt->base.pd_reload_mask); } if (ring != &dev_priv->ring[RCS]) { @@ -670,6 +685,8 @@ static int do_switch(struct intel_engine_cs *ring, */ if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) hw_flags |= MI_RESTORE_INHIBIT; + else if (to->ppgtt && test_and_clear_bit(ring->id, &to->ppgtt->base.pd_reload_mask)) + hw_flags |= MI_FORCE_RESTORE; ret = mi_set_context(ring, to, hw_flags); if (ret) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 8330660..09d864f 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1199,6 +1199,13 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, if (ret) goto error; + if (ctx->ppgtt) +
Re: [Intel-gfx] [PATCH v2 12/24] drm/i915: Track page table reload need
On Tue, Dec 23, 2014 at 05:16:15PM +, Michel Thierry wrote: > From: Ben Widawsky > > This patch was formerly known as, "Force pd restore when PDEs change, > gen6-7." I had to change the name because it is needed for GEN8 too. > > The real issue this is trying to solve is when a new object is mapped > into the current address space. The GPU does not snoop the new mapping > so we must do the gen specific action to reload the page tables. > > GEN8 and GEN7 do differ in the way they load page tables for the RCS. > GEN8 does so with the context restore, while GEN7 requires the proper > load commands in the command streamer. Non-render is similar for both. > > Caveat for GEN7 > The docs say you cannot change the PDEs of a currently running context. > We never map new PDEs of a running context, and expect them to be > present - so I think this is okay. (We can unmap, but this should also > be okay since we only unmap unreferenced objects that the GPU shouldn't > be tryingto va->pa xlate.) The MI_SET_CONTEXT command does have a flag > to signal that even if the context is the same, force a reload. It's > unclear exactly what this does, but I have a hunch it's the right thing > to do. > > The logic assumes that we always emit a context switch after mapping new > PDEs, and before we submit a batch. This is the case today, and has been > the case since the inception of hardware contexts. A note in the comment > let's the user know. > > Signed-off-by: Ben Widawsky > > squash! drm/i915: Force pd restore when PDEs change, gen6-7 > > It's not just for gen8. If the current context has mappings change, we > need a context reload to switch > > v2: Rebased after ppgtt clean up patches. Split the warning for aliasing > and true ppgtt options. And do not break aliasing ppgtt, where to->ppgtt > is always null. > > v3: Invalidate PPGTT TLBs inside alloc_va_range and teardown_va_range. > Signed-off-by: Michel Thierry (v2+) > --- > drivers/gpu/drm/i915/i915_gem_context.c| 27 ++- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++ > drivers/gpu/drm/i915/i915_gem_gtt.c| 12 > drivers/gpu/drm/i915/i915_gem_gtt.h| 2 ++ > 4 files changed, 47 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 7b20bd4..fa9d4a1 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -567,8 +567,18 @@ static inline bool should_skip_switch(struct > intel_engine_cs *ring, > struct intel_context *from, > struct intel_context *to) > { > - if (from == to && !to->remap_slice) > - return true; > + struct drm_i915_private *dev_priv = ring->dev->dev_private; > + > + if (to->remap_slice) > + return false; > + > + if (to->ppgtt) { > + if (from == to && !test_bit(ring->id, > &to->ppgtt->base.pd_reload_mask)) > + return true; > + } else { > + if (from == to && !test_bit(ring->id, > &dev_priv->mm.aliasing_ppgtt->base.pd_reload_mask)) > + return true; > + } > > return false; > } > @@ -585,9 +595,8 @@ needs_pd_load_pre(struct intel_engine_cs *ring, struct > intel_context *to) > static bool > needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to) > { > - return (!to->legacy_hw_ctx.initialized || > - i915_gem_context_is_default(to)) && > - to->ppgtt && IS_GEN8(ring->dev); > + return IS_GEN8(ring->dev) && > + (to->ppgtt || &to->ppgtt->base.pd_reload_mask); > } > > static int do_switch(struct intel_engine_cs *ring, > @@ -632,6 +641,12 @@ static int do_switch(struct intel_engine_cs *ring, > ret = to->ppgtt->switch_mm(to->ppgtt, ring); > if (ret) > goto unpin_out; > + > + /* Doing a PD load always reloads the page dirs */ > + if (to->ppgtt) > + clear_bit(ring->id, &to->ppgtt->base.pd_reload_mask); > + else > + clear_bit(ring->id, > &dev_priv->mm.aliasing_ppgtt->base.pd_reload_mask); > } > > if (ring != &dev_priv->ring[RCS]) { > @@ -670,6 +685,8 @@ static int do_switch(struct intel_engine_cs *ring, >*/ > if (!to->legacy_hw_ctx.initialized || i915_gem_context_is_default(to)) > hw_flags |= MI_RESTORE_INHIBIT; > + else if (to->ppgtt && test_and_clear_bit(ring->id, > &to->ppgtt->base.pd_reload_mask)) > + hw_flags |= MI_FORCE_RESTORE; > > ret = mi_set_context(ring, to, hw_flags); > if (ret) > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > index 8330660..09d864f 100644 > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer