* [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.