Re: [PATCH 1/2] drm/i915: Rework ppgtt init to no require an aliasing ppgtt

From: Daniel Vetter <daniel@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 1/2] drm/i915: Rework ppgtt init to no require an aliasing ppgtt
Date: Wed, 6 Aug 2014 21:55:58 +0200	[thread overview]
Message-ID: <20140806195558.GV8727@phenom.ffwll.local> (raw)
In-Reply-To: <1407349194-3601-1-git-send-email-daniel.vetter@ffwll.ch>

On Wed, Aug 06, 2014 at 08:19:53PM +0200, Daniel Vetter wrote:
> Currently we abuse the aliasing ppgtt to set up the ppgtt support in
> general. Which is a bit backwards since with full ppgtt we don't ever
> need the aliasing ppgtt.
> 
> So untangling this and separate the ppgtt init from the aliasing
> ppgtt. While at it drag it out of the context enabling (which just
> does a switch to the default context).
> 
> Note that we still have the differentiation between synchronous and
> asynchronous ppgtt setup, but that will soon vanish. So also correctly
> wire up the return value handling to be prepared for when ->switch_mm
> drops the synchronous parameter and could start to fail.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside: This will conflict quite a bit with Alistars gpu reset rework to
ditch the synchronous parameter from ppgtt->switch_mm. If Alistars patch
is ready first I'll rebase and resend, otherwise I'll fix it up while
applying. It shouldn't be too hairy really.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c         |  8 ++++
>  drivers/gpu/drm/i915/i915_gem_context.c |  7 ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c     | 84 +++++++++++++--------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h     |  1 +
>  4 files changed, 42 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a79deb189146..caf92bac2152 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4742,6 +4742,14 @@ i915_gem_init_hw(struct drm_device *dev)
>  	if (ret && ret != -EIO) {
>  		DRM_ERROR("Context enable failed %d\n", ret);
>  		i915_gem_cleanup_ringbuffer(dev);
> +
> +		return ret;
> +	}
> +
> +	ret = i915_ppgtt_init_hw(dev);
> +	if (ret && ret != -EIO) {
> +		DRM_ERROR("PPGTT enable failed %d\n", ret);
> +		i915_gem_cleanup_ringbuffer(dev);
>  	}
>  
>  	return ret;
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 3b8367aa8404..4af5f05b24e8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -424,13 +424,6 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv)
>  	struct intel_engine_cs *ring;
>  	int ret, i;
>  
> -	/* This is the only place the aliasing PPGTT gets enabled, which means
> -	 * it has to happen before we bail on reset */
> -	if (dev_priv->mm.aliasing_ppgtt) {
> -		struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
> -		ppgtt->enable(ppgtt);
> -	}
> -
>  	/* FIXME: We should make this work, even in reset */
>  	if (i915_reset_in_progress(&dev_priv->gpu_error))
>  		return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4fa7807ed4d5..bf70ab44b968 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -78,7 +78,6 @@ static void ppgtt_bind_vma(struct i915_vma *vma,
>  			   enum i915_cache_level cache_level,
>  			   u32 flags);
>  static void ppgtt_unbind_vma(struct i915_vma *vma);
> -static int gen8_ppgtt_enable(struct i915_hw_ppgtt *ppgtt);
>  
>  static inline gen8_gtt_pte_t gen8_pte_encode(dma_addr_t addr,
>  					     enum i915_cache_level level,
> @@ -615,7 +614,6 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  		kunmap_atomic(pd_vaddr);
>  	}
>  
> -	ppgtt->enable = gen8_ppgtt_enable;
>  	ppgtt->switch_mm = gen8_mm_switch;
>  	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
> @@ -836,39 +834,20 @@ static int gen6_mm_switch(struct i915_hw_ppgtt *ppgtt,
>  	return 0;
>  }
>  
> -static int gen8_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
> +static void gen8_ppgtt_enable(struct drm_device *dev)
>  {
> -	struct drm_device *dev = ppgtt->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *ring;
> -	int j, ret;
> +	int j;
>  
>  	for_each_ring(ring, dev_priv, j) {
>  		I915_WRITE(RING_MODE_GEN7(ring),
>  			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> -
> -		/* We promise to do a switch later with FULL PPGTT. If this is
> -		 * aliasing, this is the one and only switch we'll do */
> -		if (USES_FULL_PPGTT(dev))
> -			continue;
> -
> -		ret = ppgtt->switch_mm(ppgtt, ring, true);
> -		if (ret)
> -			goto err_out;
>  	}
> -
> -	return 0;
> -
> -err_out:
> -	for_each_ring(ring, dev_priv, j)
> -		I915_WRITE(RING_MODE_GEN7(ring),
> -			   _MASKED_BIT_DISABLE(GFX_PPGTT_ENABLE));
> -	return ret;
>  }
>  
> -static int gen7_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
> +static void gen7_ppgtt_enable(struct drm_device *dev)
>  {
> -	struct drm_device *dev = ppgtt->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_engine_cs *ring;
>  	uint32_t ecochk, ecobits;
> @@ -887,31 +866,16 @@ static int gen7_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
>  	I915_WRITE(GAM_ECOCHK, ecochk);
>  
>  	for_each_ring(ring, dev_priv, i) {
> -		int ret;
>  		/* GFX_MODE is per-ring on gen7+ */
>  		I915_WRITE(RING_MODE_GEN7(ring),
>  			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> -
> -		/* We promise to do a switch later with FULL PPGTT. If this is
> -		 * aliasing, this is the one and only switch we'll do */
> -		if (USES_FULL_PPGTT(dev))
> -			continue;
> -
> -		ret = ppgtt->switch_mm(ppgtt, ring, true);
> -		if (ret)
> -			return ret;
>  	}
> -
> -	return 0;
>  }
>  
> -static int gen6_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
> +static void gen6_ppgtt_enable(struct drm_device *dev)
>  {
> -	struct drm_device *dev = ppgtt->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_engine_cs *ring;
>  	uint32_t ecochk, gab_ctl, ecobits;
> -	int i;
>  
>  	ecobits = I915_READ(GAC_ECO_BITS);
>  	I915_WRITE(GAC_ECO_BITS, ecobits | ECOBITS_SNB_BIT |
> @@ -924,14 +888,6 @@ static int gen6_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
>  	I915_WRITE(GAM_ECOCHK, ecochk | ECOCHK_SNB_BIT | ECOCHK_PPGTT_CACHE64B);
>  
>  	I915_WRITE(GFX_MODE, _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
> -
> -	for_each_ring(ring, dev_priv, i) {
> -		int ret = ppgtt->switch_mm(ppgtt, ring, true);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return 0;
>  }
>  
>  /* PPGTT support for Sandybdrige/Gen6 and later */
> @@ -1151,13 +1107,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  
>  	ppgtt->base.pte_encode = dev_priv->gtt.base.pte_encode;
>  	if (IS_GEN6(dev)) {
> -		ppgtt->enable = gen6_ppgtt_enable;
>  		ppgtt->switch_mm = gen6_mm_switch;
>  	} else if (IS_HASWELL(dev)) {
> -		ppgtt->enable = gen7_ppgtt_enable;
>  		ppgtt->switch_mm = hsw_mm_switch;
>  	} else if (IS_GEN7(dev)) {
> -		ppgtt->enable = gen7_ppgtt_enable;
>  		ppgtt->switch_mm = gen7_mm_switch;
>  	} else
>  		BUG();
> @@ -1222,6 +1175,35 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	return ret;
>  }
>  
> +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;
> +
> +	if (!USES_PPGTT(dev))
> +		return 0;
> +
> +	if (IS_GEN6(dev))
> +		gen6_ppgtt_enable(dev);
> +	else if (IS_GEN7(dev))
> +		gen7_ppgtt_enable(dev);
> +	else if (INTEL_INFO(dev)->gen >= 8)
> +		gen8_ppgtt_enable(dev);
> +	else
> +		WARN_ON(1);
> +
> +	if (ppgtt) {
> +		for_each_ring(ring, dev_priv, i) {
> +			ret = ppgtt->switch_mm(ppgtt, ring, true);
> +			if (ret != 0)
> +				return ret;
> +		}
> +	}
> +
> +	return ret;
> +}
>  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 bea3541d5525..45aa15fa4af2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -277,6 +277,7 @@ int i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start,
>  bool intel_enable_ppgtt(struct drm_device *dev, bool full);
>  
>  int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt);
> +int i915_ppgtt_init_hw(struct drm_device *dev);
>  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.9.3
> 

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