| Message ID | 1409922796-453-1-git-send-email-michel.thierry@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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
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.
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
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
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
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
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; }
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(-)