Re: [PATCH v3 03/17] drm/i915/gen8: Abstract PDP usage

From: "Goel, Akash" <akash.goel@intel.com>
To: Michel Thierry <michel.thierry@intel.com>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 03/17] drm/i915/gen8: Abstract PDP usage
Date: Tue, 7 Jul 2015 18:13:08 +0530	[thread overview]
Message-ID: <559BC95C.1010801@intel.com> (raw)
In-Reply-To: <1435764453-11954-4-git-send-email-michel.thierry@intel.com>

On 7/1/2015 8:57 PM, Michel Thierry wrote:
> Up until now, ppgtt->pdp has always been the root of our page tables.
> Legacy 32b addresses acted like it had 1 PDP with 4 PDPEs.
>
> In preparation for 4 level page tables, we need to stop use ppgtt->pdp
> directly unless we know it's what we want. The future structure will use
> ppgtt->pml4 for the top level, and the pdp is just one of the entries
> being pointed to by a pml4e.
>
> v2: Updated after dynamic page allocation changes.
> v3: Rebase after s/page_tables/page_table/.
> v4: Rebase after changes in "Dynamic page table allocations" patch.
> v5: Rebase after Mika's ppgtt cleanup / scratch merge patch series.
> v6: Rebase after final merged version of Mika's ppgtt/scratch patches.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v2+)
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 136 +++++++++++++++++++++++-------------
>   1 file changed, 88 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index cdcc778..41a18ff 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -529,6 +529,25 @@ static void gen8_initialize_pd(struct i915_address_space *vm,
>   	fill_px(vm->dev, pd, scratch_pde);
>   }
>
> +/* It's likely we'll map more than one page table at a time. This function will
> + * save us unnecessary kmap calls, but do no more functionally than multiple
> + * calls to pde_encode. The ppgtt is only needed to reuse the kunmap macro. */
> +static void gen8_map_pagetable_range(struct i915_hw_ppgtt *ppgtt,
> +				     struct i915_page_directory *pd,
> +				     uint64_t start,
> +				     uint64_t length)
> +{
> +	gen8_pde_t * const page_directory = kmap_px(pd);
> +	struct i915_page_table *pt;
> +	uint64_t temp, pde;
> +
> +	gen8_for_each_pde(pt, pd, start, length, temp, pde)
> +		page_directory[pde] = gen8_pde_encode(px_dma(pt),
> +						      I915_CACHE_LLC);
> +
> +	kunmap_px(ppgtt, page_directory);
> +}
> +
>   static int __pdp_init(struct drm_device *dev,
>   		      struct i915_page_directory_pointer *pdp)
>   {
> @@ -616,6 +635,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>   {
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
> +	struct i915_page_directory_pointer *pdp = &ppgtt->pdp; /* FIXME: 48b */
>   	gen8_pte_t *pt_vaddr, scratch_pte;
>   	unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
>   	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
> @@ -630,10 +650,10 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>   		struct i915_page_directory *pd;
>   		struct i915_page_table *pt;
>
> -		if (WARN_ON(!ppgtt->pdp.page_directory[pdpe]))
> +		if (WARN_ON(!pdp->page_directory[pdpe]))
>   			break;
>
> -		pd = ppgtt->pdp.page_directory[pdpe];
> +		pd = pdp->page_directory[pdpe];
>
>   		if (WARN_ON(!pd->page_table[pde]))
>   			break;
> @@ -671,6 +691,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>   {
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
> +	struct i915_page_directory_pointer *pdp = &ppgtt->pdp; /* FIXME: 48b */
>   	gen8_pte_t *pt_vaddr;
>   	unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
>   	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
> @@ -681,7 +702,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>
>   	for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
>   		if (pt_vaddr == NULL) {
> -			struct i915_page_directory *pd = ppgtt->pdp.page_directory[pdpe];
> +			struct i915_page_directory *pd = pdp->page_directory[pdpe];
>   			struct i915_page_table *pt = pd->page_table[pde];
>   			pt_vaddr = kmap_px(pt);
>   		}
> @@ -763,23 +784,28 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>   		container_of(vm, struct i915_hw_ppgtt, base);
>   	int i;
>
> -	for_each_set_bit(i, ppgtt->pdp.used_pdpes,
> -				I915_PDPES_PER_PDP(ppgtt->base.dev)) {
> -		if (WARN_ON(!ppgtt->pdp.page_directory[i]))
> -			continue;
> +	if (!USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
> +		for_each_set_bit(i, ppgtt->pdp.used_pdpes,
> +				 I915_PDPES_PER_PDP(ppgtt->base.dev)) {
> +			if (WARN_ON(!ppgtt->pdp.page_directory[i]))
> +				continue;
>
> -		gen8_free_page_tables(ppgtt->base.dev,
> -				      ppgtt->pdp.page_directory[i]);
> -		free_pd(ppgtt->base.dev, ppgtt->pdp.page_directory[i]);
> +			gen8_free_page_tables(ppgtt->base.dev,
> +					      ppgtt->pdp.page_directory[i]);
> +			free_pd(ppgtt->base.dev,
> +				ppgtt->pdp.page_directory[i]);
> +		}
> +		free_pdp(ppgtt->base.dev, &ppgtt->pdp);
> +	} else {
> +		WARN_ON(1); /* to be implemented later */
>   	}
>
> -	free_pdp(ppgtt->base.dev, &ppgtt->pdp);
>   	gen8_free_scratch(vm);
>   }
>
>   /**
>    * gen8_ppgtt_alloc_pagetabs() - Allocate page tables for VA range.
> - * @ppgtt:	Master ppgtt structure.
> + * @vm:		Master vm structure.
>    * @pd:		Page directory for this address range.
>    * @start:	Starting virtual address to begin allocations.
>    * @length	Size of the allocations.
> @@ -795,13 +821,15 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>    *
>    * Return: 0 if success; negative error code otherwise.
>    */
> -static int gen8_ppgtt_alloc_pagetabs(struct i915_hw_ppgtt *ppgtt,
> +static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
>   				     struct i915_page_directory *pd,
>   				     uint64_t start,
>   				     uint64_t length,
>   				     unsigned long *new_pts)
>   {
> -	struct drm_device *dev = ppgtt->base.dev;
> +	struct i915_hw_ppgtt *ppgtt =
> +	    container_of(vm, struct i915_hw_ppgtt, base);
> +	struct drm_device *dev = vm->dev;

The 'ppgtt' pointer can be completely dispensed with, by the use of 'vm' 
pointer.

>   	struct i915_page_table *pt;
>   	uint64_t temp;
>   	uint32_t pde;
> @@ -818,7 +846,7 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_hw_ppgtt *ppgtt,
>   		if (IS_ERR(pt))
>   			goto unwind_out;
>
> -		gen8_initialize_pt(&ppgtt->base, pt);
> +		gen8_initialize_pt(vm, pt);
>   		pd->page_table[pde] = pt;
>   		__set_bit(pde, new_pts);
>   	}
> @@ -834,7 +862,7 @@ unwind_out:
>
>   /**
>    * gen8_ppgtt_alloc_page_directories() - Allocate page directories for VA range.
> - * @ppgtt:	Master ppgtt structure.
> + * @vm:		Master vm structure.
>    * @pdp:	Page directory pointer for this address range.
>    * @start:	Starting virtual address to begin allocations.
>    * @length	Size of the allocations.
> @@ -855,17 +883,18 @@ unwind_out:
>    *
>    * Return: 0 if success; negative error code otherwise.
>    */
> -static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
> -				     struct i915_page_directory_pointer *pdp,
> -				     uint64_t start,
> -				     uint64_t length,
> -				     unsigned long *new_pds)
> +static int
> +gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
> +				  struct i915_page_directory_pointer *pdp,
> +				  uint64_t start,
> +				  uint64_t length,
> +				  unsigned long *new_pds)
>   {
> -	struct drm_device *dev = ppgtt->base.dev;
> +	struct drm_device *dev = vm->dev;
>   	struct i915_page_directory *pd;
>   	uint64_t temp;
>   	uint32_t pdpe;
> -	uint32_t pdpes =  I915_PDPES_PER_PDP(ppgtt->base.dev);
> +	uint32_t pdpes =  I915_PDPES_PER_PDP(vm->dev);
>
>   	WARN_ON(!bitmap_empty(new_pds, pdpes));
>
> @@ -877,7 +906,7 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>   		if (IS_ERR(pd))
>   			goto unwind_out;
>
> -		gen8_initialize_pd(&ppgtt->base, pd);
> +		gen8_initialize_pd(vm, pd);
>   		pdp->page_directory[pdpe] = pd;
>   		__set_bit(pdpe, new_pds);
>   	}
> @@ -952,13 +981,15 @@ static void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt)
>   	ppgtt->pd_dirty_rings = INTEL_INFO(ppgtt->base.dev)->ring_mask;
>   }
>
> -static int gen8_alloc_va_range(struct i915_address_space *vm,
> -			       uint64_t start,
> -			       uint64_t length)
> +static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
> +				    struct i915_page_directory_pointer *pdp,
> +				    uint64_t start,
> +				    uint64_t length)
>   {
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
>   	unsigned long *new_page_dirs, **new_page_tables;
> +	struct drm_device *dev = vm->dev;
>   	struct i915_page_directory *pd;
>   	const uint64_t orig_start = start;
>   	const uint64_t orig_length = length;
> @@ -981,16 +1012,15 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>   		return ret;
>
>   	/* Do the allocations first so we can easily bail out */
> -	ret = gen8_ppgtt_alloc_page_directories(ppgtt, &ppgtt->pdp, start, length,
> -					new_page_dirs);
> +	ret = gen8_ppgtt_alloc_page_directories(vm, pdp, start, length,
> +						new_page_dirs);
>   	if (ret) {
>   		free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
>   		return ret;
>   	}
>
> -	/* For every page directory referenced, allocate page tables */
> -	gen8_for_each_pdpe(pd, &ppgtt->pdp, start, length, temp, pdpe) {
> -		ret = gen8_ppgtt_alloc_pagetabs(ppgtt, pd, start, length,
> +	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
> +		ret = gen8_ppgtt_alloc_pagetabs(vm, pd, start, length,
>   						new_page_tables[pdpe]);
>   		if (ret)
>   			goto err_out;
> @@ -999,10 +1029,7 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>   	start = orig_start;
>   	length = orig_length;
>
> -	/* Allocations have completed successfully, so set the bitmaps, and do
> -	 * the mappings. */
> -	gen8_for_each_pdpe(pd, &ppgtt->pdp, start, length, temp, pdpe) {
> -		gen8_pde_t *const page_directory = kmap_px(pd);
> +	gen8_for_each_pdpe(pd, pdp, start, length, temp, pdpe) {
>   		struct i915_page_table *pt;
>   		uint64_t pd_len = length;
>   		uint64_t pd_start = start;
> @@ -1024,18 +1051,10 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>
>   			/* Our pde is now pointing to the pagetable, pt */
>   			__set_bit(pde, pd->used_pdes);
> -
> -			/* Map the PDE to the page table */
> -			page_directory[pde] = gen8_pde_encode(px_dma(pt),
> -							      I915_CACHE_LLC);
> -
> -			/* NB: We haven't yet mapped ptes to pages. At this
> -			 * point we're still relying on insert_entries() */
>   		}
>
> -		kunmap_px(ppgtt, page_directory);
> -
> -		__set_bit(pdpe, ppgtt->pdp.used_pdpes);
> +		__set_bit(pdpe, pdp->used_pdpes);
> +		gen8_map_pagetable_range(ppgtt, pd, start, length);

No apparent benefit in use of "gen8_map_pagetable_range", considering 
that the Page Directory page is mapped at the start of outer loop 
(PDPEs) only and the inner loop (PDEs) maps the PDEs to the page tables, 
in an inline manner.
The 'gen8_map_pagetable_range', will repeat the same inner loop of PDEs.

>   	}
>
>   	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
> @@ -1045,17 +1064,38 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>   err_out:
>   	while (pdpe--) {
>   		for_each_set_bit(temp, new_page_tables[pdpe], I915_PDES)
> -			free_pt(vm->dev, ppgtt->pdp.page_directory[pdpe]->page_table[temp]);
> +			free_pt(dev, pdp->page_directory[pdpe]->page_table[temp]);
>   	}
>
>   	for_each_set_bit(pdpe, new_page_dirs, pdpes)
> -		free_pd(vm->dev, ppgtt->pdp.page_directory[pdpe]);
> +		free_pd(dev, pdp->page_directory[pdpe]);
>
>   	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
>   	mark_tlbs_dirty(ppgtt);
>   	return ret;
>   }
>
> +static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
> +				    struct i915_pml4 *pml4,
> +				    uint64_t start,
> +				    uint64_t length)
> +{
> +	WARN_ON(1); /* to be implemented later */
> +	return 0;
> +}
> +
> +static int gen8_alloc_va_range(struct i915_address_space *vm,
> +			       uint64_t start, uint64_t length)
> +{
> +	struct i915_hw_ppgtt *ppgtt =
> +		container_of(vm, struct i915_hw_ppgtt, base);
> +
> +	if (!USES_FULL_48BIT_PPGTT(vm->dev))
> +		return gen8_alloc_va_range_3lvl(vm, &ppgtt->pdp, start, length);
> +	else
> +		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
> +}
> +
>   /*
>    * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP registers
>    * with a net effect resembling a 2-level page table in normal x86 terms. Each
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx