Re: [PATCH 10/9] drm/i915/bdw: Kill ppgtt->num_pt_pages

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4284 bytes --]

On Fri, 2014-02-21 at 13:06 -0800, Ben Widawsky wrote:
> With the original PPGTT implementation if the number of PDPs was not a
> power of two, the number of pages for the page tables would end up being
> rounded up. The code actually had a bug here afaict, but this is a
> theoretical bug as I don't believe this can actually occur with the
> current code/HW..
> 
> With the rework of the page table allocations, there is no longer a
> distinction between number of page table pages, and number of page
> directory entries. To avoid confusion, kill the redundant (and newer)
> struct member.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Nitpick: keeping num_pt_pages instead would make the code more
understandable to me and symmetric with num_pd_pages, but that would've
been much more churn. In any case nice simplification,

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h     |  3 +--
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++----------
>  3 files changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 509e2e1..e0c42a6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1757,7 +1757,7 @@ static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev)
>  		return;
>  
>  	seq_printf(m, "Page directories: %d\n", ppgtt->num_pd_pages);
> -	seq_printf(m, "Page tables: %d\n", ppgtt->num_pt_pages);
> +	seq_printf(m, "Page tables: %d\n", ppgtt->num_pd_entries);
>  	for_each_ring(ring, dev_priv, unused) {
>  		seq_printf(m, "%s\n", ring->name);
>  		for (i = 0; i < 4; i++) {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2f29558..a9f1cae 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -698,13 +698,12 @@ struct i915_hw_ppgtt {
>  	struct kref ref;
>  	struct drm_mm_node node;
>  	unsigned num_pd_entries;
> +	unsigned num_pd_pages; /* gen8+ */
>  	union {
>  		struct page **pt_pages;
>  		struct page **gen8_pt_pages[GEN8_LEGACY_PDPS];
>  	};
>  	struct page *pd_pages;
> -	int num_pd_pages;
> -	int num_pt_pages;
>  	union {
>  		uint32_t pd_offset;
>  		dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPS];
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 6c03929..bd815d7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -433,7 +433,6 @@ static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt,
>  					   const int max_pdp)
>  {
>  	struct page **pt_pages[GEN8_LEGACY_PDPS];
> -	const int num_pt_pages = GEN8_PDES_PER_PAGE * max_pdp;
>  	int i, ret;
>  
>  	for (i = 0; i < max_pdp; i++) {
> @@ -450,8 +449,6 @@ static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt,
>  	for (i = 0; i < max_pdp; i++)
>  		ppgtt->gen8_pt_pages[i] = pt_pages[i];
>  
> -	ppgtt->num_pt_pages = 1 << get_order(num_pt_pages << PAGE_SHIFT);
> -
>  	return 0;
>  
>  unwind_out:
> @@ -618,18 +615,15 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
>  	ppgtt->base.start = 0;
> -	ppgtt->base.total = ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE;
> +	ppgtt->base.total = ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE * PAGE_SIZE;
>  
> -	ppgtt->base.clear_range(&ppgtt->base, 0,
> -				ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE * PAGE_SIZE,
> -				true);
> +	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
>  
>  	DRM_DEBUG_DRIVER("Allocated %d pages for page directories (%d wasted)\n",
>  			 ppgtt->num_pd_pages, ppgtt->num_pd_pages - max_pdp);
>  	DRM_DEBUG_DRIVER("Allocated %d pages for page tables (%lld wasted)\n",
> -			 ppgtt->num_pt_pages,
> -			 (ppgtt->num_pt_pages - min_pt_pages) +
> -			 size % (1<<30));
> +			 ppgtt->num_pd_entries,
> +			 (ppgtt->num_pd_entries - min_pt_pages) + size % (1<<30));
>  	return 0;
>  
>  bail:


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx