[PATCH] drm/i915: Enable full PPGTT on gen7

[PATCH] drm/i915: Enable full PPGTT on gen7

[Michel Thierry]

Use full PPGTT as the default option in gen7.
Note that aliasing PPGTT is the default option for gen8 (see HAS_PPGTT).

This may well come back to bite me later.

Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index fc46647..30ab56a 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -59,7 +59,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 		return 0;
 	}
 
-	return HAS_ALIASING_PPGTT(dev) ? 1 : 0;
+	return HAS_PPGTT(dev) ? 2 : HAS_ALIASING_PPGTT(dev) ? 1 : 0;
 }
 
 
-- 
2.0.3

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Daniel Vetter]

On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> Use full PPGTT as the default option in gen7.
> Note that aliasing PPGTT is the default option for gen8 (see HAS_PPGTT).
> 
> This may well come back to bite me later.
> 
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>

Queued for -next, thanks for the patch. And I've added a note that gen8 is
blocked on execlists and other troubles for now.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index fc46647..30ab56a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -59,7 +59,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>  		return 0;
>  	}
>  
> -	return HAS_ALIASING_PPGTT(dev) ? 1 : 0;
> +	return HAS_PPGTT(dev) ? 2 : HAS_ALIASING_PPGTT(dev) ? 1 : 0;
>  }
>  
>  
> -- 
> 2.0.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Chris Wilson]

On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> Use full PPGTT as the default option in gen7.
> Note that aliasing PPGTT is the default option for gen8 (see HAS_PPGTT).
> 
> This may well come back to bite me later.

Indeed. So something I spotted was that bspec mentions that the per-ring
PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in the
logical context and so the registers are restored along with the
context. If this is correct what happens when we switch logical contexts
on RCS whilst we have active work on BCS etc? Does this mean that we
have to serialise context switches across rings, or is my reading of the
bspec false?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Ville Syrjälä]

On Tue, Sep 09, 2014 at 12:57:11PM +0100, Chris Wilson wrote:
> On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> > Use full PPGTT as the default option in gen7.
> > Note that aliasing PPGTT is the default option for gen8 (see HAS_PPGTT).
> > 
> > This may well come back to bite me later.
> 
> Indeed. So something I spotted was that bspec mentions that the per-ring
> PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in the
> logical context and so the registers are restored along with the
> context. If this is correct what happens when we switch logical contexts
> on RCS whilst we have active work on BCS etc? Does this mean that we
> have to serialise context switches across rings, or is my reading of the
> bspec false?

How does rcs PP_DIR_* affect bcs? Also IIRC that stuff is part of
the execlist context which isn't saved/restored unless execlists
are actually enabled. IIRC when I tried it, snb did reserve the
space for that stuff in the context image but didn't save/restore
it, but ivb+ didn't even reserve the space.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Thierry, Michel]

[-- Attachment #1.1: Type: text/plain, Size: 1339 bytes --]



On Tue, Sep 9, 2014 at 1:34 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote: 
> On Tue, Sep 09, 2014 at 12:57:11PM +0100, Chris Wilson wrote:
> > On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> > > Use full PPGTT as the default option in gen7.
> > > Note that aliasing PPGTT is the default option for gen8 (see
HAS_PPGTT).
> > >
> > > This may well come back to bite me later.
> >
> > Indeed. So something I spotted was that bspec mentions that the per-ring
> > PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in
> the
> > logical context and so the registers are restored along with the
> > context. If this is correct what happens when we switch logical contexts
> > on RCS whilst we have active work on BCS etc? Does this mean that we
> > have to serialise context switches across rings, or is my reading of the
> > bspec false?
> 
> How does rcs PP_DIR_* affect bcs? Also IIRC that stuff is part of
> the execlist context which isn't saved/restored unless execlists
> are actually enabled. IIRC when I tried it, snb did reserve the
> space for that stuff in the context image but didn't save/restore
> it, but ivb+ didn't even reserve the space.
> 
Yes, my understanding is that these registers are per engine, and bcs
couldn't be affected by rcs.  

-Michel

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 6656 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Chris Wilson]

On Tue, Sep 09, 2014 at 12:41:34PM +0000, Thierry, Michel wrote:
> 
> 
> On Tue, Sep 9, 2014 at 1:34 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote: 
> > On Tue, Sep 09, 2014 at 12:57:11PM +0100, Chris Wilson wrote:
> > > On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> > > > Use full PPGTT as the default option in gen7.
> > > > Note that aliasing PPGTT is the default option for gen8 (see
> HAS_PPGTT).
> > > >
> > > > This may well come back to bite me later.
> > >
> > > Indeed. So something I spotted was that bspec mentions that the per-ring
> > > PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in
> > the
> > > logical context and so the registers are restored along with the
> > > context. If this is correct what happens when we switch logical contexts
> > > on RCS whilst we have active work on BCS etc? Does this mean that we
> > > have to serialise context switches across rings, or is my reading of the
> > > bspec false?
> > 
> > How does rcs PP_DIR_* affect bcs? Also IIRC that stuff is part of
> > the execlist context which isn't saved/restored unless execlists
> > are actually enabled. IIRC when I tried it, snb did reserve the
> > space for that stuff in the context image but didn't save/restore
> > it, but ivb+ didn't even reserve the space.
> > 
> Yes, my understanding is that these registers are per engine, and bcs
> couldn't be affected by rcs.  

They are per-engine, but are they stored in the logical context (which
is what bspec says afaict) and so reloaded with the wrong values when
RCS executes MI_SET_CONTEXT? That is the question.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Ville Syrjälä]

On Tue, Sep 09, 2014 at 02:07:24PM +0100, Chris Wilson wrote:
> On Tue, Sep 09, 2014 at 12:41:34PM +0000, Thierry, Michel wrote:
> > 
> > 
> > On Tue, Sep 9, 2014 at 1:34 PM, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote: 
> > > On Tue, Sep 09, 2014 at 12:57:11PM +0100, Chris Wilson wrote:
> > > > On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> > > > > Use full PPGTT as the default option in gen7.
> > > > > Note that aliasing PPGTT is the default option for gen8 (see
> > HAS_PPGTT).
> > > > >
> > > > > This may well come back to bite me later.
> > > >
> > > > Indeed. So something I spotted was that bspec mentions that the per-ring
> > > > PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in
> > > the
> > > > logical context and so the registers are restored along with the
> > > > context. If this is correct what happens when we switch logical contexts
> > > > on RCS whilst we have active work on BCS etc? Does this mean that we
> > > > have to serialise context switches across rings, or is my reading of the
> > > > bspec false?
> > > 
> > > How does rcs PP_DIR_* affect bcs? Also IIRC that stuff is part of
> > > the execlist context which isn't saved/restored unless execlists
> > > are actually enabled. IIRC when I tried it, snb did reserve the
> > > space for that stuff in the context image but didn't save/restore
> > > it, but ivb+ didn't even reserve the space.
> > > 
> > Yes, my understanding is that these registers are per engine, and bcs
> > couldn't be affected by rcs.  
> 
> They are per-engine, but are they stored in the logical context (which
> is what bspec says afaict) and so reloaded with the wrong values when
> RCS executes MI_SET_CONTEXT? That is the question.

I don't see any !RCS PP_DIR_* registers listed in the execlist context.
Also BSpec says that RCS PP_DIR_* registers are stored in the power context
rather than the execlist context in ring buffer mode. BSpec is a bit thin
on what happens to these registers on other engines eg. during rc6. I guess
they must have their own power contexts.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Chris Wilson]

On Tue, Sep 09, 2014 at 04:30:06PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 09, 2014 at 02:07:24PM +0100, Chris Wilson wrote:
> > On Tue, Sep 09, 2014 at 12:41:34PM +0000, Thierry, Michel wrote:
> > > 
> > > 
> > > On Tue, Sep 9, 2014 at 1:34 PM, Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote: 
> > > > On Tue, Sep 09, 2014 at 12:57:11PM +0100, Chris Wilson wrote:
> > > > > On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> > > > > > Use full PPGTT as the default option in gen7.
> > > > > > Note that aliasing PPGTT is the default option for gen8 (see
> > > HAS_PPGTT).
> > > > > >
> > > > > > This may well come back to bite me later.
> > > > >
> > > > > Indeed. So something I spotted was that bspec mentions that the per-ring
> > > > > PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in
> > > > the
> > > > > logical context and so the registers are restored along with the
> > > > > context. If this is correct what happens when we switch logical contexts
> > > > > on RCS whilst we have active work on BCS etc? Does this mean that we
> > > > > have to serialise context switches across rings, or is my reading of the
> > > > > bspec false?
> > > > 
> > > > How does rcs PP_DIR_* affect bcs? Also IIRC that stuff is part of
> > > > the execlist context which isn't saved/restored unless execlists
> > > > are actually enabled. IIRC when I tried it, snb did reserve the
> > > > space for that stuff in the context image but didn't save/restore
> > > > it, but ivb+ didn't even reserve the space.
> > > > 
> > > Yes, my understanding is that these registers are per engine, and bcs
> > > couldn't be affected by rcs.  
> > 
> > They are per-engine, but are they stored in the logical context (which
> > is what bspec says afaict) and so reloaded with the wrong values when
> > RCS executes MI_SET_CONTEXT? That is the question.
> 
> I don't see any !RCS PP_DIR_* registers listed in the execlist context.
> Also BSpec says that RCS PP_DIR_* registers are stored in the power context
> rather than the execlist context in ring buffer mode. BSpec is a bit thin
> on what happens to these registers on other engines eg. during rc6. I guess
> they must have their own power contexts.

Cool. I was read an old copy of the bspec offline, so hopefully I can
just blame it on early cut'n'pasting.

I wrote igt/gem_ppgtt to try and race MI_SET_CONTEXT against a BCS
workload and that seems fairly convincing that the PP_DIR registers are
unaffected by the context restore.

Oh, wait. It just uses the default-context on each fd, i.e. doesn't
actually perform the context restores. Will check back later...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Daniel Vetter]

On Tue, Sep 09, 2014 at 02:55:15PM +0100, Chris Wilson wrote:
> On Tue, Sep 09, 2014 at 04:30:06PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 09, 2014 at 02:07:24PM +0100, Chris Wilson wrote:
> > > On Tue, Sep 09, 2014 at 12:41:34PM +0000, Thierry, Michel wrote:
> > > > 
> > > > 
> > > > On Tue, Sep 9, 2014 at 1:34 PM, Ville Syrjälä
> > > > <ville.syrjala@linux.intel.com> wrote: 
> > > > > On Tue, Sep 09, 2014 at 12:57:11PM +0100, Chris Wilson wrote:
> > > > > > On Fri, Sep 05, 2014 at 02:13:16PM +0100, Michel Thierry wrote:
> > > > > > > Use full PPGTT as the default option in gen7.
> > > > > > > Note that aliasing PPGTT is the default option for gen8 (see
> > > > HAS_PPGTT).
> > > > > > >
> > > > > > > This may well come back to bite me later.
> > > > > >
> > > > > > Indeed. So something I spotted was that bspec mentions that the per-ring
> > > > > > PDE registers (RING_PP_DIR_DCLV and RING_PP_DIR_BASE) are stored in
> > > > > the
> > > > > > logical context and so the registers are restored along with the
> > > > > > context. If this is correct what happens when we switch logical contexts
> > > > > > on RCS whilst we have active work on BCS etc? Does this mean that we
> > > > > > have to serialise context switches across rings, or is my reading of the
> > > > > > bspec false?
> > > > > 
> > > > > How does rcs PP_DIR_* affect bcs? Also IIRC that stuff is part of
> > > > > the execlist context which isn't saved/restored unless execlists
> > > > > are actually enabled. IIRC when I tried it, snb did reserve the
> > > > > space for that stuff in the context image but didn't save/restore
> > > > > it, but ivb+ didn't even reserve the space.
> > > > > 
> > > > Yes, my understanding is that these registers are per engine, and bcs
> > > > couldn't be affected by rcs.  
> > > 
> > > They are per-engine, but are they stored in the logical context (which
> > > is what bspec says afaict) and so reloaded with the wrong values when
> > > RCS executes MI_SET_CONTEXT? That is the question.
> > 
> > I don't see any !RCS PP_DIR_* registers listed in the execlist context.
> > Also BSpec says that RCS PP_DIR_* registers are stored in the power context
> > rather than the execlist context in ring buffer mode. BSpec is a bit thin
> > on what happens to these registers on other engines eg. during rc6. I guess
> > they must have their own power contexts.
> 
> Cool. I was read an old copy of the bspec offline, so hopefully I can
> just blame it on early cut'n'pasting.
> 
> I wrote igt/gem_ppgtt to try and race MI_SET_CONTEXT against a BCS
> workload and that seems fairly convincing that the PP_DIR registers are
> unaffected by the context restore.
> 
> Oh, wait. It just uses the default-context on each fd, i.e. doesn't
> actually perform the context restores. Will check back later...

We should throw a ->switch_mm on top of each context switch already to
make sure the pp switch actually happened. So worst we do load the pp on
rcs twicw and we could optimize that away if we just restore the context.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Chris Wilson]

On Tue, Sep 09, 2014 at 05:12:23PM +0200, Daniel Vetter wrote:
> On Tue, Sep 09, 2014 at 02:55:15PM +0100, Chris Wilson wrote:
> > Oh, wait. It just uses the default-context on each fd, i.e. doesn't
> > actually perform the context restores. Will check back later...
> 
> We should throw a ->switch_mm on top of each context switch already to
> make sure the pp switch actually happened. So worst we do load the pp on
> rcs twicw and we could optimize that away if we just restore the context.

I just meant the test is not actually testing my theory that
MI_SET_CONTEXT could interfere with the PP_DIR on other engines.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Enable full PPGTT on gen7

[Chris Wilson]

On Tue, Sep 09, 2014 at 02:55:15PM +0100, Chris Wilson wrote:
> Oh, wait. It just uses the default-context on each fd, i.e. doesn't
> actually perform the context restores. Will check back later...

A bit more experimentation (igt/gem_ppgtt/bcs-vs-rcs-ctxN) and I am happy
that this was a non-issue.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre