| Message ID | 1402586752-22484-1-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <ben@bwidawsk.net> > --- > 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 > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 12, 2014 at 07:16:48PM +0300, Ville Syrjälä wrote: > 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? Yes, I believe you are correct. However, I don't see any harm with the existing solution either, and I was hesitant to change stuff around, since at the time I just needed to make it work for the 64b series. But I think what you're saying is just adding an "&& !to->is_initialized" to the USES_FULL_PPGTT check, right? I can try that. > > 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. This is not meant for IVB/HSW. Empirically we've found it doesn't have the same behavior, as you said above. So that's definite an issue which requires a patch rev. > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > 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 > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Thu, Jun 12, 2014 at 09:55:50AM -0700, Ben Widawsky wrote: > On Thu, Jun 12, 2014 at 07:16:48PM +0300, Ville Syrjälä wrote: > > 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? > > Yes, I believe you are correct. However, I don't see any harm with the > existing solution either, Yeah just a bit wasteful to load twice. Should still work fine. > and I was hesitant to change stuff around, > since at the time I just needed to make it work for the 64b series. But > I think what you're saying is just adding an "&& !to->is_initialized" to > the USES_FULL_PPGTT check, right? I can try that. or '&& (hw_flags & MI_RESTORE_INHIBIT)' to catch both default and !initialized contexts. > > > > > 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. > > This is not meant for IVB/HSW. Empirically we've found it doesn't > have the same behavior, as you said above. So that's definite an issue > which requires a patch rev. OK > > > > > > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > > --- > > > 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 > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > -- > Ben Widawsky, Intel Open Source Technology Center
On Thu, Jun 12, 2014 at 09:55:50AM -0700, Ben Widawsky wrote: > This is not meant for IVB/HSW. Empirically we've found it doesn't > have the same behavior, as you said above. So that's definite an issue > which requires a patch rev. Sounds like we might need a per-platform do_ctx_swithc function to encode these details? Maybe not given that execlist is the rage and that has a completely different submission path. -Daniel
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;
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. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_gem_context.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)