Re: [PATCH 4/5] drm/i915: Aliased PPGTT size abstraction

[Jani Nikula <jani.nikula@linux.intel.com>]

On Sat, 26 Jan 2013, Ben Widawsky <ben@bwidawsk.net> wrote:
> Aside from being a nice prep for some work I'm doing in PPGTT, this also
> leaves us with a more accurate size in our gtt_total_entries member.
> Since we can't actually use the PPGTT reserved part of the GGTT.
>
> This will help some of the existing assertions find issues which would
> overwrite the PPGTT PDEs.
>
> Another benefit is for platforms which don't use the entire 2GB GTT,
> this will save GTT space. For example if a platform has 1 1GB GTT, this
> patch should save 1MB of GTT space (512MB has an even greater savings).
> I don't have a platform to easily test this however.
>
> If/when we ever move to real PPGTT, I think allocating the PDEs as
> objects will make the most sense.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 35 ++++++++++++++++++++++++++---------
>  3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 384f193..4545357 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1605,6 +1605,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data)
>  
>  		seq_printf(m, "aliasing PPGTT:\n");
>  		seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset);
> +		seq_printf(m, "mapped size: %lldM\n",
> +			   ppgtt->mapped_size>>20);
>  	}
>  	seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK));
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1af8e73..60ba4ab 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -388,10 +388,11 @@ struct i915_gtt {
>  	struct page *scratch_page;
>  };
>  
> -#define I915_PPGTT_PD_ENTRIES 512
> +#define GEN6_MAX_PD_ENTRIES 512
>  #define I915_PPGTT_PT_ENTRIES 1024
>  struct i915_hw_ppgtt {
>  	struct drm_device *dev;
> +	uint64_t mapped_size;
>  	unsigned num_pd_entries;
>  	struct page **pt_pages;
>  	uint32_t pd_offset;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index f79f427..d9af815 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt,
>  	}
>  }
>  
> -static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
> +static int gen6_init_ppgtt(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  {
>  	struct drm_device *dev = ppgtt->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -116,15 +116,22 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
>  	int i;
>  	int ret = -ENOMEM;
>  
> +	if (dev_priv->mm.aliasing_ppgtt)
> +		return -EBUSY;
> +
> +	/* Each PDE maps 4MB (1k PAGE_SIZE pages)*/
> +	ppgtt->mapped_size = round_up(size, 1<<22);
> +	ppgtt->num_pd_entries = ppgtt->mapped_size >> 22;
> +	BUG_ON(ppgtt->num_pd_entries > GEN6_MAX_PD_ENTRIES);
> +
>  	/* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024
>  	 * entries. For aliasing ppgtt support we just steal them at the end for
>  	 * now. */
> -	first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES;
> +	first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries -
> +		ppgtt->num_pd_entries;
>  
> -	ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES;
>  	ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries,
>  				  GFP_KERNEL);
> -
>  	if (!ppgtt->pt_pages)
>  		return -ENOMEM;
>  
> @@ -156,7 +163,10 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt)
>  	i915_ppgtt_clear_range(ppgtt, 0,
>  			       ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES);
>  
> -	ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t);
> +	ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gtt_pte_t);
> +
> +	DRM_INFO("Allocated %d PDEs for PPGTT\n", ppgtt->num_pd_entries);
> +	DRM_DEBUG_DRIVER("First PDE: %d\n", first_pd_entry_in_global_pt);
>  
>  	return 0;
>  
> @@ -192,7 +202,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
>  	ppgtt->dev = dev;
>  	ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma;
>  
> -	ret = gen6_init_aliasing_ppgtt(ppgtt);
> +	ret = gen6_init_ppgtt(ppgtt, dev_priv->gtt.total);
>  	if (ret)
>  		kfree(ppgtt);
>  	else
> @@ -625,20 +635,27 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
>  	mappable_size = dev_priv->gtt.mappable_end;
>  
>  	if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
> +		/* Each PDE represents 1k PAGE_SIZE objects */
> +		const int reserve_pd_space = gtt_size >> 10;

Maybe (likely) I'm being dense, but care to explain this shift a bit?
Should it match ppgtt->num_pd_entries * PAGE_SIZE after
i915_gem_init_aliasing_ppgtt()?

BR,
Jani.

>  		int ret;
> +
> +		BUG_ON(!reserve_pd_space);
> +
>  		/* PPGTT pdes are stolen from global gtt ptes, so shrink the
>  		 * aperture accordingly when using aliasing ppgtt. */
> -		gtt_size -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> +		gtt_size -= reserve_pd_space;
>  
>  		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
>  
>  		ret = i915_gem_init_aliasing_ppgtt(dev);
> -		if (!ret)
> +		if (!ret) {
> +			dev_priv->mm.gtt->gtt_total_entries = gtt_size >> PAGE_SHIFT;
>  			return;
> +		}
>  
>  		DRM_ERROR("Aliased PPGTT setup failed %d\n", ret);
>  		drm_mm_takedown(&dev_priv->mm.gtt_space);
> -		gtt_size += I915_PPGTT_PD_ENTRIES*PAGE_SIZE;
> +		gtt_size += reserve_pd_space;
>  	}
>  	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
>  }
> -- 
> 1.8.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx