Re: [PATCH 17/53] drm/i915: Split i915_ppgtt_init_hw() in half - generic and per ring

Daniel, Thomas
2015-02-24T13:55:33Z

> -----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