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

[Michel Thierry <michel.thierry@intel.com>]

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

On 2/18/2015 11:27 AM, Mika Kuoppala wrote:
> 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.

num_pd_pages will go away in patch #19, so I rather not change that.
All other comments addressed in v4.

Thanks,

-Michel
>> +		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


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5510 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