Hi Chris, thanks very much for your detailed comments! I'll try
to address them as well as in your other emails.
On 09/19/2014 03:25 PM, Chris Wilson wrote:
On Sat, Sep 20, 2014 at 02:47:01AM +0800, Jike Song wrote:
From: Yu Zhang <yu.c.zhang@xxxxxxxxx>
Introduce a PV INFO structure, to facilitate the Intel GVT-g
technology, which is a GPU virtualization solution with mediated
pass-through(previously known as XenGT). This page contains the
shared information between i915 driver and the mediator. For now,
this structure utilizes an area of 4K bypes on HSW GPU's unused
MMIO space to support existing production. Future hardware will
have the reserved window architecturally defined, and layout of
the page will be added in future BSpec.
The i915 driver load routine detects if it is running in a VM by
reading the contents of this PV INFO page. Thereafter a flag,
vgpu_active is set, and intel_vgpu_active() is used by checking
this flag to conclude if gpu is virtualized with Intel GVT-g. By
now, it will return true only when the driver is running in the
XenGT environment on HSW.
Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxx>
Signed-off-by: Jike Song <jike.song@xxxxxxxxx>
Signed-off-by: Eddie Dong <eddie.dong@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_dma.c | 2 ++
drivers/gpu/drm/i915/i915_drv.h | 52 +++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_gem_gtt.c | 21 +++++++++++++++
drivers/gpu/drm/i915/i915_reg.h | 13 ++++++++++
4 files changed, 88 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 09dc0d0..927acea 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1739,6 +1739,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
goto out_freewq;
}
+ i915_check_vgpu(dev);
+
intel_irq_init(dev);
intel_uncore_sanitize(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e3ca8df..caae6ed 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1442,6 +1442,48 @@ struct i915_frontbuffer_tracking {
unsigned flip_bits;
};
+/*
+ * The following structure pages are defined in GEN MMIO space
+ * for virtualization. (One page for now)
+ */
+
+struct vgt_if {
+ uint64_t magic; /* VGT_MAGIC */
+ uint16_t version_major;
+ uint16_t version_minor;
+ uint32_t vgt_id; /* ID of vGT instance */
+ uint32_t rsv1[12]; /* pad to offset 0x40 */
+ /*
+ * Data structure to describe the balooning info of resources.
+ * Each VM can only have one portion of continuous area for now.
+ * (May support scattered resource in future)
+ * (starting from offset 0x40)
+ */
+ struct {
+ /* Aperture register balooning */
+ struct {
+ uint32_t my_base;
+ uint32_t my_size;
+ } low_gmadr; /* aperture */
+ /* GMADR register balooning */
+ struct {
+ uint32_t my_base;
+ uint32_t my_size;
+ } high_gmadr; /* non aperture */
+ /* allowed fence registers */
+ uint32_t fence_num;
+ uint32_t rsv2[3];
+ } avail_rs; /* available/assigned resource */
+ 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 */
+};
Where is the other half of this if? Shouldn't this be defined there and
included here?
Among the bottom half page, current only display_ready is used by i915.
+
struct drm_i915_private {
struct drm_device *dev;
struct kmem_cache *slab;
@@ -1454,6 +1496,8 @@ struct drm_i915_private {
struct intel_uncore uncore;
+ bool vgpu_active;
struct i915_virtual_gpu {
bool active;
} vgu;
will be tidier and future proof.
Okay.
+
struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
@@ -2289,6 +2333,14 @@ extern void intel_uncore_check_errors(struct drm_device *dev);
extern void intel_uncore_fini(struct drm_device *dev);
extern void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore);
+extern void i915_check_vgpu(struct drm_device *dev);
+static inline bool intel_vgpu_active(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+
+ return dev_priv->vgpu_active;
return to_i915(dev)->vgpu_active;
Or just use struct drm_i915_private and drop all the pointer dancing.
Thanks for 2 good points, I prefer the former one :)
+}
+
void
i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
u32 status_mask);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 14f078c..0309a2d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,27 @@
#include "i915_trace.h"
#include "intel_drv.h"
+void i915_check_vgpu(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ uint64_t magic;
+ uint32_t version;
BUILD_BUG_ON(sizeof(struct vgt_if) != VGT_PVINFO_SIZE);
Okay
+
+ dev_priv->vgpu_active = false;
Already false on entry.
Okay
+
+ if (!IS_HASWELL(dev))
+ return;
+
+ magic = I915_READ64(vgt_info_off(magic));
+ if (magic != VGT_MAGIC)
+ return;
+
+ version = (I915_READ16(vgt_info_off(version_major)) << 16) |
+ I915_READ16(vgt_info_off(version_minor));
+ if (version == INTEL_VGT_IF_VERSION)
+ dev_priv->vgpu_active = true;
A debug trace would be very useful here, maybe even INFO since it is
useful information to the user (when enabled).
You mean a DRM_INFO? Okay.
What should the failure path be? Presumably you want to disable the
driver under a virtual environment with an abi mismatch.
For the failure path, we want to simply let the i915 driver think that
it's running in a native environment.
+}
+
static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b65bdfc..a70f12e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6661,4 +6661,17 @@ enum punit_power_well {
#define _PALETTE_A (dev_priv->info.display_mmio_offset + 0xa000)
#define _PALETTE_B (dev_priv->info.display_mmio_offset + 0xa800)
+/* The MMIO offset of the shared info between i915 and vgt driver */
+#define VGT_PVINFO_PAGE 0x78000
+#define VGT_PVINFO_SIZE 0x1000
+
+#define VGT_MAGIC 0x4776544776544776 /* 'vGTvGTvG' */
+#define VGT_VERSION_MAJOR 1
+#define VGT_VERSION_MINOR 0
+#define INTEL_VGT_IF_VERSION ((VGT_VERSION_MAJOR << 16) | VGT_VERSION_MINOR)
#define INTEL_VGT_IF_VERSION_ENCODE(major, minor) ((major) << 16 | (minor))
#define INTEL_VGT_IF_VERSION \
INTEL_VGT_IF_VERSION_ENCODE(VGT_VERSION_MAJOR, VGT_VERSION_MiN)
and then
version = INTEL_VGT_IF_VERSION_ENCODE(I915_READ16(vgt_info_off(version_major)),
I915_READ16(vgt_info_off(version_minor));
Okay.
+
+#define vgt_info_off(x) \
+ (VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
(VGT_PVINFO_PAGE + offsetof(struct vgt_if, x))
vgt_info_off feels a little redundant given usage. Perhaps vgt_reg(x).
Okay. What about vgtif_reg(x)?
Now, given that these are simply trapped memory access, wouldn't it be
simply to have:
struct i915_virtual_gpu {
struct vgt_if *if;
} vgu;
static inline bool intel_vgpu_active(struct drm_i915_private *i915) { return i915->vgpu.if; }
Thanks for your demo code, can I do some minor change?
static inline bool intel_vgpu_active(struct drm_device *dev)
{
return to_i915(dev)->vgpu.if != NULL;
}
then you have constructs like
void i915_check_vgpu(struct drm_i915_private *i915)
{
struct vgt_if *if;
if = i915->regs + VGT_PVINFO_PAGE;
if (if->magic != VGT_MAGIC)
return;
if (INTEL_VGT_IF_VERSION !=
INTEL_VGT_IF_VERSION_ENCODE(if->version_major,
if->version_minor))
return;
i915->vgpu.if = if;
}
And later, for example:
if (intel_vgpu_active(dev_priv))
dev_priv->num_fence_regs = dev_priv->vgpu.if->fence_num;
Thanks again for the demo code!
-Chris
--
Thanks,
Jike
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
