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

[Daniel, Thomas]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
> John.C.Harrison at Intel.com
> Sent: Thursday, February 19, 2015 5:17 PM
> To: Intel-GFX at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx