On Thu, Jun 12, 2014 at 08:25:52AM -0700, Ben Widawsky wrote: > On GEN8 the PDPs are saved and restored with context, which means we > must set them after the context switch has occurred. If we do not do > this, we end up saving the new PDPs for the old context. > > Example of a problem > LRI PDPs 1 > MI_SET_CONTEXT bar > LRI_PDPs 2 > MI_SET_CONTEXT foo // save PDPs 2 to bar's context > // load foos PDPs > LRI PDPs 1 > MI_SET_CONTEXT bar // save PDPs 1 to foo's context > > It's all wacky. This should allow full PPGTT on Broadwell to work.
Hmm. I had this impression too but now I'm not sure. The PDPs are listed as being in the execlist context. Do we save/restore that in ring buffer mode too? IIRC on ivb/hsw it got skipped. And if the PDPs are really part of the context we shouldn't need to load them at all I think, except when we skip the restore (unitialized or default context). Or am I missing something here? And what about ivb/hsw? Doesn't this reordering risk the context restore accessing the wrong PPGTT. That's assuming the context restore can already chase some pointers. > > Signed-off-by: Ben Widawsky <[email protected]> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++------ > 1 file changed, 18 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index 3ffe308..b2434e0 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -623,13 +623,12 @@ static int do_switch(struct intel_engine_cs *ring, > */ > from = ring->last_context; > > - if (USES_FULL_PPGTT(ring->dev)) { > - ret = ppgtt->switch_mm(ppgtt, ring, false); > - if (ret) > - goto unpin_out; > - } > - > if (ring != &dev_priv->ring[RCS]) { > + if (USES_FULL_PPGTT(ring->dev)) { > + ret = ppgtt->switch_mm(ppgtt, ring, false); > + if (ret) > + goto unpin_out; > + } > if (from) > i915_gem_context_unreference(from); > goto done; > @@ -660,6 +659,19 @@ static int do_switch(struct intel_engine_cs *ring, > if (ret) > goto unpin_out; > > + if (USES_FULL_PPGTT(ring->dev)) { > + ret = ppgtt->switch_mm(ppgtt, ring, false); > + /* The hardware context switch is emitted, but we haven't > + * actually changed the state - so it's probably safe to bail > + * here. Still, let the user know something dangerous has > + * happened. > + */ > + if (ret) { > + DRM_ERROR("Failed to change address space on context > switch\n"); > + goto unpin_out; > + } > + } > + > for (i = 0; i < MAX_L3_SLICES; i++) { > if (!(to->remap_slice & (1<<i))) > continue; > -- > 2.0.0 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
