| Message ID | 1424366285-29232-18-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of > John.C.Harrison@Intel.com > Sent: Thursday, February 19, 2015 5:17 PM > To: Intel-GFX@Lists.FreeDesktop.Org > Subject: [Intel-gfx] [PATCH 17/53] drm/i915: Split i915_ppgtt_init_hw() in half - > generic and per ring > > From: John Harrison <John.C.Harrison@Intel.com> > > The i915_gem_init_hw() function calls a bunch of smaller initialisation > functions. Multiple of which have generic sections and per ring sections. This > means multiple passes are done over the rings. Each pass writes data to the ring > which floats around in that ring's OLR until some random point in the future > when an add_request() is done by some random other piece of code. > > This patch breaks i915_ppgtt_init_hw() in two with the per ring initialisation > now being done in i915_ppgtt_init_ring(). The ring looping is now done at the > top level in i915_gem_init_hw(). > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++++++++++++------ > drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++------------- > drivers/gpu/drm/i915/i915_gem_gtt.h | 1 + > 3 files changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c > index 51f719c..9bc60d7 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4844,19 +4844,32 @@ i915_gem_init_hw(struct drm_device *dev) > */ > init_unused_rings(dev); > > + ret = i915_ppgtt_init_hw(dev); > + if (ret) { > + DRM_ERROR("PPGTT enable HW failed %d\n", ret); > + return ret; > + } > + > + /* Need to do basic initialisation of all rings first: */ > for_each_ring(ring, dev_priv, i) { > ret = ring->init_hw(ring); > if (ret) > return ret; > } > > - for (i = 0; i < NUM_L3_SLICES(dev); i++) > - i915_gem_l3_remap(&dev_priv->ring[RCS], i); > + /* Now it is safe to go back round and do everything else: */ > + for_each_ring(ring, dev_priv, i) { > + if (ring->id == RCS) { > + for (i = 0; i < NUM_L3_SLICES(dev); i++) > + i915_gem_l3_remap(ring, i); > + } > > - ret = i915_ppgtt_init_hw(dev); > - if (ret && ret != -EIO) { > - DRM_ERROR("PPGTT enable failed %d\n", ret); > - i915_gem_cleanup_ringbuffer(dev); > + ret = i915_ppgtt_init_ring(ring); > + if (ret && ret != -EIO) { > + DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, > ret); > + i915_gem_cleanup_ringbuffer(dev); > + return ret; > + } > } > > ret = i915_gem_context_enable(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index e54b2a0..428d2f6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1206,11 +1206,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct > i915_hw_ppgtt *ppgtt) > > int i915_ppgtt_init_hw(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_engine_cs *ring; > - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > - int i, ret = 0; > - > /* In the case of execlists, PPGTT is enabled by the context descriptor > * and the PDPs are contained within the context itself. We don't > * need to do anything here. */ > @@ -1229,16 +1224,20 @@ int i915_ppgtt_init_hw(struct drm_device *dev) > else > MISSING_CASE(INTEL_INFO(dev)->gen); > > - if (ppgtt) { > - for_each_ring(ring, dev_priv, i) { > - ret = ppgtt->switch_mm(ppgtt, ring); > - if (ret != 0) > - return ret; > - } > - } > + return 0; > +} > > - return ret; > +int i915_ppgtt_init_ring(struct intel_engine_cs *ring) > +{ > + struct drm_i915_private *dev_priv = ring->dev->dev_private; > + struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > + > + if (!ppgtt) > + return 0; > + > + return ppgtt->switch_mm(ppgtt, ring); > } This breaks alias PPGTT for execlists. I915_ppgtt_init_hw() is a noop for execlists mode, but the new i915_ppgtt_init_ring() will try to do a switch_mm() which should not be done for execlists. Thomas. > + > struct i915_hw_ppgtt * > i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv) > { > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 8f76990..5a6cef9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -300,6 +300,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev); > > int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt); > int i915_ppgtt_init_hw(struct drm_device *dev); > +int i915_ppgtt_init_ring(struct intel_engine_cs *ring); > void i915_ppgtt_release(struct kref *kref); > struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev, > struct drm_i915_file_private *fpriv); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 19/02/2015 17:17, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > The i915_gem_init_hw() function calls a bunch of smaller initialisation > functions. Multiple of which have generic sections and per ring sections. This > means multiple passes are done over the rings. Each pass writes data to the ring > which floats around in that ring's OLR until some random point in the future > when an add_request() is done by some random other piece of code. > > This patch breaks i915_ppgtt_init_hw() in two with the per ring initialisation > now being done in i915_ppgtt_init_ring(). The ring looping is now done at the > top level in i915_gem_init_hw(). > > For: VIZ-5115 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 25 +++++++++++++++++++------ > drivers/gpu/drm/i915/i915_gem_gtt.c | 25 ++++++++++++------------- > drivers/gpu/drm/i915/i915_gem_gtt.h | 1 + > 3 files changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 51f719c..9bc60d7 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -4844,19 +4844,32 @@ i915_gem_init_hw(struct drm_device *dev) > */ > init_unused_rings(dev); > > + ret = i915_ppgtt_init_hw(dev); > + if (ret) { > + DRM_ERROR("PPGTT enable HW failed %d\n", ret); > + return ret; > + } > + > + /* Need to do basic initialisation of all rings first: */ Nitpick: No need for trailing colon in comment > for_each_ring(ring, dev_priv, i) { > ret = ring->init_hw(ring); > if (ret) > return ret; > } > > - for (i = 0; i < NUM_L3_SLICES(dev); i++) > - i915_gem_l3_remap(&dev_priv->ring[RCS], i); > + /* Now it is safe to go back round and do everything else: */ Nitpick: No need for trailing colon in comment Reviewed-by: Tomas Elf <tomas.elf@intel.com> Thanks, Tomas > + for_each_ring(ring, dev_priv, i) { > + if (ring->id == RCS) { > + for (i = 0; i < NUM_L3_SLICES(dev); i++) > + i915_gem_l3_remap(ring, i); > + } > > - ret = i915_ppgtt_init_hw(dev); > - if (ret && ret != -EIO) { > - DRM_ERROR("PPGTT enable failed %d\n", ret); > - i915_gem_cleanup_ringbuffer(dev); > + ret = i915_ppgtt_init_ring(ring); > + if (ret && ret != -EIO) { > + DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret); > + i915_gem_cleanup_ringbuffer(dev); > + return ret; > + } > } > > ret = i915_gem_context_enable(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index e54b2a0..428d2f6 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -1206,11 +1206,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > > int i915_ppgtt_init_hw(struct drm_device *dev) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_engine_cs *ring; > - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > - int i, ret = 0; > - > /* In the case of execlists, PPGTT is enabled by the context descriptor > * and the PDPs are contained within the context itself. We don't > * need to do anything here. */ > @@ -1229,16 +1224,20 @@ int i915_ppgtt_init_hw(struct drm_device *dev) > else > MISSING_CASE(INTEL_INFO(dev)->gen); > > - if (ppgtt) { > - for_each_ring(ring, dev_priv, i) { > - ret = ppgtt->switch_mm(ppgtt, ring); > - if (ret != 0) > - return ret; > - } > - } > + return 0; > +} > > - return ret; > +int i915_ppgtt_init_ring(struct intel_engine_cs *ring) > +{ > + struct drm_i915_private *dev_priv = ring->dev->dev_private; > + struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; > + > + if (!ppgtt) > + return 0; > + > + return ppgtt->switch_mm(ppgtt, ring); > } > + > struct i915_hw_ppgtt * > i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv) > { > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 8f76990..5a6cef9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -300,6 +300,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev); > > int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt); > int i915_ppgtt_init_hw(struct drm_device *dev); > +int i915_ppgtt_init_ring(struct intel_engine_cs *ring); > void i915_ppgtt_release(struct kref *kref); > struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev, > struct drm_i915_file_private *fpriv); >
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 51f719c..9bc60d7 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4844,19 +4844,32 @@ i915_gem_init_hw(struct drm_device *dev) */ init_unused_rings(dev); + ret = i915_ppgtt_init_hw(dev); + if (ret) { + DRM_ERROR("PPGTT enable HW failed %d\n", ret); + return ret; + } + + /* Need to do basic initialisation of all rings first: */ for_each_ring(ring, dev_priv, i) { ret = ring->init_hw(ring); if (ret) return ret; } - for (i = 0; i < NUM_L3_SLICES(dev); i++) - i915_gem_l3_remap(&dev_priv->ring[RCS], i); + /* Now it is safe to go back round and do everything else: */ + for_each_ring(ring, dev_priv, i) { + if (ring->id == RCS) { + for (i = 0; i < NUM_L3_SLICES(dev); i++) + i915_gem_l3_remap(ring, i); + } - ret = i915_ppgtt_init_hw(dev); - if (ret && ret != -EIO) { - DRM_ERROR("PPGTT enable failed %d\n", ret); - i915_gem_cleanup_ringbuffer(dev); + ret = i915_ppgtt_init_ring(ring); + if (ret && ret != -EIO) { + DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret); + i915_gem_cleanup_ringbuffer(dev); + return ret; + } } ret = i915_gem_context_enable(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index e54b2a0..428d2f6 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1206,11 +1206,6 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) int i915_ppgtt_init_hw(struct drm_device *dev) { - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_engine_cs *ring; - struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; - int i, ret = 0; - /* In the case of execlists, PPGTT is enabled by the context descriptor * and the PDPs are contained within the context itself. We don't * need to do anything here. */ @@ -1229,16 +1224,20 @@ int i915_ppgtt_init_hw(struct drm_device *dev) else MISSING_CASE(INTEL_INFO(dev)->gen); - if (ppgtt) { - for_each_ring(ring, dev_priv, i) { - ret = ppgtt->switch_mm(ppgtt, ring); - if (ret != 0) - return ret; - } - } + return 0; +} - return ret; +int i915_ppgtt_init_ring(struct intel_engine_cs *ring) +{ + struct drm_i915_private *dev_priv = ring->dev->dev_private; + struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt; + + if (!ppgtt) + return 0; + + return ppgtt->switch_mm(ppgtt, ring); } + struct i915_hw_ppgtt * i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv) { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 8f76990..5a6cef9 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -300,6 +300,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev); int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt); int i915_ppgtt_init_hw(struct drm_device *dev); +int i915_ppgtt_init_ring(struct intel_engine_cs *ring); void i915_ppgtt_release(struct kref *kref); struct i915_hw_ppgtt *i915_ppgtt_create(struct drm_device *dev, struct drm_i915_file_private *fpriv);