From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jike Song Subject: Re: [PATCH 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g. Date: Fri, 19 Sep 2014 18:23:50 +0800 Message-ID: <541C0436.3010104@intel.com> References: <1411152428-7226-1-git-send-email-jike.song@intel.com> <1411152428-7226-2-git-send-email-jike.song@intel.com> <20140919072546.GB21738@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 1922E6E604 for ; Fri, 19 Sep 2014 03:27:43 -0700 (PDT) In-Reply-To: <20140919072546.GB21738@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org, "Zhang, Yu C" List-Id: intel-gfx@lists.freedesktop.org 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 >> >> 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 >> Signed-off-by: Jike Song >> Signed-off-by: Eddie Dong >> --- >> 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