[PATCH 0/8] GTT bitmaps purge

[PATCH 0/8] GTT bitmaps purge

[Michał Winiarski]

It seems that bitmaps are redundant and we can remove them without causing much
trouble.
Let's try to do that!
There should be no functional changes, but it may be helpful if anyone can point
out tests/benchmarks that could show any difference in performance.

Michał Winiarski (8):
  drm/i915/gtt: Don't pass ppgtt to ppgtt_cleanup_4lvl
  drm/i915/gtt: Rename orig_start/orig_length
  drm/i915/gtt: Extract unwind to separate function for
    gen6_alloc_va_range
  drm/i915/gtt: Don't use temp bitmaps to unwind gen8_alloc_va_range
  drm/i915/gtt: Purge temp bitmaps
  drm/i915: Prepare i915_page_table_entry_map tracepoint for bitmap
    purge
  drm/i915/gtt: Purge page tracking bitmaps
  drm/i915: Clear range when unbinding closed vma

 drivers/gpu/drm/i915/i915_gem_gtt.c | 440 ++++++++++--------------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  10 +-
 drivers/gpu/drm/i915/i915_trace.h   |  21 +-
 drivers/gpu/drm/i915/i915_vma.c     |   7 +-
 4 files changed, 138 insertions(+), 340 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/8] drm/i915/gtt: Don't pass ppgtt to ppgtt_cleanup_4lvl

[Michał Winiarski]

It's not operating on ppgtt, and it also makes things consistent with
analogous ppgtt_cleanup_3lvl function.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ef00d36..11ec68c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1051,19 +1051,19 @@ static void gen8_ppgtt_cleanup_3lvl(struct drm_i915_private *dev_priv,
 	free_pdp(dev_priv, pdp);
 }
 
-static void gen8_ppgtt_cleanup_4lvl(struct i915_hw_ppgtt *ppgtt)
+static void gen8_ppgtt_cleanup_4lvl(struct drm_i915_private *dev_priv,
+				    struct i915_pml4 *pml4)
 {
-	struct drm_i915_private *dev_priv = ppgtt->base.i915;
 	int i;
 
-	for_each_set_bit(i, ppgtt->pml4.used_pml4es, GEN8_PML4ES_PER_PML4) {
-		if (WARN_ON(!ppgtt->pml4.pdps[i]))
+	for_each_set_bit(i, pml4->used_pml4es, GEN8_PML4ES_PER_PML4) {
+		if (WARN_ON(!pml4->pdps[i]))
 			continue;
 
-		gen8_ppgtt_cleanup_3lvl(dev_priv, ppgtt->pml4.pdps[i]);
+		gen8_ppgtt_cleanup_3lvl(dev_priv, pml4->pdps[i]);
 	}
 
-	cleanup_px(dev_priv, &ppgtt->pml4);
+	cleanup_px(dev_priv, pml4);
 }
 
 static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
@@ -1077,7 +1077,7 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 	if (!USES_FULL_48BIT_PPGTT(dev_priv))
 		gen8_ppgtt_cleanup_3lvl(dev_priv, &ppgtt->pdp);
 	else
-		gen8_ppgtt_cleanup_4lvl(ppgtt);
+		gen8_ppgtt_cleanup_4lvl(dev_priv, &ppgtt->pml4);
 
 	gen8_free_scratch(vm);
 }
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/i915/gtt: Rename orig_start/orig_length

[Michał Winiarski]

Small rename to make things consistent with naming for gen6.
Let's also remove redundant start_in/length_in vars in gen6 path.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 11ec68c..49e1006 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1297,8 +1297,8 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	unsigned long *new_page_dirs, *new_page_tables;
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_directory *pd;
-	const uint64_t orig_start = start;
-	const uint64_t orig_length = length;
+	const uint64_t start_save = start;
+	const uint64_t length_save = length;
 	uint32_t pdpe;
 	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
 	int ret;
@@ -1332,8 +1332,8 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 			goto err_out;
 	}
 
-	start = orig_start;
-	length = orig_length;
+	start = start_save;
+	length = length_save;
 
 	/* Allocations have completed successfully, so set the bitmaps, and do
 	 * the mappings. */
@@ -1918,22 +1918,22 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
 }
 
 static int gen6_alloc_va_range(struct i915_address_space *vm,
-			       uint64_t start_in, uint64_t length_in)
+			       uint64_t start, uint64_t length)
 {
 	DECLARE_BITMAP(new_page_tables, I915_PDES);
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct i915_page_table *pt;
-	uint32_t start, length, start_save, length_save;
+	uint32_t start_save, length_save;
 	uint32_t pde;
 	int ret;
 
-	if (WARN_ON(start_in + length_in > ppgtt->base.total))
+	if (WARN_ON(start + length > ppgtt->base.total))
 		return -ENODEV;
 
-	start = start_save = start_in;
-	length = length_save = length_in;
+	start_save = start;
+	length_save = length;
 
 	bitmap_zero(new_page_tables, I915_PDES);
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/8] drm/i915/gtt: Extract unwind to separate function for gen6_alloc_va_range

[Michał Winiarski]

In case of error during allocation we should free the entries allocated
during the current "transaction".
We can't simply reuse the clear_range, since for gen6 we're not
shrinking ppgtt. We can extract unwind to a function though, and use
range rather than bitmaps.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 49e1006..f760c3e 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1917,6 +1917,23 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
 		kunmap_px(ppgtt, pt_vaddr);
 }
 
+static void gen6_ppgtt_unwind_pd(struct i915_address_space *vm,
+				 struct i915_page_directory *pd,
+				 uint64_t start, uint64_t length) {
+
+	struct i915_page_table *pt;
+	uint64_t pde;
+
+	/* We're only using this for cleanup on PT allocation failure,
+	 * and since in this case scratch_pt is already encoded in pde there's
+	 * no need to call gen6_write_pde.
+	 */
+	gen6_for_each_pde(pt, pd, start, length, pde) {
+		pd->page_table[pde] = vm->scratch_pt;
+		free_pt(vm->i915, pt);
+	}
+}
+
 static int gen6_alloc_va_range(struct i915_address_space *vm,
 			       uint64_t start, uint64_t length)
 {
@@ -1954,7 +1971,10 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 		pt = alloc_pt(dev_priv);
 		if (IS_ERR(pt)) {
 			ret = PTR_ERR(pt);
-			goto unwind_out;
+			gen6_ppgtt_unwind_pd(vm, &ppgtt->pd,
+					     start_save, start - start_save);
+			mark_tlbs_dirty(ppgtt);
+			return ret;
 		}
 
 		gen6_initialize_pt(vm, pt);
@@ -1993,17 +2013,6 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 
 	mark_tlbs_dirty(ppgtt);
 	return 0;
-
-unwind_out:
-	for_each_set_bit(pde, new_page_tables, I915_PDES) {
-		struct i915_page_table *pt = ppgtt->pd.page_table[pde];
-
-		ppgtt->pd.page_table[pde] = vm->scratch_pt;
-		free_pt(dev_priv, pt);
-	}
-
-	mark_tlbs_dirty(ppgtt);
-	return ret;
 }
 
 static int gen6_init_scratch(struct i915_address_space *vm)
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/8] drm/i915/gtt: Don't use temp bitmaps to unwind gen8_alloc_va_range

[Michał Winiarski]

We can just operate on ranges and make use of cleanup functions
introduced with ppgtt shrinking.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 82 ++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f760c3e..c6f0708 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1109,6 +1109,7 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_table *pt;
 	uint32_t pde;
+	const uint64_t start_save = start;
 
 	gen8_for_each_pde(pt, pd, start, length, pde) {
 		/* Don't reallocate page tables */
@@ -1119,8 +1120,11 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 		}
 
 		pt = alloc_pt(dev_priv);
-		if (IS_ERR(pt))
-			goto unwind_out;
+		if (IS_ERR(pt)) {
+			gen8_ppgtt_clear_pd(vm, pd, start_save,
+					    start - start_save);
+			return PTR_ERR(pt);
+		}
 
 		gen8_initialize_pt(vm, pt);
 		pd->page_table[pde] = pt;
@@ -1129,12 +1133,6 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 	}
 
 	return 0;
-
-unwind_out:
-	for_each_set_bit(pde, new_pts, I915_PDES)
-		free_pt(dev_priv, pd->page_table[pde]);
-
-	return -ENOMEM;
 }
 
 /**
@@ -1171,6 +1169,7 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
 	struct i915_page_directory *pd;
 	uint32_t pdpe;
 	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
+	const uint64_t start_save = start;
 
 	WARN_ON(!bitmap_empty(new_pds, pdpes));
 
@@ -1179,8 +1178,11 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
 			continue;
 
 		pd = alloc_pd(dev_priv);
-		if (IS_ERR(pd))
-			goto unwind_out;
+		if (IS_ERR(pd)) {
+			gen8_ppgtt_clear_pdp(vm, pdp, start_save,
+					     start - start_save);
+			return PTR_ERR(pd);
+		}
 
 		gen8_initialize_pd(vm, pd);
 		pdp->page_directory[pdpe] = pd;
@@ -1189,12 +1191,6 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
 	}
 
 	return 0;
-
-unwind_out:
-	for_each_set_bit(pdpe, new_pds, pdpes)
-		free_pd(dev_priv, pdp->page_directory[pdpe]);
-
-	return -ENOMEM;
 }
 
 /**
@@ -1223,14 +1219,18 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_directory_pointer *pdp;
 	uint32_t pml4e;
+	const uint64_t start_save = start;
 
 	WARN_ON(!bitmap_empty(new_pdps, GEN8_PML4ES_PER_PML4));
 
 	gen8_for_each_pml4e(pdp, pml4, start, length, pml4e) {
 		if (!test_bit(pml4e, pml4->used_pml4es)) {
 			pdp = alloc_pdp(dev_priv);
-			if (IS_ERR(pdp))
-				goto unwind_out;
+			if (IS_ERR(pdp)) {
+				gen8_ppgtt_clear_pml4(vm, pml4, start_save,
+						      start - start_save);
+				return PTR_ERR(pdp);
+			}
 
 			gen8_initialize_pdp(vm, pdp);
 			pml4->pdps[pml4e] = pdp;
@@ -1243,12 +1243,6 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 	}
 
 	return 0;
-
-unwind_out:
-	for_each_set_bit(pml4e, new_pdps, GEN8_PML4ES_PER_PML4)
-		free_pdp(dev_priv, pml4->pdps[pml4e]);
-
-	return -ENOMEM;
 }
 
 static void
@@ -1295,7 +1289,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	unsigned long *new_page_dirs, *new_page_tables;
-	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_directory *pd;
 	const uint64_t start_save = start;
 	const uint64_t length_save = length;
@@ -1328,8 +1321,12 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	gen8_for_each_pdpe(pd, pdp, start, length, pdpe) {
 		ret = gen8_ppgtt_alloc_pagetabs(vm, pd, start, length,
 						new_page_tables + pdpe * BITS_TO_LONGS(I915_PDES));
-		if (ret)
-			goto err_out;
+		if (ret) {
+			gen8_ppgtt_clear_pdp(vm, pdp, start_save,
+					     start - start_save);
+			mark_tlbs_dirty(ppgtt);
+			return ret;
+		}
 	}
 
 	start = start_save;
@@ -1381,23 +1378,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
 	mark_tlbs_dirty(ppgtt);
 	return 0;
-
-err_out:
-	while (pdpe--) {
-		unsigned long temp;
-
-		for_each_set_bit(temp, new_page_tables + pdpe *
-				BITS_TO_LONGS(I915_PDES), I915_PDES)
-			free_pt(dev_priv,
-				pdp->page_directory[pdpe]->page_table[temp]);
-	}
-
-	for_each_set_bit(pdpe, new_page_dirs, pdpes)
-		free_pd(dev_priv, pdp->page_directory[pdpe]);
-
-	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
-	mark_tlbs_dirty(ppgtt);
-	return ret;
 }
 
 static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
@@ -1410,6 +1390,7 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 	struct i915_page_directory_pointer *pdp;
 	uint64_t pml4e;
 	int ret = 0;
+	const uint64_t start_save = start;
 
 	/* Do the pml4 allocations first, so we don't need to track the newly
 	 * allocated tables below the pdp */
@@ -1431,8 +1412,11 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 		WARN_ON(!pdp);
 
 		ret = gen8_alloc_va_range_3lvl(vm, pdp, start, length);
-		if (ret)
-			goto err_out;
+		if (ret) {
+			gen8_ppgtt_clear_pml4(vm, pml4, start_save,
+					      start - start_save);
+			return ret;
+		}
 
 		gen8_setup_page_directory_pointer(ppgtt, pml4, pdp, pml4e);
 	}
@@ -1441,12 +1425,6 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 		  GEN8_PML4ES_PER_PML4);
 
 	return 0;
-
-err_out:
-	for_each_set_bit(pml4e, new_pdps, GEN8_PML4ES_PER_PML4)
-		gen8_ppgtt_cleanup_3lvl(vm->i915, pml4->pdps[pml4e]);
-
-	return ret;
 }
 
 static int gen8_alloc_va_range(struct i915_address_space *vm,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] drm/i915/gtt: Purge temp bitmaps

[Michał Winiarski]

Temp bitmaps were being used to revert the page tracking structures to
original state after error arises in the middle of the process.
We could just use the range though, and indeed, right now temp bitmaps
are not used for anything useful. We can simply remove them without any
functional changes.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 181 ++++++++----------------------------
 1 file changed, 37 insertions(+), 144 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index c6f0708..7013967 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1088,8 +1088,6 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
  * @pd:	Page directory for this address range.
  * @start:	Starting virtual address to begin allocations.
  * @length:	Size of the allocations.
- * @new_pts:	Bitmap set by function with new allocations. Likely used by the
- *		caller to free on error.
  *
  * Allocate the required number of page tables. Extremely similar to
  * gen8_ppgtt_alloc_page_directories(). The main difference is here we are limited by
@@ -1103,8 +1101,7 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 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)
+				     uint64_t length)
 {
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_table *pt;
@@ -1112,12 +1109,8 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 	const uint64_t start_save = start;
 
 	gen8_for_each_pde(pt, pd, start, length, pde) {
-		/* Don't reallocate page tables */
-		if (test_bit(pde, pd->used_pdes)) {
-			/* Scratch is never allocated this way */
-			WARN_ON(pt == vm->scratch_pt);
+		if (test_bit(pde, pd->used_pdes))
 			continue;
-		}
 
 		pt = alloc_pt(dev_priv);
 		if (IS_ERR(pt)) {
@@ -1128,7 +1121,7 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 
 		gen8_initialize_pt(vm, pt);
 		pd->page_table[pde] = pt;
-		__set_bit(pde, new_pts);
+		__set_bit(pde, pd->used_pdes);
 		trace_i915_page_table_entry_alloc(vm, pde, start, GEN8_PDE_SHIFT);
 	}
 
@@ -1141,14 +1134,11 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
  * @pdp:	Page directory pointer for this address range.
  * @start:	Starting virtual address to begin allocations.
  * @length:	Size of the allocations.
- * @new_pds:	Bitmap set by function with new allocations. Likely used by the
- *		caller to free on error.
  *
  * Allocate the required number of page directories starting at the pde index of
  * @start, and ending at the pde index @start + @length. This function will skip
  * over already allocated page directories within the range, and only allocate
- * new ones, setting the appropriate pointer within the pdp as well as the
- * correct position in the bitmap @new_pds.
+ * new ones, setting the appropriate pointer within the pdp.
  *
  * The function will only allocate the pages within the range for a give page
  * directory pointer. In other words, if @start + @length straddles a virtually
@@ -1162,17 +1152,13 @@ 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)
+				  uint64_t length)
 {
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_directory *pd;
 	uint32_t pdpe;
-	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
 	const uint64_t start_save = start;
 
-	WARN_ON(!bitmap_empty(new_pds, pdpes));
-
 	gen8_for_each_pdpe(pd, pdp, start, length, pdpe) {
 		if (test_bit(pdpe, pdp->used_pdpes))
 			continue;
@@ -1186,7 +1172,7 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
 
 		gen8_initialize_pd(vm, pd);
 		pdp->page_directory[pdpe] = pd;
-		__set_bit(pdpe, new_pds);
+		__set_bit(pdpe, pdp->used_pdpes);
 		trace_i915_page_directory_entry_alloc(vm, pdpe, start, GEN8_PDPE_SHIFT);
 	}
 
@@ -1199,8 +1185,6 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
  * @pml4:	Page map level 4 for this address range.
  * @start:	Starting virtual address to begin allocations.
  * @length:	Size of the allocations.
- * @new_pdps:	Bitmap set by function with new allocations. Likely used by the
- *		caller to free on error.
  *
  * Allocate the required number of page directory pointers. Extremely similar to
  * gen8_ppgtt_alloc_page_directories() and gen8_ppgtt_alloc_pagetabs().
@@ -1213,73 +1197,34 @@ static int
 gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 				  struct i915_pml4 *pml4,
 				  uint64_t start,
-				  uint64_t length,
-				  unsigned long *new_pdps)
+				  uint64_t length)
 {
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_page_directory_pointer *pdp;
 	uint32_t pml4e;
 	const uint64_t start_save = start;
 
-	WARN_ON(!bitmap_empty(new_pdps, GEN8_PML4ES_PER_PML4));
-
 	gen8_for_each_pml4e(pdp, pml4, start, length, pml4e) {
-		if (!test_bit(pml4e, pml4->used_pml4es)) {
-			pdp = alloc_pdp(dev_priv);
-			if (IS_ERR(pdp)) {
-				gen8_ppgtt_clear_pml4(vm, pml4, start_save,
-						      start - start_save);
-				return PTR_ERR(pdp);
-			}
+		if (test_bit(pml4e, pml4->used_pml4es))
+			continue;
 
-			gen8_initialize_pdp(vm, pdp);
-			pml4->pdps[pml4e] = pdp;
-			__set_bit(pml4e, new_pdps);
-			trace_i915_page_directory_pointer_entry_alloc(vm,
-								      pml4e,
-								      start,
-								      GEN8_PML4E_SHIFT);
+		pdp = alloc_pdp(dev_priv);
+		if (IS_ERR(pdp)) {
+			gen8_ppgtt_clear_pml4(vm, pml4, start_save,
+					      start - start_save);
+			return PTR_ERR(pdp);
 		}
-	}
-
-	return 0;
-}
-
-static void
-free_gen8_temp_bitmaps(unsigned long *new_pds, unsigned long *new_pts)
-{
-	kfree(new_pts);
-	kfree(new_pds);
-}
-
-/* Fills in the page directory bitmap, and the array of page tables bitmap. Both
- * of these are based on the number of PDPEs in the system.
- */
-static
-int __must_check alloc_gen8_temp_bitmaps(unsigned long **new_pds,
-					 unsigned long **new_pts,
-					 uint32_t pdpes)
-{
-	unsigned long *pds;
-	unsigned long *pts;
-
-	pds = kcalloc(BITS_TO_LONGS(pdpes), sizeof(unsigned long), GFP_TEMPORARY);
-	if (!pds)
-		return -ENOMEM;
 
-	pts = kcalloc(pdpes, BITS_TO_LONGS(I915_PDES) * sizeof(unsigned long),
-		      GFP_TEMPORARY);
-	if (!pts)
-		goto err_out;
-
-	*new_pds = pds;
-	*new_pts = pts;
+		gen8_initialize_pdp(vm, pdp);
+		pml4->pdps[pml4e] = pdp;
+		__set_bit(pml4e, pml4->used_pml4es);
+		trace_i915_page_directory_pointer_entry_alloc(vm,
+							      pml4e,
+							      start,
+							      GEN8_PML4E_SHIFT);
+	}
 
 	return 0;
-
-err_out:
-	free_gen8_temp_bitmaps(pds, pts);
-	return -ENOMEM;
 }
 
 static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
@@ -1288,12 +1233,10 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 				    uint64_t length)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
-	unsigned long *new_page_dirs, *new_page_tables;
 	struct i915_page_directory *pd;
 	const uint64_t start_save = start;
 	const uint64_t length_save = length;
 	uint32_t pdpe;
-	uint32_t pdpes = I915_PDPES_PER_PDP(dev_priv);
 	int ret;
 
 	/* Wrap is never okay since we can only represent 48b, and we don't
@@ -1305,22 +1248,14 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 	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;
-
 	/* Do the allocations first so we can easily bail out */
-	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);
+	ret = gen8_ppgtt_alloc_page_directories(vm, pdp, start, length);
+	if (ret)
 		return ret;
-	}
 
 	/* For every page directory referenced, allocate page tables */
 	gen8_for_each_pdpe(pd, pdp, start, length, pdpe) {
-		ret = gen8_ppgtt_alloc_pagetabs(vm, pd, start, length,
-						new_page_tables + pdpe * BITS_TO_LONGS(I915_PDES));
+		ret = gen8_ppgtt_alloc_pagetabs(vm, pd, start, length);
 		if (ret) {
 			gen8_ppgtt_clear_pdp(vm, pdp, start_save,
 					     start - start_save);
@@ -1355,9 +1290,6 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 				   gen8_pte_index(pd_start),
 				   gen8_pte_count(pd_start, pd_len));
 
-			/* 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);
@@ -1371,11 +1303,9 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 		}
 
 		kunmap_px(ppgtt, page_directory);
-		__set_bit(pdpe, pdp->used_pdpes);
 		gen8_setup_page_directory(ppgtt, pdp, pd, pdpe);
 	}
 
-	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
 	mark_tlbs_dirty(ppgtt);
 	return 0;
 }
@@ -1385,26 +1315,20 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 				    uint64_t start,
 				    uint64_t length)
 {
-	DECLARE_BITMAP(new_pdps, GEN8_PML4ES_PER_PML4);
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct i915_page_directory_pointer *pdp;
 	uint64_t pml4e;
-	int ret = 0;
+	int ret;
 	const uint64_t start_save = 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);
-
 	/* The pagedirectory and pagetable allocations are done in the shared 3
 	 * and 4 level code. Just allocate the pdps.
 	 */
-	ret = gen8_ppgtt_alloc_page_dirpointers(vm, pml4, start, length,
-						new_pdps);
+	ret = gen8_ppgtt_alloc_page_dirpointers(vm, pml4, start, length);
 	if (ret)
 		return ret;
 
-	WARN(bitmap_weight(new_pdps, GEN8_PML4ES_PER_PML4) > 2,
+	WARN(length > (1ULL << GEN8_PML4E_SHIFT),
 	     "The allocation has spanned more than 512GB. "
 	     "It is highly likely this is incorrect.");
 
@@ -1421,9 +1345,6 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 		gen8_setup_page_directory_pointer(ppgtt, pml4, pdp, pml4e);
 	}
 
-	bitmap_or(pml4->used_pml4es, new_pdps, pml4->used_pml4es,
-		  GEN8_PML4ES_PER_PML4);
-
 	return 0;
 }
 
@@ -1522,27 +1443,18 @@ static void gen8_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 
 static int gen8_preallocate_top_level_pdps(struct i915_hw_ppgtt *ppgtt)
 {
-	unsigned long *new_page_dirs, *new_page_tables;
-	uint32_t pdpes = I915_PDPES_PER_PDP(to_i915(ppgtt->base.dev));
+	const uint64_t start = 0;
+	const uint64_t length = 1ULL << 32;
 	int ret;
 
-	/* We allocate temp bitmap for page tables for no gain
-	 * but as this is for init only, lets keep the things simple
-	 */
-	ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
-	if (ret)
-		return ret;
-
 	/* Allocate for all pdps regardless of how the ppgtt
 	 * was defined.
 	 */
 	ret = gen8_ppgtt_alloc_page_directories(&ppgtt->base, &ppgtt->pdp,
-						0, 1ULL << 32,
-						new_page_dirs);
+						start, length);
 	if (!ret)
-		*ppgtt->pdp.used_pdpes = *new_page_dirs;
-
-	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
+		bitmap_set(ppgtt->pdp.used_pdpes, gen8_pdpe_index(start),
+			   i915_pte_count(start, length, GEN8_PDPE_SHIFT));
 
 	return ret;
 }
@@ -1915,7 +1827,6 @@ static void gen6_ppgtt_unwind_pd(struct i915_address_space *vm,
 static int gen6_alloc_va_range(struct i915_address_space *vm,
 			       uint64_t start, uint64_t length)
 {
-	DECLARE_BITMAP(new_page_tables, I915_PDES);
 	struct drm_i915_private *dev_priv = vm->i915;
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
@@ -1930,8 +1841,6 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 	start_save = start;
 	length_save = length;
 
-	bitmap_zero(new_page_tables, I915_PDES);
-
 	/* The allocation is done in two stages so that we can bail out with
 	 * minimal amount of pain. The first stage finds new page tables that
 	 * need allocation. The second stage marks use ptes within the page
@@ -1954,37 +1863,21 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 			mark_tlbs_dirty(ppgtt);
 			return ret;
 		}
-
-		gen6_initialize_pt(vm, pt);
-
-		ppgtt->pd.page_table[pde] = pt;
-		__set_bit(pde, new_page_tables);
 		trace_i915_page_table_entry_alloc(vm, pde, start, GEN6_PDE_SHIFT);
-	}
 
-	start = start_save;
-	length = length_save;
-
-	gen6_for_each_pde(pt, &ppgtt->pd, start, length, pde) {
-		DECLARE_BITMAP(tmp_bitmap, GEN6_PTES);
-
-		bitmap_zero(tmp_bitmap, GEN6_PTES);
-		bitmap_set(tmp_bitmap, gen6_pte_index(start),
+		gen6_initialize_pt(vm, pt);
+		bitmap_set(pt->used_ptes, gen6_pte_index(start),
 			   gen6_pte_count(start, length));
 
-		if (__test_and_clear_bit(pde, new_page_tables))
-			gen6_write_pde(&ppgtt->pd, pde, pt);
+		ppgtt->pd.page_table[pde] = pt;
+		gen6_write_pde(&ppgtt->pd, pde, pt);
 
 		trace_i915_page_table_entry_map(vm, pde, pt,
 					 gen6_pte_index(start),
 					 gen6_pte_count(start, length),
 					 GEN6_PTES);
-		bitmap_or(pt->used_ptes, tmp_bitmap, pt->used_ptes,
-				GEN6_PTES);
 	}
 
-	WARN_ON(!bitmap_empty(new_page_tables, I915_PDES));
-
 	/* Make sure write is complete before other code can use this page
 	 * table. Also require for WC mapped PTEs */
 	readl(ggtt->gsm);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/8] drm/i915: Prepare i915_page_table_entry_map tracepoint for bitmap purge

[Michał Winiarski]

There's no fast way to provide the info on which entries are present for
this tracepoint once bitmaps dissapear. Let's just pass the counter
instead.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 |  4 ++--
 drivers/gpu/drm/i915/i915_trace.h   | 21 ++++++---------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7013967..a7d6f78 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1296,7 +1296,7 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 			trace_i915_page_table_entry_map(&ppgtt->base, pde, pt,
 							gen8_pte_index(start),
 							gen8_pte_count(start, length),
-							GEN8_PTES);
+							bitmap_weight(pt->used_ptes, GEN8_PTES));
 
 			/* NB: We haven't yet mapped ptes to pages. At this
 			 * point we're still relying on insert_entries() */
@@ -1875,7 +1875,7 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 		trace_i915_page_table_entry_map(vm, pde, pt,
 					 gen6_pte_index(start),
 					 gen6_pte_count(start, length),
-					 GEN6_PTES);
+					 bitmap_weight(pt->used_ptes, GEN6_PTES));
 	}
 
 	/* Make sure write is complete before other code can use this page
diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
index 18ae37c..86a4e39 100644
--- a/drivers/gpu/drm/i915/i915_trace.h
+++ b/drivers/gpu/drm/i915/i915_trace.h
@@ -238,22 +238,17 @@ DEFINE_EVENT_PRINT(i915_px_entry, i915_page_directory_pointer_entry_alloc,
 			     __entry->vm, __entry->px, __entry->start, __entry->end)
 );
 
-/* Avoid extra math because we only support two sizes. The format is defined by
- * bitmap_scnprintf. Each 32 bits is 8 HEX digits followed by comma */
-#define TRACE_PT_SIZE(bits) \
-	((((bits) == 1024) ? 288 : 144) + 1)
-
 DECLARE_EVENT_CLASS(i915_page_table_entry_update,
 	TP_PROTO(struct i915_address_space *vm, u32 pde,
-		 struct i915_page_table *pt, u32 first, u32 count, u32 bits),
-	TP_ARGS(vm, pde, pt, first, count, bits),
+		 struct i915_page_table *pt, u32 first, u32 count, u32 num),
+	TP_ARGS(vm, pde, pt, first, count, num),
 
 	TP_STRUCT__entry(
 		__field(struct i915_address_space *, vm)
 		__field(u32, pde)
 		__field(u32, first)
 		__field(u32, last)
-		__dynamic_array(char, cur_ptes, TRACE_PT_SIZE(bits))
+		__field(u32, num)
 	),
 
 	TP_fast_assign(
@@ -261,16 +256,12 @@ DECLARE_EVENT_CLASS(i915_page_table_entry_update,
 		__entry->pde = pde;
 		__entry->first = first;
 		__entry->last = first + count - 1;
-		scnprintf(__get_str(cur_ptes),
-			  TRACE_PT_SIZE(bits),
-			  "%*pb",
-			  bits,
-			  pt->used_ptes);
+		__entry->num = num;
 	),
 
-	TP_printk("vm=%p, pde=%d, updating %u:%u\t%s",
+	TP_printk("vm=%p, pde=%d, updating %u:%u\t%u",
 		  __entry->vm, __entry->pde, __entry->last, __entry->first,
-		  __get_str(cur_ptes))
+		  __entry->num)
 );
 
 DEFINE_EVENT(i915_page_table_entry_update, i915_page_table_entry_map,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/8] drm/i915/gtt: Purge page tracking bitmaps

[Michał Winiarski]

All of the page tracking structures contain a bitmap holding currently
used entries.
It's redundant, since we can just inspect the pointer.
The only actual place where we're taking advantage of bitmaps is during
ppgtt cleanup - since we're able to reduce the number of iterations.
Still, we can improve that in the following commits, and iterate over
the full range here.

Let's replace the bitmaps with a simple counter.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 138 +++++++++++++-----------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  10 +--
 2 files changed, 54 insertions(+), 94 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a7d6f78..50d4861 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -426,38 +426,26 @@ static void cleanup_scratch_page(struct drm_i915_private *dev_priv,
 static struct i915_page_table *alloc_pt(struct drm_i915_private *dev_priv)
 {
 	struct i915_page_table *pt;
-	const size_t count = INTEL_GEN(dev_priv) >= 8 ? GEN8_PTES : GEN6_PTES;
 	int ret = -ENOMEM;
 
 	pt = kzalloc(sizeof(*pt), GFP_KERNEL);
 	if (!pt)
 		return ERR_PTR(-ENOMEM);
 
-	pt->used_ptes = kcalloc(BITS_TO_LONGS(count), sizeof(*pt->used_ptes),
-				GFP_KERNEL);
-
-	if (!pt->used_ptes)
-		goto fail_bitmap;
-
 	ret = setup_px(dev_priv, pt);
-	if (ret)
-		goto fail_page_m;
+	if (ret) {
+		kfree(pt);
+		return ERR_PTR(ret);
+	}
 
 	return pt;
 
-fail_page_m:
-	kfree(pt->used_ptes);
-fail_bitmap:
-	kfree(pt);
-
-	return ERR_PTR(ret);
 }
 
 static void free_pt(struct drm_i915_private *dev_priv,
 		    struct i915_page_table *pt)
 {
 	cleanup_px(dev_priv, pt);
-	kfree(pt->used_ptes);
 	kfree(pt);
 }
 
@@ -494,23 +482,13 @@ static struct i915_page_directory *alloc_pd(struct drm_i915_private *dev_priv)
 	if (!pd)
 		return ERR_PTR(-ENOMEM);
 
-	pd->used_pdes = kcalloc(BITS_TO_LONGS(I915_PDES),
-				sizeof(*pd->used_pdes), GFP_KERNEL);
-	if (!pd->used_pdes)
-		goto fail_bitmap;
-
 	ret = setup_px(dev_priv, pd);
-	if (ret)
-		goto fail_page_m;
+	if (ret) {
+		kfree(pd);
+		return ERR_PTR(ret);
+	}
 
 	return pd;
-
-fail_page_m:
-	kfree(pd->used_pdes);
-fail_bitmap:
-	kfree(pd);
-
-	return ERR_PTR(ret);
 }
 
 static void free_pd(struct drm_i915_private *dev_priv,
@@ -518,7 +496,6 @@ static void free_pd(struct drm_i915_private *dev_priv,
 {
 	if (px_page(pd)) {
 		cleanup_px(dev_priv, pd);
-		kfree(pd->used_pdes);
 		kfree(pd);
 	}
 }
@@ -538,28 +515,16 @@ static int __pdp_init(struct drm_i915_private *dev_priv,
 {
 	size_t pdpes = I915_PDPES_PER_PDP(dev_priv);
 
-	pdp->used_pdpes = kcalloc(BITS_TO_LONGS(pdpes),
-				  sizeof(unsigned long),
-				  GFP_KERNEL);
-	if (!pdp->used_pdpes)
-		return -ENOMEM;
-
 	pdp->page_directory = kcalloc(pdpes, sizeof(*pdp->page_directory),
 				      GFP_KERNEL);
-	if (!pdp->page_directory) {
-		kfree(pdp->used_pdpes);
-		/* the PDP might be the statically allocated top level. Keep it
-		 * as clean as possible */
-		pdp->used_pdpes = NULL;
+	if (!pdp->page_directory)
 		return -ENOMEM;
-	}
 
 	return 0;
 }
 
 static void __pdp_fini(struct i915_page_directory_pointer *pdp)
 {
-	kfree(pdp->used_pdpes);
 	kfree(pdp->page_directory);
 	pdp->page_directory = NULL;
 }
@@ -733,9 +698,9 @@ static bool gen8_ppgtt_clear_pt(struct i915_address_space *vm,
 
 	GEM_BUG_ON(pte_end > GEN8_PTES);
 
-	bitmap_clear(pt->used_ptes, pte, num_entries);
+	pt->num_ptes -= num_entries;
 
-	if (bitmap_empty(pt->used_ptes, GEN8_PTES))
+	if (pt->num_ptes == 0)
 		return true;
 
 	pt_vaddr = kmap_px(pt);
@@ -768,15 +733,16 @@ static bool gen8_ppgtt_clear_pd(struct i915_address_space *vm,
 			break;
 
 		if (gen8_ppgtt_clear_pt(vm, pt, start, length)) {
-			__clear_bit(pde, pd->used_pdes);
+			pd->num_pdes--;
 			pde_vaddr = kmap_px(pd);
 			pde_vaddr[pde] = scratch_pde;
 			kunmap_px(ppgtt, pde_vaddr);
 			free_pt(vm->i915, pt);
+			pd->page_table[pde] = NULL;
 		}
 	}
 
-	if (bitmap_empty(pd->used_pdes, I915_PDES))
+	if (pd->num_pdes == 0)
 		return true;
 
 	return false;
@@ -802,19 +768,20 @@ static bool gen8_ppgtt_clear_pdp(struct i915_address_space *vm,
 			break;
 
 		if (gen8_ppgtt_clear_pd(vm, pd, start, length)) {
-			__clear_bit(pdpe, pdp->used_pdpes);
+			pdp->num_pdpes--;
 			if (USES_FULL_48BIT_PPGTT(dev_priv)) {
 				pdpe_vaddr = kmap_px(pdp);
 				pdpe_vaddr[pdpe] = scratch_pdpe;
 				kunmap_px(ppgtt, pdpe_vaddr);
 			}
 			free_pd(vm->i915, pd);
+			pdp->page_directory[pdpe] = NULL;
 		}
 	}
 
 	mark_tlbs_dirty(ppgtt);
 
-	if (bitmap_empty(pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)))
+	if (pdp->num_pdpes == 0)
 		return true;
 
 	return false;
@@ -843,11 +810,12 @@ static void gen8_ppgtt_clear_pml4(struct i915_address_space *vm,
 			break;
 
 		if (gen8_ppgtt_clear_pdp(vm, pdp, start, length)) {
-			__clear_bit(pml4e, pml4->used_pml4es);
+			pml4->num_pml4es--;
 			pml4e_vaddr = kmap_px(pml4);
 			pml4e_vaddr[pml4e] = scratch_pml4e;
 			kunmap_px(ppgtt, pml4e_vaddr);
 			free_pdp(vm->i915, pdp);
+			pml4->pdps[pml4e] = NULL;
 		}
 	}
 }
@@ -938,12 +906,11 @@ static void gen8_free_page_tables(struct drm_i915_private *dev_priv,
 	if (!px_page(pd))
 		return;
 
-	for_each_set_bit(i, pd->used_pdes, I915_PDES) {
-		if (WARN_ON(!pd->page_table[i]))
-			continue;
-
-		free_pt(dev_priv, pd->page_table[i]);
-		pd->page_table[i] = NULL;
+	for (i = 0; i < I915_PDES; i++) {
+		if (pd->page_table[i]) {
+			free_pt(dev_priv, pd->page_table[i]);
+			pd->page_table[i] = NULL;
+		}
 	}
 }
 
@@ -1040,12 +1007,11 @@ static void gen8_ppgtt_cleanup_3lvl(struct drm_i915_private *dev_priv,
 {
 	int i;
 
-	for_each_set_bit(i, pdp->used_pdpes, I915_PDPES_PER_PDP(dev_priv)) {
-		if (WARN_ON(!pdp->page_directory[i]))
-			continue;
-
-		gen8_free_page_tables(dev_priv, pdp->page_directory[i]);
-		free_pd(dev_priv, pdp->page_directory[i]);
+	for (i = 0; i < I915_PDPES_PER_PDP(dev_priv); i++) {
+		if (pdp->page_directory[i]) {
+			gen8_free_page_tables(dev_priv, pdp->page_directory[i]);
+			pdp->page_directory[i] = NULL;
+		}
 	}
 
 	free_pdp(dev_priv, pdp);
@@ -1056,11 +1022,11 @@ static void gen8_ppgtt_cleanup_4lvl(struct drm_i915_private *dev_priv,
 {
 	int i;
 
-	for_each_set_bit(i, pml4->used_pml4es, GEN8_PML4ES_PER_PML4) {
-		if (WARN_ON(!pml4->pdps[i]))
-			continue;
-
-		gen8_ppgtt_cleanup_3lvl(dev_priv, pml4->pdps[i]);
+	for (i = 0; i < GEN8_PML4ES_PER_PML4; i++) {
+		if (pml4->pdps[i]) {
+			gen8_ppgtt_cleanup_3lvl(dev_priv, pml4->pdps[i]);
+			pml4->pdps[i] = NULL;
+		}
 	}
 
 	cleanup_px(dev_priv, pml4);
@@ -1109,7 +1075,7 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 	const uint64_t start_save = start;
 
 	gen8_for_each_pde(pt, pd, start, length, pde) {
-		if (test_bit(pde, pd->used_pdes))
+		if (pd->page_table[pde])
 			continue;
 
 		pt = alloc_pt(dev_priv);
@@ -1121,7 +1087,7 @@ static int gen8_ppgtt_alloc_pagetabs(struct i915_address_space *vm,
 
 		gen8_initialize_pt(vm, pt);
 		pd->page_table[pde] = pt;
-		__set_bit(pde, pd->used_pdes);
+		pd->num_pdes++;
 		trace_i915_page_table_entry_alloc(vm, pde, start, GEN8_PDE_SHIFT);
 	}
 
@@ -1160,7 +1126,7 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
 	const uint64_t start_save = start;
 
 	gen8_for_each_pdpe(pd, pdp, start, length, pdpe) {
-		if (test_bit(pdpe, pdp->used_pdpes))
+		if (pdp->page_directory[pdpe])
 			continue;
 
 		pd = alloc_pd(dev_priv);
@@ -1172,7 +1138,7 @@ gen8_ppgtt_alloc_page_directories(struct i915_address_space *vm,
 
 		gen8_initialize_pd(vm, pd);
 		pdp->page_directory[pdpe] = pd;
-		__set_bit(pdpe, pdp->used_pdpes);
+		pdp->num_pdpes++;
 		trace_i915_page_directory_entry_alloc(vm, pdpe, start, GEN8_PDPE_SHIFT);
 	}
 
@@ -1205,7 +1171,7 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 	const uint64_t start_save = start;
 
 	gen8_for_each_pml4e(pdp, pml4, start, length, pml4e) {
-		if (test_bit(pml4e, pml4->used_pml4es))
+		if (pml4->pdps[pml4e])
 			continue;
 
 		pdp = alloc_pdp(dev_priv);
@@ -1217,7 +1183,7 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 
 		gen8_initialize_pdp(vm, pdp);
 		pml4->pdps[pml4e] = pdp;
-		__set_bit(pml4e, pml4->used_pml4es);
+		pml4->num_pml4es++;
 		trace_i915_page_directory_pointer_entry_alloc(vm,
 							      pml4e,
 							      start,
@@ -1286,9 +1252,7 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 			WARN_ON(!gen8_pte_count(pd_start, pd_len));
 
 			/* Set our used ptes within the page table */
-			bitmap_set(pt->used_ptes,
-				   gen8_pte_index(pd_start),
-				   gen8_pte_count(pd_start, pd_len));
+			pt->num_ptes += gen8_pte_count(pd_start, pd_len);
 
 			/* Map the PDE to the page table */
 			page_directory[pde] = gen8_pde_encode(px_dma(pt),
@@ -1296,7 +1260,7 @@ static int gen8_alloc_va_range_3lvl(struct i915_address_space *vm,
 			trace_i915_page_table_entry_map(&ppgtt->base, pde, pt,
 							gen8_pte_index(start),
 							gen8_pte_count(start, length),
-							bitmap_weight(pt->used_ptes, GEN8_PTES));
+							pt->num_ptes);
 
 			/* NB: We haven't yet mapped ptes to pages. At this
 			 * point we're still relying on insert_entries() */
@@ -1373,7 +1337,7 @@ static void gen8_dump_pdp(struct i915_page_directory_pointer *pdp,
 		uint64_t pd_start = start;
 		uint32_t pde;
 
-		if (!test_bit(pdpe, pdp->used_pdpes))
+		if (!pdp->page_directory[pdpe])
 			continue;
 
 		seq_printf(m, "\tPDPE #%d\n", pdpe);
@@ -1381,7 +1345,7 @@ static void gen8_dump_pdp(struct i915_page_directory_pointer *pdp,
 			uint32_t  pte;
 			gen8_pte_t *pt_vaddr;
 
-			if (!test_bit(pde, pd->used_pdes))
+			if (!pd->page_table[pde])
 				continue;
 
 			pt_vaddr = kmap_px(pt);
@@ -1432,7 +1396,7 @@ static void gen8_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 		struct i915_page_directory_pointer *pdp;
 
 		gen8_for_each_pml4e(pdp, pml4, start, length, pml4e) {
-			if (!test_bit(pml4e, pml4->used_pml4es))
+			if (!pml4->pdps[pml4e])
 				continue;
 
 			seq_printf(m, "    PML4E #%llu\n", pml4e);
@@ -1452,9 +1416,6 @@ static int gen8_preallocate_top_level_pdps(struct i915_hw_ppgtt *ppgtt)
 	 */
 	ret = gen8_ppgtt_alloc_page_directories(&ppgtt->base, &ppgtt->pdp,
 						start, length);
-	if (!ret)
-		bitmap_set(ppgtt->pdp.used_pdpes, gen8_pdpe_index(start),
-			   i915_pte_count(start, length, GEN8_PDPE_SHIFT));
 
 	return ret;
 }
@@ -1848,12 +1809,12 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 	 */
 	gen6_for_each_pde(pt, &ppgtt->pd, start, length, pde) {
 		if (pt != vm->scratch_pt) {
-			WARN_ON(bitmap_empty(pt->used_ptes, GEN6_PTES));
+			WARN_ON(pt->num_ptes == 0);
 			continue;
 		}
 
 		/* We've already allocated a page table */
-		WARN_ON(!bitmap_empty(pt->used_ptes, GEN6_PTES));
+		WARN_ON(pt->num_ptes != 0);
 
 		pt = alloc_pt(dev_priv);
 		if (IS_ERR(pt)) {
@@ -1866,8 +1827,7 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 		trace_i915_page_table_entry_alloc(vm, pde, start, GEN6_PDE_SHIFT);
 
 		gen6_initialize_pt(vm, pt);
-		bitmap_set(pt->used_ptes, gen6_pte_index(start),
-			   gen6_pte_count(start, length));
+		pt->num_ptes += gen6_pte_count(start, length);
 
 		ppgtt->pd.page_table[pde] = pt;
 		gen6_write_pde(&ppgtt->pd, pde, pt);
@@ -1875,7 +1835,7 @@ static int gen6_alloc_va_range(struct i915_address_space *vm,
 		trace_i915_page_table_entry_map(vm, pde, pt,
 					 gen6_pte_index(start),
 					 gen6_pte_count(start, length),
-					 bitmap_weight(pt->used_ptes, GEN6_PTES));
+					 pt->num_ptes);
 	}
 
 	/* Make sure write is complete before other code can use this page
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 8965bbb..1fc65ac 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -193,27 +193,27 @@ struct i915_page_dma {
 struct i915_page_table {
 	struct i915_page_dma base;
 
-	unsigned long *used_ptes;
+	unsigned int num_ptes;
 };
 
 struct i915_page_directory {
 	struct i915_page_dma base;
 
-	unsigned long *used_pdes;
+	unsigned int num_pdes;
 	struct i915_page_table *page_table[I915_PDES]; /* PDEs */
 };
 
 struct i915_page_directory_pointer {
 	struct i915_page_dma base;
 
-	unsigned long *used_pdpes;
+	unsigned int num_pdpes;
 	struct i915_page_directory **page_directory;
 };
 
 struct i915_pml4 {
 	struct i915_page_dma base;
 
-	DECLARE_BITMAP(used_pml4es, GEN8_PML4ES_PER_PML4);
+	unsigned int num_pml4es;
 	struct i915_page_directory_pointer *pdps[GEN8_PML4ES_PER_PML4];
 };
 
@@ -477,7 +477,7 @@ static inline size_t gen8_pte_count(uint64_t address, uint64_t length)
 static inline dma_addr_t
 i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 {
-	return test_bit(n, ppgtt->pdp.used_pdpes) ?
+	return ppgtt->pdp.page_directory[n] ?
 		px_dma(ppgtt->pdp.page_directory[n]) :
 		px_dma(ppgtt->base.scratch_pd);
 }
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/8] drm/i915: Clear range when unbinding closed vma

[Michał Winiarski]

Since we've introduced ppgtt shrinking, most of the vmas should already
be unbounded when ppgtt is being released.
This allows us to take care of leftovers and assert that all vmas are
unbounded, removing the iteration during ppgtt release.

Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
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 | 36 ++----------------------------------
 drivers/gpu/drm/i915/i915_vma.c     |  7 +++----
 2 files changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 50d4861..4326dcc 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -898,22 +898,6 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 	}
 }
 
-static void gen8_free_page_tables(struct drm_i915_private *dev_priv,
-				  struct i915_page_directory *pd)
-{
-	int i;
-
-	if (!px_page(pd))
-		return;
-
-	for (i = 0; i < I915_PDES; i++) {
-		if (pd->page_table[i]) {
-			free_pt(dev_priv, pd->page_table[i]);
-			pd->page_table[i] = NULL;
-		}
-	}
-}
-
 static int gen8_init_scratch(struct i915_address_space *vm)
 {
 	struct drm_i915_private *dev_priv = vm->i915;
@@ -1005,30 +989,14 @@ static void gen8_free_scratch(struct i915_address_space *vm)
 static void gen8_ppgtt_cleanup_3lvl(struct drm_i915_private *dev_priv,
 				    struct i915_page_directory_pointer *pdp)
 {
-	int i;
-
-	for (i = 0; i < I915_PDPES_PER_PDP(dev_priv); i++) {
-		if (pdp->page_directory[i]) {
-			gen8_free_page_tables(dev_priv, pdp->page_directory[i]);
-			pdp->page_directory[i] = NULL;
-		}
-	}
-
+	WARN_ON(pdp->num_pdpes != 0);
 	free_pdp(dev_priv, pdp);
 }
 
 static void gen8_ppgtt_cleanup_4lvl(struct drm_i915_private *dev_priv,
 				    struct i915_pml4 *pml4)
 {
-	int i;
-
-	for (i = 0; i < GEN8_PML4ES_PER_PML4; i++) {
-		if (pml4->pdps[i]) {
-			gen8_ppgtt_cleanup_3lvl(dev_priv, pml4->pdps[i]);
-			pml4->pdps[i] = NULL;
-		}
-	}
-
+	WARN_ON(pml4->num_pml4es != 0);
 	cleanup_px(dev_priv, pml4);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 37c3eeb..248875f 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -603,10 +603,9 @@ int i915_vma_unbind(struct i915_vma *vma)
 		vma->flags &= ~I915_VMA_CAN_FENCE;
 	}
 
-	if (likely(!vma->vm->closed)) {
-		trace_i915_vma_unbind(vma);
-		vma->vm->unbind_vma(vma);
-	}
+	trace_i915_vma_unbind(vma);
+	vma->vm->unbind_vma(vma);
+
 	vma->flags &= ~(I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND);
 
 	drm_mm_remove_node(&vma->node);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915/gtt: Don't pass ppgtt to ppgtt_cleanup_4lvl

[Chris Wilson]

On Mon, Dec 12, 2016 at 12:44:10PM +0100, Michał Winiarski wrote:
> It's not operating on ppgtt, and it also makes things consistent with
> analogous ppgtt_cleanup_3lvl function.

Nah, if you've looked at my series accomplishing the same thing and
more, you'll see that we do want to pass struct i915_address_space to
cleanup_page_dma.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 8/8] drm/i915: Clear range when unbinding closed vma

[Chris Wilson]

On Mon, Dec 12, 2016 at 12:48:45PM +0100, Michał Winiarski wrote:
> Since we've introduced ppgtt shrinking, most of the vmas should already
> be unbounded when ppgtt is being released.
> This allows us to take care of leftovers and assert that all vmas are
> unbounded, removing the iteration during ppgtt release.

No. The ppgtt is dead, we don't need to unbind piecemeal under the
struct_mutex. This is also used to avoid writes during suspend/resume.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx