Re: [PATCH 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.

[Jike Song <jike.song@intel.com>]

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@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@intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Eddie Dong <eddie.dong@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