[PATCH 1/2] drm/i915: Support per-PPGTT address space mode

[PATCH 1/2] drm/i915: Support per-PPGTT address space mode

[Matthew Auld]

From: "Wang, Zhi A" <zhi.a.wang@intel.com>

Previously the address space mode of each PPGTT instance is hard-coded to
host system configuration, e.g. if the host system is configured to use
48bit full PPGTT, then address space mode of all PPGTT instances is 48bit.

Per Daniel and Kevin's advice, GVT-g will leverage i915 PPGTT interface to
populate its shadow PPGTT page table. Under GVT-g the address space mode
of PPGTT instances could be various, some guest may use 32bit, some guest
may use 48bit.

We store the address space mode into i915_hw_ppgtt, and let i915 page
table manipulation routines / LRC context population routines read the
address space mode from it instead of the system configuration.

v2:
(Matthew Auld)
  - rebase on latest -nightly
  - prefer i915_vm_to_ppgtt instead of container_of
  - initialise address_space_mode _before_ we attempt to init
    scratch, otherwise breakage will ensue

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 84 +++++++++++++++++++------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  3 ++
 drivers/gpu/drm/i915/intel_lrc.c    |  6 +--
 3 files changed, 50 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0d666b3..9919fa6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -564,22 +564,22 @@ static void __pdp_fini(struct i915_page_directory_pointer *pdp)
 }
 
 static struct
-i915_page_directory_pointer *alloc_pdp(struct drm_device *dev)
+i915_page_directory_pointer *alloc_pdp(struct i915_hw_ppgtt *ppgtt)
 {
 	struct i915_page_directory_pointer *pdp;
 	int ret = -ENOMEM;
 
-	WARN_ON(!USES_FULL_48BIT_PPGTT(dev));
+	WARN_ON(!IS_48BIT_PPGTT(ppgtt));
 
 	pdp = kzalloc(sizeof(*pdp), GFP_KERNEL);
 	if (!pdp)
 		return ERR_PTR(-ENOMEM);
 
-	ret = __pdp_init(dev, pdp);
+	ret = __pdp_init(ppgtt->base.dev, pdp);
 	if (ret)
 		goto fail_bitmap;
 
-	ret = setup_px(dev, pdp);
+	ret = setup_px(ppgtt->base.dev, pdp);
 	if (ret)
 		goto fail_page_m;
 
@@ -593,12 +593,12 @@ fail_bitmap:
 	return ERR_PTR(ret);
 }
 
-static void free_pdp(struct drm_device *dev,
+static void free_pdp(struct i915_hw_ppgtt *ppgtt,
 		     struct i915_page_directory_pointer *pdp)
 {
 	__pdp_fini(pdp);
-	if (USES_FULL_48BIT_PPGTT(dev)) {
-		cleanup_px(dev, pdp);
+	if (IS_48BIT_PPGTT(ppgtt)) {
+		cleanup_px(ppgtt->base.dev, pdp);
 		kfree(pdp);
 	}
 }
@@ -632,7 +632,7 @@ gen8_setup_page_directory(struct i915_hw_ppgtt *ppgtt,
 {
 	gen8_ppgtt_pdpe_t *page_directorypo;
 
-	if (!USES_FULL_48BIT_PPGTT(ppgtt->base.dev))
+	if (!IS_48BIT_PPGTT(ppgtt))
 		return;
 
 	page_directorypo = kmap_px(pdp);
@@ -648,7 +648,7 @@ gen8_setup_page_directory_pointer(struct i915_hw_ppgtt *ppgtt,
 {
 	gen8_ppgtt_pml4e_t *pagemap = kmap_px(pml4);
 
-	WARN_ON(!USES_FULL_48BIT_PPGTT(ppgtt->base.dev));
+	WARN_ON(!IS_48BIT_PPGTT(ppgtt));
 	pagemap[index] = gen8_pml4e_encode(px_dma(pdp), I915_CACHE_LLC);
 	kunmap_px(ppgtt, pagemap);
 }
@@ -765,7 +765,7 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
 	gen8_pte_t scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
 						 I915_CACHE_LLC, use_scratch);
 
-	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+	if (!IS_48BIT_PPGTT(ppgtt)) {
 		gen8_ppgtt_clear_pte_range(vm, &ppgtt->pdp, start, length,
 					   scratch_pte);
 	} else {
@@ -831,7 +831,7 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
 
 	__sg_page_iter_start(&sg_iter, pages->sgl, sg_nents(pages->sgl), 0);
 
-	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+	if (!IS_48BIT_PPGTT(ppgtt)) {
 		gen8_ppgtt_insert_pte_entries(vm, &ppgtt->pdp, &sg_iter, start,
 					      cache_level);
 	} else {
@@ -865,6 +865,7 @@ static void gen8_free_page_tables(struct drm_device *dev,
 
 static int gen8_init_scratch(struct i915_address_space *vm)
 {
+	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct drm_device *dev = vm->dev;
 
 	vm->scratch_page = alloc_scratch_page(dev);
@@ -884,8 +885,8 @@ static int gen8_init_scratch(struct i915_address_space *vm)
 		return PTR_ERR(vm->scratch_pd);
 	}
 
-	if (USES_FULL_48BIT_PPGTT(dev)) {
-		vm->scratch_pdp = alloc_pdp(dev);
+	if (IS_48BIT_PPGTT(ppgtt)) {
+		vm->scratch_pdp = alloc_pdp(ppgtt);
 		if (IS_ERR(vm->scratch_pdp)) {
 			free_pd(dev, vm->scratch_pd);
 			free_pt(dev, vm->scratch_pt);
@@ -896,7 +897,7 @@ static int gen8_init_scratch(struct i915_address_space *vm)
 
 	gen8_initialize_pt(vm, vm->scratch_pt);
 	gen8_initialize_pd(vm, vm->scratch_pd);
-	if (USES_FULL_48BIT_PPGTT(dev))
+	if (IS_48BIT_PPGTT(ppgtt))
 		gen8_initialize_pdp(vm, vm->scratch_pdp);
 
 	return 0;
@@ -908,7 +909,7 @@ static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
 	struct drm_i915_private *dev_priv = to_i915(ppgtt->base.dev);
 	int i;
 
-	if (USES_FULL_48BIT_PPGTT(dev_priv)) {
+	if (IS_48BIT_PPGTT(ppgtt)) {
 		u64 daddr = px_dma(&ppgtt->pml4);
 
 		I915_WRITE(vgtif_reg(pdp[0].lo), lower_32_bits(daddr));
@@ -936,15 +937,16 @@ static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
 static void gen8_free_scratch(struct i915_address_space *vm)
 {
 	struct drm_device *dev = vm->dev;
+	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 
-	if (USES_FULL_48BIT_PPGTT(dev))
-		free_pdp(dev, vm->scratch_pdp);
+	if (IS_48BIT_PPGTT(ppgtt))
+		free_pdp(ppgtt, vm->scratch_pdp);
 	free_pd(dev, vm->scratch_pd);
 	free_pt(dev, vm->scratch_pt);
 	free_scratch_page(dev, vm->scratch_page);
 }
 
-static void gen8_ppgtt_cleanup_3lvl(struct drm_device *dev,
+static void gen8_ppgtt_cleanup_3lvl(struct i915_hw_ppgtt *ppgtt,
 				    struct i915_page_directory_pointer *pdp)
 {
 	int i;
@@ -953,11 +955,11 @@ static void gen8_ppgtt_cleanup_3lvl(struct drm_device *dev,
 		if (WARN_ON(!pdp->page_directory[i]))
 			continue;
 
-		gen8_free_page_tables(dev, pdp->page_directory[i]);
-		free_pd(dev, pdp->page_directory[i]);
+		gen8_free_page_tables(ppgtt->base.dev, pdp->page_directory[i]);
+		free_pd(ppgtt->base.dev, pdp->page_directory[i]);
 	}
 
-	free_pdp(dev, pdp);
+	free_pdp(ppgtt, pdp);
 }
 
 static void gen8_ppgtt_cleanup_4lvl(struct i915_hw_ppgtt *ppgtt)
@@ -968,7 +970,7 @@ static void gen8_ppgtt_cleanup_4lvl(struct i915_hw_ppgtt *ppgtt)
 		if (WARN_ON(!ppgtt->pml4.pdps[i]))
 			continue;
 
-		gen8_ppgtt_cleanup_3lvl(ppgtt->base.dev, ppgtt->pml4.pdps[i]);
+		gen8_ppgtt_cleanup_3lvl(ppgtt, ppgtt->pml4.pdps[i]);
 	}
 
 	cleanup_px(ppgtt->base.dev, &ppgtt->pml4);
@@ -981,8 +983,8 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 	if (intel_vgpu_active(vm->dev))
 		gen8_ppgtt_notify_vgt(ppgtt, false);
 
-	if (!USES_FULL_48BIT_PPGTT(ppgtt->base.dev))
-		gen8_ppgtt_cleanup_3lvl(ppgtt->base.dev, &ppgtt->pdp);
+	if (!IS_48BIT_PPGTT(ppgtt))
+		gen8_ppgtt_cleanup_3lvl(ppgtt, &ppgtt->pdp);
 	else
 		gen8_ppgtt_cleanup_4lvl(ppgtt);
 
@@ -1127,7 +1129,7 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 				  uint64_t length,
 				  unsigned long *new_pdps)
 {
-	struct drm_device *dev = vm->dev;
+	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 	struct i915_page_directory_pointer *pdp;
 	uint32_t pml4e;
 
@@ -1135,7 +1137,7 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 
 	gen8_for_each_pml4e(pdp, pml4, start, length, pml4e) {
 		if (!test_bit(pml4e, pml4->used_pml4es)) {
-			pdp = alloc_pdp(dev);
+			pdp = alloc_pdp(ppgtt);
 			if (IS_ERR(pdp))
 				goto unwind_out;
 
@@ -1153,7 +1155,7 @@ gen8_ppgtt_alloc_page_dirpointers(struct i915_address_space *vm,
 
 unwind_out:
 	for_each_set_bit(pml4e, new_pdps, GEN8_PML4ES_PER_PML4)
-		free_pdp(dev, pml4->pdps[pml4e]);
+		free_pdp(ppgtt, pml4->pdps[pml4e]);
 
 	return -ENOMEM;
 }
@@ -1360,7 +1362,7 @@ static int gen8_alloc_va_range_4lvl(struct i915_address_space *vm,
 
 err_out:
 	for_each_set_bit(pml4e, new_pdps, GEN8_PML4ES_PER_PML4)
-		gen8_ppgtt_cleanup_3lvl(vm->dev, pml4->pdps[pml4e]);
+		gen8_ppgtt_cleanup_3lvl(ppgtt, pml4->pdps[pml4e]);
 
 	return ret;
 }
@@ -1370,7 +1372,7 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
 
-	if (USES_FULL_48BIT_PPGTT(vm->dev))
+	if (IS_48BIT_PPGTT(ppgtt))
 		return gen8_alloc_va_range_4lvl(vm, &ppgtt->pml4, start, length);
 	else
 		return gen8_alloc_va_range_3lvl(vm, &ppgtt->pdp, start, length);
@@ -1441,7 +1443,7 @@ static void gen8_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 	gen8_pte_t scratch_pte = gen8_pte_encode(px_dma(vm->scratch_page),
 						 I915_CACHE_LLC, true);
 
-	if (!USES_FULL_48BIT_PPGTT(vm->dev)) {
+	if (!IS_48BIT_PPGTT(ppgtt)) {
 		gen8_dump_pdp(&ppgtt->pdp, start, length, scratch_pte, m);
 	} else {
 		uint64_t pml4e;
@@ -1492,14 +1494,10 @@ static int gen8_preallocate_top_level_pdps(struct i915_hw_ppgtt *ppgtt)
  * space.
  *
  */
-static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
+static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, int address_space_mode)
 {
 	int ret;
 
-	ret = gen8_init_scratch(&ppgtt->base);
-	if (ret)
-		return ret;
-
 	ppgtt->base.start = 0;
 	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
 	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
@@ -1508,8 +1506,13 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
 	ppgtt->base.bind_vma = ppgtt_bind_vma;
 	ppgtt->debug_dump = gen8_dump_ppgtt;
+	ppgtt->address_space_mode = address_space_mode;
+
+	ret = gen8_init_scratch(&ppgtt->base);
+	if (ret)
+		return ret;
 
-	if (USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
+	if (IS_48BIT_PPGTT(ppgtt)) {
 		ret = setup_px(ppgtt->base.dev, &ppgtt->pml4);
 		if (ret)
 			goto free_scratch;
@@ -2101,14 +2104,15 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	return 0;
 }
 
-static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
+static int __hw_ppgtt_init(struct drm_device *dev,
+		struct i915_hw_ppgtt *ppgtt, int address_space_mode)
 {
 	ppgtt->base.dev = dev;
 
 	if (INTEL_INFO(dev)->gen < 8)
 		return gen6_ppgtt_init(ppgtt);
 	else
-		return gen8_ppgtt_init(ppgtt);
+		return gen8_ppgtt_init(ppgtt, address_space_mode);
 }
 
 static void i915_address_space_init(struct i915_address_space *vm,
@@ -2145,7 +2149,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret = 0;
 
-	ret = __hw_ppgtt_init(dev, ppgtt);
+	ret = __hw_ppgtt_init(dev, ppgtt, USES_FULL_48BIT_PPGTT(dev) ? 48 : 32);
 	if (ret == 0) {
 		kref_init(&ppgtt->ref);
 		i915_address_space_init(&ppgtt->base, dev_priv);
@@ -2772,7 +2776,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 		if (!ppgtt)
 			return -ENOMEM;
 
-		ret = __hw_ppgtt_init(dev, ppgtt);
+		ret = __hw_ppgtt_init(dev, ppgtt, 32);
 		if (ret) {
 			ppgtt->base.cleanup(&ppgtt->base);
 			kfree(ppgtt);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index d7dd3d8..e26d9cb 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -110,6 +110,7 @@ typedef uint64_t gen8_ppgtt_pml4e_t;
 
 #define I915_PDPES_PER_PDP(dev) (USES_FULL_48BIT_PPGTT(dev) ?\
 				 GEN8_PML4ES_PER_PML4 : GEN8_LEGACY_PDPES)
+#define IS_48BIT_PPGTT(ppgtt)	((ppgtt) && ((ppgtt)->address_space_mode == 48))
 
 #define PPAT_UNCACHED_INDEX		(_PAGE_PWT | _PAGE_PCD)
 #define PPAT_CACHED_PDE_INDEX		0 /* WB LLC */
@@ -372,6 +373,8 @@ struct i915_hw_ppgtt {
 		struct i915_page_directory pd;		/* GEN6-7 */
 	};
 
+	int address_space_mode;
+
 	struct drm_i915_file_private *file_priv;
 
 	gen6_pte_t __iomem *pd_addr;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2b7e6bb..13cb1b3 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -407,7 +407,7 @@ static void execlists_update_context(struct drm_i915_gem_request *rq)
 	 * PML4 is allocated during ppgtt init, so this is not needed
 	 * in 48-bit mode.
 	 */
-	if (ppgtt && !USES_FULL_48BIT_PPGTT(ppgtt->base.dev))
+	if (ppgtt && !IS_48BIT_PPGTT(ppgtt))
 		execlists_update_context_pdps(ppgtt, reg_state);
 }
 
@@ -1706,7 +1706,7 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 	 * not needed in 48-bit.*/
 	if (req->ctx->ppgtt &&
 	    (intel_engine_flag(req->engine) & req->ctx->ppgtt->pd_dirty_rings)) {
-		if (!USES_FULL_48BIT_PPGTT(req->i915) &&
+		if (!IS_48BIT_PPGTT(req->ctx->ppgtt) &&
 		    !intel_vgpu_active(req->i915->dev)) {
 			ret = intel_logical_ring_emit_pdps(req);
 			if (ret)
@@ -2539,7 +2539,7 @@ populate_lr_context(struct intel_context *ctx,
 	ASSIGN_CTX_REG(reg_state, CTX_PDP0_LDW, GEN8_RING_PDP_LDW(engine, 0),
 		       0);
 
-	if (USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
+	if (IS_48BIT_PPGTT(ppgtt)) {
 		/* 64b PPGTT (48bit canonical)
 		 * PDP0_DESCRIPTOR contains the base address to PML4 and
 		 * other PDP Descriptors are ignored.
-- 
2.4.11

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

[PATCH 2/2] drm/i915: generate address mode bit from PPGTT instance

[Matthew Auld]

From: "Wang, Zhi A" <zhi.a.wang@intel.com>

After the per-PPGTT address mode gets support, the LRC submission should
generate the address mode bit from PPGTT instance, instead of the
hard-coded system configuration.

v2:
(Matthew Auld)
  - rebase on latest -nightly

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 13cb1b3..17bd811 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -214,7 +214,7 @@ enum {
 	LEGACY_64B_CONTEXT
 };
 #define GEN8_CTX_ADDRESSING_MODE_SHIFT 3
-#define GEN8_CTX_ADDRESSING_MODE(dev)  (USES_FULL_48BIT_PPGTT(dev) ?\
+#define GEN8_CTX_ADDRESSING_MODE(ppgtt) (IS_48BIT_PPGTT(ppgtt) ? \
 		LEGACY_64B_CONTEXT :\
 		LEGACY_32B_CONTEXT)
 enum {
@@ -276,8 +276,6 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine)
 					(engine->id == VCS || engine->id == VCS2);
 
 	engine->ctx_desc_template = GEN8_CTX_VALID;
-	engine->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev) <<
-				   GEN8_CTX_ADDRESSING_MODE_SHIFT;
 	if (IS_GEN8(dev))
 		engine->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
 	engine->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
@@ -319,7 +317,9 @@ intel_lr_context_descriptor_update(struct intel_context *ctx,
 	lrca = ctx->engine[engine->id].lrc_vma->node.start +
 	       LRC_PPHWSP_PN * PAGE_SIZE;
 
-	desc = engine->ctx_desc_template;			   /* bits  0-11 */
+	desc = engine->ctx_desc_template;		   /* bits  0-11 */
+	desc |= GEN8_CTX_ADDRESSING_MODE(ctx->ppgtt) <<	   /* bits  3-4 */
+			GEN8_CTX_ADDRESSING_MODE_SHIFT;
 	desc |= lrca;					   /* bits 12-31 */
 	desc |= (lrca >> PAGE_SHIFT) << GEN8_CTX_ID_SHIFT; /* bits 32-51 */
 
-- 
2.4.11

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

Re: [PATCH 1/2] drm/i915: Support per-PPGTT address space mode

[Chris Wilson]

On Tue, Apr 26, 2016 at 04:17:51PM +0100, Matthew Auld wrote:
> From: "Wang, Zhi A" <zhi.a.wang@intel.com>
> 
> Previously the address space mode of each PPGTT instance is hard-coded to
> host system configuration, e.g. if the host system is configured to use
> 48bit full PPGTT, then address space mode of all PPGTT instances is 48bit.
> 
> Per Daniel and Kevin's advice, GVT-g will leverage i915 PPGTT interface to
> populate its shadow PPGTT page table. Under GVT-g the address space mode
> of PPGTT instances could be various, some guest may use 32bit, some guest
> may use 48bit.
> 
> We store the address space mode into i915_hw_ppgtt, and let i915 page
> table manipulation routines / LRC context population routines read the
> address space mode from it instead of the system configuration.
> 
> v2:
> (Matthew Auld)
>   - rebase on latest -nightly
>   - prefer i915_vm_to_ppgtt instead of container_of
>   - initialise address_space_mode _before_ we attempt to init
>     scratch, otherwise breakage will ensue

Worth adding each address space type (i915.enable_ppgtt=) to
drv_module_reload_basic ? Probably better to add new a module loader
test to exercise these nondefault flags that can find bugs without
overburdening BAT.

This patch could be split up into the feeding of pggtt instead of dev
and then we see the impact of switchable address modes more clearly.

>  #define I915_PDPES_PER_PDP(dev) (USES_FULL_48BIT_PPGTT(dev) ?\
>  				 GEN8_PML4ES_PER_PML4 : GEN8_LEGACY_PDPES)
> +#define IS_48BIT_PPGTT(ppgtt)	((ppgtt) && ((ppgtt)->address_space_mode == 48))
>  
>  #define PPAT_UNCACHED_INDEX		(_PAGE_PWT | _PAGE_PCD)
>  #define PPAT_CACHED_PDE_INDEX		0 /* WB LLC */
> @@ -372,6 +373,8 @@ struct i915_hw_ppgtt {
>  		struct i915_page_directory pd;		/* GEN6-7 */
>  	};
>  
> +	int address_space_mode;

The only question is why int, and why 48 rather than either a boolean
flag or an enum?
-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 2/2] drm/i915: generate address mode bit from PPGTT instance

[Chris Wilson]

On Tue, Apr 26, 2016 at 04:17:52PM +0100, Matthew Auld wrote:
> From: "Wang, Zhi A" <zhi.a.wang@intel.com>
> 
> After the per-PPGTT address mode gets support, the LRC submission should
> generate the address mode bit from PPGTT instance, instead of the
> hard-coded system configuration.
> 
> v2:
> (Matthew Auld)
>   - rebase on latest -nightly
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 13cb1b3..17bd811 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -214,7 +214,7 @@ enum {
>  	LEGACY_64B_CONTEXT
>  };
>  #define GEN8_CTX_ADDRESSING_MODE_SHIFT 3
> -#define GEN8_CTX_ADDRESSING_MODE(dev)  (USES_FULL_48BIT_PPGTT(dev) ?\
> +#define GEN8_CTX_ADDRESSING_MODE(ppgtt) (IS_48BIT_PPGTT(ppgtt) ? \
>  		LEGACY_64B_CONTEXT :\
>  		LEGACY_32B_CONTEXT)
>  enum {
> @@ -276,8 +276,6 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine)
>  					(engine->id == VCS || engine->id == VCS2);
>  
>  	engine->ctx_desc_template = GEN8_CTX_VALID;
> -	engine->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev) <<
> -				   GEN8_CTX_ADDRESSING_MODE_SHIFT;
>  	if (IS_GEN8(dev))
>  		engine->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
>  	engine->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
> @@ -319,7 +317,9 @@ intel_lr_context_descriptor_update(struct intel_context *ctx,
>  	lrca = ctx->engine[engine->id].lrc_vma->node.start +
>  	       LRC_PPHWSP_PN * PAGE_SIZE;
>  
> -	desc = engine->ctx_desc_template;			   /* bits  0-11 */
> +	desc = engine->ctx_desc_template;		   /* bits  0-11 */
> +	desc |= GEN8_CTX_ADDRESSING_MODE(ctx->ppgtt) <<	   /* bits  3-4 */
> +			GEN8_CTX_ADDRESSING_MODE_SHIFT;

Would it not be simpler for us to use the GEN8_CTX_ADDRESSING_MODE() as
our enum, then we would just do
desc |= ctx->ppgtt->addressing_mode << GEN8_CTX_ADDRESSING_MODE_SHIFT;

And then we would have an enum already defined!
-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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915: Support per-PPGTT address space mode

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Support per-PPGTT address space mode
URL   : https://patchwork.freedesktop.org/series/6347/
State : failure

== Summary ==

Series 6347v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/6347/revisions/1/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> INCOMPLETE (hsw-gt2)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-dellxps)
        Subgroup hang-read-crc-pipe-b:
                incomplete -> PASS       (snb-dellxps)

bdw-nuci7        total:200  pass:188  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:200  pass:175  dwarn:0   dfail:0   fail:0   skip:25 
bsw-nuc-2        total:199  pass:158  dwarn:0   dfail:0   fail:0   skip:41 
byt-nuc          total:199  pass:158  dwarn:0   dfail:0   fail:0   skip:41 
hsw-brixbox      total:200  pass:174  dwarn:0   dfail:0   fail:0   skip:26 
hsw-gt2          total:93   pass:79   dwarn:0   dfail:0   fail:1   skip:12 
ivb-t430s        total:200  pass:169  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:200  pass:173  dwarn:0   dfail:0   fail:0   skip:27 
skl-nuci5        total:200  pass:189  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:200  pass:157  dwarn:1   dfail:0   fail:0   skip:42 
snb-x220t        total:200  pass:158  dwarn:0   dfail:0   fail:1   skip:41 

Results at /archive/results/CI_IGT_test/Patchwork_2077/

e005db1cb2c60d18abe837ac683d8993ea77b239 drm-intel-nightly: 2016y-04m-26d-12h-51m-57s UTC integration manifest
7857fe0 drm/i915: generate address mode bit from PPGTT instance
d7432f1 drm/i915: Support per-PPGTT address space mode

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

Re: [PATCH 2/2] drm/i915: generate address mode bit from PPGTT instance

[Wang, Zhi A]

Hi Matthew:
    Thanks for spending efforts on rebase! :) I'm sorry that we will drop this 2 patches in the next round patch review. 

In the previous discussion, there were two options:

Option A. Unify GVT-g PPGTT shadow with i915 PPGTT routines, which needs these 2 patches, then LRC submission routines could generate context descriptor according to the addressing space mode in i915_hw_ppgtt. (Daniel)

The reason this option is dropped because:
a. Different requirement. In i915, The caller of PPGTT routines mostly is VMA-oriented. It doesn't care about how the page table created. Usually i915 will create the upper level of page table according to the wanted VMA. And GVT-g shadow is PTE-oriented, mostly it will populate the page table according to how guest modifies its PPGTT.

b. Different abstraction. GVT-g shadow is based on an unique-leveled page table manipulation design. E.g. both PML4/PDP/PDE population could use same functions, different with i915, which uses different functions to handle different PML4/PDP/PDE population. It hard to merge them together without big changes. We definitely want to prevent heavy modifications of i915, as that might cause regressions.

Option B. Modify the related functions in intel_lrc.c to update the PDP root pointers in each submission.
And the pros is we don't need to modify i915 PPGTT now. Definitely cons here is we need to modify LRC related routines. Chris suggested we could use ppgtt->pd_dirty_rings before, and the ppgtt->drity_pd_rings is highly combined with 32-bit page table routines. I think we might needs some modifications

The ideal option we are currently thinking is:

a. Issue LRIs to update PDPs in shadow ring buffer so that we don't need to modify PDP loading routines in intel_lrc.c
b. Add addressing mode bit in intel_context and make it configurable. Definitely i915 host context will configure this bit according to i915.enable_ppgtt. And GVT-g could configure it by guest requirements.

Thanks,
Zhi.

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Tuesday, April 26, 2016 8:56 AM
To: Auld, Matthew <matthew.auld@intel.com>
Cc: intel-gfx@lists.freedesktop.org; Wang, Zhi A <zhi.a.wang@intel.com>; Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Subject: Re: [PATCH 2/2] drm/i915: generate address mode bit from PPGTT instance

On Tue, Apr 26, 2016 at 04:17:52PM +0100, Matthew Auld wrote:
> From: "Wang, Zhi A" <zhi.a.wang@intel.com>
> 
> After the per-PPGTT address mode gets support, the LRC submission 
> should generate the address mode bit from PPGTT instance, instead of 
> the hard-coded system configuration.
> 
> v2:
> (Matthew Auld)
>   - rebase on latest -nightly
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 13cb1b3..17bd811 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -214,7 +214,7 @@ enum {
>  	LEGACY_64B_CONTEXT
>  };
>  #define GEN8_CTX_ADDRESSING_MODE_SHIFT 3 -#define 
> GEN8_CTX_ADDRESSING_MODE(dev)  (USES_FULL_48BIT_PPGTT(dev) ?\
> +#define GEN8_CTX_ADDRESSING_MODE(ppgtt) (IS_48BIT_PPGTT(ppgtt) ? \
>  		LEGACY_64B_CONTEXT :\
>  		LEGACY_32B_CONTEXT)
>  enum {
> @@ -276,8 +276,6 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine)
>  					(engine->id == VCS || engine->id == VCS2);
>  
>  	engine->ctx_desc_template = GEN8_CTX_VALID;
> -	engine->ctx_desc_template |= GEN8_CTX_ADDRESSING_MODE(dev) <<
> -				   GEN8_CTX_ADDRESSING_MODE_SHIFT;
>  	if (IS_GEN8(dev))
>  		engine->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
>  	engine->ctx_desc_template |= GEN8_CTX_PRIVILEGE; @@ -319,7 +317,9 @@ 
> intel_lr_context_descriptor_update(struct intel_context *ctx,
>  	lrca = ctx->engine[engine->id].lrc_vma->node.start +
>  	       LRC_PPHWSP_PN * PAGE_SIZE;
>  
> -	desc = engine->ctx_desc_template;			   /* bits  0-11 */
> +	desc = engine->ctx_desc_template;		   /* bits  0-11 */
> +	desc |= GEN8_CTX_ADDRESSING_MODE(ctx->ppgtt) <<	   /* bits  3-4 */
> +			GEN8_CTX_ADDRESSING_MODE_SHIFT;

Would it not be simpler for us to use the GEN8_CTX_ADDRESSING_MODE() as our enum, then we would just do desc |= ctx->ppgtt->addressing_mode << GEN8_CTX_ADDRESSING_MODE_SHIFT;

And then we would have an enum already defined!
-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