[PATCH 0/7] drm/intel: guest i915 changes for Broadwell to run inside VM with Intel GVT-g

[PATCH 0/7] drm/intel: guest i915 changes for Broadwell to run inside VM with Intel GVT-g

[Zhiyuan Lv]

I915 kernel driver can now work inside a virtual machine on Haswell
with Intel GVT-g. In order to do the same thing on Broadwell, there
are some extra changes needed. The two main things are to support the
more complicated PPGTT page table structure and EXECLIST contexts.
GVT-g will perform shadow PPGTT and shadow context, which requires
guest driver to explicitly notify host device model the life cycle of
PPGTT and EXECLIST contexts.

The first and the forth patches added some restrictions to drivers in
virtualization scenario to make the shadow work easier. The first
patch is based on Mika's earlier one, but we use it for vgpu only.
The sixth patch is the implementation of the notification for
shadowing.

Zhiyuan Lv (7):
  drm/i915: preallocate pdps for 32 bit vgpu
  drm/i915: Enable full ppgtt for vgpu
  drm/i915: Always enable execlists on BDW for vgpu
  drm/i915: always pin lrc context for vgpu with Intel GVT-g
  drm/i915: Update PV INFO page definition for Intel GVT-g
  drm/i915: guest i915 notification for Intel-GVTg
  drm/i915: Allow Broadwell guest with Intel GVT-g

 drivers/gpu/drm/i915/i915_gem_gtt.c | 77 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_vgpu.c    |  2 +-
 drivers/gpu/drm/i915/i915_vgpu.h    | 34 +++++++++++++++-
 drivers/gpu/drm/i915/intel_lrc.c    | 44 ++++++++++++++++++---
 4 files changed, 145 insertions(+), 12 deletions(-)

-- 
1.9.1

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

[PATCH 1/7] drm/i915: preallocate pdps for 32 bit vgpu

[Zhiyuan Lv]

This is based on Mika Kuoppala's patch below:
http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/61104/match=workaround+hw+preload

The patch will preallocate the page directories for 32-bit PPGTT when
i915 runs inside a virtual machine with Intel GVT-g. With this change,
the root pointers in EXECLIST context will always keep the same.

The change is needed for vGPU because Intel GVT-g will do page table
shadowing, and needs to track all the page table changes from guest
i915 driver. However, if guest PPGTT is modified through GPU commands
like LRI, it is not possible to trap the operations in the right time,
so it will be hard to make shadow PPGTT to work correctly.

Shadow PPGTT could be much simpler with this change. Meanwhile
hypervisor could simply prohibit any attempt of PPGTT modification
through GPU command for security.

The function gen8_preallocate_top_level_pdps() in the patch is from
Mika, with only one change to set "used_pdpes" to avoid duplicated
allocation later.

Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Michel Thierry <michel.thierry@intel.com>
Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c    |  3 ++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4a76807..ed10e77 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1441,6 +1441,33 @@ 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(dev);
+	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);
+	if (!ret)
+		*ppgtt->pdp.used_pdpes = *new_page_dirs;
+
+	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
+
+	return ret;
+}
+
 /*
  * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP registers
  * with a net effect resembling a 2-level page table in normal x86 terms. Each
@@ -1484,6 +1511,12 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 		trace_i915_page_directory_pointer_entry_alloc(&ppgtt->base,
 							      0, 0,
 							      GEN8_PML4E_SHIFT);
+
+		if (intel_vgpu_active(ppgtt->base.dev)) {
+			ret = gen8_preallocate_top_level_pdps(ppgtt);
+			if (ret)
+				goto free_scratch;
+		}
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index e77b6b0..2dc8709 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1540,7 +1540,8 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 	 * not needed in 48-bit.*/
 	if (req->ctx->ppgtt &&
 	    (intel_ring_flag(req->ring) & req->ctx->ppgtt->pd_dirty_rings)) {
-		if (!USES_FULL_48BIT_PPGTT(req->i915)) {
+		if (!USES_FULL_48BIT_PPGTT(req->i915) &&
+		    !intel_vgpu_active(req->i915->dev)) {
 			ret = intel_logical_ring_emit_pdps(req);
 			if (ret)
 				return ret;
-- 
1.9.1

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

[PATCH 2/7] drm/i915: Enable full ppgtt for vgpu

[Zhiyuan Lv]

The full ppgtt is supported in Intel GVT-g device model. So the
restriction can be removed.

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ed10e77..823005c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -108,9 +108,6 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 	has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
 	has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
 
-	if (intel_vgpu_active(dev))
-		has_full_ppgtt = false; /* emulation is too hard */
-
 	/*
 	 * We don't allow disabling PPGTT for gen9+ as it's a requirement for
 	 * execlists, the sole mechanism available to submit work.
-- 
1.9.1

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

[PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Zhiyuan Lv]

Broadwell hardware supports both ring buffer mode and execlist mode.
When i915 runs inside a VM with Intel GVT-g, we allow execlist mode
only. The reason is that GVT-g does not support the dynamic mode
switch between ring buffer mode and execlist mode when running
multiple virtual machines. Consider that ring buffer mode is legacy
mode, it makes sense to drop it inside virtual machines.

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 2dc8709..39df304 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -239,6 +239,9 @@ int intel_sanitize_enable_execlists(struct drm_device *dev, int enable_execlists
 	if (INTEL_INFO(dev)->gen >= 9)
 		return 1;
 
+	if (HAS_LOGICAL_RING_CONTEXTS(dev) && intel_vgpu_active(dev))
+		return 1;
+
 	if (enable_execlists == 0)
 		return 0;
 
-- 
1.9.1

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

[PATCH 4/7] drm/i915: always pin lrc context for vgpu with Intel GVT-g

[Zhiyuan Lv]

Intel GVT-g will perform EXECLIST context shadowing and ring buffer
shadowing. The shadow copy is created when guest creates a context.
If a context changes its LRCA address, the hypervisor is hard to know
whether it is a new context or not. We always pin context objects to
global GTT to make life easier.

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 39df304..4b2ac37 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2282,7 +2282,8 @@ void intel_lr_context_free(struct intel_context *ctx)
 					ctx->engine[i].ringbuf;
 			struct intel_engine_cs *ring = ringbuf->ring;
 
-			if (ctx == ring->default_context) {
+			if ((ctx == ring->default_context) ||
+			    (intel_vgpu_active(ring->dev))) {
 				intel_unpin_ringbuffer_obj(ringbuf);
 				i915_gem_object_ggtt_unpin(ctx_obj);
 			}
@@ -2353,6 +2354,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 				     struct intel_engine_cs *ring)
 {
 	const bool is_global_default_ctx = (ctx == ring->default_context);
+	const bool need_to_pin_ctx = (is_global_default_ctx ||
+				      (intel_vgpu_active(ring->dev)));
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *ctx_obj;
@@ -2374,7 +2377,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 		return -ENOMEM;
 	}
 
-	if (is_global_default_ctx) {
+	if (need_to_pin_ctx) {
 		ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
 				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 		if (ret) {
@@ -2415,7 +2418,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 			goto error_free_rbuf;
 		}
 
-		if (is_global_default_ctx) {
+		if (need_to_pin_ctx) {
 			ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
 			if (ret) {
 				DRM_ERROR(
@@ -2464,14 +2467,14 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 	return 0;
 
 error:
-	if (is_global_default_ctx)
+	if (need_to_pin_ctx)
 		intel_unpin_ringbuffer_obj(ringbuf);
 error_destroy_rbuf:
 	intel_destroy_ringbuffer_obj(ringbuf);
 error_free_rbuf:
 	kfree(ringbuf);
 error_unpin_ctx:
-	if (is_global_default_ctx)
+	if (need_to_pin_ctx)
 		i915_gem_object_ggtt_unpin(ctx_obj);
 	drm_gem_object_unreference(&ctx_obj->base);
 	return ret;
-- 
1.9.1

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

[PATCH 5/7] drm/i915: Update PV INFO page definition for Intel GVT-g

[Zhiyuan Lv]

Some more definitions in the PV info page are added. They are mainly
for the guest notification to Intel GVT-g device model. They are used
for Broadwell enabling.

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_vgpu.h | 34 ++++++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
index 97a88b5..21c97f4 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.h
+++ b/drivers/gpu/drm/i915/i915_vgpu.h
@@ -40,6 +40,19 @@
 #define INTEL_VGT_IF_VERSION \
 	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MINOR)
 
+/*
+ * notifications from guest to vgpu device model
+ */
+enum vgt_g2v_type {
+	VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE = 2,
+	VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY,
+	VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE,
+	VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY,
+	VGT_G2V_EXECLIST_CONTEXT_CREATE,
+	VGT_G2V_EXECLIST_CONTEXT_DESTROY,
+	VGT_G2V_MAX,
+};
+
 struct vgt_if {
 	uint64_t magic;		/* VGT_MAGIC */
 	uint16_t version_major;
@@ -70,11 +83,28 @@ struct vgt_if {
 	uint32_t rsv3[0x200 - 24];	/* pad to half page */
 	/*
 	 * The bottom half page is for response from Gfx driver to hypervisor.
-	 * Set to reserved fields temporarily by now.
 	 */
 	uint32_t rsv4;
 	uint32_t display_ready;	/* ready for display owner switch */
-	uint32_t rsv5[0x200 - 2];	/* pad to one page */
+
+	uint32_t rsv5[4];
+
+	uint32_t g2v_notify;
+	uint32_t rsv6[7];
+
+	uint32_t pdp0_lo;
+	uint32_t pdp0_hi;
+	uint32_t pdp1_lo;
+	uint32_t pdp1_hi;
+	uint32_t pdp2_lo;
+	uint32_t pdp2_hi;
+	uint32_t pdp3_lo;
+	uint32_t pdp3_hi;
+
+	uint32_t execlist_context_descriptor_lo;
+	uint32_t execlist_context_descriptor_hi;
+
+	uint32_t  rsv7[0x200 - 24];    /* pad to one page */
 } __packed;
 
 #define vgtif_reg(x) \
-- 
1.9.1

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

[PATCH 6/7] drm/i915: guest i915 notification for Intel-GVTg

[Zhiyuan Lv]

When i915 drivers run inside a VM with Intel-GVTg, some explicit
notifications are needed from guest to host device model through PV
INFO page write. The notifications include:

	PPGTT create/destroy
	EXECLIST create/destroy

They are used for the shadow implementation of PPGTT and EXECLIST
context. Intel GVT-g needs to write-protect the guest pages of PPGTT
and contexts, and clear the write protection when they end their life
cycle.

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 41 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_lrc.c    | 25 ++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 823005c..00dafb0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -896,6 +896,41 @@ static int gen8_init_scratch(struct i915_address_space *vm)
 	return 0;
 }
 
+static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool create)
+{
+	enum vgt_g2v_type msg;
+	struct drm_device *dev = ppgtt->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	unsigned int offset = vgtif_reg(pdp0_lo);
+	int i;
+
+	if (USES_FULL_48BIT_PPGTT(dev)) {
+		u64 daddr = px_dma(&ppgtt->pml4);
+
+		I915_WRITE(offset, daddr & 0xffffffff);
+		I915_WRITE(offset + 4, daddr >> 32);
+
+		msg = (create ? VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE :
+				VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY);
+	} else {
+		for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
+			u64 daddr = i915_page_dir_dma_addr(ppgtt, i);
+
+			I915_WRITE(offset, daddr & 0xffffffff);
+			I915_WRITE(offset + 4, daddr >> 32);
+
+			offset += 8;
+		}
+
+		msg = (create ? VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE :
+				VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY);
+	}
+
+	I915_WRITE(vgtif_reg(g2v_notify), msg);
+
+	return 0;
+}
+
 static void gen8_free_scratch(struct i915_address_space *vm)
 {
 	struct drm_device *dev = vm->dev;
@@ -942,6 +977,9 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 	struct i915_hw_ppgtt *ppgtt =
 		container_of(vm, struct i915_hw_ppgtt, base);
 
+	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);
 	else
@@ -1516,6 +1554,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 		}
 	}
 
+	if (intel_vgpu_active(ppgtt->base.dev))
+		gen8_ppgtt_notify_vgt(ppgtt, true);
+
 	return 0;
 
 free_scratch:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4b2ac37..80d424b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -136,6 +136,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "intel_mocs.h"
+#include "i915_vgpu.h"
 
 #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
 #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
@@ -2122,6 +2123,22 @@ make_rpcs(struct drm_device *dev)
 	return rpcs;
 }
 
+static void intel_lr_context_notify_vgt(struct intel_context *ctx,
+					struct intel_engine_cs *ring,
+					int msg)
+{
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u64 tmp = intel_lr_context_descriptor(ctx, ring);
+
+	I915_WRITE(vgtif_reg(execlist_context_descriptor_lo),
+			tmp & 0xffffffff);
+	I915_WRITE(vgtif_reg(execlist_context_descriptor_hi),
+			tmp >> 32);
+
+	I915_WRITE(vgtif_reg(g2v_notify), msg);
+}
+
 static int
 populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
 		    struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
@@ -2282,6 +2299,10 @@ void intel_lr_context_free(struct intel_context *ctx)
 					ctx->engine[i].ringbuf;
 			struct intel_engine_cs *ring = ringbuf->ring;
 
+			if (intel_vgpu_active(ringbuf->ring->dev))
+				intel_lr_context_notify_vgt(ctx, ring,
+					VGT_G2V_EXECLIST_CONTEXT_DESTROY);
+
 			if ((ctx == ring->default_context) ||
 			    (intel_vgpu_active(ring->dev))) {
 				intel_unpin_ringbuffer_obj(ringbuf);
@@ -2439,6 +2460,10 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 	ctx->engine[ring->id].ringbuf = ringbuf;
 	ctx->engine[ring->id].state = ctx_obj;
 
+	if (intel_vgpu_active(dev))
+		intel_lr_context_notify_vgt(ctx, ring,
+				VGT_G2V_EXECLIST_CONTEXT_CREATE);
+
 	if (ctx == ring->default_context)
 		lrc_setup_hardware_status_page(ring, ctx_obj);
 	else if (ring->id == RCS && !ctx->rcs_initialized) {
-- 
1.9.1

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

[PATCH 7/7] drm/i915: Allow Broadwell guest with Intel GVT-g

[Zhiyuan Lv]

I915 Broadwell guest driver is now supported to run inside a VM with
Intel GVT-g

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/i915_vgpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 5eee75b..fdeb461 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -66,7 +66,7 @@ void i915_check_vgpu(struct drm_device *dev)
 
 	BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
 
-	if (!IS_HASWELL(dev))
+	if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
 		return;
 
 	magic = readq(dev_priv->regs + vgtif_reg(magic));
-- 
1.9.1

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

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Chris Wilson]

On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> Broadwell hardware supports both ring buffer mode and execlist mode.
> When i915 runs inside a VM with Intel GVT-g, we allow execlist mode
> only. The reason is that GVT-g does not support the dynamic mode
> switch between ring buffer mode and execlist mode when running
> multiple virtual machines. Consider that ring buffer mode is legacy
> mode, it makes sense to drop it inside virtual machines.

If that is the case, you should query the host as to what mode it is
running.
-Chris

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

Re: [PATCH 4/7] drm/i915: always pin lrc context for vgpu with Intel GVT-g

[Chris Wilson]

On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> shadowing. The shadow copy is created when guest creates a context.
> If a context changes its LRCA address, the hypervisor is hard to know
> whether it is a new context or not. We always pin context objects to
> global GTT to make life easier.

Nak. Please explain why we need to workaround a bug in the host. We
cannot pin the context as that breaks userspace (e.g. synmark) who can
and will try to use more contexts than we have room.
-Chris

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

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Zhiyuan Lv]

Hi Chris,

On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > Broadwell hardware supports both ring buffer mode and execlist mode.
> > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode
> > only. The reason is that GVT-g does not support the dynamic mode
> > switch between ring buffer mode and execlist mode when running
> > multiple virtual machines. Consider that ring buffer mode is legacy
> > mode, it makes sense to drop it inside virtual machines.
> 
> If that is the case, you should query the host as to what mode it is
> running.

Thanks for the reply! You mean we query the host mode, then tell the
guest driver inside VM, so that it could use the same mode as host
right? That might be a little complicated, and the only benefit is to
support legacy ring buffer mode ...

And GVT-g host implementation was not well tested with ring buffer mode
since it cannot start windows VM. Probably I should change the commit
message to say explicitly that ring buffer mode is not supported with
GVT-g? Thanks!

Regards,
-Zhiyuan

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

Re: [PATCH 4/7] drm/i915: always pin lrc context for vgpu with Intel GVT-g

[Zhiyuan Lv]

Hi Chris,

On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > shadowing. The shadow copy is created when guest creates a context.
> > If a context changes its LRCA address, the hypervisor is hard to know
> > whether it is a new context or not. We always pin context objects to
> > global GTT to make life easier.
> 
> Nak. Please explain why we need to workaround a bug in the host. We
> cannot pin the context as that breaks userspace (e.g. synmark) who can
> and will try to use more contexts than we have room.

This change is only for i915 running inside a VM. Host i915 driver's
behavior will not be impacted.

The reason we want to pin context is that our hypervisor identifies
guest contexts with their LRCA, and need LRCA unchanged during
contexts' life cycle so that shadow copy of contexts can easily find
their counterparts. If we cannot pin them, we have to consider more
complicated shadow implementation ...

BTW Chris, are there many apps like synmark that may used up GGTT with
contexts if they are all pinned? Thanks!

Regards,
-Zhiyuan

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

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Chris Wilson]

On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> Hi Chris,
> 
> On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > Broadwell hardware supports both ring buffer mode and execlist mode.
> > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode
> > > only. The reason is that GVT-g does not support the dynamic mode
> > > switch between ring buffer mode and execlist mode when running
> > > multiple virtual machines. Consider that ring buffer mode is legacy
> > > mode, it makes sense to drop it inside virtual machines.
> > 
> > If that is the case, you should query the host as to what mode it is
> > running.
> 
> Thanks for the reply! You mean we query the host mode, then tell the
> guest driver inside VM, so that it could use the same mode as host
> right? That might be a little complicated, and the only benefit is to
> support legacy ring buffer mode ...

The only benefit being that the guest works no matter what the host
does?
-Chris

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

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Zhiyuan Lv]

On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote:
> On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> > Hi Chris,
> > 
> > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > > Broadwell hardware supports both ring buffer mode and execlist mode.
> > > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode
> > > > only. The reason is that GVT-g does not support the dynamic mode
> > > > switch between ring buffer mode and execlist mode when running
> > > > multiple virtual machines. Consider that ring buffer mode is legacy
> > > > mode, it makes sense to drop it inside virtual machines.
> > > 
> > > If that is the case, you should query the host as to what mode it is
> > > running.
> > 
> > Thanks for the reply! You mean we query the host mode, then tell the
> > guest driver inside VM, so that it could use the same mode as host
> > right? That might be a little complicated, and the only benefit is to
> > support legacy ring buffer mode ...
> 
> The only benefit being that the guest works no matter what the host
> does?

Supporting ring buffer mode may need more work in GVT-g. When we started to
enable BDW support, ring buffer mode used to work but was not well tested. And
the inter-VM switch (all in ringbuffer mode) may be tricker comparing with
driver context switch with "MI_SET_CONTEXT", because we need to switch ring
buffer whereas driver does not. Regarding this, the EXECLIST mode looks
cleaner. In order to support that, we may have to: 1, change more LRI commands
to MMIO in current driver; 2, more testing/debugging of inter-VM context
switch flow.

Based on that, I think we should really make statement that "ring buffer mode"
is not supported by GVT-g on BDW :-)

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

Re: [PATCH 1/7] drm/i915: preallocate pdps for 32 bit vgpu

[Joonas Lahtinen]

Hi,

Added Michel and Dave as CC too, to notice this, as they are specified
in the patch as CC.

On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> This is based on Mika Kuoppala's patch below:
> http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/61
> 104/match=workaround+hw+preload
> 
> The patch will preallocate the page directories for 32-bit PPGTT when
> i915 runs inside a virtual machine with Intel GVT-g. With this 
> change,
> the root pointers in EXECLIST context will always keep the same.
> 
> The change is needed for vGPU because Intel GVT-g will do page table
> shadowing, and needs to track all the page table changes from guest
> i915 driver. However, if guest PPGTT is modified through GPU commands
> like LRI, it is not possible to trap the operations in the right 
> time,
> so it will be hard to make shadow PPGTT to work correctly.
> 
> Shadow PPGTT could be much simpler with this change. Meanwhile
> hypervisor could simply prohibit any attempt of PPGTT modification
> through GPU command for security.
> 
> The function gen8_preallocate_top_level_pdps() in the patch is from
> Mika, with only one change to set "used_pdpes" to avoid duplicated
> allocation later.
> 
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Dave Gordon <david.s.gordon@intel.com>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> 

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

I'm just wondering if it's worth keeping the LRI method of updating the
PDPS at all, for the sake of a couple of KBs per PPGTT, now that we
have an occasional need for making them static. So this patch is R-b:d,
but I'd suggest discussion about removing the LRI update method, and
favoring static PDPS always for 32-bit.

Regards, Joonas

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 33 
> +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c    |  3 ++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4a76807..ed10e77 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1441,6 +1441,33 @@ 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(dev);
> +	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);
> +	if (!ret)
> +		*ppgtt->pdp.used_pdpes = *new_page_dirs;
> +
> +	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, 
> pdpes);
> +
> +	return ret;
> +}
> +
>  /*
>   * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP 
> registers
>   * with a net effect resembling a 2-level page table in normal x86 
> terms. Each
> @@ -1484,6 +1511,12 @@ static int gen8_ppgtt_init(struct 
> i915_hw_ppgtt *ppgtt)
>  		trace_i915_page_directory_pointer_entry_alloc(&ppgtt
> ->base,
>  							      0, 0,
>  							     
>  GEN8_PML4E_SHIFT);
> +
> +		if (intel_vgpu_active(ppgtt->base.dev)) {
> +			ret = 
> gen8_preallocate_top_level_pdps(ppgtt);
> +			if (ret)
> +				goto free_scratch;
> +		}
>  	}
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> b/drivers/gpu/drm/i915/intel_lrc.c
> index e77b6b0..2dc8709 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1540,7 +1540,8 @@ static int gen8_emit_bb_start(struct 
> drm_i915_gem_request *req,
>  	 * not needed in 48-bit.*/
>  	if (req->ctx->ppgtt &&
>  	    (intel_ring_flag(req->ring) & req->ctx->ppgtt
> ->pd_dirty_rings)) {
> -		if (!USES_FULL_48BIT_PPGTT(req->i915)) {
> +		if (!USES_FULL_48BIT_PPGTT(req->i915) &&
> +		    !intel_vgpu_active(req->i915->dev)) {
>  			ret = intel_logical_ring_emit_pdps(req);
>  			if (ret)
>  				return ret;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Enable full ppgtt for vgpu

[Joonas Lahtinen]

On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> The full ppgtt is supported in Intel GVT-g device model. So the
> restriction can be removed.
> 
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ed10e77..823005c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -108,9 +108,6 @@ static int sanitize_enable_ppgtt(struct 
> drm_device *dev, int enable_ppgtt)
>  	has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
>  	has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
>  
> -	if (intel_vgpu_active(dev))
> -		has_full_ppgtt = false; /* emulation is too hard */
> -
>  	/*
>  	 * We don't allow disabling PPGTT for gen9+ as it's a 
> requirement for
>  	 * execlists, the sole mechanism available to submit work.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Joonas Lahtinen]

Hi,

On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote:
> On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> > > Hi Chris,
> > > 
> > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > > > Broadwell hardware supports both ring buffer mode and 
> > > > > execlist mode.
> > > > > When i915 runs inside a VM with Intel GVT-g, we allow 
> > > > > execlist mode
> > > > > only. The reason is that GVT-g does not support the dynamic 
> > > > > mode
> > > > > switch between ring buffer mode and execlist mode when 
> > > > > running
> > > > > multiple virtual machines. Consider that ring buffer mode is 
> > > > > legacy
> > > > > mode, it makes sense to drop it inside virtual machines.
> > > > 
> > > > If that is the case, you should query the host as to what mode 
> > > > it is
> > > > running.
> > > 
> > > Thanks for the reply! You mean we query the host mode, then tell 
> > > the
> > > guest driver inside VM, so that it could use the same mode as 
> > > host
> > > right? That might be a little complicated, and the only benefit 
> > > is to
> > > support legacy ring buffer mode ...
> > 
> > The only benefit being that the guest works no matter what the host
> > does?
> 
> Supporting ring buffer mode may need more work in GVT-g. When we 
> started to
> enable BDW support, ring buffer mode used to work but was not well 
> tested. And
> the inter-VM switch (all in ringbuffer mode) may be tricker comparing 
> with
> driver context switch with "MI_SET_CONTEXT", because we need to 
> switch ring
> buffer whereas driver does not. Regarding this, the EXECLIST mode 
> looks
> cleaner. In order to support that, we may have to: 1, change more LRI 
> commands
> to MMIO in current driver; 2, more testing/debugging of inter-VM 
> context
> switch flow.
> 
> Based on that, I think we should really make statement that "ring 
> buffer mode"
> is not supported by GVT-g on BDW :-)
> 

I think just move the vpgu test even before test for GEN9 just making
it return true on intel_vgpu_active(dev) and add a comment that
currently vGPU only supports execlist command submission, and then add
an error early in the init in some appropriate spot if
intel_vgpu_active is true but logical ring context are not available
(which would practically mean GEN < 8).

Does that sound OK?

Regards, Joonas

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

Re: [PATCH 5/7] drm/i915: Update PV INFO page definition for Intel GVT-g

[Joonas Lahtinen]

On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> Some more definitions in the PV info page are added. They are mainly
> for the guest notification to Intel GVT-g device model. They are used
> for Broadwell enabling.
> 
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> 

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Is there any public document about the interface?

> ---
>  drivers/gpu/drm/i915/i915_vgpu.h | 34 
> ++++++++++++++++++++++++++++++++--
>  1 file changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h 
> b/drivers/gpu/drm/i915/i915_vgpu.h
> index 97a88b5..21c97f4 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -40,6 +40,19 @@
>  #define INTEL_VGT_IF_VERSION \
>  	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, 
> VGT_VERSION_MINOR)
>  
> +/*
> + * notifications from guest to vgpu device model
> + */
> +enum vgt_g2v_type {
> +	VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE = 2,
> +	VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY,
> +	VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE,
> +	VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY,
> +	VGT_G2V_EXECLIST_CONTEXT_CREATE,
> +	VGT_G2V_EXECLIST_CONTEXT_DESTROY,
> +	VGT_G2V_MAX,
> +};
> +
>  struct vgt_if {
>  	uint64_t magic;		/* VGT_MAGIC */
>  	uint16_t version_major;
> @@ -70,11 +83,28 @@ struct vgt_if {
>  	uint32_t rsv3[0x200 - 24];	/* pad to half page */
>  	/*
>  	 * The bottom half page is for response from Gfx driver to 
> hypervisor.
> -	 * Set to reserved fields temporarily by now.
>  	 */
>  	uint32_t rsv4;
>  	uint32_t display_ready;	/* ready for display owner 
> switch */
> -	uint32_t rsv5[0x200 - 2];	/* pad to one page */
> +
> +	uint32_t rsv5[4];
> +
> +	uint32_t g2v_notify;
> +	uint32_t rsv6[7];
> +
> +	uint32_t pdp0_lo;
> +	uint32_t pdp0_hi;
> +	uint32_t pdp1_lo;
> +	uint32_t pdp1_hi;
> +	uint32_t pdp2_lo;
> +	uint32_t pdp2_hi;
> +	uint32_t pdp3_lo;
> +	uint32_t pdp3_hi;
> +
> +	uint32_t execlist_context_descriptor_lo;
> +	uint32_t execlist_context_descriptor_hi;
> +
> +	uint32_t  rsv7[0x200 - 24];    /* pad to one page */
>  } __packed;
>  
>  #define vgtif_reg(x) \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: guest i915 notification for Intel-GVTg

[Joonas Lahtinen]

Hi,

Notes below.

On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> When i915 drivers run inside a VM with Intel-GVTg, some explicit
> notifications are needed from guest to host device model through PV
> INFO page write. The notifications include:
> 
> 	PPGTT create/destroy
> 	EXECLIST create/destroy
> 
> They are used for the shadow implementation of PPGTT and EXECLIST
> context. Intel GVT-g needs to write-protect the guest pages of PPGTT
> and contexts, and clear the write protection when they end their life
> cycle.
> 
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 41 
> +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_lrc.c    | 25 ++++++++++++++++++++++
>  2 files changed, 66 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 823005c..00dafb0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -896,6 +896,41 @@ static int gen8_init_scratch(struct 
> i915_address_space *vm)
>  	return 0;
>  }
>  
> +static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool 
> create)
> +{
> +	enum vgt_g2v_type msg;
> +	struct drm_device *dev = ppgtt->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	unsigned int offset = vgtif_reg(pdp0_lo);
> +	int i;
> +
> +	if (USES_FULL_48BIT_PPGTT(dev)) {

With regards the patch "preallocate pdps for 32 bit vgpu", is this code
path ever taken?

> +		u64 daddr = px_dma(&ppgtt->pml4);
> +
> +		I915_WRITE(offset, daddr & 0xffffffff);
> +		I915_WRITE(offset + 4, daddr >> 32);
> +
> +		msg = (create ? VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE :
> +				VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY)
> ;
> +	} else {
> +		for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
> +			u64 daddr = i915_page_dir_dma_addr(ppgtt, 
> i);
> +
> +			I915_WRITE(offset, daddr & 0xffffffff);
> +			I915_WRITE(offset + 4, daddr >> 32);
> +
> +			offset += 8;
> +		}
> +
> +		msg = (create ? VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE :
> +				VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY)
> ;
> +	}
> +
> +	I915_WRITE(vgtif_reg(g2v_notify), msg);
> +
> +	return 0;
> +}
> +
>  static void gen8_free_scratch(struct i915_address_space *vm)
>  {
>  	struct drm_device *dev = vm->dev;
> @@ -942,6 +977,9 @@ static void gen8_ppgtt_cleanup(struct 
> i915_address_space *vm)
>  	struct i915_hw_ppgtt *ppgtt =
>  		container_of(vm, struct i915_hw_ppgtt, base);
>  
> +	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);
>  	else
> @@ -1516,6 +1554,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt 
> *ppgtt)
>  		}
>  	}
>  
> +	if (intel_vgpu_active(ppgtt->base.dev))
> +		gen8_ppgtt_notify_vgt(ppgtt, true);
> +
>  	return 0;
>  
>  free_scratch:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> b/drivers/gpu/drm/i915/intel_lrc.c
> index 4b2ac37..80d424b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -136,6 +136,7 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include "intel_mocs.h"
> +#include "i915_vgpu.h"
>  
>  #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
>  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
> @@ -2122,6 +2123,22 @@ make_rpcs(struct drm_device *dev)
>  	return rpcs;
>  }
>  
> +static void intel_lr_context_notify_vgt(struct intel_context *ctx,
> +					struct intel_engine_cs 
> *ring,
> +					int msg)
> +{
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u64 tmp = intel_lr_context_descriptor(ctx, ring);
> +
> +	I915_WRITE(vgtif_reg(execlist_context_descriptor_lo),
> +			tmp & 0xffffffff);
> +	I915_WRITE(vgtif_reg(execlist_context_descriptor_hi),
> +			tmp >> 32);
> +
> +	I915_WRITE(vgtif_reg(g2v_notify), msg);
> +}
> +

Why the other interface has bool for action and the other msg?

Regards, Joonas

>  static int
>  populate_lr_context(struct intel_context *ctx, struct 
> drm_i915_gem_object *ctx_obj,
>  		    struct intel_engine_cs *ring, struct 
> intel_ringbuffer *ringbuf)
> @@ -2282,6 +2299,10 @@ void intel_lr_context_free(struct 
> intel_context *ctx)
>  					ctx->engine[i].ringbuf;
>  			struct intel_engine_cs *ring = ringbuf
> ->ring;
>  
> +			if (intel_vgpu_active(ringbuf->ring->dev))
> +				intel_lr_context_notify_vgt(ctx, 
> ring,
> +					VGT_G2V_EXECLIST_CONTEXT_DES
> TROY);
> +
>  			if ((ctx == ring->default_context) ||
>  			    (intel_vgpu_active(ring->dev))) {
>  				intel_unpin_ringbuffer_obj(ringbuf);
> @@ -2439,6 +2460,10 @@ int intel_lr_context_deferred_create(struct 
> intel_context *ctx,
>  	ctx->engine[ring->id].ringbuf = ringbuf;
>  	ctx->engine[ring->id].state = ctx_obj;
>  
> +	if (intel_vgpu_active(dev))
> +		intel_lr_context_notify_vgt(ctx, ring,
> +				VGT_G2V_EXECLIST_CONTEXT_CREATE);
> +
>  	if (ctx == ring->default_context)
>  		lrc_setup_hardware_status_page(ring, ctx_obj);
>  	else if (ring->id == RCS && !ctx->rcs_initialized) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915: Allow Broadwell guest with Intel GVT-g

[Joonas Lahtinen]

On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> I915 Broadwell guest driver is now supported to run inside a VM with
> Intel GVT-g
> 
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>

After the other relevant patches are settled;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_vgpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c 
> b/drivers/gpu/drm/i915/i915_vgpu.c
> index 5eee75b..fdeb461 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -66,7 +66,7 @@ void i915_check_vgpu(struct drm_device *dev)
>  
>  	BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
>  
> -	if (!IS_HASWELL(dev))
> +	if (!IS_HASWELL(dev) && !IS_BROADWELL(dev))
>  		return;
>  
>  	magic = readq(dev_priv->regs + vgtif_reg(magic));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Zhiyuan Lv]

Hi Joonas,

Thanks for the review! And my reply inline.

Regards,
-Zhiyuan

On Thu, Aug 20, 2015 at 02:23:11PM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote:
> > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote:
> > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> > > > Hi Chris,
> > > > 
> > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > > > > Broadwell hardware supports both ring buffer mode and 
> > > > > > execlist mode.
> > > > > > When i915 runs inside a VM with Intel GVT-g, we allow 
> > > > > > execlist mode
> > > > > > only. The reason is that GVT-g does not support the dynamic 
> > > > > > mode
> > > > > > switch between ring buffer mode and execlist mode when 
> > > > > > running
> > > > > > multiple virtual machines. Consider that ring buffer mode is 
> > > > > > legacy
> > > > > > mode, it makes sense to drop it inside virtual machines.
> > > > > 
> > > > > If that is the case, you should query the host as to what mode 
> > > > > it is
> > > > > running.
> > > > 
> > > > Thanks for the reply! You mean we query the host mode, then tell 
> > > > the
> > > > guest driver inside VM, so that it could use the same mode as 
> > > > host
> > > > right? That might be a little complicated, and the only benefit 
> > > > is to
> > > > support legacy ring buffer mode ...
> > > 
> > > The only benefit being that the guest works no matter what the host
> > > does?
> > 
> > Supporting ring buffer mode may need more work in GVT-g. When we 
> > started to
> > enable BDW support, ring buffer mode used to work but was not well 
> > tested. And
> > the inter-VM switch (all in ringbuffer mode) may be tricker comparing 
> > with
> > driver context switch with "MI_SET_CONTEXT", because we need to 
> > switch ring
> > buffer whereas driver does not. Regarding this, the EXECLIST mode 
> > looks
> > cleaner. In order to support that, we may have to: 1, change more LRI 
> > commands
> > to MMIO in current driver; 2, more testing/debugging of inter-VM 
> > context
> > switch flow.
> > 
> > Based on that, I think we should really make statement that "ring 
> > buffer mode"
> > is not supported by GVT-g on BDW :-)
> > 
> 
> I think just move the vpgu test even before test for GEN9 just making
> it return true on intel_vgpu_active(dev) and add a comment that
> currently vGPU only supports execlist command submission, and then add

Will do. Thanks!

> an error early in the init in some appropriate spot if
> intel_vgpu_active is true but logical ring context are not available
> (which would practically mean GEN < 8).
> 
> Does that sound OK?

Ring buffer mode for Haswell with vgpu is still supported. So probably I have
the check like below? Thanks!

@@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device *dev)
        if (WARN_ON(dev_priv->ring[RCS].default_context))
                return 0;
 
+       if (intel_vgpu_active(dev)) {
+               if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) &&
+                           !i915.enable_execlist))
+                       return 0;
+       }
+
        if (i915.enable_execlists) {
                /* NB: intentionally left blank. We will allocate our own
                 * backing objects as we need them, thank you very much */

> 
> Regards, Joonas
> 
> > > -Chris
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: Update PV INFO page definition for Intel GVT-g

[Zhiyuan Lv]

On Thu, Aug 20, 2015 at 03:58:43PM +0300, Joonas Lahtinen wrote:
> On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > Some more definitions in the PV info page are added. They are mainly
> > for the guest notification to Intel GVT-g device model. They are used
> > for Broadwell enabling.
> > 
> > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> > 
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Is there any public document about the interface?

So far no ... The information we got is that the 4K MMIO range from 0x78000
is reserved for virtualization usage, but the detailed definition will not
appear in spec. Thanks!

> 
> > ---
> >  drivers/gpu/drm/i915/i915_vgpu.h | 34 
> > ++++++++++++++++++++++++++++++++--
> >  1 file changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_vgpu.h 
> > b/drivers/gpu/drm/i915/i915_vgpu.h
> > index 97a88b5..21c97f4 100644
> > --- a/drivers/gpu/drm/i915/i915_vgpu.h
> > +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> > @@ -40,6 +40,19 @@
> >  #define INTEL_VGT_IF_VERSION \
> >  	INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, 
> > VGT_VERSION_MINOR)
> >  
> > +/*
> > + * notifications from guest to vgpu device model
> > + */
> > +enum vgt_g2v_type {
> > +	VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE = 2,
> > +	VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY,
> > +	VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE,
> > +	VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY,
> > +	VGT_G2V_EXECLIST_CONTEXT_CREATE,
> > +	VGT_G2V_EXECLIST_CONTEXT_DESTROY,
> > +	VGT_G2V_MAX,
> > +};
> > +
> >  struct vgt_if {
> >  	uint64_t magic;		/* VGT_MAGIC */
> >  	uint16_t version_major;
> > @@ -70,11 +83,28 @@ struct vgt_if {
> >  	uint32_t rsv3[0x200 - 24];	/* pad to half page */
> >  	/*
> >  	 * The bottom half page is for response from Gfx driver to 
> > hypervisor.
> > -	 * Set to reserved fields temporarily by now.
> >  	 */
> >  	uint32_t rsv4;
> >  	uint32_t display_ready;	/* ready for display owner 
> > switch */
> > -	uint32_t rsv5[0x200 - 2];	/* pad to one page */
> > +
> > +	uint32_t rsv5[4];
> > +
> > +	uint32_t g2v_notify;
> > +	uint32_t rsv6[7];
> > +
> > +	uint32_t pdp0_lo;
> > +	uint32_t pdp0_hi;
> > +	uint32_t pdp1_lo;
> > +	uint32_t pdp1_hi;
> > +	uint32_t pdp2_lo;
> > +	uint32_t pdp2_hi;
> > +	uint32_t pdp3_lo;
> > +	uint32_t pdp3_hi;
> > +
> > +	uint32_t execlist_context_descriptor_lo;
> > +	uint32_t execlist_context_descriptor_hi;
> > +
> > +	uint32_t  rsv7[0x200 - 24];    /* pad to one page */
> >  } __packed;
> >  
> >  #define vgtif_reg(x) \
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915: guest i915 notification for Intel-GVTg

[Zhiyuan Lv]

On Thu, Aug 20, 2015 at 04:11:57PM +0300, Joonas Lahtinen wrote:
> Hi,
> 
> Notes below.
> 
> On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > When i915 drivers run inside a VM with Intel-GVTg, some explicit
> > notifications are needed from guest to host device model through PV
> > INFO page write. The notifications include:
> > 
> > 	PPGTT create/destroy
> > 	EXECLIST create/destroy
> > 
> > They are used for the shadow implementation of PPGTT and EXECLIST
> > context. Intel GVT-g needs to write-protect the guest pages of PPGTT
> > and contexts, and clear the write protection when they end their life
> > cycle.
> > 
> > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 41 
> > +++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_lrc.c    | 25 ++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 823005c..00dafb0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -896,6 +896,41 @@ static int gen8_init_scratch(struct 
> > i915_address_space *vm)
> >  	return 0;
> >  }
> >  
> > +static int gen8_ppgtt_notify_vgt(struct i915_hw_ppgtt *ppgtt, bool 
> > create)
> > +{
> > +	enum vgt_g2v_type msg;
> > +	struct drm_device *dev = ppgtt->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned int offset = vgtif_reg(pdp0_lo);
> > +	int i;
> > +
> > +	if (USES_FULL_48BIT_PPGTT(dev)) {
> 
> With regards the patch "preallocate pdps for 32 bit vgpu", is this code
> path ever taken?

48-bit PPGTT will create root pml4 table in gen8_ppgtt_init(), and
after that, the PPGTT structure is complete, so we still need the
notification.

I did not tested the path though, because I found currently
i915.enable_ppgtt will not equal to 3. The whole 48-bit PPGTT support
in GVT-g is verified with windows VM. Thanks!

> 
> > +		u64 daddr = px_dma(&ppgtt->pml4);
> > +
> > +		I915_WRITE(offset, daddr & 0xffffffff);
> > +		I915_WRITE(offset + 4, daddr >> 32);
> > +
> > +		msg = (create ? VGT_G2V_PPGTT_L4_PAGE_TABLE_CREATE :
> > +				VGT_G2V_PPGTT_L4_PAGE_TABLE_DESTROY)
> > ;
> > +	} else {
> > +		for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
> > +			u64 daddr = i915_page_dir_dma_addr(ppgtt, 
> > i);
> > +
> > +			I915_WRITE(offset, daddr & 0xffffffff);
> > +			I915_WRITE(offset + 4, daddr >> 32);
> > +
> > +			offset += 8;
> > +		}
> > +
> > +		msg = (create ? VGT_G2V_PPGTT_L3_PAGE_TABLE_CREATE :
> > +				VGT_G2V_PPGTT_L3_PAGE_TABLE_DESTROY)
> > ;
> > +	}
> > +
> > +	I915_WRITE(vgtif_reg(g2v_notify), msg);
> > +
> > +	return 0;
> > +}
> > +
> >  static void gen8_free_scratch(struct i915_address_space *vm)
> >  {
> >  	struct drm_device *dev = vm->dev;
> > @@ -942,6 +977,9 @@ static void gen8_ppgtt_cleanup(struct 
> > i915_address_space *vm)
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(vm, struct i915_hw_ppgtt, base);
> >  
> > +	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);
> >  	else
> > @@ -1516,6 +1554,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt 
> > *ppgtt)
> >  		}
> >  	}
> >  
> > +	if (intel_vgpu_active(ppgtt->base.dev))
> > +		gen8_ppgtt_notify_vgt(ppgtt, true);
> > +
> >  	return 0;
> >  
> >  free_scratch:
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 4b2ac37..80d424b 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -136,6 +136,7 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  #include "intel_mocs.h"
> > +#include "i915_vgpu.h"
> >  
> >  #define GEN9_LR_CONTEXT_RENDER_SIZE (22 * PAGE_SIZE)
> >  #define GEN8_LR_CONTEXT_RENDER_SIZE (20 * PAGE_SIZE)
> > @@ -2122,6 +2123,22 @@ make_rpcs(struct drm_device *dev)
> >  	return rpcs;
> >  }
> >  
> > +static void intel_lr_context_notify_vgt(struct intel_context *ctx,
> > +					struct intel_engine_cs 
> > *ring,
> > +					int msg)
> > +{
> > +	struct drm_device *dev = ring->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u64 tmp = intel_lr_context_descriptor(ctx, ring);
> > +
> > +	I915_WRITE(vgtif_reg(execlist_context_descriptor_lo),
> > +			tmp & 0xffffffff);
> > +	I915_WRITE(vgtif_reg(execlist_context_descriptor_hi),
> > +			tmp >> 32);
> > +
> > +	I915_WRITE(vgtif_reg(g2v_notify), msg);
> > +}
> > +
> 
> Why the other interface has bool for action and the other msg?

It would be better to change the "int" to "bool", so that the
notification type can be hidden inside this function. I will make the
change. Thanks!

> 
> Regards, Joonas
> 
> >  static int
> >  populate_lr_context(struct intel_context *ctx, struct 
> > drm_i915_gem_object *ctx_obj,
> >  		    struct intel_engine_cs *ring, struct 
> > intel_ringbuffer *ringbuf)
> > @@ -2282,6 +2299,10 @@ void intel_lr_context_free(struct 
> > intel_context *ctx)
> >  					ctx->engine[i].ringbuf;
> >  			struct intel_engine_cs *ring = ringbuf
> > ->ring;
> >  
> > +			if (intel_vgpu_active(ringbuf->ring->dev))
> > +				intel_lr_context_notify_vgt(ctx, 
> > ring,
> > +					VGT_G2V_EXECLIST_CONTEXT_DES
> > TROY);
> > +
> >  			if ((ctx == ring->default_context) ||
> >  			    (intel_vgpu_active(ring->dev))) {
> >  				intel_unpin_ringbuffer_obj(ringbuf);
> > @@ -2439,6 +2460,10 @@ int intel_lr_context_deferred_create(struct 
> > intel_context *ctx,
> >  	ctx->engine[ring->id].ringbuf = ringbuf;
> >  	ctx->engine[ring->id].state = ctx_obj;
> >  
> > +	if (intel_vgpu_active(dev))
> > +		intel_lr_context_notify_vgt(ctx, ring,
> > +				VGT_G2V_EXECLIST_CONTEXT_CREATE);
> > +
> >  	if (ctx == ring->default_context)
> >  		lrc_setup_hardware_status_page(ring, ctx_obj);
> >  	else if (ring->id == RCS && !ctx->rcs_initialized) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [iGVT-g] [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Tian, Kevin]

> From: iGVT-g [mailto:igvt-g-bounces@lists.01.org] On Behalf Of Zhiyuan Lv
> Sent: Thursday, August 20, 2015 5:40 PM
> 
> On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> > > Hi Chris,
> > >
> > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > > > Broadwell hardware supports both ring buffer mode and execlist mode.
> > > > > When i915 runs inside a VM with Intel GVT-g, we allow execlist mode
> > > > > only. The reason is that GVT-g does not support the dynamic mode
> > > > > switch between ring buffer mode and execlist mode when running
> > > > > multiple virtual machines. Consider that ring buffer mode is legacy
> > > > > mode, it makes sense to drop it inside virtual machines.
> > > >
> > > > If that is the case, you should query the host as to what mode it is
> > > > running.
> > >
> > > Thanks for the reply! You mean we query the host mode, then tell the
> > > guest driver inside VM, so that it could use the same mode as host
> > > right? That might be a little complicated, and the only benefit is to
> > > support legacy ring buffer mode ...
> >
> > The only benefit being that the guest works no matter what the host
> > does?
> 
> Supporting ring buffer mode may need more work in GVT-g. When we started to
> enable BDW support, ring buffer mode used to work but was not well tested. And
> the inter-VM switch (all in ringbuffer mode) may be tricker comparing with
> driver context switch with "MI_SET_CONTEXT", because we need to switch ring
> buffer whereas driver does not. Regarding this, the EXECLIST mode looks
> cleaner. In order to support that, we may have to: 1, change more LRI commands
> to MMIO in current driver; 2, more testing/debugging of inter-VM context
> switch flow.
> 
> Based on that, I think we should really make statement that "ring buffer mode"
> is not supported by GVT-g on BDW :-)
> 

I think the primary reason is that Windows gfx driver only uses execution
list mode. They don't have plan to add back ring buffer mode. So we
have to assume only execution list for all guests powered by GVT-g (being
that dynamic mode switch is not feasible).

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

Re: [PATCH 4/7] drm/i915: always pin lrc context for vgpu with Intel GVT-g

[Zhiyuan Lv]

Hi Chris,

If we cannot always pin lr context into GGTT, the LRCA cannot be used
as a context identifier for us. Then we have to consider a proper
interface for i915 in VM to notify GVT-g device model.

The context creation might be OK. We can pin context first, then
notify the context creation, after that, unpin the context. In GVT-g,
we can get the context's guest physical addresses from GTT table, and
use that to identify a guest context.

But the destroy can be a problem. It does not make sense to pin
context and unpin that time right?

Do you have any suggestions/comments? Thanks in advance!

Regards,
-Zhiyuan

On Thu, Aug 20, 2015 at 05:16:54PM +0800, Zhiyuan Lv wrote:
> Hi Chris,
> 
> On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > > shadowing. The shadow copy is created when guest creates a context.
> > > If a context changes its LRCA address, the hypervisor is hard to know
> > > whether it is a new context or not. We always pin context objects to
> > > global GTT to make life easier.
> > 
> > Nak. Please explain why we need to workaround a bug in the host. We
> > cannot pin the context as that breaks userspace (e.g. synmark) who can
> > and will try to use more contexts than we have room.
> 
> This change is only for i915 running inside a VM. Host i915 driver's
> behavior will not be impacted.
> 
> The reason we want to pin context is that our hypervisor identifies
> guest contexts with their LRCA, and need LRCA unchanged during
> contexts' life cycle so that shadow copy of contexts can easily find
> their counterparts. If we cannot pin them, we have to consider more
> complicated shadow implementation ...
> 
> BTW Chris, are there many apps like synmark that may used up GGTT with
> contexts if they are all pinned? Thanks!
> 
> Regards,
> -Zhiyuan
> 
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

About the iGVT-g's requirement to pin guest contexts in VM

[Zhiyuan Lv]

Hi Chris,

On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > shadowing. The shadow copy is created when guest creates a context.
> > If a context changes its LRCA address, the hypervisor is hard to know
> > whether it is a new context or not. We always pin context objects to
> > global GTT to make life easier.
> 
> Nak. Please explain why we need to workaround a bug in the host. We
> cannot pin the context as that breaks userspace (e.g. synmark) who can
> and will try to use more contexts than we have room.

Could you have a look at below reasons and kindly give us your inputs?

1, Due to the GGTT partitioning, the global graphics memory available
inside virtual machines is much smaller than native case. We cannot
support some graphics memory intensive workloads anyway. So it looks
affordable to just pin contexts which do not take much GGTT.

2, Our hypervisor needs to change i915 guest context in the shadow
context implementation. That part will be tricky if the context is not
always pinned. One scenario is that when a context finishes running,
we need to copy shadow context, which has been updated by hardware, to
guest context. The hypervisor knows context finishing by context
interrupt, but that time shrinker may have unpin the context and its
backing storage may have been swap-out. Such copy may fail. 

Look forward to your comments. Thanks!

Regards,
-Zhiyuan


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

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Chris Wilson]

On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> Hi Chris,
> 
> On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > > shadowing. The shadow copy is created when guest creates a context.
> > > If a context changes its LRCA address, the hypervisor is hard to know
> > > whether it is a new context or not. We always pin context objects to
> > > global GTT to make life easier.
> > 
> > Nak. Please explain why we need to workaround a bug in the host. We
> > cannot pin the context as that breaks userspace (e.g. synmark) who can
> > and will try to use more contexts than we have room.
> 
> Could you have a look at below reasons and kindly give us your inputs?
> 
> 1, Due to the GGTT partitioning, the global graphics memory available
> inside virtual machines is much smaller than native case. We cannot
> support some graphics memory intensive workloads anyway. So it looks
> affordable to just pin contexts which do not take much GGTT.

Wrong. It exposes the guest to a trivial denial-of-service attack. A
smaller GGTT does not actually limit clients (there is greater aperture
pressure and some paths are less likely but an individual client will
function just fine).
 
> 2, Our hypervisor needs to change i915 guest context in the shadow
> context implementation. That part will be tricky if the context is not
> always pinned. One scenario is that when a context finishes running,
> we need to copy shadow context, which has been updated by hardware, to
> guest context. The hypervisor knows context finishing by context
> interrupt, but that time shrinker may have unpin the context and its
> backing storage may have been swap-out. Such copy may fail. 

That is just a bug in your code. Firstly allowing swapout on an object
you still are using, secondly not being able to swapin.
-Chris

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

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Joonas Lahtinen]

On pe, 2015-08-21 at 10:24 +0800, Zhiyuan Lv wrote:
> Hi Joonas,
> 
> Thanks for the review! And my reply inline.
> 
> Regards,
> -Zhiyuan
> 
> On Thu, Aug 20, 2015 at 02:23:11PM +0300, Joonas Lahtinen wrote:
> > Hi,
> > 
> > On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote:
> > > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote:
> > > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> > > > > Hi Chris,
> > > > > 
> > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > > > > > Broadwell hardware supports both ring buffer mode and 
> > > > > > > execlist mode.
> > > > > > > When i915 runs inside a VM with Intel GVT-g, we allow 
> > > > > > > execlist mode
> > > > > > > only. The reason is that GVT-g does not support the 
> > > > > > > dynamic 
> > > > > > > mode
> > > > > > > switch between ring buffer mode and execlist mode when 
> > > > > > > running
> > > > > > > multiple virtual machines. Consider that ring buffer mode 
> > > > > > > is 
> > > > > > > legacy
> > > > > > > mode, it makes sense to drop it inside virtual machines.
> > > > > > 
> > > > > > If that is the case, you should query the host as to what 
> > > > > > mode 
> > > > > > it is
> > > > > > running.
> > > > > 
> > > > > Thanks for the reply! You mean we query the host mode, then 
> > > > > tell 
> > > > > the
> > > > > guest driver inside VM, so that it could use the same mode as 
> > > > > host
> > > > > right? That might be a little complicated, and the only 
> > > > > benefit 
> > > > > is to
> > > > > support legacy ring buffer mode ...
> > > > 
> > > > The only benefit being that the guest works no matter what the 
> > > > host
> > > > does?
> > > 
> > > Supporting ring buffer mode may need more work in GVT-g. When we 
> > > started to
> > > enable BDW support, ring buffer mode used to work but was not 
> > > well 
> > > tested. And
> > > the inter-VM switch (all in ringbuffer mode) may be tricker 
> > > comparing 
> > > with
> > > driver context switch with "MI_SET_CONTEXT", because we need to 
> > > switch ring
> > > buffer whereas driver does not. Regarding this, the EXECLIST mode 
> > > looks
> > > cleaner. In order to support that, we may have to: 1, change more 
> > > LRI 
> > > commands
> > > to MMIO in current driver; 2, more testing/debugging of inter-VM 
> > > context
> > > switch flow.
> > > 
> > > Based on that, I think we should really make statement that "ring 
> > > buffer mode"
> > > is not supported by GVT-g on BDW :-)
> > > 
> > 
> > I think just move the vpgu test even before test for GEN9 just 
> > making
> > it return true on intel_vgpu_active(dev) and add a comment that
> > currently vGPU only supports execlist command submission, and then 
> > add
> 
> Will do. Thanks!
> 
> > an error early in the init in some appropriate spot if
> > intel_vgpu_active is true but logical ring context are not 
> > available
> > (which would practically mean GEN < 8).
> > 
> > Does that sound OK?
> 
> Ring buffer mode for Haswell with vgpu is still supported. So 
> probably I have
> the check like below? Thanks!
> 

Ah sorry, I missed that.

> @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device 
> *dev)
>         if (WARN_ON(dev_priv->ring[RCS].default_context))
>                 return 0;
>  
> +       if (intel_vgpu_active(dev)) {
> +               if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) &&
> +                           !i915.enable_execlist))
> +                       return 0;
> +       }
> +

This looks fine to me. Maybe comment might be in place stating that
support is not yet implemented, but could be.

Regards, Joonas

>         if (i915.enable_execlists) {
>                 /* NB: intentionally left blank. We will allocate our 
> own
>                  * backing objects as we need them, thank you very 
> much */
> 
> > 
> > Regards, Joonas
> > 
> > > > -Chris
> > > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Wang, Zhi A]

Attach the big picture to help the discussion:

Windows Guest     Linux Guest      Linux Guest (i915 guest mode)  * We are here
+--------------+ +--------------+ +-------------------------------------------+ 
|              | |              | |    Guest Context Lifecycle Management     |
|Windows Driver| |     i915     | | +---------------------------------------+ |
|              | |          +---->| |        CREATE/DESTROY/PIN/UNPIN       | |
| (GUEST MODE) | | (GUEST MODE) | | | +----------------+ +----------------+ | | 
|              | |              | | | |Execlist Context| |Execlist Context| | | 
|              | |              | | | +-----^----------+ +------^---------+ | |
|              | |              | | +-------|-------------------|-----------+ | 
+--------------+ +--------------+ +---------|-------------------|-------------+
                                            |    NOTIFICATION   | 
                  +-------------------------|-------------------|-------------+
                  |               XenGT Shadow Context Lifecycle|Management   |
                  |                         |                   |             | 
                  |               +---------|-------------------|----------+  |  
                  | +-----------+ | +-------v-------+ +---------v------+   |  | 
                  | | i915      | | | Shadow Context| | Shadow Context |   |  | 
                  | | Host Mode | | +---------------+ +----------------+   |  |
                  | +-----------+ +----------------------------------------+  | 
                  |             DOM0 Linux (i915 host mode w/XenGT)           | 
                  +-----------------------------------------------------------+
                  +-----------------------------------------------------------+ 
                  |                     Hypervisor                            | 
                  +-----------------------------------------------------------+

SHADOW CONTEXT SUBMISSION

As you can see, in this picture, each guest execlist context will have an related shadow context in host, and XenGT will be responsible for
a. update SHADOW context via GUEST context when guest wants to submit an workload.
b. submitting the shadow context into hardware.
c. update the guest context via shadow context, when shadow context is finished.
d. inject virtual context switch interrupt into guest.

Then guest will see "hey, my job was retired from hardware, then I can do something to my context."

NOTIFICATION BETWEEN HOST AND GUEST

Now in our design we have built a notification channel for guest to notify XenGT due to performance reason.
With this channel, guest can play like this "Hey XenGT, I created a new context, please shadow it! Oh, I have destroyed the context, please stop tracking this context."

But when this trick comes before guest pin/unpin, it has problems.

PROBLEMS

First, guest context pin/unpin will cause LRCA change, which breaks relationship between guest context and shadow context.
As you can see that in our design, XenGT needs an approach to find the related shadow context according to something of an guest context.
For now we find the related shadow context via guest context ID(LRCA in fact).
But whatever it is, I think there should be something unique.

XenGT has no knowledge about guest context pin/unpin. Guest may swap out an context if it sees the seqno has been passed.
When the XenGT wants to access guest context, the guest context may be not there (swapped-out already).

It's hard and complicated for XenGT to track guest context without an unique context ID and a stable execlist context backing store.
For now the whole tricks are only works under virtualization environment and will not affect the native i915. 

Welcome to discussions! :)

Thanks,
Zhi.

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Monday, August 24, 2015 6:23 PM
To: intel-gfx@lists.freedesktop.org; igvt-g@lists.01.org; Wang, Zhi A; Tian, Kevin; joonas.lahtinen@linux.intel.com
Subject: Re: About the iGVT-g's requirement to pin guest contexts in VM

On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> Hi Chris,
> 
> On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > Intel GVT-g will perform EXECLIST context shadowing and ring 
> > > buffer shadowing. The shadow copy is created when guest creates a context.
> > > If a context changes its LRCA address, the hypervisor is hard to 
> > > know whether it is a new context or not. We always pin context 
> > > objects to global GTT to make life easier.
> > 
> > Nak. Please explain why we need to workaround a bug in the host. We 
> > cannot pin the context as that breaks userspace (e.g. synmark) who 
> > can and will try to use more contexts than we have room.
> 
> Could you have a look at below reasons and kindly give us your inputs?
> 
> 1, Due to the GGTT partitioning, the global graphics memory available 
> inside virtual machines is much smaller than native case. We cannot 
> support some graphics memory intensive workloads anyway. So it looks 
> affordable to just pin contexts which do not take much GGTT.

Wrong. It exposes the guest to a trivial denial-of-service attack. A smaller GGTT does not actually limit clients (there is greater aperture pressure and some paths are less likely but an individual client will function just fine).
 
> 2, Our hypervisor needs to change i915 guest context in the shadow 
> context implementation. That part will be tricky if the context is not 
> always pinned. One scenario is that when a context finishes running, 
> we need to copy shadow context, which has been updated by hardware, to 
> guest context. The hypervisor knows context finishing by context 
> interrupt, but that time shrinker may have unpin the context and its 
> backing storage may have been swap-out. Such copy may fail.

That is just a bug in your code. Firstly allowing swapout on an object you still are using, secondly not being able to swapin.
-Chris

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

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Zhiyuan Lv]

Hi Chris,

On Mon, Aug 24, 2015 at 11:23:13AM +0100, Chris Wilson wrote:
> On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> > Hi Chris,
> > 
> > On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > > > shadowing. The shadow copy is created when guest creates a context.
> > > > If a context changes its LRCA address, the hypervisor is hard to know
> > > > whether it is a new context or not. We always pin context objects to
> > > > global GTT to make life easier.
> > > 
> > > Nak. Please explain why we need to workaround a bug in the host. We
> > > cannot pin the context as that breaks userspace (e.g. synmark) who can
> > > and will try to use more contexts than we have room.
> > 
> > Could you have a look at below reasons and kindly give us your inputs?
> > 
> > 1, Due to the GGTT partitioning, the global graphics memory available
> > inside virtual machines is much smaller than native case. We cannot
> > support some graphics memory intensive workloads anyway. So it looks
> > affordable to just pin contexts which do not take much GGTT.
> 
> Wrong. It exposes the guest to a trivial denial-of-service attack. A

Inside a VM, indeed.

> smaller GGTT does not actually limit clients (there is greater aperture
> pressure and some paths are less likely but an individual client will
> function just fine).
>  
> > 2, Our hypervisor needs to change i915 guest context in the shadow
> > context implementation. That part will be tricky if the context is not
> > always pinned. One scenario is that when a context finishes running,
> > we need to copy shadow context, which has been updated by hardware, to
> > guest context. The hypervisor knows context finishing by context
> > interrupt, but that time shrinker may have unpin the context and its
> > backing storage may have been swap-out. Such copy may fail. 
> 
> That is just a bug in your code. Firstly allowing swapout on an object
> you still are using, secondly not being able to swapin.

As Zhi replied in another email, we do not have the knowledge of guest
driver's swap operations. If we cannot pin context, we may have to ask
guest driver not to swap out context pages. Do you think that would be
the right way to go? Thanks!

Regards,
-Zhiyuan

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

Re: [PATCH 2/7] drm/i915: Enable full ppgtt for vgpu

[Daniel Vetter]

On Thu, Aug 20, 2015 at 01:57:13PM +0300, Joonas Lahtinen wrote:
> On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > The full ppgtt is supported in Intel GVT-g device model. So the
> > restriction can be removed.
> > 
> > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index ed10e77..823005c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -108,9 +108,6 @@ static int sanitize_enable_ppgtt(struct 
> > drm_device *dev, int enable_ppgtt)
> >  	has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
> >  	has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
> >  
> > -	if (intel_vgpu_active(dev))
> > -		has_full_ppgtt = false; /* emulation is too hard */

Don't we need a feature check for the virtual gpu here? Or at least a
platform check? Seems like the backwards/forwards compat story isn't too
thought out yet here. Note that the kernel of the host and the guest might
not be the same at all, much less the kvm part.
-Daniel

> > -
> >  	/*
> >  	 * We don't allow disabling PPGTT for gen9+ as it's a 
> > requirement for
> >  	 * execlists, the sole mechanism available to submit work.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Daniel Vetter]

On Mon, Aug 24, 2015 at 03:36:46PM +0300, Joonas Lahtinen wrote:
> On pe, 2015-08-21 at 10:24 +0800, Zhiyuan Lv wrote:
> > Hi Joonas,
> > 
> > Thanks for the review! And my reply inline.
> > 
> > Regards,
> > -Zhiyuan
> > 
> > On Thu, Aug 20, 2015 at 02:23:11PM +0300, Joonas Lahtinen wrote:
> > > Hi,
> > > 
> > > On to, 2015-08-20 at 17:40 +0800, Zhiyuan Lv wrote:
> > > > On Thu, Aug 20, 2015 at 10:22:37AM +0100, Chris Wilson wrote:
> > > > > On Thu, Aug 20, 2015 at 04:55:08PM +0800, Zhiyuan Lv wrote:
> > > > > > Hi Chris,
> > > > > > 
> > > > > > On Thu, Aug 20, 2015 at 09:34:05AM +0100, Chris Wilson wrote:
> > > > > > > On Thu, Aug 20, 2015 at 03:45:20PM +0800, Zhiyuan Lv wrote:
> > > > > > > > Broadwell hardware supports both ring buffer mode and 
> > > > > > > > execlist mode.
> > > > > > > > When i915 runs inside a VM with Intel GVT-g, we allow 
> > > > > > > > execlist mode
> > > > > > > > only. The reason is that GVT-g does not support the 
> > > > > > > > dynamic 
> > > > > > > > mode
> > > > > > > > switch between ring buffer mode and execlist mode when 
> > > > > > > > running
> > > > > > > > multiple virtual machines. Consider that ring buffer mode 
> > > > > > > > is 
> > > > > > > > legacy
> > > > > > > > mode, it makes sense to drop it inside virtual machines.
> > > > > > > 
> > > > > > > If that is the case, you should query the host as to what 
> > > > > > > mode 
> > > > > > > it is
> > > > > > > running.
> > > > > > 
> > > > > > Thanks for the reply! You mean we query the host mode, then 
> > > > > > tell 
> > > > > > the
> > > > > > guest driver inside VM, so that it could use the same mode as 
> > > > > > host
> > > > > > right? That might be a little complicated, and the only 
> > > > > > benefit 
> > > > > > is to
> > > > > > support legacy ring buffer mode ...
> > > > > 
> > > > > The only benefit being that the guest works no matter what the 
> > > > > host
> > > > > does?
> > > > 
> > > > Supporting ring buffer mode may need more work in GVT-g. When we 
> > > > started to
> > > > enable BDW support, ring buffer mode used to work but was not 
> > > > well 
> > > > tested. And
> > > > the inter-VM switch (all in ringbuffer mode) may be tricker 
> > > > comparing 
> > > > with
> > > > driver context switch with "MI_SET_CONTEXT", because we need to 
> > > > switch ring
> > > > buffer whereas driver does not. Regarding this, the EXECLIST mode 
> > > > looks
> > > > cleaner. In order to support that, we may have to: 1, change more 
> > > > LRI 
> > > > commands
> > > > to MMIO in current driver; 2, more testing/debugging of inter-VM 
> > > > context
> > > > switch flow.
> > > > 
> > > > Based on that, I think we should really make statement that "ring 
> > > > buffer mode"
> > > > is not supported by GVT-g on BDW :-)
> > > > 
> > > 
> > > I think just move the vpgu test even before test for GEN9 just 
> > > making
> > > it return true on intel_vgpu_active(dev) and add a comment that
> > > currently vGPU only supports execlist command submission, and then 
> > > add
> > 
> > Will do. Thanks!
> > 
> > > an error early in the init in some appropriate spot if
> > > intel_vgpu_active is true but logical ring context are not 
> > > available
> > > (which would practically mean GEN < 8).
> > > 
> > > Does that sound OK?
> > 
> > Ring buffer mode for Haswell with vgpu is still supported. So 
> > probably I have
> > the check like below? Thanks!
> > 
> 
> Ah sorry, I missed that.
> 
> > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device 
> > *dev)
> >         if (WARN_ON(dev_priv->ring[RCS].default_context))
> >                 return 0;
> >  
> > +       if (intel_vgpu_active(dev)) {
> > +               if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) &&
> > +                           !i915.enable_execlist))
> > +                       return 0;
> > +       }
> > +
> 
> This looks fine to me. Maybe comment might be in place stating that
> support is not yet implemented, but could be.

You should fail this instead so that i915.ko knows that the render side of
the gpu doesn't work. And maybe just DRM_INFO with a useful informational
notice?

Also same comment here: Don't we need to coordinate this a bit better with
the host?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Daniel Vetter]

On Tue, Aug 25, 2015 at 08:17:05AM +0800, Zhiyuan Lv wrote:
> Hi Chris,
> 
> On Mon, Aug 24, 2015 at 11:23:13AM +0100, Chris Wilson wrote:
> > On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> > > Hi Chris,
> > > 
> > > On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > > > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > > > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > > > > shadowing. The shadow copy is created when guest creates a context.
> > > > > If a context changes its LRCA address, the hypervisor is hard to know
> > > > > whether it is a new context or not. We always pin context objects to
> > > > > global GTT to make life easier.
> > > > 
> > > > Nak. Please explain why we need to workaround a bug in the host. We
> > > > cannot pin the context as that breaks userspace (e.g. synmark) who can
> > > > and will try to use more contexts than we have room.
> > > 
> > > Could you have a look at below reasons and kindly give us your inputs?
> > > 
> > > 1, Due to the GGTT partitioning, the global graphics memory available
> > > inside virtual machines is much smaller than native case. We cannot
> > > support some graphics memory intensive workloads anyway. So it looks
> > > affordable to just pin contexts which do not take much GGTT.
> > 
> > Wrong. It exposes the guest to a trivial denial-of-service attack. A
> 
> Inside a VM, indeed.
> 
> > smaller GGTT does not actually limit clients (there is greater aperture
> > pressure and some paths are less likely but an individual client will
> > function just fine).
> >  
> > > 2, Our hypervisor needs to change i915 guest context in the shadow
> > > context implementation. That part will be tricky if the context is not
> > > always pinned. One scenario is that when a context finishes running,
> > > we need to copy shadow context, which has been updated by hardware, to
> > > guest context. The hypervisor knows context finishing by context
> > > interrupt, but that time shrinker may have unpin the context and its
> > > backing storage may have been swap-out. Such copy may fail. 
> > 
> > That is just a bug in your code. Firstly allowing swapout on an object
> > you still are using, secondly not being able to swapin.
> 
> As Zhi replied in another email, we do not have the knowledge of guest
> driver's swap operations. If we cannot pin context, we may have to ask
> guest driver not to swap out context pages. Do you think that would be
> the right way to go? Thanks!

It doesn't matter at all - if the guest unpins the ctx and puts something
else in there before the host tells it that the ctx is completed, that's a
bug in the guest. Same with real hw, we guarantee that the context stays
around for long enough.

Also you obviously have to complete the copying from shadow->guest ctx
before you send the irq to the guest to signal ctx completion. Which means
there's really no overall problem here from a design pov, the only thing
you have to do is fix up bugs in the host code (probably you should just
write through the ggtt).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915: preallocate pdps for 32 bit vgpu

[Mika Kuoppala]

Joonas Lahtinen <joonas.lahtinen@linux.intel.com> writes:

> Hi,
>
> Added Michel and Dave as CC too, to notice this, as they are specified
> in the patch as CC.
>
> On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
>> This is based on Mika Kuoppala's patch below:
>> http://article.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/61
>> 104/match=workaround+hw+preload
>> 
>> The patch will preallocate the page directories for 32-bit PPGTT when
>> i915 runs inside a virtual machine with Intel GVT-g. With this 
>> change,
>> the root pointers in EXECLIST context will always keep the same.
>> 
>> The change is needed for vGPU because Intel GVT-g will do page table
>> shadowing, and needs to track all the page table changes from guest
>> i915 driver. However, if guest PPGTT is modified through GPU commands
>> like LRI, it is not possible to trap the operations in the right 
>> time,
>> so it will be hard to make shadow PPGTT to work correctly.
>> 
>> Shadow PPGTT could be much simpler with this change. Meanwhile
>> hypervisor could simply prohibit any attempt of PPGTT modification
>> through GPU command for security.
>> 
>> The function gen8_preallocate_top_level_pdps() in the patch is from
>> Mika, with only one change to set "used_pdpes" to avoid duplicated
>> allocation later.
>> 
>> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
>> Cc: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Michel Thierry <michel.thierry@intel.com>
>> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
>> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
>> 
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> I'm just wondering if it's worth keeping the LRI method of updating the
> PDPS at all, for the sake of a couple of KBs per PPGTT, now that we
> have an occasional need for making them static. So this patch is R-b:d,
> but I'd suggest discussion about removing the LRI update method, and
> favoring static PDPS always for 32-bit.
>

LRI update doesn't add context creation overhead. But it adds
stuff to command stream whenever the virtual address space grows,
as we flush the dirty status with the lri update. So i suspect
we flush way more than we should. And this is causing atleast
some runtime overhead.

It should be enough to only do lri update when the top level pdps
change. This could be achieved by doing more finegrained dirty
tracking. But as we never shrink the virtual memory, the
amount of lri flushes should diminish as the client
will eventually reach the max allocation pattern.

If we would go with static top level pdps regardless of
if vgpu is active, we would waste memory, usually 3 
pages worth. And trade gpu runtime overhead for context
setup overhead (those pages needs to be filled to point
to scratch). And some win with reduced complexity also.

-Mika


> Regards, Joonas
>
>> ---
>>  drivers/gpu/drm/i915/i915_gem_gtt.c | 33 
>> +++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_lrc.c    |  3 ++-
>>  2 files changed, 35 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
>> b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 4a76807..ed10e77 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -1441,6 +1441,33 @@ 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(dev);
>> +	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);
>> +	if (!ret)
>> +		*ppgtt->pdp.used_pdpes = *new_page_dirs;
>> +
>> +	free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, 
>> pdpes);
>> +
>> +	return ret;
>> +}
>> +
>>  /*
>>   * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP 
>> registers
>>   * with a net effect resembling a 2-level page table in normal x86 
>> terms. Each
>> @@ -1484,6 +1511,12 @@ static int gen8_ppgtt_init(struct 
>> i915_hw_ppgtt *ppgtt)
>>  		trace_i915_page_directory_pointer_entry_alloc(&ppgtt
>> ->base,
>>  							      0, 0,
>>  							     
>>  GEN8_PML4E_SHIFT);
>> +
>> +		if (intel_vgpu_active(ppgtt->base.dev)) {
>> +			ret = 
>> gen8_preallocate_top_level_pdps(ppgtt);
>> +			if (ret)
>> +				goto free_scratch;
>> +		}
>>  	}
>>  
>>  	return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index e77b6b0..2dc8709 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1540,7 +1540,8 @@ static int gen8_emit_bb_start(struct 
>> drm_i915_gem_request *req,
>>  	 * not needed in 48-bit.*/
>>  	if (req->ctx->ppgtt &&
>>  	    (intel_ring_flag(req->ring) & req->ctx->ppgtt
>> ->pd_dirty_rings)) {
>> -		if (!USES_FULL_48BIT_PPGTT(req->i915)) {
>> +		if (!USES_FULL_48BIT_PPGTT(req->i915) &&
>> +		    !intel_vgpu_active(req->i915->dev)) {
>>  			ret = intel_logical_ring_emit_pdps(req);
>>  			if (ret)
>>  				return ret;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Wang, Zhi A]

Attach the some introduction to help the discussion:

[ Current Implementation - Performance Oriented ! ]


             Guest create                   Guest submits                   
             a new context                  its context                     
                  +                                    +     ^              
                  |                                    |     |              
  +-------+       |Notify XenGT                        |     |              
  | Guest |       |                                    |     |              
  +-------+       |                                    |     |      TIMELINE
==================v====================================v=====+=============>
  +-------+          <------------------------------->                      
  | XenGT |       +                                    +     ^              
  +-------+       |   +-----------------------------+  |     |              
                  |   |  *XENGT TRACK THE WHOLE     |  |     |              
   Start to track |   | LIFE CYCLE OF GUEST CONTEXT!|  |     |              
   guest context  |   +-----------------------------+  |     |              
                  |                                    |     |              
                  v                                    v     +              
            XenGT creates                 XenGT submits     XenGT updates   
            Shadow context                shadow context    guest context   


[Track the whole life cycle of guest context via notification channel between guest and host]

ADVANTAGE

*Avoid CPU usage peak at the time of guest context submission due to shadow context translation
    - Build shadow context as early as possible
    - No need to postpone the shadow context translation into the time of guest context submission 

*Avoid the cost of frequently re-recreating the shadow context
   - XenGT just create/destroy shadow context with message from notification channel
   - No need to re-create / destroy shadow context in each time of guest context submission(PERFORMANCE!)

DISADVANTAGE

* Need an unique identification to locate the shadow context via information from guest context. Current it's LRCA.
    - Break by dynamically context GGTT pin/unpin in i915

* Guest needs extra modification of leveraging the notification channel to assist XenGT manage the lifecycle of shadow context.
    - Introduce extra functions into guest driver: i915_{execlist, ppgtt}_notify_vgt()

* Backing store of GEM object has to be pinned, as hypervisor tracks guest memory via pfn.
    - Break by: shmem(tmpfs) swap-in/out will cause context pfn change when there are no available pages in page cache and swap cache.

SOLUTION

* Pin guest context to get an unique LRCA and stable backing store.
    - Windows driver has behave like this already without extra modification.

[ Simple Implantation - Architecture Clean Oriented ! ]


                                              Guest sees updated            
              Guest creates   Guest submits   context via virtual           
              context         context         context switch interrupt      
                 +                +              ^                          
                 |                |              |                          
 +-----------+   |                |              |                          
 |   Guest   |   |                |              |                          
 +-----------+   |                |              |                  TIMELINE
=================v================v==============+=========================>
+------------+                                                   |          
| DOM0 XenGT |                    +         ^                    |          
+------------+                    |         |  XenGT update      |          
                    +-------------+         |  guest context     |          
                    |                       |  via shadow        |          
                    |                       |  context           |          
                    |                       |                    |          
                    |                       |                    |          
                    v                       +                    |          
            XenGT creates           XenGT submits       XenGT destroy shadow
            shadow context +----->  shadow context      context             
            from guest              into hardware                           
            context                                                         
                        +-------------------------------+                   
                        | * IN-SUBMISSION-TIME          |                   
                        | SHADOW CONTEXT CREATE/DESTROY |                   
                        +-------------------------------+                   


ADVANTAGE

* No need to use notification channel
    - Avoid introduce extra modification into i915

* No need to pin guest GEM object
    - Guest backing store is pinned by i915 at the time of guest context submission.
    - Shadow context only survive in the time of submission, so unique identification between guest context and shadow context is not needed.

DISADVANTAGE

* Frequently shadow context re-create/destroy at the time of guest context submission(PERFORMANCE! MEMORY FOOTPRINT!)
    - Even trivial screen refresh will trigger guest context submission, like moving mouse over an event-sensed button in X-window.
    - May cause heavy performance drop and heavy memory footprints, due to frequently shadow context re-create/destroy. 

* High CPU usage peak due to frequently shadow context re-create/destroy at the time of guest context submission.

Hope to help the discussion! :)

Thanks,
Zhi.

-----Original Message-----
From: Wang, Zhi A 
Sent: Tuesday, August 25, 2015 1:19 AM
To: Chris Wilson; intel-gfx@lists.freedesktop.org; igvt-g@lists.01.org; Tian, Kevin; joonas.lahtinen@linux.intel.com
Subject: RE: About the iGVT-g's requirement to pin guest contexts in VM

Attach the big picture to help the discussion:

Windows Guest     Linux Guest      Linux Guest (i915 guest mode)  * We are here
+--------------+ +--------------+ +-------------------------------------------+ 
|              | |              | |    Guest Context Lifecycle Management     |
|Windows Driver| |     i915     | | +---------------------------------------+ |
|              | |          +---->| |        CREATE/DESTROY/PIN/UNPIN       | |
| (GUEST MODE) | | (GUEST MODE) | | | +----------------+ +----------------+ | | 
|              | |              | | | |Execlist Context| |Execlist Context| | | 
|              | |              | | | +-----^----------+ +------^---------+ | |
|              | |              | | +-------|-------------------|-----------+ | 
+--------------+ +--------------+ +---------|-------------------|-------------+
                                            |    NOTIFICATION   | 
                  +-------------------------|-------------------|-------------+
                  |               XenGT Shadow Context Lifecycle|Management   |
                  |                         |                   |             | 
                  |               +---------|-------------------|----------+  |  
                  | +-----------+ | +-------v-------+ +---------v------+   |  | 
                  | | i915      | | | Shadow Context| | Shadow Context |   |  | 
                  | | Host Mode | | +---------------+ +----------------+   |  |
                  | +-----------+ +----------------------------------------+  | 
                  |             DOM0 Linux (i915 host mode w/XenGT)           | 
                  +-----------------------------------------------------------+
                  +-----------------------------------------------------------+ 
                  |                     Hypervisor                            | 
                  +-----------------------------------------------------------+

SHADOW CONTEXT SUBMISSION

As you can see, in this picture, each guest execlist context will have an related shadow context in host, and XenGT will be responsible for
a. update SHADOW context via GUEST context when guest wants to submit an workload.
b. submitting the shadow context into hardware.
c. update the guest context via shadow context, when shadow context is finished.
d. inject virtual context switch interrupt into guest.

Then guest will see "hey, my job was retired from hardware, then I can do something to my context."

NOTIFICATION BETWEEN HOST AND GUEST

Now in our design we have built a notification channel for guest to notify XenGT due to performance reason.
With this channel, guest can play like this "Hey XenGT, I created a new context, please shadow it! Oh, I have destroyed the context, please stop tracking this context."

But when this trick comes before guest pin/unpin, it has problems.

PROBLEMS

First, guest context pin/unpin will cause LRCA change, which breaks relationship between guest context and shadow context.
As you can see that in our design, XenGT needs an approach to find the related shadow context according to something of an guest context.
For now we find the related shadow context via guest context ID(LRCA in fact).
But whatever it is, I think there should be something unique.

XenGT has no knowledge about guest context pin/unpin. Guest may swap out an context if it sees the seqno has been passed.
When the XenGT wants to access guest context, the guest context may be not there (swapped-out already).

It's hard and complicated for XenGT to track guest context without an unique context ID and a stable execlist context backing store.
For now the whole tricks are only works under virtualization environment and will not affect the native i915. 

Welcome to discussions! :)

Thanks,
Zhi.

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Monday, August 24, 2015 6:23 PM
To: intel-gfx@lists.freedesktop.org; igvt-g@lists.01.org; Wang, Zhi A; Tian, Kevin; joonas.lahtinen@linux.intel.com
Subject: Re: About the iGVT-g's requirement to pin guest contexts in VM

On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> Hi Chris,
> 
> On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > Intel GVT-g will perform EXECLIST context shadowing and ring 
> > > buffer shadowing. The shadow copy is created when guest creates a context.
> > > If a context changes its LRCA address, the hypervisor is hard to 
> > > know whether it is a new context or not. We always pin context 
> > > objects to global GTT to make life easier.
> > 
> > Nak. Please explain why we need to workaround a bug in the host. We 
> > cannot pin the context as that breaks userspace (e.g. synmark) who 
> > can and will try to use more contexts than we have room.
> 
> Could you have a look at below reasons and kindly give us your inputs?
> 
> 1, Due to the GGTT partitioning, the global graphics memory available 
> inside virtual machines is much smaller than native case. We cannot 
> support some graphics memory intensive workloads anyway. So it looks 
> affordable to just pin contexts which do not take much GGTT.

Wrong. It exposes the guest to a trivial denial-of-service attack. A smaller GGTT does not actually limit clients (there is greater aperture pressure and some paths are less likely but an individual client will function just fine).
 
> 2, Our hypervisor needs to change i915 guest context in the shadow 
> context implementation. That part will be tricky if the context is not 
> always pinned. One scenario is that when a context finishes running, 
> we need to copy shadow context, which has been updated by hardware, to 
> guest context. The hypervisor knows context finishing by context 
> interrupt, but that time shrinker may have unpin the context and its 
> backing storage may have been swap-out. Such copy may fail.

That is just a bug in your code. Firstly allowing swapout on an object you still are using, secondly not being able to swapin.
-Chris

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

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Zhiyuan Lv]

Hi Daniel,

On Wed, Aug 26, 2015 at 10:56:00AM +0200, Daniel Vetter wrote:
> On Tue, Aug 25, 2015 at 08:17:05AM +0800, Zhiyuan Lv wrote:
> > Hi Chris,
> > 
> > On Mon, Aug 24, 2015 at 11:23:13AM +0100, Chris Wilson wrote:
> > > On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> > > > Hi Chris,
> > > > 
> > > > On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > > > > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > > > > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > > > > > shadowing. The shadow copy is created when guest creates a context.
> > > > > > If a context changes its LRCA address, the hypervisor is hard to know
> > > > > > whether it is a new context or not. We always pin context objects to
> > > > > > global GTT to make life easier.
> > > > > 
> > > > > Nak. Please explain why we need to workaround a bug in the host. We
> > > > > cannot pin the context as that breaks userspace (e.g. synmark) who can
> > > > > and will try to use more contexts than we have room.
> > > > 
> > > > Could you have a look at below reasons and kindly give us your inputs?
> > > > 
> > > > 1, Due to the GGTT partitioning, the global graphics memory available
> > > > inside virtual machines is much smaller than native case. We cannot
> > > > support some graphics memory intensive workloads anyway. So it looks
> > > > affordable to just pin contexts which do not take much GGTT.
> > > 
> > > Wrong. It exposes the guest to a trivial denial-of-service attack. A
> > 
> > Inside a VM, indeed.
> > 
> > > smaller GGTT does not actually limit clients (there is greater aperture
> > > pressure and some paths are less likely but an individual client will
> > > function just fine).
> > >  
> > > > 2, Our hypervisor needs to change i915 guest context in the shadow
> > > > context implementation. That part will be tricky if the context is not
> > > > always pinned. One scenario is that when a context finishes running,
> > > > we need to copy shadow context, which has been updated by hardware, to
> > > > guest context. The hypervisor knows context finishing by context
> > > > interrupt, but that time shrinker may have unpin the context and its
> > > > backing storage may have been swap-out. Such copy may fail. 
> > > 
> > > That is just a bug in your code. Firstly allowing swapout on an object
> > > you still are using, secondly not being able to swapin.
> > 
> > As Zhi replied in another email, we do not have the knowledge of guest
> > driver's swap operations. If we cannot pin context, we may have to ask
> > guest driver not to swap out context pages. Do you think that would be
> > the right way to go? Thanks!
> 
> It doesn't matter at all - if the guest unpins the ctx and puts something
> else in there before the host tells it that the ctx is completed, that's a
> bug in the guest. Same with real hw, we guarantee that the context stays
> around for long enough.

You are right. Previously I did not realize that shrinker will check
not only the seqno, but also "ACTIVE_TO_IDLE" context interrupt for
unpinning a context, then had above concern. Thanks for the
explanation!

> 
> Also you obviously have to complete the copying from shadow->guest ctx
> before you send the irq to the guest to signal ctx completion. Which means
> there's really no overall problem here from a design pov, the only thing

Right. We cannot control when guest driver sees seqno change, but we
can control when guest sees context interrupts. The guest CSB update
and interrupt injection will be after we finish writing guest
contexts.

So right now we have two options of context shadowing: one is to track
the whole life-cycle of guest context, and another is to do the shadow
work in context schedule-in/schedule-out time. Zhi draws a nice
picture of them.

Currently we do not have concrete performance comparison of the two
approaches. We will have a try and see. And about this patchset, I
will remove the "context notification" part and send out an updated
version. Thanks!

> you have to do is fix up bugs in the host code (probably you should just
> write through the ggtt).

Sorry could you elaborate a little more about this? Guest context may
not always be in aperture right?

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Enable full ppgtt for vgpu

[Zhiyuan Lv]

Hi Danie,

On Wed, Aug 26, 2015 at 10:47:37AM +0200, Daniel Vetter wrote:
> On Thu, Aug 20, 2015 at 01:57:13PM +0300, Joonas Lahtinen wrote:
> > On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > > The full ppgtt is supported in Intel GVT-g device model. So the
> > > restriction can be removed.
> > > 
> > > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> > 
> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
> > >  1 file changed, 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index ed10e77..823005c 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -108,9 +108,6 @@ static int sanitize_enable_ppgtt(struct 
> > > drm_device *dev, int enable_ppgtt)
> > >  	has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
> > >  	has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
> > >  
> > > -	if (intel_vgpu_active(dev))
> > > -		has_full_ppgtt = false; /* emulation is too hard */
> 
> Don't we need a feature check for the virtual gpu here? Or at least a
> platform check? Seems like the backwards/forwards compat story isn't too
> thought out yet here. Note that the kernel of the host and the guest might
> not be the same at all, much less the kvm part.

Yeah, backwards/forwards compatibility is not considered, since we are
just to start the upstream of iGVT-g host changes. Right now if people
uses new enough iGVT-g code (off-tree now), both HSW and BDW should
work with PPGTT.

So new host with both old guest or new guest are working. The only
thing impacted is old host with new guest kernel. In order to keep it
work, I can change the code like:

+	if (intel_vgpu_active(dev))
+		has_full_ppgtt = !IS_HASWELL(dev);

Any comments? Thanks for the review!

Regards,
-Zhiyuan

> -Daniel
> 
> > > -
> > >  	/*
> > >  	 * We don't allow disabling PPGTT for gen9+ as it's a 
> > > requirement for
> > >  	 * execlists, the sole mechanism available to submit work.
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Zhiyuan Lv]

Hi Daniel,

On Wed, Aug 26, 2015 at 10:50:23AM +0200, Daniel Vetter wrote:
> > > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device 
> > > *dev)
> > >         if (WARN_ON(dev_priv->ring[RCS].default_context))
> > >                 return 0;
> > >  
> > > +       if (intel_vgpu_active(dev)) {
> > > +               if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) &&
> > > +                           !i915.enable_execlist))
> > > +                       return 0;
> > > +       }
> > > +
> > 
> > This looks fine to me. Maybe comment might be in place stating that
> > support is not yet implemented, but could be.
> 
> You should fail this instead so that i915.ko knows that the render side of
> the gpu doesn't work. And maybe just DRM_INFO with a useful informational
> notice?

Thanks for the comments! Will change.

> 
> Also same comment here: Don't we need to coordinate this a bit better with
> the host?

In host side, if driver is running in ring buffer mode, we will let GVT-g
initialization fail, so that guest cannot set vgpu active. Would that be good
enough? Thanks!

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915: Enable full ppgtt for vgpu

[Daniel Vetter]

On Thu, Aug 27, 2015 at 10:28:49AM +0800, Zhiyuan Lv wrote:
> Hi Danie,
> 
> On Wed, Aug 26, 2015 at 10:47:37AM +0200, Daniel Vetter wrote:
> > On Thu, Aug 20, 2015 at 01:57:13PM +0300, Joonas Lahtinen wrote:
> > > On to, 2015-08-20 at 15:45 +0800, Zhiyuan Lv wrote:
> > > > The full ppgtt is supported in Intel GVT-g device model. So the
> > > > restriction can be removed.
> > > > 
> > > > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > > > Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> > > 
> > > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ---
> > > >  1 file changed, 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > index ed10e77..823005c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -108,9 +108,6 @@ static int sanitize_enable_ppgtt(struct 
> > > > drm_device *dev, int enable_ppgtt)
> > > >  	has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
> > > >  	has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
> > > >  
> > > > -	if (intel_vgpu_active(dev))
> > > > -		has_full_ppgtt = false; /* emulation is too hard */
> > 
> > Don't we need a feature check for the virtual gpu here? Or at least a
> > platform check? Seems like the backwards/forwards compat story isn't too
> > thought out yet here. Note that the kernel of the host and the guest might
> > not be the same at all, much less the kvm part.
> 
> Yeah, backwards/forwards compatibility is not considered, since we are
> just to start the upstream of iGVT-g host changes. Right now if people
> uses new enough iGVT-g code (off-tree now), both HSW and BDW should
> work with PPGTT.
> 
> So new host with both old guest or new guest are working. The only
> thing impacted is old host with new guest kernel. In order to keep it
> work, I can change the code like:
> 
> +	if (intel_vgpu_active(dev))
> +		has_full_ppgtt = !IS_HASWELL(dev);
> 
> Any comments? Thanks for the review!

In the end that's your call (well until we have users screaming that a
kernel upgrade breaks their xengt setup, then you have to keep backwards
compat). But since it's a requirement from Linus Torvalds for upstream I
highly suggest you start thinking/testing for backwards compat (both on
host and guest side when the other part is older) right away. Otherwise
trying to shoehorn a good backwards compat scheme later on could be really
painful.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915: Always enable execlists on BDW for vgpu

[Daniel Vetter]

On Thu, Aug 27, 2015 at 10:49:28AM +0800, Zhiyuan Lv wrote:
> Hi Daniel,
> 
> On Wed, Aug 26, 2015 at 10:50:23AM +0200, Daniel Vetter wrote:
> > > > @@ -332,6 +332,12 @@ int i915_gem_context_init(struct drm_device 
> > > > *dev)
> > > >         if (WARN_ON(dev_priv->ring[RCS].default_context))
> > > >                 return 0;
> > > >  
> > > > +       if (intel_vgpu_active(dev)) {
> > > > +               if (WARN_ON(HAS_LOGICAL_RING_CONTEXTS(dev) &&
> > > > +                           !i915.enable_execlist))
> > > > +                       return 0;
> > > > +       }
> > > > +
> > > 
> > > This looks fine to me. Maybe comment might be in place stating that
> > > support is not yet implemented, but could be.
> > 
> > You should fail this instead so that i915.ko knows that the render side of
> > the gpu doesn't work. And maybe just DRM_INFO with a useful informational
> > notice?
> 
> Thanks for the comments! Will change.
> 
> > 
> > Also same comment here: Don't we need to coordinate this a bit better with
> > the host?
> 
> In host side, if driver is running in ring buffer mode, we will let GVT-g
> initialization fail, so that guest cannot set vgpu active. Would that be good
> enough? Thanks!

Yeah that seems sensible.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Daniel Vetter]

On Thu, Aug 27, 2015 at 09:50:03AM +0800, Zhiyuan Lv wrote:
> Hi Daniel,
> 
> On Wed, Aug 26, 2015 at 10:56:00AM +0200, Daniel Vetter wrote:
> > On Tue, Aug 25, 2015 at 08:17:05AM +0800, Zhiyuan Lv wrote:
> > > Hi Chris,
> > > 
> > > On Mon, Aug 24, 2015 at 11:23:13AM +0100, Chris Wilson wrote:
> > > > On Mon, Aug 24, 2015 at 06:04:28PM +0800, Zhiyuan Lv wrote:
> > > > > Hi Chris,
> > > > > 
> > > > > On Thu, Aug 20, 2015 at 09:36:00AM +0100, Chris Wilson wrote:
> > > > > > On Thu, Aug 20, 2015 at 03:45:21PM +0800, Zhiyuan Lv wrote:
> > > > > > > Intel GVT-g will perform EXECLIST context shadowing and ring buffer
> > > > > > > shadowing. The shadow copy is created when guest creates a context.
> > > > > > > If a context changes its LRCA address, the hypervisor is hard to know
> > > > > > > whether it is a new context or not. We always pin context objects to
> > > > > > > global GTT to make life easier.
> > > > > > 
> > > > > > Nak. Please explain why we need to workaround a bug in the host. We
> > > > > > cannot pin the context as that breaks userspace (e.g. synmark) who can
> > > > > > and will try to use more contexts than we have room.
> > > > > 
> > > > > Could you have a look at below reasons and kindly give us your inputs?
> > > > > 
> > > > > 1, Due to the GGTT partitioning, the global graphics memory available
> > > > > inside virtual machines is much smaller than native case. We cannot
> > > > > support some graphics memory intensive workloads anyway. So it looks
> > > > > affordable to just pin contexts which do not take much GGTT.
> > > > 
> > > > Wrong. It exposes the guest to a trivial denial-of-service attack. A
> > > 
> > > Inside a VM, indeed.
> > > 
> > > > smaller GGTT does not actually limit clients (there is greater aperture
> > > > pressure and some paths are less likely but an individual client will
> > > > function just fine).
> > > >  
> > > > > 2, Our hypervisor needs to change i915 guest context in the shadow
> > > > > context implementation. That part will be tricky if the context is not
> > > > > always pinned. One scenario is that when a context finishes running,
> > > > > we need to copy shadow context, which has been updated by hardware, to
> > > > > guest context. The hypervisor knows context finishing by context
> > > > > interrupt, but that time shrinker may have unpin the context and its
> > > > > backing storage may have been swap-out. Such copy may fail. 
> > > > 
> > > > That is just a bug in your code. Firstly allowing swapout on an object
> > > > you still are using, secondly not being able to swapin.
> > > 
> > > As Zhi replied in another email, we do not have the knowledge of guest
> > > driver's swap operations. If we cannot pin context, we may have to ask
> > > guest driver not to swap out context pages. Do you think that would be
> > > the right way to go? Thanks!
> > 
> > It doesn't matter at all - if the guest unpins the ctx and puts something
> > else in there before the host tells it that the ctx is completed, that's a
> > bug in the guest. Same with real hw, we guarantee that the context stays
> > around for long enough.
> 
> You are right. Previously I did not realize that shrinker will check
> not only the seqno, but also "ACTIVE_TO_IDLE" context interrupt for
> unpinning a context, then had above concern. Thanks for the
> explanation!
> 
> > 
> > Also you obviously have to complete the copying from shadow->guest ctx
> > before you send the irq to the guest to signal ctx completion. Which means
> > there's really no overall problem here from a design pov, the only thing
> 
> Right. We cannot control when guest driver sees seqno change, but we
> can control when guest sees context interrupts. The guest CSB update
> and interrupt injection will be after we finish writing guest
> contexts.
> 
> So right now we have two options of context shadowing: one is to track
> the whole life-cycle of guest context, and another is to do the shadow
> work in context schedule-in/schedule-out time. Zhi draws a nice
> picture of them.
> 
> Currently we do not have concrete performance comparison of the two
> approaches. We will have a try and see. And about this patchset, I
> will remove the "context notification" part and send out an updated
> version. Thanks!
> 
> > you have to do is fix up bugs in the host code (probably you should just
> > write through the ggtt).
> 
> Sorry could you elaborate a little more about this? Guest context may
> not always be in aperture right?

Yeah the high-level problem is that global gtt is contended (we already
have trouble with that on xengt and there's the ongoing but unfished
partial mmap support for that). And permanently pinning execlist contexts
will cause lots of troubles.

Windows can do this because it segments the global gtt into different
parts (at least last time I looked at their memory manager), which means
execlist will never sit in the middle of the range used for mmaps. But
linux has a unified memory manager, which means execlist can sit anywhere,
and therefore badly fragment the global gtt. If we pin them then that will
cause trouble after long system uptime. And afaiui xengt is mostly aimed
at servers, where the uptime assumption should be "runs forever".

Compounding factor is that despite that I raised this in the original
review execlist is still not yet using the active list in upstream and
instead does short-time pinning. It's better than pinning forever but
still breaks the eviction logic.

What Chris Wilson and I talked about forever is adding an object-specific
global_gtt_unbind hook. The idea is that this would be called when
unbinding/evicting a special object (e.g. hw context), and you could use
that to do the host signalling. That would be the perfect combination of
both approaches:

- Fast: host signalling (and therefore shadow context recreation) would
  only be done when the execlist context has actually moved around. That
  almost never happens, and hence per-execbuf overhead would be as low as
  with your pinning solution.

- Flexible: The i915 memory manager is still in full control since we
  don't pin any objects unecessarily.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Zhiyuan Lv]

Hi Daniel,

Thanks for the comments! And my reply in line:

On Wed, Sep 02, 2015 at 10:19:03AM +0200, Daniel Vetter wrote:
> > > 
> > > Also you obviously have to complete the copying from shadow->guest ctx
> > > before you send the irq to the guest to signal ctx completion. Which means
> > > there's really no overall problem here from a design pov, the only thing
> > 
> > Right. We cannot control when guest driver sees seqno change, but we
> > can control when guest sees context interrupts. The guest CSB update
> > and interrupt injection will be after we finish writing guest
> > contexts.
> > 
> > So right now we have two options of context shadowing: one is to track
> > the whole life-cycle of guest context, and another is to do the shadow
> > work in context schedule-in/schedule-out time. Zhi draws a nice
> > picture of them.
> > 
> > Currently we do not have concrete performance comparison of the two
> > approaches. We will have a try and see. And about this patchset, I
> > will remove the "context notification" part and send out an updated
> > version. Thanks!
> > 
> > > you have to do is fix up bugs in the host code (probably you should just
> > > write through the ggtt).
> > 
> > Sorry could you elaborate a little more about this? Guest context may
> > not always be in aperture right?
> 
> Yeah the high-level problem is that global gtt is contended (we already
> have trouble with that on xengt and there's the ongoing but unfished
> partial mmap support for that). And permanently pinning execlist contexts
> will cause lots of troubles.
> 
> Windows can do this because it segments the global gtt into different
> parts (at least last time I looked at their memory manager), which means
> execlist will never sit in the middle of the range used for mmaps. But
> linux has a unified memory manager, which means execlist can sit anywhere,
> and therefore badly fragment the global gtt. If we pin them then that will
> cause trouble after long system uptime. And afaiui xengt is mostly aimed
> at servers, where the uptime assumption should be "runs forever".

In server side, we do not expect host to run much graphics workloads
(all should be in virtual machines with shorter uptime). But yeah, gm
fragmentation is still an issue. Thanks for the explanation!

> 
> Compounding factor is that despite that I raised this in the original
> review execlist is still not yet using the active list in upstream and
> instead does short-time pinning. It's better than pinning forever but
> still breaks the eviction logic.
> 
> What Chris Wilson and I talked about forever is adding an object-specific
> global_gtt_unbind hook. The idea is that this would be called when
> unbinding/evicting a special object (e.g. hw context), and you could use
> that to do the host signalling. That would be the perfect combination of
> both approaches:

Thanks for the suggestion! That sounds great! Right now in XenGT part
we still plan to try the shadow context work in context
schedule-in/out time. With this way, we do not need to pin context as
well as the host notification. We will collect some performance data
to see how it works.

I sent out v2 patch set which has removed the pin/unpin of execlist
contexts. The patchset addressed review comments from you, Chris and
Joonas(Sorry I forgot to add CC to related people). Is that patch set
OK to be merged? Thanks!

Regards,
-Zhiyuan

> 
> - Fast: host signalling (and therefore shadow context recreation) would
>   only be done when the execlist context has actually moved around. That
>   almost never happens, and hence per-execbuf overhead would be as low as
>   with your pinning solution.
> 
> - Flexible: The i915 memory manager is still in full control since we
>   don't pin any objects unecessarily.
> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: About the iGVT-g's requirement to pin guest contexts in VM

[Daniel Vetter]

On Wed, Sep 02, 2015 at 05:20:34PM +0800, Zhiyuan Lv wrote:
> Hi Daniel,
> 
> Thanks for the comments! And my reply in line:
> 
> On Wed, Sep 02, 2015 at 10:19:03AM +0200, Daniel Vetter wrote:
> > > > 
> > > > Also you obviously have to complete the copying from shadow->guest ctx
> > > > before you send the irq to the guest to signal ctx completion. Which means
> > > > there's really no overall problem here from a design pov, the only thing
> > > 
> > > Right. We cannot control when guest driver sees seqno change, but we
> > > can control when guest sees context interrupts. The guest CSB update
> > > and interrupt injection will be after we finish writing guest
> > > contexts.
> > > 
> > > So right now we have two options of context shadowing: one is to track
> > > the whole life-cycle of guest context, and another is to do the shadow
> > > work in context schedule-in/schedule-out time. Zhi draws a nice
> > > picture of them.
> > > 
> > > Currently we do not have concrete performance comparison of the two
> > > approaches. We will have a try and see. And about this patchset, I
> > > will remove the "context notification" part and send out an updated
> > > version. Thanks!
> > > 
> > > > you have to do is fix up bugs in the host code (probably you should just
> > > > write through the ggtt).
> > > 
> > > Sorry could you elaborate a little more about this? Guest context may
> > > not always be in aperture right?
> > 
> > Yeah the high-level problem is that global gtt is contended (we already
> > have trouble with that on xengt and there's the ongoing but unfished
> > partial mmap support for that). And permanently pinning execlist contexts
> > will cause lots of troubles.
> > 
> > Windows can do this because it segments the global gtt into different
> > parts (at least last time I looked at their memory manager), which means
> > execlist will never sit in the middle of the range used for mmaps. But
> > linux has a unified memory manager, which means execlist can sit anywhere,
> > and therefore badly fragment the global gtt. If we pin them then that will
> > cause trouble after long system uptime. And afaiui xengt is mostly aimed
> > at servers, where the uptime assumption should be "runs forever".
> 
> In server side, we do not expect host to run much graphics workloads
> (all should be in virtual machines with shorter uptime). But yeah, gm
> fragmentation is still an issue. Thanks for the explanation!

Fragmentation is a lot worse on the guest side (due to the reduce global
gtt space due to ballooning). Host isn't really any different.

> > Compounding factor is that despite that I raised this in the original
> > review execlist is still not yet using the active list in upstream and
> > instead does short-time pinning. It's better than pinning forever but
> > still breaks the eviction logic.
> > 
> > What Chris Wilson and I talked about forever is adding an object-specific
> > global_gtt_unbind hook. The idea is that this would be called when
> > unbinding/evicting a special object (e.g. hw context), and you could use
> > that to do the host signalling. That would be the perfect combination of
> > both approaches:
> 
> Thanks for the suggestion! That sounds great! Right now in XenGT part
> we still plan to try the shadow context work in context
> schedule-in/out time. With this way, we do not need to pin context as
> well as the host notification. We will collect some performance data
> to see how it works.
> 
> I sent out v2 patch set which has removed the pin/unpin of execlist
> contexts. The patchset addressed review comments from you, Chris and
> Joonas(Sorry I forgot to add CC to related people). Is that patch set
> OK to be merged? Thanks!

I'll defer to detailed reviewers, but if the static pinning is gone then
I'm ok. We can add the optimization I described here later on.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915: always pin lrc context for vgpu with Intel GVT-g

[Zhiyuan Lv]

Intel GVT-g will perform EXECLIST context shadowing and ring buffer
shadowing. The shadow copy is created when guest creates a context.
If a context changes its LRCA address, the hypervisor is hard to know
whether it is a new context or not. We always pin context objects to
global GTT to make life easier.

Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 39df304..4b2ac37 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2282,7 +2282,8 @@ void intel_lr_context_free(struct intel_context *ctx)
 					ctx->engine[i].ringbuf;
 			struct intel_engine_cs *ring = ringbuf->ring;
 
-			if (ctx == ring->default_context) {
+			if ((ctx == ring->default_context) ||
+			    (intel_vgpu_active(ring->dev))) {
 				intel_unpin_ringbuffer_obj(ringbuf);
 				i915_gem_object_ggtt_unpin(ctx_obj);
 			}
@@ -2353,6 +2354,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 				     struct intel_engine_cs *ring)
 {
 	const bool is_global_default_ctx = (ctx == ring->default_context);
+	const bool need_to_pin_ctx = (is_global_default_ctx ||
+				      (intel_vgpu_active(ring->dev)));
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *ctx_obj;
@@ -2374,7 +2377,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 		return -ENOMEM;
 	}
 
-	if (is_global_default_ctx) {
+	if (need_to_pin_ctx) {
 		ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN,
 				PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
 		if (ret) {
@@ -2415,7 +2418,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 			goto error_free_rbuf;
 		}
 
-		if (is_global_default_ctx) {
+		if (need_to_pin_ctx) {
 			ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
 			if (ret) {
 				DRM_ERROR(
@@ -2464,14 +2467,14 @@ int intel_lr_context_deferred_create(struct intel_context *ctx,
 	return 0;
 
 error:
-	if (is_global_default_ctx)
+	if (need_to_pin_ctx)
 		intel_unpin_ringbuffer_obj(ringbuf);
 error_destroy_rbuf:
 	intel_destroy_ringbuffer_obj(ringbuf);
 error_free_rbuf:
 	kfree(ringbuf);
 error_unpin_ctx:
-	if (is_global_default_ctx)
+	if (need_to_pin_ctx)
 		i915_gem_object_ggtt_unpin(ctx_obj);
 	drm_gem_object_unreference(&ctx_obj->base);
 	return ret;
-- 
1.9.1

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