Re: [PATCH 04/17] drm/i915: Unify aliasing ppgtt handling

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> With the dynamic pagetable alloc code aliasing ppgtt special-cases
> where again mixed in all over the place with the low-level init code.
>
> Extract the va preallocation and clearing again into the common code
> where aliasing ppgtt gets set up.
>
> Note that with this we don't set the size of the aliasing ppgtt to the
> size of the parent ggtt address space. Which isn't required at all
> since except for the ppgtt setup/cleanup code no one ever looks at
> this.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 134 +++++++-----------------------------
>  1 file changed, 24 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index abb11f139d25..75787f1d2751 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -387,50 +387,6 @@ fail_bitmap:
>  	return ERR_PTR(ret);
>  }
>  
> -/**
> - * alloc_pt_range() - Allocate a multiple page tables
> - * @pd:		The page directory which will have at least @count entries
> - *		available to point to the allocated page tables.
> - * @pde:	First page directory entry for which we are allocating.
> - * @count:	Number of pages to allocate.
> - * @dev:	DRM device.
> - *
> - * Allocates multiple page table pages and sets the appropriate entries in the
> - * page table structure within the page directory. Function cleans up after
> - * itself on any failures.
> - *
> - * Return: 0 if allocation succeeded.
> - */
> -static int alloc_pt_range(struct i915_page_directory *pd, uint16_t pde, size_t count,
> -			  struct drm_device *dev)
> -{
> -	int i, ret;
> -
> -	/* 512 is the max page tables per page_directory on any platform. */
> -	if (WARN_ON(pde + count > I915_PDES))
> -		return -EINVAL;
> -
> -	for (i = pde; i < pde + count; i++) {
> -		struct i915_page_table *pt = alloc_pt_single(dev);
> -
> -		if (IS_ERR(pt)) {
> -			ret = PTR_ERR(pt);
> -			goto err_out;
> -		}
> -		WARN(pd->page_table[i],
> -		     "Leaking page directory entry %d (%p)\n",
> -		     i, pd->page_table[i]);
> -		pd->page_table[i] = pt;
> -	}
> -
> -	return 0;
> -
> -err_out:
> -	while (i-- > pde)
> -		unmap_and_free_pt(pd->page_table[i], dev);
> -	return ret;
> -}
> -
>  static void unmap_and_free_pd(struct i915_page_directory *pd,
>  			      struct drm_device *dev)
>  {
> @@ -971,7 +927,7 @@ err_out:
>   * space.
>   *
>   */
> -static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
> +static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  {
>  	ppgtt->scratch_pt = alloc_pt_single(ppgtt->base.dev);
>  	if (IS_ERR(ppgtt->scratch_pt))
> @@ -985,8 +941,9 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd);
>  
>  	ppgtt->base.start = 0;
> -	ppgtt->base.total = size;
> +	ppgtt->base.total = 1ULL << 32;

This warns on 32bit builds due to overflow.

Seems that I have already tripped on this when I reviewed 4gb
default ppgtt size patch.

We should do a follow-up patch with:

--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -229,7 +229,7 @@ struct i915_address_space {
        struct drm_device *dev;
        struct list_head global_link;
        unsigned long start;            /* Start offset always 0 for
        dri2 */
-       size_t total;           /* size addr space maps (ex. 2GB for ggtt) */
+       u64 size;               /* size addr space maps (ex. 2GB for ggtt) */

-Mika


>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
> +	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>  	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> @@ -997,46 +954,6 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	return 0;
>  }
>  
> -static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> -{
> -	struct drm_device *dev = ppgtt->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint64_t start = 0, size = dev_priv->gtt.base.total;
> -	int ret;
> -
> -	ret = gen8_ppgtt_init_common(ppgtt, dev_priv->gtt.base.total);
> -	if (ret)
> -		return ret;
> -
> -	/* Aliasing PPGTT has to always work and be mapped because of the way we
> -	 * use RESTORE_INHIBIT in the context switch. This will be fixed
> -	 * eventually. */
> -	ret = gen8_alloc_va_range(&ppgtt->base, start, size);
> -	if (ret) {
> -		unmap_and_free_pd(ppgtt->scratch_pd, ppgtt->base.dev);
> -		unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev);
> -		return ret;
> -	}
> -
> -	ppgtt->base.allocate_va_range = NULL;
> -	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> -
> -	return 0;
> -}
> -
> -static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> -{
> -	int ret;
> -
> -	ret = gen8_ppgtt_init_common(ppgtt, (1ULL << 32));
> -	if (ret)
> -		return ret;
> -
> -	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
> -
> -	return 0;
> -}
> -
>  static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
>  {
>  	struct i915_address_space *vm = &ppgtt->base;
> @@ -1533,7 +1450,7 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt,
>  		ppgtt->pd.page_table[pde] = ppgtt->scratch_pt;
>  }
>  
> -static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
> +static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_device *dev = ppgtt->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1556,18 +1473,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	if (ret)
>  		return ret;
>  
> -	if (aliasing) {
> -		/* preallocate all pts */
> -		ret = alloc_pt_range(&ppgtt->pd, 0, I915_PDES,
> -				ppgtt->base.dev);
> -
> -		if (ret) {
> -			gen6_ppgtt_cleanup(&ppgtt->base);
> -			return ret;
> -		}
> -	}
> -
> -	ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range;
> +	ppgtt->base.allocate_va_range = gen6_alloc_va_range;
>  	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>  	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> @@ -1583,10 +1489,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	ppgtt->pd_addr = (gen6_pte_t __iomem *)dev_priv->gtt.gsm +
>  		ppgtt->pd.pd_offset / sizeof(gen6_pte_t);
>  
> -	if (aliasing)
> -		ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> -	else
> -		gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
> +	gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
>  
>  	gen6_write_page_range(dev_priv, &ppgtt->pd, 0, ppgtt->base.total);
>  
> @@ -1600,8 +1503,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	return 0;
>  }
>  
> -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
> -		bool aliasing)
> +static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -1609,9 +1511,7 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
>  
>  	if (INTEL_INFO(dev)->gen < 8)
> -		return gen6_ppgtt_init(ppgtt, aliasing);
> -	else if (aliasing)
> -		return gen8_aliasing_ppgtt_init(ppgtt);
> +		return gen6_ppgtt_init(ppgtt);
>  	else
>  		return gen8_ppgtt_init(ppgtt);
>  }
> @@ -1620,7 +1520,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret = 0;
>  
> -	ret = __hw_ppgtt_init(dev, ppgtt, false);
> +	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (ret == 0) {
>  		kref_init(&ppgtt->ref);
>  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> @@ -2255,13 +2155,27 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  		if (!ppgtt)
>  			return -ENOMEM;
>  
> -		ret = __hw_ppgtt_init(dev, ppgtt, true);
> +		ret = __hw_ppgtt_init(dev, ppgtt);
> +		if (ret) {
> +			ppgtt->base.cleanup(&ppgtt->base);
> +			kfree(ppgtt);
> +			return ret;
> +		}
> +
> +		if (ppgtt->base.allocate_va_range)
> +			ret = ppgtt->base.allocate_va_range(&ppgtt->base, 0,
> +							    ppgtt->base.total);
>  		if (ret) {
>  			ppgtt->base.cleanup(&ppgtt->base);
>  			kfree(ppgtt);
>  			return ret;
>  		}
>  
> +		ppgtt->base.clear_range(&ppgtt->base,
> +					ppgtt->base.start,
> +					ppgtt->base.total,
> +					true);
> +
>  		dev_priv->mm.aliasing_ppgtt = ppgtt;
>  	}
>  
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx