diff mbox

[2/2] drm/i915/gtt: Avoid using addresses in non-canonical form.

Message ID 1442323816-4712-2-git-send-email-michal.winiarski@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

MichaƂ Winiarski Sept. 15, 2015, 1:30 p.m. UTC
According to bspec, some parts of HW expect the addresses to be in
a canonical form where bits [63:48] == [47]. Lets satisfy the HW by
converting the address prior to allocating/clearing the entries in page
tables.

Cc: Michel Thierry <michel.thierry@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Micha? Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 13 +++++++------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++++
 2 files changed, 12 insertions(+), 6 deletions(-)

Comments

Chris Wilson Sept. 15, 2015, 1:52 p.m. UTC | #1
On Tue, Sep 15, 2015 at 03:30:16PM +0200, Micha? Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47]. Lets satisfy the HW by
> converting the address prior to allocating/clearing the entries in page
> tables.

You should be a little more specific as to why this is only required for
the ppgtt paths.
-Chris
Michel Thierry Sept. 15, 2015, 2:01 p.m. UTC | #2
On 9/15/2015 2:30 PM, Micha? Winiarski wrote:
> According to bspec, some parts of HW expect the addresses to be in
> a canonical form where bits [63:48] == [47]. Lets satisfy the HW by
> converting the address prior to allocating/clearing the entries in page
> tables.

Thanks for sending this. A couple of comments below.
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7ff7239..f25a33f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -766,6 +766,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
>   	} else {
>   		uint64_t templ4, pml4e;
>   		struct i915_page_directory_pointer *pdp;
> +		start = gen8_canonical_addr(start);
>
>   		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
>   			gen8_ppgtt_clear_pte_range(vm, pdp, start, length,
> @@ -1227,15 +1228,10 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
>   	uint32_t pdpes = I915_PDPES_PER_PDP(dev);
>   	int ret;
>
> -	/* Wrap is never okay since we can only represent 48b, and we don't
> -	 * actually use the other side of the canonical address space.
> -	 */
> +	/* Wrap is never okay */
>   	if (WARN_ON(start + length < start))
>   		return -ENODEV;

start can be in canonical form here (alloc_va_range_4lvl calls 
alloc_va_range_3lvl), and then this check could fail, e.g: first alloc 
when using top-down.
Just move it to gen8_alloc_va_range like you did to the check below.

>
> -	if (WARN_ON(start + length > vm->total))
> -		return -ENODEV;
> -
>   	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
>   	if (ret)
>   		return ret;
> @@ -1333,6 +1329,8 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
>   	uint64_t temp, pml4e;
>   	int ret = 0;
>
> +	start = gen8_canonical_addr(start);
> +

Can you move this to gen8_alloc_va_range (48bit path) instead?
That will keep it in sync to _clear_range.

Also, I know it isn't really required in gen8_ppgtt_insert_entries (as 
it only relies in pdp/pdp/pt indexes), but it will be nice to have 
alloc/insert/clear functions in sync.

>   	/* Do the pml4 allocations first, so we don't need to track the newly
>   	 * allocated tables below the pdp */
>   	bitmap_zero(new_pdps, GEN8_PML4ES_PER_PML4);
> @@ -1377,6 +1375,9 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>   	struct i915_hw_ppgtt *ppgtt =
>   		container_of(vm, struct i915_hw_ppgtt, base);
>
> +	if (WARN_ON(start + length > vm->total))
> +		return -ENODEV;
> +
>   	if (USES_FULL_48BIT_PPGTT(vm->dev))
>   		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
>   	else
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 8275007..9397387 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -503,6 +503,11 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
>   	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
>   }
>
> +static inline uint64_t gen8_canonical_addr(uint64_t address)

Feel free to ignore this one, but this isn't gen8 per se, so I vote for 
to_canonical_addr().

> +{
> +	return ((int64_t)address << 16) >> 16;
> +}
> +
>   static inline dma_addr_t
>   i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
>   {
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7ff7239..f25a33f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -766,6 +766,7 @@  static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
 	} else {
 		uint64_t templ4, pml4e;
 		struct i915_page_directory_pointer *pdp;
+		start = gen8_canonical_addr(start);
 
 		gen8_for_each_pml4e(pdp, &ppgtt->pml4, start, length, templ4, pml4e) {
 			gen8_ppgtt_clear_pte_range(vm, pdp, start, length,
@@ -1227,15 +1228,10 @@  static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	uint32_t pdpes = I915_PDPES_PER_PDP(dev);
 	int ret;
 
-	/* Wrap is never okay since we can only represent 48b, and we don't
-	 * actually use the other side of the canonical address space.
-	 */
+	/* Wrap is never okay */
 	if (WARN_ON(start + length < start))
 		return -ENODEV;
 
-	if (WARN_ON(start + length > vm->total))
-		return -ENODEV;
-
 	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
 	if (ret)
 		return ret;
@@ -1333,6 +1329,8 @@  static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 	uint64_t temp, pml4e;
 	int ret = 0;
 
+	start = gen8_canonical_addr(start);
+
 	/* Do the pml4 allocations first, so we don't need to track the newly
 	 * allocated tables below the pdp */
 	bitmap_zero(new_pdps, GEN8_PML4ES_PER_PML4);
@@ -1377,6 +1375,9 @@  static int gen8_alloc_va_range(struct i915_address_space *vm,
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 
+	if (WARN_ON(start + length > vm->total))
+		return -ENODEV;
+
 	if (USES_FULL_48BIT_PPGTT(vm->dev))
 		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
 	else
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8275007..9397387 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -503,6 +503,11 @@  static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
 	return i915_pte_count(address, length, GEN8_PDE_SHIFT);
 }
 
+static inline uint64_t gen8_canonical_addr(uint64_t address)
+{
+	return ((int64_t)address << 16) >> 16;
+}
+
 static inline dma_addr_t
 i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 {