[Intel-gfx] [PATCH 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.
Jike Song
jike.song at intel.com
Fri Sep 19 12:23:50 CEST 2014
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 at intel.com>
>>
>> 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 at intel.com>
>> Signed-off-by: Jike Song <jike.song at intel.com>
>> Signed-off-by: Eddie Dong <eddie.dong at intel.com>
>> ---
>> 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
More information about the Intel-gfx
mailing list