[v3,09/17] drm/i915/gen8: Add 4 level support in insert_entries and clear_range

Submitted by Michel Thierry on July 1, 2015, 3:27 p.m.

Details

Message ID 1435764453-11954-10-git-send-email-michel.thierry@intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Michel Thierry July 1, 2015, 3:27 p.m.
When 48b is enabled, gen8_ppgtt_insert_entries needs to read the Page Map
Level 4 (PML4), before it selects which Page Directory Pointer (PDP)
it will write to.

Similarly, gen8_ppgtt_clear_range needs to get the correct PDP/PD range.

This patch was inspired by Ben's "Depend exclusively on map and
unmap_vma".

v2: Rebase after s/page_tables/page_table/.
v3: Remove unnecessary pdpe loop in gen8_ppgtt_clear_range_4lvl and use
clamp_pdp in gen8_ppgtt_insert_entries (Akash).
v4: Merge gen8_ppgtt_clear_range_4lvl into gen8_ppgtt_clear_range to
maintain symmetry with gen8_ppgtt_insert_entries (Akash).
v5: Do not mix pages and bytes in insert_entries (Akash).
v6: Prevent overflow in sg_nents << PAGE_SHIFT, when inserting 4GB at
once.
v7: Rebase after Mika's ppgtt cleanup / scratch merge patch series.
Use gen8_px_index functions, and remove unnecessary number of pages
parameter in insert_pte_entries.

Cc: Akash Goel <akash.goel@intel.com>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++++++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/i915_gem_gtt.h | 11 ++++++++
 2 files changed, 50 insertions(+), 12 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 67d02b9..d16fbce 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -712,9 +712,9 @@  static void gen8_ppgtt_clear_pte_range(struct i915_address_space *vm,
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 	gen8_pte_t *pt_vaddr;
-	unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
-	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
-	unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
+	unsigned pdpe = gen8_pdpe_index(start);
+	unsigned pde = gen8_pde_index(start);
+	unsigned pte = gen8_pte_index(start);
 	unsigned num_entries = length >> PAGE_SHIFT;
 	unsigned last_pte, i;
 
@@ -763,12 +763,24 @@  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 scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
 						 I915_CACHE_LLC, use_scratch);
 
-	gen8_ppgtt_clear_pte_range(vm, pdp, start, length, scratch_pte);
+	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+		gen8_ppgtt_clear_pte_range(vm, &ppgtt->pdp, start, length,
+					   scratch_pte);
+	} else {
+		uint64_t templ4, pml4e;
+		struct i915_page_directory_pointer *pdp;
+
+		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
+			uint64_t pdp_len = gen8_clamp_pdp(start, length);
+			uint64_t pdp_start = start;
+
+			gen8_ppgtt_clear_pte_range(vm, pdp, pdp_start, pdp_len,
+						   scratch_pte);
+		}
+	}
 }
 
 static void
@@ -781,9 +793,9 @@  gen8_ppgtt_insert_pte_entries(struct i915_address_space *vm,
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 	gen8_pte_t *pt_vaddr;
-	unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
-	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
-	unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
+	unsigned pdpe = gen8_pdpe_index(start);
+	unsigned pde = gen8_pde_index(start);
+	unsigned pte = gen8_pte_index(start);
 
 	pt_vaddr = NULL;
 
@@ -801,7 +813,8 @@  gen8_ppgtt_insert_pte_entries(struct i915_address_space *vm,
 			kunmap_px(ppgtt, pt_vaddr);
 			pt_vaddr = NULL;
 			if (++pde == I915_PDES) {
-				pdpe++;
+				if (++pdpe == I915_PDPES_PER_PDP(vm->dev))
+					break;
 				pde = 0;
 			}
 			pte = 0;
@@ -820,11 +833,25 @@  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 */
 	struct sg_page_iter sg_iter;
 
 	__sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl), 0);
-	gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter, start, cache_level);
+
+	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+		gen8_ppgtt_insert_pte_entries(vm, &ppgtt->pdp, &sg_iter, start,
+					      cache_level);
+	} else {
+		struct i915_page_directory_pointer *pdp;
+		uint64_t templ4, pml4e;
+		uint64_t length = (uint64_t)sg_nents(pages->sgl) << PAGE_SHIFT;
+
+		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
+			uint64_t pdp_start = start;
+
+			gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter,
+						      pdp_start, cache_level);
+		}
+	}
 }
 
 static void gen8_free_page_tables(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index fb939fb..fd61325 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -478,6 +478,17 @@  static inline uint32_t gen6_pde_index(uint32_t addr)
 #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)		\
 	gen8_for_each_pdpe_e(pd, pdp, start, length, temp, iter, I915_PDPES_PER_PDP(dev))
 
+/* Clamp length to the next page_directory pointer boundary */
+static inline uint64_t gen8_clamp_pdp(uint64_t start, uint64_t length)
+{
+	uint64_t next_pdp = ALIGN(start + 1, 1ULL << GEN8_PML4E_SHIFT);
+
+	if (next_pdp > (start + length))
+		return length;
+
+	return next_pdp - start;
+}
+
 static inline uint32_t gen8_pte_index(uint64_t address)
 {
 	return i915_pte_index(address, GEN8_PDE_SHIFT);

Comments

On 7/1/2015 8:57 PM, Michel Thierry wrote:
> When 48b is enabled, gen8_ppgtt_insert_entries needs to read the Page Map
> Level 4 (PML4), before it selects which Page Directory Pointer (PDP)
> it will write to.
>
> Similarly, gen8_ppgtt_clear_range needs to get the correct PDP/PD range.
>
> This patch was inspired by Ben's "Depend exclusively on map and
> unmap_vma".
>
> v2: Rebase after s/page_tables/page_table/.
> v3: Remove unnecessary pdpe loop in gen8_ppgtt_clear_range_4lvl and use
> clamp_pdp in gen8_ppgtt_insert_entries (Akash).
> v4: Merge gen8_ppgtt_clear_range_4lvl into gen8_ppgtt_clear_range to
> maintain symmetry with gen8_ppgtt_insert_entries (Akash).
> v5: Do not mix pages and bytes in insert_entries (Akash).
> v6: Prevent overflow in sg_nents << PAGE_SHIFT, when inserting 4GB at
> once.
> v7: Rebase after Mika's ppgtt cleanup / scratch merge patch series.
> Use gen8_px_index functions, and remove unnecessary number of pages
> parameter in insert_pte_entries.
>
> Cc: Akash Goel <akash.goel@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 51 ++++++++++++++++++++++++++++---------
>   drivers/gpu/drm/i915/i915_gem_gtt.h | 11 ++++++++
>   2 files changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 67d02b9..d16fbce 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -712,9 +712,9 @@ static void gen8_ppgtt_clear_pte_range(struct i915_address_space *vm,
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
>   	gen8_pte_t *pt_vaddr;
> -	unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
> -	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
> -	unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
> +	unsigned pdpe = gen8_pdpe_index(start);
> +	unsigned pde = gen8_pde_index(start);
> +	unsigned pte = gen8_pte_index(start);
>   	unsigned num_entries = length >> PAGE_SHIFT;
>   	unsigned last_pte, i;
>
> @@ -763,12 +763,24 @@ 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 scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
>   						 I915_CACHE_LLC, use_scratch);
>
> -	gen8_ppgtt_clear_pte_range(vm, pdp, start, length, scratch_pte);
> +	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
> +		gen8_ppgtt_clear_pte_range(vm, &ppgtt->pdp, start, length,
> +					   scratch_pte);
> +	} else {
> +		uint64_t templ4, pml4e;
> +		struct i915_page_directory_pointer *pdp;
> +
> +		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
> +			uint64_t pdp_len = gen8_clamp_pdp(start, length);
> +			uint64_t pdp_start = start;
> +
> +			gen8_ppgtt_clear_pte_range(vm, pdp, pdp_start, pdp_len,
> +						   scratch_pte);
> +		}
> +	}
>   }
>
>   static void
> @@ -781,9 +793,9 @@ gen8_ppgtt_insert_pte_entries(struct i915_address_space *vm,
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
>   	gen8_pte_t *pt_vaddr;
> -	unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
> -	unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
> -	unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
> +	unsigned pdpe = gen8_pdpe_index(start);
> +	unsigned pde = gen8_pde_index(start);
> +	unsigned pte = gen8_pte_index(start);
>
>   	pt_vaddr = NULL;
>
> @@ -801,7 +813,8 @@ gen8_ppgtt_insert_pte_entries(struct i915_address_space *vm,
>   			kunmap_px(ppgtt, pt_vaddr);
>   			pt_vaddr = NULL;
>   			if (++pde == I915_PDES) {
> -				pdpe++;
> +				if (++pdpe == I915_PDPES_PER_PDP(vm->dev))
> +					break;

Can the same pdpe check (for Page directory pointer boundary) be added 
in the gen8_ppgtt_clear_pte_range function also, to make it consistent 
with gen8_ppgtt_insert_pte_entries and this will also obviate the need 
for gen8_clamp_pdp macro.

>   				pde = 0;
>   			}
>   			pte = 0;
> @@ -820,11 +833,25 @@ 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 */
>   	struct sg_page_iter sg_iter;
>
>   	__sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl), 0);
> -	gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter, start, cache_level);
> +
> +	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
> +		gen8_ppgtt_insert_pte_entries(vm, &ppgtt->pdp, &sg_iter, start,
> +					      cache_level);
> +	} else {
> +		struct i915_page_directory_pointer *pdp;
> +		uint64_t templ4, pml4e;
> +		uint64_t length = (uint64_t)sg_nents(pages->sgl) << PAGE_SHIFT;
> +
> +		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
> +			uint64_t pdp_start = start;
> +

Isn't the 'pdp_start' dispensable here ? ‘start’ can be used directly.

> +			gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter,
> +						      pdp_start, cache_level);
> +		}
> +	}
>   }
>
>   static void gen8_free_page_tables(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index fb939fb..fd61325 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -478,6 +478,17 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>   #define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)		\
>   	gen8_for_each_pdpe_e(pd, pdp, start, length, temp, iter, I915_PDPES_PER_PDP(dev))
>
> +/* Clamp length to the next page_directory pointer boundary */
> +static inline uint64_t gen8_clamp_pdp(uint64_t start, uint64_t length)
> +{
> +	uint64_t next_pdp = ALIGN(start + 1, 1ULL << GEN8_PML4E_SHIFT);
> +
> +	if (next_pdp > (start + length))
> +		return length;
> +
> +	return next_pdp - start;
> +}
> +
>   static inline uint32_t gen8_pte_index(uint64_t address)
>   {
>   	return i915_pte_index(address, GEN8_PDE_SHIFT);
>
On 7/7/2015 1:51 PM, Goel, Akash wrote:
> On 7/1/2015 8:57 PM, Michel Thierry wrote:
>>   static void
>> @@ -781,9 +793,9 @@ gen8_ppgtt_insert_pte_entries(struct
>> i915_address_space *vm,
>>       struct i915_hw_ppgtt *ppgtt =
>>           container_of(vm, struct i915_hw_ppgtt, base);
>>       gen8_pte_t *pt_vaddr;
>> -    unsigned pdpe = start >> GEN8_PDPE_SHIFT & GEN8_PDPE_MASK;
>> -    unsigned pde = start >> GEN8_PDE_SHIFT & GEN8_PDE_MASK;
>> -    unsigned pte = start >> GEN8_PTE_SHIFT & GEN8_PTE_MASK;
>> +    unsigned pdpe = gen8_pdpe_index(start);
>> +    unsigned pde = gen8_pde_index(start);
>> +    unsigned pte = gen8_pte_index(start);
>>
>>       pt_vaddr = NULL;
>>
>> @@ -801,7 +813,8 @@ gen8_ppgtt_insert_pte_entries(struct
>> i915_address_space *vm,
>>               kunmap_px(ppgtt, pt_vaddr);
>>               pt_vaddr = NULL;
>>               if (++pde == I915_PDES) {
>> -                pdpe++;
>> +                if (++pdpe == I915_PDPES_PER_PDP(vm->dev))
>> +                    break;
>
> Can the same pdpe check (for Page directory pointer boundary) be added
> in the gen8_ppgtt_clear_pte_range function also, to make it consistent
> with gen8_ppgtt_insert_pte_entries and this will also obviate the need
> for gen8_clamp_pdp macro.
>
I will change gen8_ppgtt_clear_pte_range to stop at PDP boundary as you 
suggests (and clamp_pdp will go away).

>>                   pde = 0;
>>               }
>>               pte = 0;
>> @@ -820,11 +833,25 @@ 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 */
>>       struct sg_page_iter sg_iter;
>>
>>       __sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl),
>> 0);
>> -    gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter, start,
>> cache_level);
>> +
>> +    if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
>> +        gen8_ppgtt_insert_pte_entries(vm, &ppgtt->pdp, &sg_iter, start,
>> +                          cache_level);
>> +    } else {
>> +        struct i915_page_directory_pointer *pdp;
>> +        uint64_t templ4, pml4e;
>> +        uint64_t length = (uint64_t)sg_nents(pages->sgl) << PAGE_SHIFT;
>> +
>> +        gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4,
>> pml4e) {
>> +            uint64_t pdp_start = start;
>> +
>
> Isn't the 'pdp_start' dispensable here ? ‘start’ can be used directly.
>
Yes, and the same applies in gen8_ppgtt_clear_range, pdp_len and 
pdp_start are redundant there.

>> +            gen8_ppgtt_insert_pte_entries(vm, pdp, &sg_iter,
>> +                              pdp_start, cache_level);
>> +        }
>> +    }
>>   }
>>