[RFC PATCH 0/8] Add host i915 support for vGPU

[RFC PATCH 0/8] Add host i915 support for vGPU

[Jike Song]

Intel GVT-g (previously known as XenGT), is a complete GPU
virtualization solution with mediated pass-through for 4th
generation Intel Core processors - Haswell platform. This
technology presents a virtual full-fledged GPU to each Virtual
Machine (VM). VMs can directly access performance-critical
resources, without intervention from the hypervisor in most
cases, while privileged operations from VMs are trap-and-emulated
at minimal cost. For details, please refer to:

  https://01.org/xen/blogs/wangbo85/2014/intel-gvt-gxengt-pubic-release

The patches of adding vGPU guest support for i915, is already posted at:

  http://lists.freedesktop.org/archives/intel-gfx/2014-September/052571.html


This patch set is about to add vGPU host support.

When running as vGPU host, the i915 driver can't simply occupy
the whole GPU resources, it needs to proactively meidate the
accesses to most hardware resources: MMIO, GTT, and interrupt.
Only by handling the hardware resources, the vgt can have
centralized management about sharing between VMs(including
host and guest).


This patch set adds:

	- an i915 extension, named vgt. vgt is the major part of Intel
	  GVT-g implementation, it manages and shares GPU among VMs
	  and host;

	- interfaces between i915 and vgt, including:

		Mediated MMIO access
		Mediated GTT access
		Mediated IRQ handling

NOTE of RFC:

	- the vgt in-kernel GPU device-model is not yet integrated, though
	  the interfaces between i915 and vgt are provided;

	- the host i915 driver has some logic same with the guest i915,
	  e.g. the ballooning. When the guest patches(see above) are
	  finalized, we need to rebase upon them;

	- vgt_suspend/vgt_resume are still under cleanup;

	- GPU reset is still under cleanup


  We send out the framework changes in this patch set for review
at first, and meanwhile, vgt integration and cleanup are ongoing.


Jike Song (8):
  drm/i915: introduce a new modparam: enable_vgt
  drm/i915: introduce the skeleton of vgt
  drm/i915: add the vgt implementation of MMIO/GTT mediations
  drm/i915: redirect MMIO accesses to vgt if enabled
  drm/i915: GTT access abstraction
  drm/i915: redirect GTT accesses to vgt if enabled
  drm/i915: vgt irq mediation - via a tasklet based mechanism
  drm/i915: enable vgt if specified by module param

 drivers/gpu/drm/i915/Kconfig        |  18 ++++
 drivers/gpu/drm/i915/Makefile       |   4 +
 drivers/gpu/drm/i915/i915_drv.c     |  10 ++
 drivers/gpu/drm/i915/i915_drv.h     | 204 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_gtt.c |  33 +++---
 drivers/gpu/drm/i915/i915_irq.c     |  35 +++++++
 drivers/gpu/drm/i915/i915_params.c  |   4 +
 drivers/gpu/drm/i915/i915_vgt.h     |  58 ++++++++++
 drivers/gpu/drm/i915/intel_uncore.c |   3 +
 drivers/gpu/drm/i915/vgt/vgt.c      | 145 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/vgt/vgt.h      |   6 ++
 11 files changed, 492 insertions(+), 28 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_vgt.h
 create mode 100644 drivers/gpu/drm/i915/vgt/vgt.c
 create mode 100644 drivers/gpu/drm/i915/vgt/vgt.h

-- 
1.9.1

[RFC PATCH 1/8] drm/i915: introduce a new modparam: enable_vgt

[Jike Song]

To provide Intel GPU virtualization, the host i915
driver needs to support vgt - an in-kernel device
model of Intel GPU.

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h    | 1 +
 drivers/gpu/drm/i915/i915_params.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ed6cf10..c3b03f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2260,6 +2260,7 @@ struct i915_params {
 	bool disable_vtd_wa;
 	int use_mmio_flip;
 	bool mmio_debug;
+	bool enable_vgt;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 139f490..552949a 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -51,6 +51,7 @@ struct i915_params i915 __read_mostly = {
 	.disable_vtd_wa = 0,
 	.use_mmio_flip = 0,
 	.mmio_debug = 0,
+	.enable_vgt = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -173,3 +174,6 @@ module_param_named(mmio_debug, i915.mmio_debug, bool, 0600);
 MODULE_PARM_DESC(mmio_debug,
 	"Enable the MMIO debug code (default: false). This may negatively "
 	"affect performance.");
+
+module_param_named(enable_vgt, i915.enable_vgt, bool, 0600);
+MODULE_PARM_DESC(enable_vgt, "Enable the Intel GVT-g Host support for GPU Virtulization");
-- 
1.9.1

[RFC PATCH 2/8] drm/i915: introduce the skeleton of vgt

[Jike Song]

This patch introduces the skeleton of vgt, an i915 add-on
for controlling physical GPU resources and sharing among VMs.

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/Kconfig    | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/Makefile   |  4 ++++
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++++
 drivers/gpu/drm/i915/i915_vgt.h | 17 +++++++++++++++++
 drivers/gpu/drm/i915/vgt/vgt.c  | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/vgt/vgt.h  |  6 ++++++
 6 files changed, 73 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/i915_vgt.h
 create mode 100644 drivers/gpu/drm/i915/vgt/vgt.c
 create mode 100644 drivers/gpu/drm/i915/vgt/vgt.h

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 4e39ab3..aa87e75 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -69,3 +69,21 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
 	  option changes the default for that module option.
 
 	  If in doubt, say "N".
+
+config I915_IGVT
+	bool "I915 Intel Graphics Virtualization Technology for shared vCPU(Intel GVT-g)"
+	depends on DRM_I915
+	default n
+	help
+	  Intel GVT-g is an efficient GPU sharing technology among multiple Virtual
+	  Machines (VMs), providing full GPU virtualization so native graphics driver
+	  can run inside a VM seamlessly. Both 3D/Media/Compute tasks can be
+	  accelerated simultaneously in multi-VMs, on an Intel Processor Graphics.
+	  Intel GVT-g adopts a mediated pass-through concept, by passing through
+	  performance-critical operations (frame buffer access and command submission),
+	  while trap-and-emulating privileged operations (I/O, GPU page table, etc.).
+	  Overall it can achieve a good balance between performance, feature and
+	  sharing capability.
+
+	  This option specifically enables 'vgt' component in i915 driver,
+	  implementing vGPU device model and GPU sharing capability.
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 2d8317d..8379d19 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -78,6 +78,10 @@ i915-y += dvo_ch7017.o \
 i915-y += i915_dma.o \
 	  i915_ums.o
 
+
+VGT				:= vgt
+i915-$(CONFIG_I915_IGVT)	+= $(VGT)/vgt.o
+
 obj-$(CONFIG_DRM_I915)  += i915.o
 
 CFLAGS_i915_trace_points.o := -I$(src)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6948877..6dbb706 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -35,6 +35,8 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+#include "i915_vgt.h"
+
 #include <linux/console.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
@@ -923,6 +925,14 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	driver.driver_features &= ~(DRIVER_USE_AGP);
 
+	if (i915.enable_vgt) {
+		if (!i915_start_vgt(pdev))
+			i915.enable_vgt = false;
+
+		DRM_INFO("i915_start_vgt %s\n", i915.enable_vgt ?
+					"succedded" : "failed");
+	}
+
 	return drm_get_pci_dev(pdev, ent, &driver);
 }
 
diff --git a/drivers/gpu/drm/i915/i915_vgt.h b/drivers/gpu/drm/i915/i915_vgt.h
new file mode 100644
index 0000000..c6a4144
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_vgt.h
@@ -0,0 +1,17 @@
+#ifndef _I915_VGT_H_
+#define _I915_VGT_H_
+
+#ifdef CONFIG_I915_IGVT
+
+bool i915_start_vgt(struct pci_dev *);
+
+#else /* !CONFIG_I915_IGVT */
+
+static inline bool i915_start_vgt(struct pci_dev *pdev)
+{
+	return false;
+}
+
+#endif /* CONFIG_I915_IGVT */
+
+#endif
diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c
new file mode 100644
index 0000000..07ccee6
--- /dev/null
+++ b/drivers/gpu/drm/i915/vgt/vgt.c
@@ -0,0 +1,18 @@
+#include <linux/module.h>
+#include <linux/kthread.h>
+#include <linux/pci.h>
+
+#include "vgt.h"
+
+
+/**
+ * Initialize Intel GVT-g
+ *
+ * \return true for success
+ * \return false for failure
+ */
+bool i915_start_vgt(struct pci_dev *pdev)
+{
+	/* vgt is not yet integrated, this only means testing */
+	return false;
+}
diff --git a/drivers/gpu/drm/i915/vgt/vgt.h b/drivers/gpu/drm/i915/vgt/vgt.h
new file mode 100644
index 0000000..62d03fd
--- /dev/null
+++ b/drivers/gpu/drm/i915/vgt/vgt.h
@@ -0,0 +1,6 @@
+#ifndef _VGT_DRV_H_
+#define _VGT_DRV_H_
+
+#include "../i915_vgt.h"
+
+#endif
-- 
1.9.1

[RFC PATCH 3/8] drm/i915: add the vgt implementation of MMIO/GTT mediations

[Jike Song]

vgt mediates GPU operations from host i915, in the same way as
mediating GPU operations from normal VMs. This way vgt can
have centralized management about sharing among host and other
VMs. To achieve that, we add a hook in critical wrapper interfaces
(MMIO/GTT).

This patch only adds the MMIO/GTT accessing functions, without
changing the existing i915 MMIO/GTT access behaviors.

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_vgt.h |  21 ++++++++
 drivers/gpu/drm/i915/vgt/vgt.c  | 105 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_vgt.h b/drivers/gpu/drm/i915/i915_vgt.h
index c6a4144..03e7f00 100644
--- a/drivers/gpu/drm/i915/i915_vgt.h
+++ b/drivers/gpu/drm/i915/i915_vgt.h
@@ -1,9 +1,14 @@
 #ifndef _I915_VGT_H_
 #define _I915_VGT_H_
 
+struct drm_i915_private;
+
 #ifdef CONFIG_I915_IGVT
 
 bool i915_start_vgt(struct pci_dev *);
+void i915_vgt_record_priv(struct drm_i915_private *);
+bool vgt_emulate_host_read(u32, void *, int, bool, bool);
+bool vgt_emulate_host_write(u32, void *, int, bool, bool);
 
 #else /* !CONFIG_I915_IGVT */
 
@@ -12,6 +17,22 @@ static inline bool i915_start_vgt(struct pci_dev *pdev)
 	return false;
 }
 
+static inline void i915_vgt_record_priv(struct drm_i915_private *priv)
+{
+}
+
+static inline bool vgt_emulate_host_read(u32 reg, void *val, int len,
+			bool is_gtt, bool trace)
+{
+	return false;
+}
+
+static inline bool vgt_emulate_host_write(u32 reg, void *val, int len,
+			bool is_gtt, bool trace)
+{
+	return false;
+}
+
 #endif /* CONFIG_I915_IGVT */
 
 #endif
diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c
index 07ccee6..f33baf3 100644
--- a/drivers/gpu/drm/i915/vgt/vgt.c
+++ b/drivers/gpu/drm/i915/vgt/vgt.c
@@ -2,8 +2,12 @@
 #include <linux/kthread.h>
 #include <linux/pci.h>
 
+#include "../i915_drv.h"
 #include "vgt.h"
 
+const static bool vgt_integration_done = false;
+static struct drm_i915_private *dev_priv = NULL;
+
 
 /**
  * Initialize Intel GVT-g
@@ -16,3 +20,104 @@ bool i915_start_vgt(struct pci_dev *pdev)
 	/* vgt is not yet integrated, this only means testing */
 	return false;
 }
+
+static bool vgt_mmio_read(off_t reg, void *val, int len, bool trace)
+{
+	switch (len) {
+	case 1:
+		*(u8 *)val = dev_priv->uncore.funcs.mmio_readb(dev_priv,
+					reg, trace);
+		break;
+	case 2:
+		*(u16 *)val = dev_priv->uncore.funcs.mmio_readw(dev_priv,
+				reg, trace);
+		break;
+	case 4:
+		*(u32 *)val = dev_priv->uncore.funcs.mmio_readl(dev_priv,
+					reg, trace);
+		break;
+	case 8:
+		*(u64 *)val = dev_priv->uncore.funcs.mmio_readq(dev_priv,
+					reg, trace);
+		break;
+	default:
+		return false;
+	}
+	return true;
+}
+
+static bool vgt_mmio_write(off_t reg, void *val, int len, bool trace)
+{
+	switch (len) {
+	case 1:
+		dev_priv->uncore.funcs.mmio_writeb(dev_priv,
+					reg, *(u8 *)val, trace);
+		break;
+	case 2:
+		dev_priv->uncore.funcs.mmio_writew(dev_priv,
+					reg, *(u16 *)val, trace);
+		break;
+	case 4:
+		dev_priv->uncore.funcs.mmio_writel(dev_priv,
+					reg, *(u32 *)val, trace);
+		break;
+	case 8:
+		dev_priv->uncore.funcs.mmio_writeq(dev_priv,
+					reg, *(u64 *)val, trace);
+		break;
+	default:
+		return false;
+	}
+	return true;
+}
+
+static bool vgt_gtt_read(off_t reg, void *val, int len)
+{
+	switch (len) {
+	case 4:
+		*(u32 *)val = readl(reg + dev_priv->gtt.gsm);
+		break;
+	case 8:
+		*(u64 *)val = readq(reg + dev_priv->gtt.gsm);
+		break;
+	default:
+		return false;
+	}
+	return true;
+}
+
+static bool vgt_gtt_write(off_t reg, void *val, int len)
+{
+	switch (len) {
+	case 4:
+		writel(*(u32 *)val, reg + dev_priv->gtt.gsm);
+		break;
+	case 8:
+		writeq(*(u64 *)val, reg + dev_priv->gtt.gsm);
+		break;
+	default:
+		return false;
+	}
+	return true;
+}
+
+bool vgt_emulate_host_read(u32 reg, void *val, int len, bool is_gtt,
+			bool trace)
+{
+	if (!vgt_integration_done)
+		return is_gtt ? vgt_gtt_read(reg, val, len) :
+				vgt_mmio_read(reg, val, len, trace);
+}
+
+bool vgt_emulate_host_write(u32 reg, void *val, int len, bool is_gtt,
+			bool trace)
+{
+	if (!vgt_integration_done)
+		return is_gtt ? vgt_gtt_write(reg, val, len) :
+				vgt_mmio_write(reg, val, len, trace);
+}
+
+void i915_vgt_record_priv(struct drm_i915_private *priv)
+{
+	dev_priv = priv;
+}
-- 
1.9.1

[RFC PATCH 4/8] drm/i915: redirect MMIO accesses to vgt if enabled

[Jike Song]

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     | 147 +++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_uncore.c |   3 +
 2 files changed, 138 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c3b03f5..ed6f14e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -38,6 +38,7 @@
 #include "intel_lrc.h"
 #include "i915_gem_gtt.h"
 #include "i915_gem_render_state.h"
+#include "i915_vgt.h"
 #include <linux/io-mapping.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
@@ -2915,19 +2916,121 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
 #define FORCEWAKE_MEDIA		(1 << 1)
 #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
 
+#define I915_READ8(reg)							\
+({									\
+	u8 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u8),		\
+					false, true);			\
+	else								\
+		__ret = dev_priv->uncore.funcs.mmio_readb(dev_priv,	\
+						(reg), true);		\
+	__ret;								\
+})
+
+#define I915_READ16(reg)						\
+({									\
+	u16 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u16),		\
+						false, true);		\
+	else								\
+		__ret = dev_priv->uncore.funcs.mmio_readw(dev_priv,	\
+						(reg), true);		\
+	__ret;								\
+})
+
+#define I915_READ16_NOTRACE(reg)					\
+({									\
+	u16 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u16),		\
+						false, false);		\
+	else								\
+		__ret = dev_priv->uncore.funcs.mmio_readw(dev_priv,	\
+						(reg), false);		\
+	__ret;								\
+})
+
+#define I915_READ(reg)							\
+({									\
+	u32 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u32),		\
+						false, true);		\
+	else								\
+		__ret = dev_priv->uncore.funcs.mmio_readl(dev_priv,	\
+						(reg), true);		\
+	__ret;								\
+})
+
+#define I915_READ_NOTRACE(reg)						\
+({									\
+	u32 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u32),		\
+						false, false);		\
+	else								\
+		__ret = dev_priv->uncore.funcs.mmio_readl(dev_priv,	\
+						(reg), false);		\
+	__ret;								\
+})
+
 
-#define I915_READ8(reg)		dev_priv->uncore.funcs.mmio_readb(dev_priv, (reg), true)
-#define I915_WRITE8(reg, val)	dev_priv->uncore.funcs.mmio_writeb(dev_priv, (reg), (val), true)
+#define I915_WRITE8(reg, val)						\
+({									\
+	u8 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u8),		\
+						false, true);		\
+	else								\
+		dev_priv->uncore.funcs.mmio_writeb(dev_priv,		\
+						reg, val, true);	\
+})
+
+#define I915_WRITE16(reg, val)						\
+({									\
+	u16 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u16),	\
+						false, true);		\
+	else								\
+		dev_priv->uncore.funcs.mmio_writew(dev_priv,		\
+						reg, val, true);	\
+})
+
+#define I915_WRITE16_NOTRACE(reg, val)					\
+({									\
+	u16 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u16),	\
+						false, false);		\
+	else								\
+		dev_priv->uncore.funcs.mmio_writew(dev_priv,		\
+						reg, val, false);	\
+})
 
-#define I915_READ16(reg)	dev_priv->uncore.funcs.mmio_readw(dev_priv, (reg), true)
-#define I915_WRITE16(reg, val)	dev_priv->uncore.funcs.mmio_writew(dev_priv, (reg), (val), true)
-#define I915_READ16_NOTRACE(reg)	dev_priv->uncore.funcs.mmio_readw(dev_priv, (reg), false)
-#define I915_WRITE16_NOTRACE(reg, val)	dev_priv->uncore.funcs.mmio_writew(dev_priv, (reg), (val), false)
+#define I915_WRITE(reg, val)						\
+({									\
+	u32 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u32),	\
+						false, true);		\
+	else								\
+		dev_priv->uncore.funcs.mmio_writel(dev_priv,		\
+						reg, val, true);	\
+})
 
-#define I915_READ(reg)		dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), true)
-#define I915_WRITE(reg, val)	dev_priv->uncore.funcs.mmio_writel(dev_priv, (reg), (val), true)
-#define I915_READ_NOTRACE(reg)		dev_priv->uncore.funcs.mmio_readl(dev_priv, (reg), false)
-#define I915_WRITE_NOTRACE(reg, val)	dev_priv->uncore.funcs.mmio_writel(dev_priv, (reg), (val), false)
+#define I915_WRITE_NOTRACE(reg, val)					\
+({									\
+	u32 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u32),	\
+						false, false);		\
+	else								\
+		dev_priv->uncore.funcs.mmio_writel(dev_priv,		\
+						reg, val, false);	\
+})
 
 /* Be very careful with read/write 64-bit values. On 32-bit machines, they
  * will be implemented using 2 32-bit writes in an arbitrary order with
@@ -2935,8 +3038,28 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
  * act upon the intermediate value, possibly leading to corruption and
  * machine death. You have been warned.
  */
-#define I915_WRITE64(reg, val)	dev_priv->uncore.funcs.mmio_writeq(dev_priv, (reg), (val), true)
-#define I915_READ64(reg)	dev_priv->uncore.funcs.mmio_readq(dev_priv, (reg), true)
+#define I915_READ64(reg)						\
+({									\
+	u64 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u64),		\
+						false, true);		\
+	else								\
+		__ret = dev_priv->uncore.funcs.mmio_readq(dev_priv,	\
+						(reg), true);		\
+	__ret;								\
+})
+
+#define I915_WRITE64(reg, val)						\
+({									\
+	u64 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u64),	\
+						false, true);		\
+	else								\
+		dev_priv->uncore.funcs.mmio_writeq(dev_priv,		\
+						reg, val, true);	\
+})
 
 #define I915_READ64_2x32(lower_reg, upper_reg) ({			\
 		u32 upper = I915_READ(upper_reg);			\
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 0b0f4f8..774e0fa 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -948,6 +948,9 @@ void intel_uncore_init(struct drm_device *dev)
 		dev_priv->uncore.funcs.mmio_readq  = gen4_read64;
 		break;
 	}
+
+	if (i915.enable_vgt)
+		i915_vgt_record_priv(dev_priv);
 }
 
 void intel_uncore_fini(struct drm_device *dev)
-- 
1.9.1

[RFC PATCH 5/8] drm/i915: GTT access abstraction

[Jike Song]

This patch convert necessary GTT read/write calls, e.g. iowrite32() and
readl() et al, to an encapsulated series: GTT_READ32, GTT_WRITE32,
GTT_READ64 and GTT_WRITE64.

This patch doesn't change the behaviors of i915 GTT access.

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h     | 23 +++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 +++++++++++++++++----------------
 2 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ed6f14e..051442e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3075,6 +3075,29 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
 #define POSTING_READ(reg)	(void)I915_READ_NOTRACE(reg)
 #define POSTING_READ16(reg)	(void)I915_READ16_NOTRACE(reg)
 
+#define GTT_READ32(addr)						\
+({									\
+	u32 ret =  readl(addr);						\
+	ret;								\
+})
+
+#define GTT_READ64(addr)						\
+({									\
+	u64 ret = readq(addr);						\
+	ret;								\
+})
+
+#define GTT_WRITE32(val, addr)						\
+({									\
+	writel((val), (addr));						\
+})
+
+#define GTT_WRITE64(val, addr)						\
+({									\
+	writeq((val), (addr));						\
+})
+
+
 /* "Broadcast RGB" property */
 #define INTEL_BROADCAST_RGB_AUTO 0
 #define INTEL_BROADCAST_RGB_FULL 1
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 273dad9..3472970 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -642,7 +642,7 @@ static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 		u32 expected;
 		gen6_gtt_pte_t *pt_vaddr;
 		dma_addr_t pt_addr = ppgtt->pt_dma_addr[pde];
-		pd_entry = readl(pd_addr + pde);
+		pd_entry = GTT_READ32(pd_addr + pde);
 		expected = (GEN6_PDE_ADDR_ENCODE(pt_addr) | GEN6_PDE_VALID);
 
 		if (pd_entry != expected)
@@ -695,9 +695,9 @@ static void gen6_write_pdes(struct i915_hw_ppgtt *ppgtt)
 		pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr);
 		pd_entry |= GEN6_PDE_VALID;
 
-		writel(pd_entry, pd_addr + i);
+		GTT_WRITE32(pd_entry, pd_addr + i);
 	}
-	readl(pd_addr);
+	GTT_READ32(pd_addr);
 }
 
 static uint32_t get_pd_offset(struct i915_hw_ppgtt *ppgtt)
@@ -1365,13 +1365,14 @@ int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static inline void gen8_set_pte(void __iomem *addr, gen8_gtt_pte_t pte)
+static inline void gen8_set_pte(void __iomem *addr, gen8_gtt_pte_t pte,
+		struct drm_i915_private *dev_priv)
 {
 #ifdef writeq
-	writeq(pte, addr);
+	GTT_WRITE64(pte, addr);
 #else
-	iowrite32((u32)pte, addr);
-	iowrite32(pte >> 32, addr + 4);
+	GTT_WRITE32((u32)pte, addr);
+	GTT_WRITE32(pte >> 32, addr + 4);
 #endif
 }
 
@@ -1392,7 +1393,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 		addr = sg_dma_address(sg_iter.sg) +
 			(sg_iter.sg_pgoffset << PAGE_SHIFT);
 		gen8_set_pte(&gtt_entries[i],
-			     gen8_pte_encode(addr, level, true));
+			     gen8_pte_encode(addr, level, true), dev_priv);
 		i++;
 	}
 
@@ -1404,7 +1405,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	 * hardware should work, we must keep this posting read for paranoia.
 	 */
 	if (i != 0)
-		WARN_ON(readq(&gtt_entries[i-1])
+		WARN_ON(GTT_READ64(&gtt_entries[i-1])
 			!= gen8_pte_encode(addr, level, true));
 
 	/* This next bit makes the above posting read even more important. We
@@ -1436,7 +1437,8 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
 		addr = sg_page_iter_dma_address(&sg_iter);
-		iowrite32(vm->pte_encode(addr, level, true, flags), &gtt_entries[i]);
+		GTT_WRITE32(vm->pte_encode(addr, level, true, flags),
+					&gtt_entries[i]);
 		i++;
 	}
 
@@ -1447,7 +1449,7 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 	 * hardware should work, we must keep this posting read for paranoia.
 	 */
 	if (i != 0) {
-		unsigned long gtt = readl(&gtt_entries[i-1]);
+		unsigned long gtt = GTT_READ32(&gtt_entries[i-1]);
 		WARN_ON(gtt != vm->pte_encode(addr, level, true, flags));
 	}
 
@@ -1481,8 +1483,8 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
 				      I915_CACHE_LLC,
 				      use_scratch);
 	for (i = 0; i < num_entries; i++)
-		gen8_set_pte(&gtt_base[i], scratch_pte);
-	readl(gtt_base);
+		gen8_set_pte(&gtt_base[i], scratch_pte, dev_priv);
+	GTT_READ32(gtt_base);
 }
 
 static void gen6_ggtt_clear_range(struct i915_address_space *vm,
@@ -1506,11 +1508,10 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 	scratch_pte = vm->pte_encode(vm->scratch.addr, I915_CACHE_LLC, use_scratch, 0);
 
 	for (i = 0; i < num_entries; i++)
-		iowrite32(scratch_pte, &gtt_base[i]);
-	readl(gtt_base);
+		GTT_WRITE32(scratch_pte, &gtt_base[i]);
+	GTT_READ32(gtt_base);
 }
 
-
 static void i915_ggtt_bind_vma(struct i915_vma *vma,
 			       enum i915_cache_level cache_level,
 			       u32 unused)
-- 
1.9.1

[RFC PATCH 6/8] drm/i915: redirect GTT accesses to vgt if enabled

[Jike Song]

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 41 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 051442e..742fe8a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3077,27 +3077,54 @@ int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
 
 #define GTT_READ32(addr)						\
 ({									\
-	u32 ret =  readl(addr);						\
-	ret;								\
+	off_t reg = (unsigned long)(addr) -				\
+			(unsigned long)(dev_priv->gtt.gsm);		\
+	u32 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u32),		\
+						true, false);		\
+	else								\
+		__ret = readl(addr);					\
+	__ret;								\
 })
 
 #define GTT_READ64(addr)						\
 ({									\
-	u64 ret = readq(addr);						\
-	ret;								\
+	off_t reg = (unsigned long)(addr) -				\
+			(unsigned long)(dev_priv->gtt.gsm);		\
+	u64 __ret = 0;							\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_read(reg, &__ret, sizeof(u64),		\
+						true, false);		\
+	else								\
+		__ret = readq(addr);					\
+	__ret;								\
 })
 
 #define GTT_WRITE32(val, addr)						\
 ({									\
-	writel((val), (addr));						\
+	off_t reg = (unsigned long)(addr) -				\
+			(unsigned long)(dev_priv->gtt.gsm);		\
+	u32 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u32),	\
+						true, false);		\
+	else								\
+		writel((val), (addr));					\
 })
 
 #define GTT_WRITE64(val, addr)						\
 ({									\
-	writeq((val), (addr));						\
+	off_t reg = (unsigned long)(addr) -				\
+			(unsigned long)(dev_priv->gtt.gsm);		\
+	u64 __val = (val);						\
+	if (i915.enable_vgt)						\
+		vgt_emulate_host_write(reg, &__val, sizeof(u64),	\
+						true, false);		\
+	else								\
+		writeq((val), (addr));					\
 })
 
-
 /* "Broadcast RGB" property */
 #define INTEL_BROADCAST_RGB_AUTO 0
 #define INTEL_BROADCAST_RGB_FULL 1
-- 
1.9.1

[RFC PATCH 7/8] drm/i915: vgt irq mediation - via a tasklet based mechanism

[Jike Song]

vgt owns the hardware interrupt of the GPU, to satisfy the
interrupt requirement from both host side and guest side
(e.g. host may have MI_USER_INTERRUPT disabled, while a VM
may have it enabled). Sometimes vgt may also need to emulate
a virtual interrupt to the host, w/o a hardware interrupt
actually triggered. So we need to split the handling
between physical interrupts to vgt and virtual interrupts
to host i915.

Regarding to above requirements, this patch registers a
vgt interrupt handler when vgt is enabled, while letting
original i915 interrupt handler instead carried in a
tasklet. Whenever a virtual interrupt needs to be injected
to host i915, tasklet_schedule gets called.

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |  6 ++++++
 drivers/gpu/drm/i915/i915_irq.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_vgt.h | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/vgt/vgt.c  | 22 ++++++++++++++++++++++
 4 files changed, 83 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 742fe8a..e33455a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1668,6 +1668,12 @@ struct drm_i915_private {
 	struct vlv_s0ix_state vlv_s0ix_state;
 
 	struct {
+		struct tasklet_struct host_irq_task;
+		irqreturn_t (*host_isr)(int, void *);
+		void (*host_irq_uninstall)(struct drm_device *);
+	} igvt;
+
+	struct {
 		/*
 		 * Raw watermark latency values:
 		 * in 0.1us units for WM0,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 080981b..df7c868 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -36,6 +36,7 @@
 #include "i915_drv.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
+#include "i915_vgt.h"
 
 static const u32 hpd_ibx[] = {
 	[HPD_CRT] = SDE_CRT_HOTPLUG,
@@ -4650,6 +4651,30 @@ static void intel_hpd_irq_reenable_work(struct work_struct *work)
 	intel_runtime_pm_put(dev_priv);
 }
 
+static void vgt_host_isr_wrapper(unsigned long data)
+{
+	struct drm_device *dev = (struct drm_device *)data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	dev_priv->igvt.host_isr(dev->pdev->irq, dev);
+}
+
+void vgt_schedule_host_isr(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	tasklet_schedule(&dev_priv->igvt.host_irq_task);
+}
+
+static void vgt_irq_uninstall(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	tasklet_kill(&dev_priv->igvt.host_irq_task);
+	dev_priv->igvt.host_irq_uninstall(dev);
+	vgt_fini_irq(dev->pdev);
+}
+
 void intel_irq_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4756,6 +4781,16 @@ void intel_irq_init(struct drm_device *dev)
 		dev->driver->enable_vblank = i915_enable_vblank;
 		dev->driver->disable_vblank = i915_disable_vblank;
 	}
+
+	if (i915.enable_vgt) {
+		vgt_init_irq(dev->pdev, dev);
+		dev_priv->igvt.host_isr = dev->driver->irq_handler;
+		dev->driver->irq_handler = vgt_interrupt;
+		dev_priv->igvt.host_irq_uninstall = dev->driver->irq_uninstall;
+		dev->driver->irq_uninstall = vgt_irq_uninstall;
+		tasklet_init(&dev_priv->igvt.host_irq_task,
+				vgt_host_isr_wrapper, (unsigned long)dev);
+	}
 }
 
 void intel_hpd_init(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_vgt.h b/drivers/gpu/drm/i915/i915_vgt.h
index 03e7f00..553f920 100644
--- a/drivers/gpu/drm/i915/i915_vgt.h
+++ b/drivers/gpu/drm/i915/i915_vgt.h
@@ -1,6 +1,9 @@
 #ifndef _I915_VGT_H_
 #define _I915_VGT_H_
 
+#include <linux/interrupt.h>
+
+struct drm_device;
 struct drm_i915_private;
 
 #ifdef CONFIG_I915_IGVT
@@ -9,6 +12,10 @@ bool i915_start_vgt(struct pci_dev *);
 void i915_vgt_record_priv(struct drm_i915_private *);
 bool vgt_emulate_host_read(u32, void *, int, bool, bool);
 bool vgt_emulate_host_write(u32, void *, int, bool, bool);
+void vgt_schedule_host_isr(struct drm_device *);
+void vgt_init_irq(struct pci_dev *, struct drm_device *);
+void vgt_fini_irq(struct pci_dev *);
+irqreturn_t vgt_interrupt(int, void *);
 
 #else /* !CONFIG_I915_IGVT */
 
@@ -33,6 +40,19 @@ static inline bool vgt_emulate_host_write(u32 reg, void *val, int len,
 	return false;
 }
 
+static inline void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev)
+{
+}
+
+static inline void vgt_fini_irq(struct pci_dev *pdev)
+{
+}
+
+static inline irqreturn_t vgt_interrupt(int irq, void *data)
+{
+	return IRQ_NONE;
+}
+
 #endif /* CONFIG_I915_IGVT */
 
 #endif
diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c
index f33baf3..dab4bfc 100644
--- a/drivers/gpu/drm/i915/vgt/vgt.c
+++ b/drivers/gpu/drm/i915/vgt/vgt.c
@@ -1,6 +1,7 @@
 #include <linux/module.h>
 #include <linux/kthread.h>
 #include <linux/pci.h>
+#include <linux/interrupt.h>
 
 #include "../i915_drv.h"
 #include "vgt.h"
@@ -121,3 +122,24 @@ void i915_vgt_record_priv(struct drm_i915_private *priv)
 {
 	dev_priv = priv;
 }
+
+static void vgt_host_irq(struct drm_device *dev)
+{
+	vgt_schedule_host_isr(dev);
+}
+
+void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev)
+{
+	/* TODO: initialize vgt-specific irq handlings after vgt integration */
+}
+
+void vgt_fini_irq(struct pci_dev *pdev)
+{
+	/* TODO: cleanup vgt-specific irq handlings after vgt integration */
+}
+
+irqreturn_t vgt_interrupt(int irq, void *data)
+{
+	vgt_host_irq(data);
+	return IRQ_HANDLED;
+}
-- 
1.9.1

[RFC PATCH 8/8] drm/i915: enable vgt if specified by module param

[Jike Song]

All of interfaces between vgt and host i915(MMIO, GTT, irq mediation)
are ready, it's safe to enable it if specified in modparam.

Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/vgt/vgt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c
index dab4bfc..e6dbe6d 100644
--- a/drivers/gpu/drm/i915/vgt/vgt.c
+++ b/drivers/gpu/drm/i915/vgt/vgt.c
@@ -19,7 +19,7 @@ static struct drm_i915_private *dev_priv = NULL;
 bool i915_start_vgt(struct pci_dev *pdev)
 {
 	/* vgt is not yet integrated, this only means testing */
-	return false;
+	return true;
 }
 
 static bool vgt_mmio_read(off_t reg, void *val, int len, bool trace)
-- 
1.9.1

Re: [RFC PATCH 7/8] drm/i915: vgt irq mediation - via a tasklet based mechanism

[Daniel Vetter]

On Tue, Sep 30, 2014 at 06:05:37PM +0800, Jike Song wrote:
> vgt owns the hardware interrupt of the GPU, to satisfy the
> interrupt requirement from both host side and guest side
> (e.g. host may have MI_USER_INTERRUPT disabled, while a VM
> may have it enabled). Sometimes vgt may also need to emulate
> a virtual interrupt to the host, w/o a hardware interrupt
> actually triggered. So we need to split the handling
> between physical interrupts to vgt and virtual interrupts
> to host i915.
> 
> Regarding to above requirements, this patch registers a
> vgt interrupt handler when vgt is enabled, while letting
> original i915 interrupt handler instead carried in a
> tasklet. Whenever a virtual interrupt needs to be injected
> to host i915, tasklet_schedule gets called.

So the tasklet requirement comes from being able to inject interrupts on
the host side from vgt to i915?

Why would you ever need to do that instead of just either calling the i915
irq handler as the tail handler from hardirq context or directly calling
whatever function you want to call anywhere else?

I guess we need even more information here about the design ;-)

Cheers, Daniel

> 
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h |  6 ++++++
>  drivers/gpu/drm/i915/i915_irq.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_vgt.h | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/vgt/vgt.c  | 22 ++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 742fe8a..e33455a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1668,6 +1668,12 @@ struct drm_i915_private {
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
>  	struct {
> +		struct tasklet_struct host_irq_task;
> +		irqreturn_t (*host_isr)(int, void *);
> +		void (*host_irq_uninstall)(struct drm_device *);
> +	} igvt;
> +
> +	struct {
>  		/*
>  		 * Raw watermark latency values:
>  		 * in 0.1us units for WM0,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 080981b..df7c868 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -36,6 +36,7 @@
>  #include "i915_drv.h"
>  #include "i915_trace.h"
>  #include "intel_drv.h"
> +#include "i915_vgt.h"
>  
>  static const u32 hpd_ibx[] = {
>  	[HPD_CRT] = SDE_CRT_HOTPLUG,
> @@ -4650,6 +4651,30 @@ static void intel_hpd_irq_reenable_work(struct work_struct *work)
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +static void vgt_host_isr_wrapper(unsigned long data)
> +{
> +	struct drm_device *dev = (struct drm_device *)data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	dev_priv->igvt.host_isr(dev->pdev->irq, dev);
> +}
> +
> +void vgt_schedule_host_isr(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	tasklet_schedule(&dev_priv->igvt.host_irq_task);
> +}
> +
> +static void vgt_irq_uninstall(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	tasklet_kill(&dev_priv->igvt.host_irq_task);
> +	dev_priv->igvt.host_irq_uninstall(dev);
> +	vgt_fini_irq(dev->pdev);
> +}
> +
>  void intel_irq_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4756,6 +4781,16 @@ void intel_irq_init(struct drm_device *dev)
>  		dev->driver->enable_vblank = i915_enable_vblank;
>  		dev->driver->disable_vblank = i915_disable_vblank;
>  	}
> +
> +	if (i915.enable_vgt) {
> +		vgt_init_irq(dev->pdev, dev);
> +		dev_priv->igvt.host_isr = dev->driver->irq_handler;
> +		dev->driver->irq_handler = vgt_interrupt;
> +		dev_priv->igvt.host_irq_uninstall = dev->driver->irq_uninstall;
> +		dev->driver->irq_uninstall = vgt_irq_uninstall;
> +		tasklet_init(&dev_priv->igvt.host_irq_task,
> +				vgt_host_isr_wrapper, (unsigned long)dev);
> +	}
>  }
>  
>  void intel_hpd_init(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/i915_vgt.h b/drivers/gpu/drm/i915/i915_vgt.h
> index 03e7f00..553f920 100644
> --- a/drivers/gpu/drm/i915/i915_vgt.h
> +++ b/drivers/gpu/drm/i915/i915_vgt.h
> @@ -1,6 +1,9 @@
>  #ifndef _I915_VGT_H_
>  #define _I915_VGT_H_
>  
> +#include <linux/interrupt.h>
> +
> +struct drm_device;
>  struct drm_i915_private;
>  
>  #ifdef CONFIG_I915_IGVT
> @@ -9,6 +12,10 @@ bool i915_start_vgt(struct pci_dev *);
>  void i915_vgt_record_priv(struct drm_i915_private *);
>  bool vgt_emulate_host_read(u32, void *, int, bool, bool);
>  bool vgt_emulate_host_write(u32, void *, int, bool, bool);
> +void vgt_schedule_host_isr(struct drm_device *);
> +void vgt_init_irq(struct pci_dev *, struct drm_device *);
> +void vgt_fini_irq(struct pci_dev *);
> +irqreturn_t vgt_interrupt(int, void *);
>  
>  #else /* !CONFIG_I915_IGVT */
>  
> @@ -33,6 +40,19 @@ static inline bool vgt_emulate_host_write(u32 reg, void *val, int len,
>  	return false;
>  }
>  
> +static inline void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev)
> +{
> +}
> +
> +static inline void vgt_fini_irq(struct pci_dev *pdev)
> +{
> +}
> +
> +static inline irqreturn_t vgt_interrupt(int irq, void *data)
> +{
> +	return IRQ_NONE;
> +}
> +
>  #endif /* CONFIG_I915_IGVT */
>  
>  #endif
> diff --git a/drivers/gpu/drm/i915/vgt/vgt.c b/drivers/gpu/drm/i915/vgt/vgt.c
> index f33baf3..dab4bfc 100644
> --- a/drivers/gpu/drm/i915/vgt/vgt.c
> +++ b/drivers/gpu/drm/i915/vgt/vgt.c
> @@ -1,6 +1,7 @@
>  #include <linux/module.h>
>  #include <linux/kthread.h>
>  #include <linux/pci.h>
> +#include <linux/interrupt.h>
>  
>  #include "../i915_drv.h"
>  #include "vgt.h"
> @@ -121,3 +122,24 @@ void i915_vgt_record_priv(struct drm_i915_private *priv)
>  {
>  	dev_priv = priv;
>  }
> +
> +static void vgt_host_irq(struct drm_device *dev)
> +{
> +	vgt_schedule_host_isr(dev);
> +}
> +
> +void vgt_init_irq(struct pci_dev *pdev, struct drm_device *dev)
> +{
> +	/* TODO: initialize vgt-specific irq handlings after vgt integration */
> +}
> +
> +void vgt_fini_irq(struct pci_dev *pdev)
> +{
> +	/* TODO: cleanup vgt-specific irq handlings after vgt integration */
> +}
> +
> +irqreturn_t vgt_interrupt(int irq, void *data)
> +{
> +	vgt_host_irq(data);
> +	return IRQ_HANDLED;
> +}
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH 7/8] drm/i915: vgt irq mediation - via a tasklet based mechanism

[Tian, Kevin]

> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, September 30, 2014 3:30 AM
> 
> On Tue, Sep 30, 2014 at 06:05:37PM +0800, Jike Song wrote:
> > vgt owns the hardware interrupt of the GPU, to satisfy the
> > interrupt requirement from both host side and guest side
> > (e.g. host may have MI_USER_INTERRUPT disabled, while a VM
> > may have it enabled). Sometimes vgt may also need to emulate
> > a virtual interrupt to the host, w/o a hardware interrupt
> > actually triggered. So we need to split the handling
> > between physical interrupts to vgt and virtual interrupts
> > to host i915.
> >
> > Regarding to above requirements, this patch registers a
> > vgt interrupt handler when vgt is enabled, while letting
> > original i915 interrupt handler instead carried in a
> > tasklet. Whenever a virtual interrupt needs to be injected
> > to host i915, tasklet_schedule gets called.
> 
> So the tasklet requirement comes from being able to inject interrupts on
> the host side from vgt to i915?

yes.

> 
> Why would you ever need to do that instead of just either calling the i915
> irq handler as the tail handler from hardirq context or directly calling
> whatever function you want to call anywhere else?
> 
> I guess we need even more information here about the design ;-)
> 

We want to separate pirq and virq handling in a consistent way for both
host and guest side. In pirq handler, it simply checks physical pending
events, and loop all VM's virtual imr/ier to decide whether to pend virtual
iir. At the end of pirq handling, kick off an asynchronous event to either 
host i915 or guest i915, if there's pending virtual iir, in the same manner 
as how a physical interrupt is triggered. 

For guest virq injection, the kick-off goes through APIC emulation to deliver
to guest i915. For host virq injection, we choose tasklet now to implement
the asynchronous manner. But definitely tasklet is not hardirq context, so
there's some limitation.

Similarly asynchronous manner is required when emulating a register access, 
e.g. to execlist submission port, which may trigger a virtual interrupt. Invoking 
i915 interrupt handler in the emulation path may cause tricky lock problem, 
since i915 handler also accesses registers which further causes nested
emulations.

>From virtualization p.o.v, the ideal case is to run host i915 irq handler in the
interrupt context, which meets all the assumption from original code. Using
tasklet or other manner still has some restriction. This is a major open we'd
like to hear more from you guys. Is it possible to have i915 driver to request
two irq numbers: irq1 for real device and irq2 is purely faked one. vgt handler
registers on irq1 and i915 hanlder registers on irq2, and then we can use self
IPI to trigger irq2 when injection is required. But I'm not sure whether this is
an existing feature in kernel, or need some core enhancement in irq sub-system...

Thanks
Kevin

Re: [RFC PATCH 3/8] drm/i915: add the vgt implementation of MMIO/GTT mediations

[Tian, Kevin]

> From: Song, Jike
> Sent: Tuesday, September 30, 2014 3:06 AM
> 
> vgt mediates GPU operations from host i915, in the same way as
> mediating GPU operations from normal VMs. This way vgt can
> have centralized management about sharing among host and other
> VMs. To achieve that, we add a hook in critical wrapper interfaces
> (MMIO/GTT).
> 
> This patch only adds the MMIO/GTT accessing functions, without
> changing the existing i915 MMIO/GTT access behaviors.
> 
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_vgt.h |  21 ++++++++
>  drivers/gpu/drm/i915/vgt/vgt.c  | 105
> ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
> 

...

> +void i915_vgt_record_priv(struct drm_i915_private *priv)
> +{
> +	dev_priv = priv;
> +}
> --

Suppose above can be carried in i915_start_vgt, instead of adding
a new interface?

Thanks
Kevin

Re: [RFC PATCH 3/8] drm/i915: add the vgt implementation of MMIO/GTT mediations

[Jike Song]

On 10/1/2014 12:34 AM, Tian, Kevin wrote:
>> +void i915_vgt_record_priv(struct drm_i915_private *priv)
>> +{
>> +	dev_priv = priv;
>> +}
>> --
>
> Suppose above can be carried in i915_start_vgt, instead of adding
> a new interface?
>
> Thanks
> Kevin

Should be not? since the drm_device is not yet allocated in 
i915_driver_load.

--
Thanks,
Jike

Re: [RFC PATCH 7/8] drm/i915: vgt irq mediation - via a tasklet based mechanism

[Jike Song]

On 10/01/2014 12:26 AM, Tian, Kevin wrote:
>  From virtualization p.o.v, the ideal case is to run host i915 irq handler in the
> interrupt context, which meets all the assumption from original code. Using
> tasklet or other manner still has some restriction. This is a major open we'd
> like to hear more from you guys. Is it possible to have i915 driver to request
> two irq numbers: irq1 for real device and irq2 is purely faked one. vgt handler
> registers on irq1 and i915 hanlder registers on irq2, and then we can use self
> IPI to trigger irq2 when injection is required. But I'm not sure whether this is
> an existing feature in kernel, or need some core enhancement in irq sub-system...

Hi Kevin, Daniel,

  I'm so excited to know that, there is an existing feature in kernel: irq_work.
Basicly it allows us to run the host i915 ISR in hardirq context prefectly, what's
needed is to select CONFIG_IRQ_WORK in Kconfig :)

  I'll use that in the v2 patches.


>
> Thanks
> Kevin
>

--
Thanks,
Jike

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Daniel Vetter]

On Tue, Sep 30, 2014 at 06:05:30PM +0800, Jike Song wrote:
> Intel GVT-g (previously known as XenGT), is a complete GPU
> virtualization solution with mediated pass-through for 4th
> generation Intel Core processors - Haswell platform. This
> technology presents a virtual full-fledged GPU to each Virtual
> Machine (VM). VMs can directly access performance-critical
> resources, without intervention from the hypervisor in most
> cases, while privileged operations from VMs are trap-and-emulated
> at minimal cost. For details, please refer to:
> 
>   https://01.org/xen/blogs/wangbo85/2014/intel-gvt-gxengt-pubic-release
> 
> The patches of adding vGPU guest support for i915, is already posted at:
> 
>   http://lists.freedesktop.org/archives/intel-gfx/2014-September/052571.html
> 
> 
> This patch set is about to add vGPU host support.
> 
> When running as vGPU host, the i915 driver can't simply occupy
> the whole GPU resources, it needs to proactively meidate the
> accesses to most hardware resources: MMIO, GTT, and interrupt.
> Only by handling the hardware resources, the vgt can have
> centralized management about sharing between VMs(including
> host and guest).
> 
> 
> This patch set adds:
> 
> 	- an i915 extension, named vgt. vgt is the major part of Intel
> 	  GVT-g implementation, it manages and shares GPU among VMs
> 	  and host;
> 
> 	- interfaces between i915 and vgt, including:
> 
> 		Mediated MMIO access
> 		Mediated GTT access
> 		Mediated IRQ handling
> 
> NOTE of RFC:
> 
> 	- the vgt in-kernel GPU device-model is not yet integrated, though
> 	  the interfaces between i915 and vgt are provided;
> 
> 	- the host i915 driver has some logic same with the guest i915,
> 	  e.g. the ballooning. When the guest patches(see above) are
> 	  finalized, we need to rebase upon them;
> 
> 	- vgt_suspend/vgt_resume are still under cleanup;
> 
> 	- GPU reset is still under cleanup
> 
> 
>   We send out the framework changes in this patch set for review
> at first, and meanwhile, vgt integration and cleanup are ongoing.

So on a very high level I don't understand this design. For the guest side
it's completely clear that we need a bunch of hooks over the driver to
make paravirtualization work.

But on the host side I expect the driver to be in full control of the
hardware. I haven't really seen the other side but it looks like with vgt
we actually have two drivers fighting over the hardware, which requires
the various hooks to avoid disaster. The drm subsystem is littered with
such attempts, and they all didn't end up in a pretty way.

So way can't we have the vgt support for guests sit on top of i915, using
the i915 functions to set up pagetables, contexts and reserve gtt areas
for the guests? Then we'd have just one driver in control of the hardware,
and vgt on the host side would just look like a really crazy interace
layer between virtual hosts and the low-level driver, similar to who the
execbuf ioctl is a really crazy interface between userspace and the
low-level driver.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Jike Song]

On 10/22/2014 05:48 PM, Daniel Vetter wrote:
> So on a very high level I don't understand this design. For the guest side
> it's completely clear that we need a bunch of hooks over the driver to
> make paravirtualization work.
>
> But on the host side I expect the driver to be in full control of the
> hardware. I haven't really seen the other side but it looks like with vgt
> we actually have two drivers fighting over the hardware, which requires
> the various hooks to avoid disaster. The drm subsystem is littered with
> such attempts, and they all didn't end up in a pretty way.
>
> So way can't we have the vgt support for guests sit on top of i915, using
> the i915 functions to set up pagetables, contexts and reserve gtt areas
> for the guests? Then we'd have just one driver in control of the hardware,
> and vgt on the host side would just look like a really crazy interace
> layer between virtual hosts and the low-level driver, similar to who the
> execbuf ioctl is a really crazy interface between userspace and the
> low-level driver.

  Yes we can do this, but that also means lots of pollution to existing i915
codes, only for virtualization purpose. Currently vgt has pretty abstractions
for both host i915 and guest graphics drivers, mixing vgt and host i915 means
breaking that.  I believe a vgt-integrated repository will be better to look
at :)

>
> Cheers, Daniel
>

--
Thanks,
Jike

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Daniel Vetter]

On Thu, Oct 23, 2014 at 11:13:51AM +0800, Jike Song wrote:
> On 10/22/2014 05:48 PM, Daniel Vetter wrote:
> >So on a very high level I don't understand this design. For the guest side
> >it's completely clear that we need a bunch of hooks over the driver to
> >make paravirtualization work.
> >
> >But on the host side I expect the driver to be in full control of the
> >hardware. I haven't really seen the other side but it looks like with vgt
> >we actually have two drivers fighting over the hardware, which requires
> >the various hooks to avoid disaster. The drm subsystem is littered with
> >such attempts, and they all didn't end up in a pretty way.
> >
> >So way can't we have the vgt support for guests sit on top of i915, using
> >the i915 functions to set up pagetables, contexts and reserve gtt areas
> >for the guests? Then we'd have just one driver in control of the hardware,
> >and vgt on the host side would just look like a really crazy interace
> >layer between virtual hosts and the low-level driver, similar to who the
> >execbuf ioctl is a really crazy interface between userspace and the
> >low-level driver.
> 
>  Yes we can do this, but that also means lots of pollution to existing i915
> codes, only for virtualization purpose. Currently vgt has pretty abstractions
> for both host i915 and guest graphics drivers, mixing vgt and host i915 means
> breaking that.  I believe a vgt-integrated repository will be better to look
> at :)

The problem is that we need to have this integration anyway. Using the
current design it will be hidden behind some thin abstraction, but it will
be as invasive and fragile as if the interactions is made explicit.

Stuf like driver load/unload, suspend/resume, runtime pm and gpu reset are
already supre-fragile as-is. Every time we change something in there, a
bunch of related things fall apart. With vgt we'll have even more
complexity in there, and I really think we need to make that complexity
explicit. Otherwise we'll always break vgt support for host systems by
accident when working on upstream. So in my experience being explicit with
these depencies massively reduces maintaince headaches longterm.

Of course for starting up the vgt effort it's better to have as much
separation as possible. But the balance between development and
maintainability do change quite a bit when merging code upstream.

Another benefit of the inverted design with vgt sitting on top of normal
i915 for host support is better automated testing. At least with kvm we
could then simply create a kvm guest without any special boot parameters
and run some really basic testcases in the guest to make sure vgt doesn't
get broken. That would fit rather nicely into i-g-t. Of course with xengt
we unfortunately can't do that, since we'd need to boot with the
hypervisor.

But I think without such smoketesting in the automated upstream test suite
we'll break vgt support constantly in upstream. So I think we really need
this too, at least long term.

I hope that explains a bit where I'm coming from. Note that this is just
about the host side, imo the guest side can be merged as soon as detailed
review has been completed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Gerd Hoffmann]

  Hi,

> Stuf like driver load/unload, suspend/resume, runtime pm and gpu reset are
> already supre-fragile as-is. Every time we change something in there, a
> bunch of related things fall apart. With vgt we'll have even more
> complexity in there, and I really think we need to make that complexity
> explicit. Otherwise we'll always break vgt support for host systems by
> accident when working on upstream. So in my experience being explicit with
> these depencies massively reduces maintaince headaches longterm.

I think that makes sense.  vGT can leave alot of the lowlevel work such
as power management to i915 then, so we don't duplicate that code in vGT
and i915.

Stacking vGT on top of i915 also makes it alot easier to have it a
runtime option (just an additional kernel module you load when you need
it).  Other way around vGT would be a hard dependency for i915, even if
you don't use it (you could compile it out probably, but distro kernels
can't do that if they want support vGT).

Another note: Right now the guest display can only be sent to one of the
crtcs.  Long term I want more options there, such as exporting the guest
display as dma-buf on the host so it can be blitted to some window.  Or
even let the gpu encode the guest display as video, so we can send it
off over the network.  I suspect that is also easier to implement if
i915 manages all resources and vGT just allocates from i915 what it
needs for the guest.

cheers,
  Gerd

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Daniel Vetter]

On Thu, Oct 23, 2014 at 01:01:28PM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > Stuf like driver load/unload, suspend/resume, runtime pm and gpu reset are
> > already supre-fragile as-is. Every time we change something in there, a
> > bunch of related things fall apart. With vgt we'll have even more
> > complexity in there, and I really think we need to make that complexity
> > explicit. Otherwise we'll always break vgt support for host systems by
> > accident when working on upstream. So in my experience being explicit with
> > these depencies massively reduces maintaince headaches longterm.
> 
> I think that makes sense.  vGT can leave alot of the lowlevel work such
> as power management to i915 then, so we don't duplicate that code in vGT
> and i915.

It's not just the duplication, but interactions. And like I've said those
are already making things in the i915 really messy. And then there's also
all the new features like gpu scheduling, runtime pm and all that which
we're constantly adding. Keeping up the appearance that i915 is in control
on the host side but actually isn't would fall appart rather quickly I
fear.

> Stacking vGT on top of i915 also makes it alot easier to have it a
> runtime option (just an additional kernel module you load when you need
> it).  Other way around vGT would be a hard dependency for i915, even if
> you don't use it (you could compile it out probably, but distro kernels
> can't do that if they want support vGT).

We probably need to make a few changes to i915, maybe on the command
submission side. But those should definitely be small enough that we don't
need a compile time option for them. Furthermore the command submission
code is getting rearchitected now (due to other features), so perfect time
to make sure vgt will work, too.

> Another note: Right now the guest display can only be sent to one of the
> crtcs.  Long term I want more options there, such as exporting the guest
> display as dma-buf on the host so it can be blitted to some window.  Or
> even let the gpu encode the guest display as video, so we can send it
> off over the network.  I suspect that is also easier to implement if
> i915 manages all resources and vGT just allocates from i915 what it
> needs for the guest.

Yeah that should be really simple to add, we already have abstraction for
the different kinds of buffer objects (native shmem backed gem object,
carveout/stolen mem, dma-buf imported, userptr).

I guess from a really high level this boils down to having a xen-like
design (where the hypervisor and dom0 driver are separate, but cooperate
somewhat) or kvm (where the virtualization sits on top of a normal
kernel). Afaics the kvm model seems to have a lot more momentum overall.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Gerd Hoffmann]

  Hi,

> I guess from a really high level this boils down to having a xen-like
> design (where the hypervisor and dom0 driver are separate, but cooperate
> somewhat) or kvm (where the virtualization sits on top of a normal
> kernel). Afaics the kvm model seems to have a lot more momentum overall.

Funny I had the very same analogy in my mind ;)

And -- guess what -- power management is[1] one of the problem fields in
xen.

cheers,
  Gerd

[1] Or used to be? It's a while back I was more deeply involved in xen.

Re: [RFC PATCH 7/8] drm/i915: vgt irq mediation - via a tasklet based mechanism

[Tian, Kevin]

> From: Song, Jike
> Sent: Wednesday, October 22, 2014 3:35 PM
> 
> On 10/01/2014 12:26 AM, Tian, Kevin wrote:
> >  From virtualization p.o.v, the ideal case is to run host i915 irq handler in
> the
> > interrupt context, which meets all the assumption from original code. Using
> > tasklet or other manner still has some restriction. This is a major open we'd
> > like to hear more from you guys. Is it possible to have i915 driver to request
> > two irq numbers: irq1 for real device and irq2 is purely faked one. vgt
> handler
> > registers on irq1 and i915 hanlder registers on irq2, and then we can use self
> > IPI to trigger irq2 when injection is required. But I'm not sure whether this is
> > an existing feature in kernel, or need some core enhancement in irq
> sub-system...
> 
> Hi Kevin, Daniel,
> 
>   I'm so excited to know that, there is an existing feature in kernel: irq_work.
> Basicly it allows us to run the host i915 ISR in hardirq context prefectly, what's
> needed is to select CONFIG_IRQ_WORK in Kconfig :)
> 
>   I'll use that in the v2 patches.
> 

Sounds like a better option. :-)

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

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Tian, Kevin]

> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Thursday, October 23, 2014 4:56 PM
> 
> On Thu, Oct 23, 2014 at 11:13:51AM +0800, Jike Song wrote:
> > On 10/22/2014 05:48 PM, Daniel Vetter wrote:
> > >So on a very high level I don't understand this design. For the guest side
> > >it's completely clear that we need a bunch of hooks over the driver to
> > >make paravirtualization work.
> > >
> > >But on the host side I expect the driver to be in full control of the
> > >hardware. I haven't really seen the other side but it looks like with vgt
> > >we actually have two drivers fighting over the hardware, which requires
> > >the various hooks to avoid disaster. The drm subsystem is littered with
> > >such attempts, and they all didn't end up in a pretty way.
> > >
> > >So way can't we have the vgt support for guests sit on top of i915, using
> > >the i915 functions to set up pagetables, contexts and reserve gtt areas
> > >for the guests? Then we'd have just one driver in control of the hardware,
> > >and vgt on the host side would just look like a really crazy interace
> > >layer between virtual hosts and the low-level driver, similar to who the
> > >execbuf ioctl is a really crazy interface between userspace and the
> > >low-level driver.
> >
> >  Yes we can do this, but that also means lots of pollution to existing i915
> > codes, only for virtualization purpose. Currently vgt has pretty abstractions
> > for both host i915 and guest graphics drivers, mixing vgt and host i915
> means
> > breaking that.  I believe a vgt-integrated repository will be better to look
> > at :)
> 
> The problem is that we need to have this integration anyway. Using the
> current design it will be hidden behind some thin abstraction, but it will
> be as invasive and fragile as if the interactions is made explicit.
> 
> Stuf like driver load/unload, suspend/resume, runtime pm and gpu reset are
> already supre-fragile as-is. Every time we change something in there, a
> bunch of related things fall apart. With vgt we'll have even more
> complexity in there, and I really think we need to make that complexity
> explicit. Otherwise we'll always break vgt support for host systems by
> accident when working on upstream. So in my experience being explicit with
> these depencies massively reduces maintaince headaches longterm.
> 
> Of course for starting up the vgt effort it's better to have as much
> separation as possible. But the balance between development and
> maintainability do change quite a bit when merging code upstream.

Yep. Just synced with Jike. Actually this is the way we'd also prefer to, 
which makes vgt a more integrated feature in i915 driver. We'll think 
about all the technical detail along this direction, and report back any 
design issue to discuss in advance. In the meantime, we'll try to provide 
a complete repo including vgt component, in next version, so you can 
see clearly the interactions between them. Likely it will take more time 
than original approach, but it's worthy of doing so... :-)

> 
> Another benefit of the inverted design with vgt sitting on top of normal
> i915 for host support is better automated testing. At least with kvm we
> could then simply create a kvm guest without any special boot parameters
> and run some really basic testcases in the guest to make sure vgt doesn't
> get broken. That would fit rather nicely into i-g-t. Of course with xengt
> we unfortunately can't do that, since we'd need to boot with the
> hypervisor.

yes, that'd be a nice step. kvm support is important in our plan, though
it's now behind Xen side regarding to quality.

> 
> But I think without such smoketesting in the automated upstream test suite
> we'll break vgt support constantly in upstream. So I think we really need
> this too, at least long term.
> 
> I hope that explains a bit where I'm coming from. Note that this is just
> about the host side, imo the guest side can be merged as soon as detailed
> review has been completed.


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

Re: [RFC PATCH 0/8] Add host i915 support for vGPU

[Tian, Kevin]

> From: Daniel Vetter
> Sent: Thursday, October 23, 2014 8:10 PM
> 
> On Thu, Oct 23, 2014 at 01:01:28PM +0200, Gerd Hoffmann wrote:
> >   Hi,
> >
> > > Stuf like driver load/unload, suspend/resume, runtime pm and gpu reset
> are
> > > already supre-fragile as-is. Every time we change something in there, a
> > > bunch of related things fall apart. With vgt we'll have even more
> > > complexity in there, and I really think we need to make that complexity
> > > explicit. Otherwise we'll always break vgt support for host systems by
> > > accident when working on upstream. So in my experience being explicit
> with
> > > these depencies massively reduces maintaince headaches longterm.
> >
> > I think that makes sense.  vGT can leave alot of the lowlevel work such
> > as power management to i915 then, so we don't duplicate that code in vGT
> > and i915.
> 
> It's not just the duplication, but interactions. And like I've said those
> are already making things in the i915 really messy. And then there's also
> all the new features like gpu scheduling, runtime pm and all that which
> we're constantly adding. Keeping up the appearance that i915 is in control
> on the host side but actually isn't would fall appart rather quickly I
> fear.

yes, for duplication we also try to avoid in original approach, e.g. when 
talking about PM or reset, we leverage the i915 interface but just adding
vgt hooks there to do vgt specific task. I believe this won't change even
with new proposed approach.

the actually thing making difference is the interception of all MMIO/GTT
accesses, which separates vgt from i915 in very low level, with the
limitation like Daniel mentioned which can be better maintained if 
exposing in a higher level.

> 
> > Stacking vGT on top of i915 also makes it alot easier to have it a
> > runtime option (just an additional kernel module you load when you need
> > it).  Other way around vGT would be a hard dependency for i915, even if
> > you don't use it (you could compile it out probably, but distro kernels
> > can't do that if they want support vGT).
> 
> We probably need to make a few changes to i915, maybe on the command
> submission side. But those should definitely be small enough that we don't
> need a compile time option for them. Furthermore the command submission
> code is getting rearchitected now (due to other features), so perfect time
> to make sure vgt will work, too.

we'll think about how to avoid low level interception, instead explicitly 
spelling out vgt requirement within high level interface. But I don't expect
this to be simple vgt-on-top-of-i915 model, i.e. vgt just becomes a caller of
i915. There's gonna various places where we need vgt hooks within the 
high level interfaces to make things correct, like PM, reset, command
submission, interrupt, etc. 

> 
> > Another note: Right now the guest display can only be sent to one of the
> > crtcs.  Long term I want more options there, such as exporting the guest
> > display as dma-buf on the host so it can be blitted to some window.  Or
> > even let the gpu encode the guest display as video, so we can send it
> > off over the network.  I suspect that is also easier to implement if
> > i915 manages all resources and vGT just allocates from i915 what it
> > needs for the guest.
> 
> Yeah that should be really simple to add, we already have abstraction for
> the different kinds of buffer objects (native shmem backed gem object,
> carveout/stolen mem, dma-buf imported, userptr).

We have internal patch for such feature, allowing an user space agent
to map VM's framebuffer and then composite for random effects. It's not
included now as not a core feature. :-)

> 
> I guess from a really high level this boils down to having a xen-like
> design (where the hypervisor and dom0 driver are separate, but cooperate
> somewhat) or kvm (where the virtualization sits on top of a normal
> kernel). Afaics the kvm model seems to have a lot more momentum overall.

right, that's part of the story, and why we initially even have vgt as a
separate kernel module. Our goal is to have a single design working for
both Xen/KVM, by keeping core logic in i915, glued w/ a shim driver 
provided by individual hypervisor.

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