Re: [PATCH v5 03/32] drm/i915: Create page table allocators

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

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

On 2/24/2015 1:56 PM, Mika Kuoppala wrote:
> Michel Thierry <michel.thierry@intel.com> writes:
>
>> From: Ben Widawsky <benjamin.widawsky@intel.com>
>>
>> As we move toward dynamic page table allocation, it becomes much easier
>> to manage our data structures if break do things less coarsely by
>> breaking up all of our actions into individual tasks.  This makes the
>> code easier to write, read, and verify.
>>
>> Aside from the dissection of the allocation functions, the patch
>> statically allocates the page table structures without a page directory.
>> This remains the same for all platforms,
>>
>> The patch itself should not have much functional difference. The primary
>> noticeable difference is the fact that page tables are no longer
>> allocated, but rather statically declared as part of the page directory.
>> This has non-zero overhead, but things gain non-trivial complexity as a
>> result.
>>
> I don't quite understand the last sentence here. We gain overhead and
> complexity.
>
> s/non-trivial/trivial?
I'll rephrase this.
>> This patch exists for a few reasons:
>> 1. Splitting out the functions allows easily combining GEN6 and GEN8
>> code. Page tables have no difference based on GEN8. As we'll see in a
>> future patch when we add the DMA mappings to the allocations, it
>> requires only one small change to make work, and error handling should
>> just fall into place.
>>
>> 2. Unless we always want to allocate all page tables under a given PDE,
>> we'll have to eventually break this up into an array of pointers (or
>> pointer to pointer).
>>
>> 3. Having the discrete functions is easier to review, and understand.
>> All allocations and frees now take place in just a couple of locations.
>> Reviewing, and catching leaks should be easy.
>>
>> 4. Less important: the GFP flags are confined to one location, which
>> makes playing around with such things trivial.
>>
>> v2: Updated commit message to explain why this patch exists
>>
>> v3: For lrc, s/pdp.page_directory[i].daddr/pdp.page_directory[i]->daddr/
>>
>> v4: Renamed free_pt/pd_single functions to unmap_and_free_pt/pd (Daniel)
>>
>> v5: Added additional safety checks in gen8 clear/free/unmap.
>>
>> v6: Use WARN_ON and return -EINVAL in alloc_pt_range (Mika).
>>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v3+)
>> ---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 252 ++++++++++++++++++++++++------------
>>   drivers/gpu/drm/i915/i915_gem_gtt.h |   4 +-
>>   drivers/gpu/drm/i915/intel_lrc.c    |  16 +--
>>   3 files changed, 178 insertions(+), 94 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index eb0714c..65c77e5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -279,6 +279,98 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
>>   	return pte;
>>   }
>>   
>> +static void unmap_and_free_pt(struct i915_page_table_entry *pt)
>> +{
>> +	if (WARN_ON(!pt->page))
>> +		return;
>> +	__free_page(pt->page);
>> +	kfree(pt);
>> +}
>> +
>> +static struct i915_page_table_entry *alloc_pt_single(void)
>> +{
>> +	struct i915_page_table_entry *pt;
>> +
>> +	pt = kzalloc(sizeof(*pt), GFP_KERNEL);
>> +	if (!pt)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> +	if (!pt->page) {
>> +		kfree(pt);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	return pt;
>> +}
>> +
>> +/**
>> + * 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.
>> + *
>> + * 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_entry *pd, uint16_t pde, size_t count)
>> +{
>> +	int i, ret;
>> +
>> +	/* 512 is the max page tables per page_directory on any platform. */
>> +	if (WARN_ON(pde + count > GEN6_PPGTT_PD_ENTRIES))
>> +		return -EINVAL;
>> +
>> +	for (i = pde; i < pde + count; i++) {
>> +		struct i915_page_table_entry *pt = alloc_pt_single();
>> +
>> +		if (IS_ERR(pt)) {
>> +			ret = PTR_ERR(pt);
>> +			goto err_out;
>> +		}
>> +		WARN(pd->page_tables[i],
>> +		     "Leaking page directory entry %d (%pa)\n",
>> +		     i, pd->page_tables[i]);
>> +		pd->page_tables[i] = pt;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_out:
>> +	while (i--)
>> +		unmap_and_free_pt(pd->page_tables[i]);
> This is suspicious as it is non symmetrical of how we allocate. If the
> plan is to free everything below pde, that should be mentioned in the
> comment above.
>
> On this patch we call this with pde == 0, but I suspect later in the
> series there will be other usecases for this.
Actually is the other way around, later on this will be used only by 
aliasing ppgtt;
others will iterate through a macro and call alloc_pt_single ("page 
table allocation rework" patch).

Anyway, it makes it clearer to have something like:
  err_out:
-    while (i--)
+    while (i-- > pde)
          unmap_and_free_pt(pd->page_tables[i]);
      return ret;

>> +	return ret;
>> +}
>> +
>> +static void unmap_and_free_pd(struct i915_page_directory_entry *pd)
>> +{
>> +	if (pd->page) {
>> +		__free_page(pd->page);
>> +		kfree(pd);
>> +	}
>> +}
>> +
>> +static struct i915_page_directory_entry *alloc_pd_single(void)
>> +{
>> +	struct i915_page_directory_entry *pd;
>> +
>> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
>> +	if (!pd)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pd->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> +	if (!pd->page) {
>> +		kfree(pd);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	return pd;
>> +}
>> +
>>   /* Broadwell Page Directory Pointer Descriptors */
>>   static int gen8_write_pdp(struct intel_engine_cs *ring, unsigned entry,
>>   			   uint64_t val)
>> @@ -311,7 +403,7 @@ static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
>>   	int used_pd = ppgtt->num_pd_entries / GEN8_PDES_PER_PAGE;
>>   
>>   	for (i = used_pd - 1; i >= 0; i--) {
>> -		dma_addr_t addr = ppgtt->pdp.page_directory[i].daddr;
>> +		dma_addr_t addr = ppgtt->pdp.page_directory[i]->daddr;
>>   		ret = gen8_write_pdp(ring, i, addr);
>>   		if (ret)
>>   			return ret;
>> @@ -338,8 +430,24 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>>   				      I915_CACHE_LLC, use_scratch);
>>   
>>   	while (num_entries) {
>> -		struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[pdpe];
>> -		struct page *page_table = pd->page_tables[pde].page;
>> +		struct i915_page_directory_entry *pd;
>> +		struct i915_page_table_entry *pt;
>> +		struct page *page_table;
>> +
>> +		if (WARN_ON(!ppgtt->pdp.page_directory[pdpe]))
>> +			continue;
>> +
>> +		pd = ppgtt->pdp.page_directory[pdpe];
>> +
>> +		if (WARN_ON(!pd->page_tables[pde]))
>> +			continue;
>> +
>> +		pt = pd->page_tables[pde];
>> +
>> +		if (WARN_ON(!pt->page))
>> +			continue;
>> +
>> +		page_table = pt->page;
>>   
>>   		last_pte = pte + num_entries;
>>   		if (last_pte > GEN8_PTES_PER_PAGE)
>> @@ -384,8 +492,9 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>>   			break;
>>   
>>   		if (pt_vaddr == NULL) {
>> -			struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[pdpe];
>> -			struct page *page_table = pd->page_tables[pde].page;
>> +			struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[pdpe];
>> +			struct i915_page_table_entry *pt = pd->page_tables[pde];
>> +			struct page *page_table = pt->page;
>>   
>>   			pt_vaddr = kmap_atomic(page_table);
>>   		}
>> @@ -416,19 +525,16 @@ static void gen8_free_page_tables(struct i915_page_directory_entry *pd)
>>   {
>>   	int i;
>>   
>> -	if (pd->page_tables == NULL)
>> +	if (!pd->page)
>>   		return;
>>   
>> -	for (i = 0; i < GEN8_PDES_PER_PAGE; i++)
>> -		if (pd->page_tables[i].page)
>> -			__free_page(pd->page_tables[i].page);
>> -}
>> +	for (i = 0; i < GEN8_PDES_PER_PAGE; i++) {
>> +		if (WARN_ON(!pd->page_tables[i]))
>> +			continue;
>>   
>> -static void gen8_free_page_directory(struct i915_page_directory_entry *pd)
>> -{
>> -	gen8_free_page_tables(pd);
>> -	kfree(pd->page_tables);
>> -	__free_page(pd->page);
>> +		unmap_and_free_pt(pd->page_tables[i]);
>> +		pd->page_tables[i] = NULL;
>> +	}
>>   }
>>   
>>   static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>> @@ -436,7 +542,11 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>>   	int i;
>>   
>>   	for (i = 0; i < ppgtt->num_pd_pages; i++) {
>> -		gen8_free_page_directory(&ppgtt->pdp.page_directory[i]);
>> +		if (WARN_ON(!ppgtt->pdp.page_directory[i]))
>> +			continue;
>> +
>> +		gen8_free_page_tables(ppgtt->pdp.page_directory[i]);
>> +		unmap_and_free_pd(ppgtt->pdp.page_directory[i]);
>>   	}
>>   }
>>   
>> @@ -448,14 +558,23 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>>   	for (i = 0; i < ppgtt->num_pd_pages; i++) {
>>   		/* TODO: In the future we'll support sparse mappings, so this
>>   		 * will have to change. */
>> -		if (!ppgtt->pdp.page_directory[i].daddr)
>> +		if (!ppgtt->pdp.page_directory[i]->daddr)
>>   			continue;
>>   
>> -		pci_unmap_page(hwdev, ppgtt->pdp.page_directory[i].daddr, PAGE_SIZE,
>> +		pci_unmap_page(hwdev, ppgtt->pdp.page_directory[i]->daddr, PAGE_SIZE,
>>   			       PCI_DMA_BIDIRECTIONAL);
>>   
>>   		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>> -			dma_addr_t addr = ppgtt->pdp.page_directory[i].page_tables[j].daddr;
>> +			struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[i];
>> +			struct i915_page_table_entry *pt;
>> +			dma_addr_t addr;
>> +
>> +			if (WARN_ON(!pd->page_tables[j]))
>> +				continue;
>> +
>> +			pt = pd->page_tables[j];
>> +			addr = pt->daddr;
>> +
>>   			if (addr)
>>   				pci_unmap_page(hwdev, addr, PAGE_SIZE,
>>   					       PCI_DMA_BIDIRECTIONAL);
>> @@ -474,25 +593,20 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>>   
>>   static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
>>   {
>> -	int i, j;
>> +	int i, ret;
>>   
>>   	for (i = 0; i < ppgtt->num_pd_pages; i++) {
>> -		struct i915_page_directory_entry *pd = &ppgtt->pdp.page_directory[i];
>> -		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>> -			struct i915_page_table_entry *pt = &pd->page_tables[j];
>> -
>> -			pt->page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>> -			if (!pt->page)
>> -				goto unwind_out;
>> -
>> -		}
>> +		ret = alloc_pt_range(ppgtt->pdp.page_directory[i],
>> +				     0, GEN8_PDES_PER_PAGE);
>> +		if (ret)
>> +			goto unwind_out;
>>   	}
>>   
>>   	return 0;
>>   
>>   unwind_out:
>>   	while (i--)
>> -		gen8_free_page_tables(&ppgtt->pdp.page_directory[i]);
>> +		gen8_free_page_tables(ppgtt->pdp.page_directory[i]);
>>   
>>   	return -ENOMEM;
>>   }
>> @@ -503,19 +617,9 @@ static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt,
>>   	int i;
>>   
>>   	for (i = 0; i < max_pdp; i++) {
>> -		struct i915_page_table_entry *pt;
>> -
>> -		pt = kcalloc(GEN8_PDES_PER_PAGE, sizeof(*pt), GFP_KERNEL);
>> -		if (!pt)
>> +		ppgtt->pdp.page_directory[i] = alloc_pd_single();
>> +		if (IS_ERR(ppgtt->pdp.page_directory[i]))
>>   			goto unwind_out;
>> -
>> -		ppgtt->pdp.page_directory[i].page = alloc_page(GFP_KERNEL);
>> -		if (!ppgtt->pdp.page_directory[i].page) {
>> -			kfree(pt);
>> -			goto unwind_out;
>> -		}
>> -
>> -		ppgtt->pdp.page_directory[i].page_tables = pt;
>>   	}
>>   
>>   	ppgtt->num_pd_pages = max_pdp;
>> @@ -524,10 +628,8 @@ static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt,
>>   	return 0;
>>   
>>   unwind_out:
>> -	while (i--) {
>> -		kfree(ppgtt->pdp.page_directory[i].page_tables);
>> -		__free_page(ppgtt->pdp.page_directory[i].page);
>> -	}
>> +	while (i--)
>> +		unmap_and_free_pd(ppgtt->pdp.page_directory[i]);
>>   
>>   	return -ENOMEM;
>>   }
>> @@ -561,14 +663,14 @@ static int gen8_ppgtt_setup_page_directories(struct i915_hw_ppgtt *ppgtt,
>>   	int ret;
>>   
>>   	pd_addr = pci_map_page(ppgtt->base.dev->pdev,
>> -			       ppgtt->pdp.page_directory[pd].page, 0,
>> +			       ppgtt->pdp.page_directory[pd]->page, 0,
>>   			       PAGE_SIZE, PCI_DMA_BIDIRECTIONAL);
>>   
>>   	ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ppgtt->pdp.page_directory[pd].daddr = pd_addr;
>> +	ppgtt->pdp.page_directory[pd]->daddr = pd_addr;
>>   
>>   	return 0;
>>   }
>> @@ -578,8 +680,8 @@ static int gen8_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt,
>>   					const int pt)
>>   {
>>   	dma_addr_t pt_addr;
>> -	struct i915_page_directory_entry *pdir = &ppgtt->pdp.page_directory[pd];
>> -	struct i915_page_table_entry *ptab = &pdir->page_tables[pt];
>> +	struct i915_page_directory_entry *pdir = ppgtt->pdp.page_directory[pd];
>> +	struct i915_page_table_entry *ptab = pdir->page_tables[pt];
>>   	struct page *p = ptab->page;
>>   	int ret;
>>   
>> @@ -642,10 +744,12 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>>   	 * will never need to touch the PDEs again.
>>   	 */
>>   	for (i = 0; i < max_pdp; i++) {
>> +		struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[i];
>>   		gen8_ppgtt_pde_t *pd_vaddr;
>> -		pd_vaddr = kmap_atomic(ppgtt->pdp.page_directory[i].page);
>> +		pd_vaddr = kmap_atomic(ppgtt->pdp.page_directory[i]->page);
>>   		for (j = 0; j < GEN8_PDES_PER_PAGE; j++) {
>> -			dma_addr_t addr = ppgtt->pdp.page_directory[i].page_tables[j].daddr;
>> +			struct i915_page_table_entry *pt = pd->page_tables[j];
>> +			dma_addr_t addr = pt->daddr;
>>   			pd_vaddr[j] = gen8_pde_encode(ppgtt->base.dev, addr,
>>   						      I915_CACHE_LLC);
>>   		}
>> @@ -696,7 +800,7 @@ static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
>>   	for (pde = 0; pde < ppgtt->num_pd_entries; pde++) {
>>   		u32 expected;
>>   		gen6_gtt_pte_t *pt_vaddr;
>> -		dma_addr_t pt_addr = ppgtt->pd.page_tables[pde].daddr;
>> +		dma_addr_t pt_addr = ppgtt->pd.page_tables[pde]->daddr;
>>   		pd_entry = readl(pd_addr + pde);
>>   		expected = (GEN6_PDE_ADDR_ENCODE(pt_addr) | GEN6_PDE_VALID);
>>   
>> @@ -707,7 +811,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->pd.page_tables[pde].page);
>> +		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) +
>> @@ -746,7 +850,7 @@ static void gen6_write_pdes(struct i915_hw_ppgtt *ppgtt)
>>   	for (i = 0; i < ppgtt->num_pd_entries; i++) {
>>   		dma_addr_t pt_addr;
>>   
>> -		pt_addr = ppgtt->pd.page_tables[i].daddr;
>> +		pt_addr = ppgtt->pd.page_tables[i]->daddr;
>>   		pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr);
>>   		pd_entry |= GEN6_PDE_VALID;
>>   
>> @@ -922,7 +1026,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->pd.page_tables[act_pt].page);
>> +		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 +1055,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->pd.page_tables[act_pt].page);
>> +			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),
>> @@ -974,7 +1078,7 @@ static void gen6_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>>   
>>   	for (i = 0; i < ppgtt->num_pd_entries; i++)
>>   		pci_unmap_page(ppgtt->base.dev->pdev,
>> -			       ppgtt->pd.page_tables[i].daddr,
>> +			       ppgtt->pd.page_tables[i]->daddr,
>>   			       4096, PCI_DMA_BIDIRECTIONAL);
>>   }
>>   
>> @@ -983,8 +1087,9 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>>   	int i;
>>   
>>   	for (i = 0; i < ppgtt->num_pd_entries; i++)
>> -		__free_page(ppgtt->pd.page_tables[i].page);
>> -	kfree(ppgtt->pd.page_tables);
>> +		unmap_and_free_pt(ppgtt->pd.page_tables[i]);
>> +
>> +	unmap_and_free_pd(&ppgtt->pd);
>>   }
>>   
>>   static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>> @@ -1039,27 +1144,6 @@ alloc:
>>   	return 0;
>>   }
>>   
>> -static int gen6_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt)
>> -{
>> -	struct i915_page_table_entry *pt;
>> -	int i;
>> -
>> -	pt = kcalloc(ppgtt->num_pd_entries, sizeof(*pt), GFP_KERNEL);
>> -	if (!pt)
>> -		return -ENOMEM;
>> -
>> -	for (i = 0; i < ppgtt->num_pd_entries; i++) {
>> -		pt[i].page = alloc_page(GFP_KERNEL);
>> -		if (!pt->page) {
>> -			gen6_ppgtt_free(ppgtt);
>> -			return -ENOMEM;
>> -		}
>> -	}
>> -
>> -	ppgtt->pd.page_tables = pt;
>> -	return 0;
>> -}
>> -
>>   static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
>>   {
>>   	int ret;
>> @@ -1068,7 +1152,7 @@ static int gen6_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt)
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = gen6_ppgtt_allocate_page_tables(ppgtt);
>> +	ret = alloc_pt_range(&ppgtt->pd, 0, ppgtt->num_pd_entries);
>>   	if (ret) {
>>   		drm_mm_remove_node(&ppgtt->node);
>>   		return ret;
>> @@ -1086,7 +1170,7 @@ static int gen6_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt)
>>   		struct page *page;
>>   		dma_addr_t pt_addr;
>>   
>> -		page = ppgtt->pd.page_tables[i].page;
>> +		page = ppgtt->pd.page_tables[i]->page;
>>   		pt_addr = pci_map_page(dev->pdev, page, 0, 4096,
>>   				       PCI_DMA_BIDIRECTIONAL);
>>   
>> @@ -1095,7 +1179,7 @@ static int gen6_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt)
>>   			return -EIO;
>>   		}
>>   
>> -		ppgtt->pd.page_tables[i].daddr = pt_addr;
>> +		ppgtt->pd.page_tables[i]->daddr = pt_addr;
>>   	}
>>   
>>   	return 0;
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index 6efeb18..e8cad72 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -199,12 +199,12 @@ struct i915_page_directory_entry {
>>   		dma_addr_t daddr;
>>   	};
>>   
>> -	struct i915_page_table_entry *page_tables;
>> +	struct i915_page_table_entry *page_tables[GEN6_PPGTT_PD_ENTRIES]; /* PDEs */
> Would you consider changing the plural here in 'tables' so that we would
> lose the discrepancy against the page_directory below?
Ok, I'll rename them, but it's cleaner to make this change in the patch 
that added it ("drm/i915: page table abstractions").

-Michel
> -Mika
>
>>   };
>>   
>>   struct i915_page_directory_pointer_entry {
>>   	/* struct page *page; */
>> -	struct i915_page_directory_entry page_directory[GEN8_LEGACY_PDPES];
>> +	struct i915_page_directory_entry *page_directory[GEN8_LEGACY_PDPES];
>>   };
>>   
>>   struct i915_address_space {
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 9e71992..bc9c7c3 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1735,14 +1735,14 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o
>>   	reg_state[CTX_PDP1_LDW] = GEN8_RING_PDP_LDW(ring, 1);
>>   	reg_state[CTX_PDP0_UDW] = GEN8_RING_PDP_UDW(ring, 0);
>>   	reg_state[CTX_PDP0_LDW] = GEN8_RING_PDP_LDW(ring, 0);
>> -	reg_state[CTX_PDP3_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[3].daddr);
>> -	reg_state[CTX_PDP3_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[3].daddr);
>> -	reg_state[CTX_PDP2_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[2].daddr);
>> -	reg_state[CTX_PDP2_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[2].daddr);
>> -	reg_state[CTX_PDP1_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[1].daddr);
>> -	reg_state[CTX_PDP1_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[1].daddr);
>> -	reg_state[CTX_PDP0_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[0].daddr);
>> -	reg_state[CTX_PDP0_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[0].daddr);
>> +	reg_state[CTX_PDP3_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[3]->daddr);
>> +	reg_state[CTX_PDP3_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[3]->daddr);
>> +	reg_state[CTX_PDP2_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[2]->daddr);
>> +	reg_state[CTX_PDP2_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[2]->daddr);
>> +	reg_state[CTX_PDP1_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[1]->daddr);
>> +	reg_state[CTX_PDP1_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[1]->daddr);
>> +	reg_state[CTX_PDP0_UDW+1] = upper_32_bits(ppgtt->pdp.page_directory[0]->daddr);
>> +	reg_state[CTX_PDP0_LDW+1] = lower_32_bits(ppgtt->pdp.page_directory[0]->daddr);
>>   	if (ring->id == RCS) {
>>   		reg_state[CTX_LRI_HEADER_2] = MI_LOAD_REGISTER_IMM(1);
>>   		reg_state[CTX_R_PWR_CLK_STATE] = 0x20c8;
>> -- 
>> 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