* [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context @ 2014-06-12 15:25 Ben Widawsky 2014-06-12 16:16 ` Ville Syrjälä 0 siblings, 1 reply; 5+ messages in thread From: Ben Widawsky @ 2014-06-12 15:25 UTC (permalink / raw) To: Intel GFX; +Cc: Ben Widawsky, Ben Widawsky 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(-) 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context 2014-06-12 15:25 [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context Ben Widawsky @ 2014-06-12 16:16 ` Ville Syrjälä 2014-06-12 16:55 ` Ben Widawsky 0 siblings, 1 reply; 5+ messages in thread From: Ville Syrjälä @ 2014-06-12 16:16 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky 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 -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context 2014-06-12 16:16 ` Ville Syrjälä @ 2014-06-12 16:55 ` Ben Widawsky 2014-06-12 17:28 ` Ville Syrjälä 2014-06-12 21:29 ` Daniel Vetter 0 siblings, 2 replies; 5+ messages in thread From: Ben Widawsky @ 2014-06-12 16:55 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Intel GFX, Ben Widawsky 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 -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context 2014-06-12 16:55 ` Ben Widawsky @ 2014-06-12 17:28 ` Ville Syrjälä 2014-06-12 21:29 ` Daniel Vetter 1 sibling, 0 replies; 5+ messages in thread From: Ville Syrjälä @ 2014-06-12 17:28 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky 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 -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context 2014-06-12 16:55 ` Ben Widawsky 2014-06-12 17:28 ` Ville Syrjälä @ 2014-06-12 21:29 ` Daniel Vetter 1 sibling, 0 replies; 5+ messages in thread From: Daniel Vetter @ 2014-06-12 21:29 UTC (permalink / raw) To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-06-12 21:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-06-12 15:25 [REPOST] [PATCH] drm/i915/ppgtt: Load address space after mi_set_context Ben Widawsky 2014-06-12 16:16 ` Ville Syrjälä 2014-06-12 16:55 ` Ben Widawsky 2014-06-12 17:28 ` Ville Syrjälä 2014-06-12 21:29 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.