Re: [PATCH v4 05/24] drm/i915: page table abstractions

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

Michel Thierry <michel.thierry@intel.com> writes:

> From: Ben Widawsky <benjamin.widawsky@intel.com>
>
> When we move to dynamic page allocation, keeping page_directory and pagetabs as
> separate structures will help to break actions into simpler tasks.
>
> To help transition the code nicely there is some wasted space in gen6/7.
> This will be ameliorated shortly.
>
> Following the x86 pagetable terminology:
> PDPE = struct i915_page_directory_pointer_entry.
> PDE = struct i915_page_directory_entry [page_directory].
> PTE = struct i915_page_table_entry [page_tables].
>
> v2: fixed mismatches after clean-up/rebase.
>
> v3: Clarify the names of the multiple levels of page tables (Daniel)
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v2, v3)
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 177 ++++++++++++++++++------------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  23 ++++-
>  2 files changed, 107 insertions(+), 93 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index b48b586..98b4698 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -334,7 +334,8 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>  				      I915_CACHE_LLC, use_scratch);
>  
>  	while (num_entries) {
> -		struct page *page_table = ppgtt->gen8_pt_pages[pdpe][pde];
> +		struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[pdpe];
> +		struct page *page_table = pd->page_tables[pde].page;
>  
>  		last_pte = pte + num_entries;
>  		if (last_pte > GEN8_PTES_PER_PAGE)
> @@ -378,8 +379,12 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>  		if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES))
>  			break;
>  
> -		if (pt_vaddr == NULL)
> -			pt_vaddr = kmap_atomic(ppgtt->gen8_pt_pages[pdpe][pde]);
> +		if (pt_vaddr == NULL) {
> +			struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[pdpe];
> +			struct page *page_table = pd->page_tables[pde].page;
> +
> +			pt_vaddr = kmap_atomic(page_table);
> +		}
>  
>  		pt_vaddr[pte] =
>  			gen8_pte_encode(sg_page_iter_dma_address(&sg_iter),
> @@ -403,29 +408,33 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>  	}
>  }
>  
> -static void gen8_free_page_tables(struct page **pt_pages)
> +static void gen8_free_page_tables(struct i915_page_directory_entry *pd)
>  {
>  	int i;
>  
> -	if (pt_pages == NULL)
> +	if (pd->page_tables == NULL)
>  		return;
>  
>  	for (i = 0; i < GEN8_PDES_PER_PAGE; i++)
> -		if (pt_pages[i])
> -			__free_pages(pt_pages[i], 0);
> +		if (pd->page_tables[i].page)
> +			__free_page(pd->page_tables[i].page);
>  }
>  
> -static void gen8_ppgtt_free(const struct i915_hw_ppgtt *ppgtt)
> +static void gen8_free_page_directories(struct i915_page_directory_entry *pd)
                                        ^
You only free one directory so why plural here?

> +{

If you free the page tables for the directory here..

> +	kfree(pd->page_tables);
> +	__free_page(pd->page);
> +}
> +
> +static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>  {
>  	int i;
>  
>  	for (i = 0; i < ppgtt->num_pd_pages; i++) {
> -		gen8_free_page_tables(ppgtt->gen8_pt_pages[i]);
> -		kfree(ppgtt->gen8_pt_pages[i]);
> +		gen8_free_page_tables(&ppgtt->pdp.page_directory[i]);

...this loop will be cleaner.

Also consider renaming 'num_pd_pages' to 'num_pd'. But if it does
cause a lot of rebase burden dont worry about it.

> +		gen8_free_page_directories(&ppgtt->pdp.page_directory[i]);
>  		kfree(ppgtt->gen8_pt_dma_addr[i]);
>  	}
> -
> -	__free_pages(ppgtt->pd_pages, get_order(ppgtt->num_pd_pages << PAGE_SHIFT));
>  }
>  
>  static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
> @@ -460,86 +469,75 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>  	gen8_ppgtt_free(ppgtt);
>  }
>  
> -static struct page **__gen8_alloc_page_tables(void)
> +static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
>  {
> -	struct page **pt_pages;
>  	int i;
>  
> -	pt_pages = kcalloc(GEN8_PDES_PER_PAGE, sizeof(struct page *), GFP_KERNEL);
> -	if (!pt_pages)
> -		return ERR_PTR(-ENOMEM);
> -
> -	for (i = 0; i < GEN8_PDES_PER_PAGE; i++) {
> -		pt_pages[i] = alloc_page(GFP_KERNEL);
> -		if (!pt_pages[i])
> -			goto bail;
> +	for (i = 0; i < ppgtt->num_pd_pages; i++) {
> +		ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
> +						     sizeof(dma_addr_t),
> +						     GFP_KERNEL);
> +		if (!ppgtt->gen8_pt_dma_addr[i])
> +			return -ENOMEM;
>  	}
>  
> -	return pt_pages;
> -
> -bail:
> -	gen8_free_page_tables(pt_pages);
> -	kfree(pt_pages);
> -	return ERR_PTR(-ENOMEM);
> +	return 0;
>  }
>  
> -static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt,
> -					   const int max_pdp)
> +static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
>  {
> -	struct page **pt_pages[GEN8_LEGACY_PDPES];
> -	int i, ret;
> +	int i, j;
>  
> -	for (i = 0; i < max_pdp; i++) {
> -		pt_pages[i] = __gen8_alloc_page_tables();
> -		if (IS_ERR(pt_pages[i])) {
> -			ret = PTR_ERR(pt_pages[i]);
> -			goto unwind_out;
> +	for (i = 0; i < ppgtt->num_pd_pages; i++) {
> +		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
> +			struct i915_page_table_entry *pt = &ppgtt->pdp.page_directory[i].page_tables[j];
> +
> +			pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> +			if (!pt->page)
> +				goto unwind_out;
>  		}
>  	}
>  
> -	/* NB: Avoid touching gen8_pt_pages until last to keep the allocation,
> -	 * "atomic" - for cleanup purposes.
> -	 */
> -	for (i = 0; i < max_pdp; i++)
> -		ppgtt->gen8_pt_pages[i] = pt_pages[i];
> -
>  	return 0;
>  
>  unwind_out:
> -	while (i--) {
> -		gen8_free_page_tables(pt_pages[i]);
> -		kfree(pt_pages[i]);
> -	}
> +	while (i--)
> +		gen8_free_page_tables(&ppgtt->pdp.page_directory[i]);
>  
> -	return ret;
> +	return -ENOMEM;
>  }
>  
> -static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt)
> +static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt,
> +						const int max_pdp)
>  {
>  	int i;
>  
> -	for (i = 0; i < ppgtt->num_pd_pages; i++) {
> -		ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE,
> -						     sizeof(dma_addr_t),
> -						     GFP_KERNEL);
> -		if (!ppgtt->gen8_pt_dma_addr[i])
> -			return -ENOMEM;
> -	}
> +	for (i = 0; i < max_pdp; i++) {
> +		struct i915_page_table_entry *pt;
>  
> -	return 0;
> -}
> +		pt = kcalloc(GEN8_PDES_PER_PAGE, sizeof(*pt), GFP_KERNEL);
> +		if (!pt)
> +			goto unwind_out;
>  
> -static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt,
> -						const int max_pdp)
> -{
> -	ppgtt->pd_pages = alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_SHIFT));
> -	if (!ppgtt->pd_pages)
> -		return -ENOMEM;
> +		ppgtt->pdp.page_directory[i].page = alloc_page(GFP_KERNEL);
> +		if (!ppgtt->pdp.page_directory[i].page)
> +			goto unwind_out;

If you end up having alloc error here you will leak the previously
allocated pt above.

Also consider that if you do gen8_ppgtt_allocate_page_directory() and
add null check for pd->page in gen8_free_page_directory you should be able to avoid
the unwinding below completely.

> +
> +		ppgtt->pdp.page_directory[i].page_tables = pt;
> +	}
>  
> -	ppgtt->num_pd_pages = 1 << get_order(max_pdp << PAGE_SHIFT);
> +	ppgtt->num_pd_pages = max_pdp;
>  	BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPES);
>  
>  	return 0;
> +
> +unwind_out:
> +	while (i--) {
> +		kfree(ppgtt->pdp.page_directory[i].page_tables);
> +		__free_page(ppgtt->pdp.page_directory[i].page);
> +	}
> +
> +	return -ENOMEM;
>  }
>  
>  static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
> @@ -551,18 +549,19 @@ static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt,
>  	if (ret)
>  		return ret;
>  
> -	ret = gen8_ppgtt_allocate_page_tables(ppgtt, max_pdp);
> -	if (ret) {
> -		__free_pages(ppgtt->pd_pages, get_order(max_pdp << PAGE_SHIFT));
> -		return ret;
> -	}
> +	ret = gen8_ppgtt_allocate_page_tables(ppgtt);
> +	if (ret)
> +		goto err_out;
>  
>  	ppgtt->num_pd_entries = max_pdp * GEN8_PDES_PER_PAGE;
>  
>  	ret = gen8_ppgtt_allocate_dma(ppgtt);
> -	if (ret)
> -		gen8_ppgtt_free(ppgtt);
> +	if (!ret)
> +		return ret;
>  
> +	/* TODO: Check this for all cases */

The check for zero return and then returning it with the comment is
confusing. Why not just do the same pattern as in above?

if (ret)
   goto err_out;

return 0;

-Mika

> +err_out:
> +	gen8_ppgtt_free(ppgtt);
>  	return ret;
>  }
>  
> @@ -573,7 +572,7 @@ static int gen8_ppgtt_setup_page_directories(struct i915_hw_ppgtt *ppgtt,
>  	int ret;
>  
>  	pd_addr = pci_map_page(ppgtt->base.dev->pdev,
> -			       &ppgtt->pd_pages[pd], 0,
> +			       ppgtt->pdp.page_directory[pd].page, 0,
>  			       PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
>  
>  	ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr);
> @@ -593,7 +592,7 @@ static int gen8_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt,
>  	struct page *p;
>  	int ret;
>  
> -	p = ppgtt->gen8_pt_pages[pd][pt];
> +	p = ppgtt->pdp.page_directory[pd].page_tables[pt].page;
>  	pt_addr = pci_map_page(ppgtt->base.dev->pdev,
>  			       p, 0, PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
>  	ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pt_addr);
> @@ -654,7 +653,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	 */
>  	for (i = 0; i < max_pdp; i++) {
>  		gen8_ppgtt_pde_t *pd_vaddr;
> -		pd_vaddr = kmap_atomic(&ppgtt->pd_pages[i]);
> +		pd_vaddr = kmap_atomic(ppgtt->pdp.page_directory[i].page);
>  		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>  			dma_addr_t addr = ppgtt->gen8_pt_dma_addr[i][j];
>  			pd_vaddr[j] = gen8_pde_encode(ppgtt->base.dev, addr,
> @@ -717,7 +716,7 @@ static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
>  				   expected);
>  		seq_printf(m, "\tPDE: %x\n", pd_entry);
>  
> -		pt_vaddr = kmap_atomic(ppgtt->pt_pages[pde]);
> +		pt_vaddr = kmap_atomic(ppgtt->pd.page_tables[pde].page);
>  		for (pte = 0; pte < I915_PPGTT_PT_ENTRIES; pte+=4) {
>  			unsigned long va =
>  				(pde * PAGE_SIZE * I915_PPGTT_PT_ENTRIES) +
> @@ -922,7 +921,7 @@ static void gen6_ppgtt_clear_range(struct i915_address_space *vm,
>  		if (last_pte > I915_PPGTT_PT_ENTRIES)
>  			last_pte = I915_PPGTT_PT_ENTRIES;
>  
> -		pt_vaddr = kmap_atomic(ppgtt->pt_pages[act_pt]);
> +		pt_vaddr = kmap_atomic(ppgtt->pd.page_tables[act_pt].page);
>  
>  		for (i = first_pte; i < last_pte; i++)
>  			pt_vaddr[i] = scratch_pte;
> @@ -951,7 +950,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
>  	pt_vaddr = NULL;
>  	for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
>  		if (pt_vaddr == NULL)
> -			pt_vaddr = kmap_atomic(ppgtt->pt_pages[act_pt]);
> +			pt_vaddr = kmap_atomic(ppgtt->pd.page_tables[act_pt].page);
>  
>  		pt_vaddr[act_pte] =
>  			vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
> @@ -986,8 +985,8 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>  
>  	kfree(ppgtt->pt_dma_addr);
>  	for (i = 0; i < ppgtt->num_pd_entries; i++)
> -		__free_page(ppgtt->pt_pages[i]);
> -	kfree(ppgtt->pt_pages);
> +		__free_page(ppgtt->pd.page_tables[i].page);
> +	kfree(ppgtt->pd.page_tables);
>  }
>  
>  static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> @@ -1044,22 +1043,22 @@ alloc:
>  
>  static int gen6_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
>  {
> +	struct i915_page_table_entry *pt;
>  	int i;
>  
> -	ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
> -				  GFP_KERNEL);
> -
> -	if (!ppgtt->pt_pages)
> +	pt = kcalloc(ppgtt->num_pd_entries, sizeof(*pt), GFP_KERNEL);
> +	if (!pt)
>  		return -ENOMEM;
>  
>  	for (i = 0; i < ppgtt->num_pd_entries; i++) {
> -		ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL);
> -		if (!ppgtt->pt_pages[i]) {
> +		pt[i].page = alloc_page(GFP_KERNEL);
> +		if (!pt->page) {
>  			gen6_ppgtt_free(ppgtt);
>  			return -ENOMEM;
>  		}
>  	}
>  
> +	ppgtt->pd.page_tables = pt;
>  	return 0;
>  }
>  
> @@ -1094,9 +1093,11 @@ static int gen6_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt)
>  	int i;
>  
>  	for (i = 0; i < ppgtt->num_pd_entries; i++) {
> +		struct page *page;
>  		dma_addr_t pt_addr;
>  
> -		pt_addr = pci_map_page(dev->pdev, ppgtt->pt_pages[i], 0, 4096,
> +		page = ppgtt->pd.page_tables[i].page;
> +		pt_addr = pci_map_page(dev->pdev, page, 0, 4096,
>  				       PCI_DMA_BIDIRECTIONAL);
>  
>  		if (pci_dma_mapping_error(dev->pdev, pt_addr)) {
> @@ -1140,7 +1141,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>  	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
>  	ppgtt->base.start = 0;
> -	ppgtt->base.total =  ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
> +	ppgtt->base.total = ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
>  	ppgtt->debug_dump = gen6_dump_ppgtt;
>  
>  	ppgtt->pd_offset =
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 8f76990..d9bc375 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -187,6 +187,20 @@ struct i915_vma {
>  			 u32 flags);
>  };
>  
> +struct i915_page_table_entry {
> +	struct page *page;
> +};
> +
> +struct i915_page_directory_entry {
> +	struct page *page; /* NULL for GEN6-GEN7 */
> +	struct i915_page_table_entry *page_tables;
> +};
> +
> +struct i915_page_directory_pointer_entry {
> +	/* struct page *page; */
> +	struct i915_page_directory_entry page_directory[GEN8_LEGACY_PDPES];
> +};
> +
>  struct i915_address_space {
>  	struct drm_mm mm;
>  	struct drm_device *dev;
> @@ -272,11 +286,6 @@ struct i915_hw_ppgtt {
>  	unsigned num_pd_entries;
>  	unsigned num_pd_pages; /* gen8+ */
>  	union {
> -		struct page **pt_pages;
> -		struct page **gen8_pt_pages[GEN8_LEGACY_PDPES];
> -	};
> -	struct page *pd_pages;
> -	union {
>  		uint32_t pd_offset;
>  		dma_addr_t pd_dma_addr[GEN8_LEGACY_PDPES];
>  	};
> @@ -284,6 +293,10 @@ struct i915_hw_ppgtt {
>  		dma_addr_t *pt_dma_addr;
>  		dma_addr_t *gen8_pt_dma_addr[GEN8_LEGACY_PDPES];
>  	};
> +	union {
> +		struct i915_page_directory_pointer_entry pdp;
> +		struct i915_page_directory_entry pd;
> +	};
>  
>  	struct drm_i915_file_private *file_priv;
>  
> -- 
> 2.1.1
>
> _______________________________________________
> 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