Re: [PATCH 7/8] drm/i915: Create vgpu specific write MMIO to reduce traps

Re: [PATCH 7/8] drm/i915: Create vgpu specific write MMIO to reduce traps

[Chris Wilson]

On Sat, Sep 20, 2014 at 02:47:07AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> In the virtualized environment, forcewake operations are not
> necessory for the driver, because mmio accesses will be trapped
> and emulated by the host side, and real forcewake operations are
> also done in the host. New mmio write handlers are added to directly
> call the __raw_i915_write, therefore will reduce many traps and
> increase the overall performance for drivers runing in the VM
> with Intel GVT-g enhancement

Explain why this is not done inside uncore_init.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

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?

> +
>  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.
> +
>  	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.

> +}
> +
>  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);

> +
> +	dev_priv->vgpu_active = false;

Already false on entry.

> +
> +	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).

What should the failure path be? Presumably you want to disable the
driver under a virtual environment with an abi mismatch.

> +}
> +
>  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));

> +
> +#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).

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; }

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;

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/8] drm/i915: Create vgpu specific write MMIO to reduce traps

[Jike Song]

[adding cc. By mistake I set suppresscc=all in my ~/.gitconfig]

On 09/19/2014 02:59 PM, Chris Wilson wrote:
> On Sat, Sep 20, 2014 at 02:47:07AM +0800, Jike Song wrote:
>> From: Yu Zhang <yu.c.zhang@intel.com>
>>
>> In the virtualized environment, forcewake operations are not
>> necessory for the driver, because mmio accesses will be trapped
>> and emulated by the host side, and real forcewake operations are
>> also done in the host. New mmio write handlers are added to directly
>> call the __raw_i915_write, therefore will reduce many traps and
>> increase the overall performance for drivers runing in the VM
>> with Intel GVT-g enhancement
>
> Explain why this is not done inside uncore_init.
> -Chris

Hi Chris,

  The gpu_active flag is set by intel_detect_vgpu(), which uses I915_READxx, should be after intel_uncore_init.


--
Thanks,
Jike

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Chris Wilson]

On Sat, Sep 20, 2014 at 02:47:02AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> In XenGT, the global graphic memory space is partitioned by multiple
> vgpu instances in different VMs. The ballooning code is added in
> i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
> mark the graphic address space which are partitioned out to other
> vgpus as reserved.
> One special treatment for this scenario is that the guard page is
> added at the end of both aperture and non-aperture spaces. This is
> to follow the behavior pattern in native case. However here we have
> a question: does the prefetch issue mentioned at the begining of
> i915_gem_setup_global_gtt() happens for all the GTT entries, or just
> at then end of the BAR0 space?

The CS prefetcher happens everywhere and so can read from the end of one
range into the beginning of another clients, so requires consideration
if not actualy a guard page. The very last entry in the whole GTT must
be a guard page (or allocated to something not prefetchable).

> If all entries may have prefetch issues,
> then this special guard page is necessary, to protect unexpected
> accesses into GTT entries partitioned out by other VMs. Otherwise,
> we may only need one guard page at the end of the physical GTT space.

I am a bit dubious how this works when userspace still believes that it
can access the whole mappable aperture, and then how every driver
attempts to pin its own planes, rings and whatnot (since it still
believes that it is talking to the actual hardware and that the hardware
requires access to its virtual address). The host should be able to move the
ranges around in order to accommodate userspace in any particular guest
(hence a balloon interface I presume). But I don't see how that is
possible, and you don't explain it either.

The implementation also looks backwards. To work correctly with the GTT
allocator, you need to preallocate the reserved space such that it can
only allocate from the allowed ranges. Similarly, it should evict any
conflicting nodes when deballooning.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Chris Wilson]

On Sat, Sep 20, 2014 at 02:47:04AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> Framebuffer compression is disabled when driver detects it's
> running in XenGT VM, because XenGT does not provide emulations
> for FBC related operations, and we do not expose stolen memory
> to the VM.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 927acea..bb4ad52 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1741,6 +1741,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	i915_check_vgpu(dev);
>  
> +	/* disable framebuffer compression in vgt */
> +	if (intel_vgpu_active(dev))
> +		i915.enable_fbc = 0;

This should be done inside intel_enable_fbc() so that the correct reason
is given as to why it is disabled.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Chris Wilson]

On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> Display switch logic is added to notify the vgt mediator that
> current vgpu have a valid surface to show. It does so by writing
> the display_ready field in PV INFO page, and then will be handled
> in vgt mediator. This is useful to avoid trickiness when the VM's
> framebuffer is being accessed in the middle of VM modesetting,
> e.g. compositing the framebuffer in the host side
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index bb4ad52..d9462e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	} else {
>  		/* Start out suspended in ums mode. */
>  		dev_priv->ums.mm_suspended = 1;
> +		if (intel_vgpu_active(dev)) {
> +			/*
> +			 * Notify a valid surface after modesetting,
> +			 * when running inside a VM.
> +			 */
> +			I915_WRITE(vgt_info_off(display_ready),
> +				VGT_DRV_DISPLAY_READY);

That's interesting. As i915.ko is definitely not ready at this point.

> +		}
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a70f12e..38d606c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6673,5 +6673,12 @@ enum punit_power_well {
>  #define vgt_info_off(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/*
> + * The information set by the guest gfx driver, through the display_ready field
> + */
> +enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> +};
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..3af9259 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			intel_modeset_check_state(set->crtc->dev);
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);

Should there not be some coordination before we start the modeset?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/8] drm/i915: Disable power management for i915 driver in VM

[Chris Wilson]

On Sat, Sep 20, 2014 at 02:47:06AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> In XenGT, GPU power management is controlled by host i915
> driver, so there is no need to provide virtualized GPU PM
> support. In the future it might be useful to gather VM
> input for freq boost, but now let's disable it simply.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 675e8a2..1535fa3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5419,6 +5419,12 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>  		mutex_unlock(&dev->struct_mutex);
>  	} else if (INTEL_INFO(dev)->gen >= 6) {
>  		/*
> +		 * Powersaving is disabled when running inside a VM.
> +		 */
> +		if (intel_vgpu_active(dev))
> +			return;

Don't special case this to gen6+, it always applies because the comment
is not accurate, whereas the changelog is a much more apt description.

/* Powersaving is controlled by the host when inside a VM */
if (intel_vgpu_active(dev))
	return;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/8] drm/i915: Support alias ppgtt in VM if ppgtt is enabled

[Chris Wilson]

On Sat, Sep 20, 2014 at 02:47:08AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> The current XenGT only supports alias ppgtt. And the emulation
> is done in XenGT host by first trapping PP_DIR_BASE mmio
> accesses. Updating PP_DIR_BASE by using instructions such as
> MI_LOAD_REGISTER_IMM are hard to emulate and are not supported
> in current XenGT. Therefore this patch also added a new callback
> routine - vgpu_mm_switch() to set the PP_DIR_BASE by mmio writes.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c     |  5 +++++
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a3fe7c0..63dccd2 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1746,6 +1746,11 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		i915.enable_fbc = 0;
>  
>  		intel_vgpu_reg_write_init(dev);
> +
> +		if (USES_FULL_PPGTT(dev)) {
> +			DRM_INFO("Only support alias ppgtt for now in VM.\n");
> +			i915.enable_ppgtt = 1;
> +		}

This should be moved to sanitize_enable_ppgtt(), probably by expanding
HAS_PPGTT(), e.g.:

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 07dafa2c2d8c..b1fa13942d14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2139,8 +2139,6 @@ struct drm_i915_cmd_table {
 
 #define HAS_HW_CONTEXTS(dev)   (INTEL_INFO(dev)->gen >= 6)
 #define HAS_LOGICAL_RING_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 8)
-#define HAS_ALIASING_PPGTT(dev)        (INTEL_INFO(dev)->gen >= 6)
-#define HAS_PPGTT(dev)         (INTEL_INFO(dev)->gen >= 7 && !IS_GEN8(dev))
 #define USES_PPGTT(dev)                (i915.enable_ppgtt)
 #define USES_FULL_PPGTT(dev)   (i915.enable_ppgtt == 2)
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a234446a8678..3bea0bdfd276 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -35,13 +35,23 @@ static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
 
 static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 {
-       if (enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev))
+       bool has_aliasing_ppgtt;
+       bool has_full_ppgtt;
+
+       has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
+       has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
+       if (IS_GEN8(dev))
+               has_full_ppgtt = false; /* XXX why? */
+       if (intel_vgpu_active(dev))
+               has_full_ppgtt = false; /* emulation is too hard */
+
+       if (enable_ppgtt == 0 || !has_aliasing_ppgtt)
                return 0;
 
        if (enable_ppgtt == 1)
                return 1;
 
-       if (enable_ppgtt == 2 && HAS_PPGTT(dev))
+       if (enable_ppgtt == 2 && has_full_ppgtt)
                return 2;
 
 #ifdef CONFIG_INTEL_IOMMU
@@ -59,7 +69,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
                return 0;
        }
 
-       return HAS_PPGTT(dev) ? 2 : HAS_ALIASING_PPGTT(dev) ? 1 : 0;
+       return has_full_ppgtt ? 2 : has_aliasing_ppgtt ? 1 : 0;
 }

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jike Song]

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

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Wang, Zhi A]

Hi Chris:
    Thanks for the comment. See my comments below.

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Chris Wilson
Sent: Friday, September 19, 2014 4:15 PM
To: Song, Jike
Cc: Vetter, Daniel; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> Display switch logic is added to notify the vgt mediator that current 
> vgpu have a valid surface to show. It does so by writing the 
> display_ready field in PV INFO page, and then will be handled in vgt 
> mediator. This is useful to avoid trickiness when the VM's framebuffer 
> is being accessed in the middle of VM modesetting, e.g. compositing 
> the framebuffer in the host side
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c 
> b/drivers/gpu/drm/i915/i915_dma.c index bb4ad52..d9462e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	} else {
>  		/* Start out suspended in ums mode. */
>  		dev_priv->ums.mm_suspended = 1;
> +		if (intel_vgpu_active(dev)) {
> +			/*
> +			 * Notify a valid surface after modesetting,
> +			 * when running inside a VM.
> +			 */
> +			I915_WRITE(vgt_info_off(display_ready),
> +				VGT_DRV_DISPLAY_READY);

That's interesting. As i915.ko is definitely not ready at this point.

Zhi: Will remove this piece in V2, as we have already had a similar logic in intel_crtc_set_config.

> +		}
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> b/drivers/gpu/drm/i915/i915_reg.h index a70f12e..38d606c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6673,5 +6673,12 @@ enum punit_power_well {  #define 
> vgt_info_off(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/*
> + * The information set by the guest gfx driver, through the 
> +display_ready field  */ enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> +};
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..3af9259 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			intel_modeset_check_state(set->crtc->dev);
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);

Should there not be some coordination before we start the modeset?

Zhi: In the design of GVT-g, there will be only one VM can occupy the display monitor at one time,
which is called "Foreground VM". Switching foreground VM is implemented by switching only a part
of display registers e.g. DSPCNTR/DSPSURF/DISPSTRIDE/DSPLINOFF/DSPTILEOFF. Other display 
mode set registers are virtualized to VM. So we may not care about the beginning of a modeset
sequence here.

But we really expect to know when a VM can be switched to foreground, which is, VM has
already configured those registers to valid values. If we switch a VM without valid values in those
registers to foreground e.g. during VM booting time, I'm afraid that's not what we expect...

-Chris

--
Chris Wilson, Intel Open Source Technology Centre _______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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 */
> +};

Needs __packed to make sure we exactly match the hw layout. We do the same
with the vbt headers, mostly as documentation.

> +
>  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 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;
> +}
> +
>  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;
> +
> +	dev_priv->vgpu_active = false;
> +
> +	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;
> +}
> +
>  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 vgt_info_off(x) \
> +	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)

I think a new header file i915_vgt.h for these definitions would be good.
i915_reg.h is giant already ...
-Daniel

> +
> +
>  #endif /* _I915_REG_H_ */
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Daniel Vetter]

On Fri, Sep 19, 2014 at 09:05:57AM +0100, Chris Wilson wrote:
> On Sat, Sep 20, 2014 at 02:47:02AM +0800, Jike Song wrote:
> > From: Yu Zhang <yu.c.zhang@intel.com>
> > If all entries may have prefetch issues,
> > then this special guard page is necessary, to protect unexpected
> > accesses into GTT entries partitioned out by other VMs. Otherwise,
> > we may only need one guard page at the end of the physical GTT space.
> 
> I am a bit dubious how this works when userspace still believes that it
> can access the whole mappable aperture, and then how every driver
> attempts to pin its own planes, rings and whatnot (since it still
> believes that it is talking to the actual hardware and that the hardware
> requires access to its virtual address). The host should be able to move the
> ranges around in order to accommodate userspace in any particular guest
> (hence a balloon interface I presume). But I don't see how that is
> possible, and you don't explain it either.

Yeah this is something we need to fix, either by pimping i915 fault
support to be able to split up a big bo into chunks, or by telling
userspace about the massively reduced contiguous mapping size. It should
be tracked somewhere as a todo task ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Fri, Sep 19, 2014 at 06:02:46PM +0200, Daniel Vetter wrote:
> I think a new header file i915_vgt.h for these definitions would be good.
> i915_reg.h is giant already ...

Separate header would also lend itself well for some kerneldoc DOC:
overview section and for properly documenting all the vgt_ interface
functions. See e.g. the cmd parser for a nice example of how to integrate
a documentation for new features into our drm driver docbook.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Daniel Vetter]

On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> From: Yu Zhang <yu.c.zhang@intel.com>
> 
> Display switch logic is added to notify the vgt mediator that
> current vgpu have a valid surface to show. It does so by writing
> the display_ready field in PV INFO page, and then will be handled
> in vgt mediator. This is useful to avoid trickiness when the VM's
> framebuffer is being accessed in the middle of VM modesetting,
> e.g. compositing the framebuffer in the host side
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> Signed-off-by: Jike Song <jike.song@intel.com>
> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
>  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index bb4ad52..d9462e1 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	} else {
>  		/* Start out suspended in ums mode. */
>  		dev_priv->ums.mm_suspended = 1;
> +		if (intel_vgpu_active(dev)) {
> +			/*
> +			 * Notify a valid surface after modesetting,
> +			 * when running inside a VM.
> +			 */
> +			I915_WRITE(vgt_info_off(display_ready),
> +				VGT_DRV_DISPLAY_READY);
> +		}
>  	}
>  
>  	i915_setup_sysfs(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index a70f12e..38d606c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6673,5 +6673,12 @@ enum punit_power_well {
>  #define vgt_info_off(x) \
>  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
>  
> +/*
> + * The information set by the guest gfx driver, through the display_ready field
> + */
> +enum vgt_display_status {
> +	VGT_DRV_DISPLAY_NOT_READY = 0,
> +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> +};
>  
>  #endif /* _I915_REG_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b78f00a..3af9259 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  			intel_modeset_check_state(set->crtc->dev);
>  	}
>  
> +	if (intel_vgpu_active(dev)) {
> +		/*
> +		 * Notify a valid surface after modesetting,
> +		 * when running inside a VM.
> +		 */
> +		struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> +	}

Since very shortly we now have the frontbuffer tracking support code.
Should we move it there? See intel_frontbuffer_flush&flip.
-Daniel

> +
>  	if (ret) {
>  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
>  			      set->crtc->base.id, ret);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Tian, Kevin]

> From: Chris Wilson
> Sent: Friday, September 19, 2014 1:06 AM
> 
> On Sat, Sep 20, 2014 at 02:47:02AM +0800, Jike Song wrote:
> > From: Yu Zhang <yu.c.zhang@intel.com>
> >
> > In XenGT, the global graphic memory space is partitioned by multiple
> > vgpu instances in different VMs. The ballooning code is added in
> > i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
> > mark the graphic address space which are partitioned out to other
> > vgpus as reserved.
> > One special treatment for this scenario is that the guard page is
> > added at the end of both aperture and non-aperture spaces. This is
> > to follow the behavior pattern in native case. However here we have
> > a question: does the prefetch issue mentioned at the begining of
> > i915_gem_setup_global_gtt() happens for all the GTT entries, or just
> > at then end of the BAR0 space?
> 
> The CS prefetcher happens everywhere and so can read from the end of one
> range into the beginning of another clients, so requires consideration
> if not actualy a guard page. The very last entry in the whole GTT must
> be a guard page (or allocated to something not prefetchable).

So this only applies to the very last page of the whole GTT, right? for 
normal non-present GTT entries, suppose CS prefetcher should behave
correctly since that can happen anywhere.

If it's true, possibly we don't need reserve a guard page for each
partition, but only reserve one for the last partition at end of the
whole GTT. Or keep current way is also fine, which has the unified 
policy for all partitions, and avoid undesired effect (not sure any)
due to bad setting of the 1st GTT entry in adjacent partition.

> 
> > If all entries may have prefetch issues,
> > then this special guard page is necessary, to protect unexpected
> > accesses into GTT entries partitioned out by other VMs. Otherwise,
> > we may only need one guard page at the end of the physical GTT space.
> 
> I am a bit dubious how this works when userspace still believes that it
> can access the whole mappable aperture, and then how every driver
> attempts to pin its own planes, rings and whatnot (since it still
> believes that it is talking to the actual hardware and that the hardware
> requires access to its virtual address). The host should be able to move the
> ranges around in order to accommodate userspace in any particular guest
> (hence a balloon interface I presume). But I don't see how that is
> possible, and you don't explain it either.

for this we discussed with Jesse/Daniel earlier. In an ideal world user space
should assume hardware resource knowledge, i.e. it should get all available
resource from KMD. In such case once ballooning is completed, all the clients
will only use allocated portion. However as you said now there's assumption
in user space, which will fail in XenGT say assuming 256MB aperture but
only have 64MB allocated. Jesse/Daniel discussed two ways to improve
this situation, either add a lightweight page fault mechanism to catch the
fault, or change mesa to remove the assumption. That can be an orthogonal
effort with this patch set.

for displays, we do have a problem for multi-monitor support, if framebuffer
size exceeds allocated memory. But that's the same situation on bare metal,
just the matter of different level of capability limitation.

and our balloon is static, i.e. everything settled down in the driver load phase.
dynamic ballooning is very complex, since the specific page may be referenced
in kernel/user level, in different structures, etc. That's a good research topic,
but let's pursue simple solution first. :-)

> 
> The implementation also looks backwards. To work correctly with the GTT
> allocator, you need to preallocate the reserved space such that it can
> only allocate from the allowed ranges. Similarly, it should evict any
> conflicting nodes when deballooning.

Could you elaborate a bit for above suggestion?

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 0/8] Add enlightenments 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 detail, please refer to
https://01.org/xen/blogs/wangbo85/2014/intel-gvt-gxengt-pubic-release

This patch set includes necessary code changes when i915 driver
runs inside a VM. Though ideally we can run an unmodified i915
driver in VM, adding such enlightenments can greatly reduce the
virtualization complexity in orders of magnitude. Code changes
for the host side, which includes the actual Intel GVT-g
implementation, are included in another patchset, and will be
sent out later. 

The primary change introduced here is to implement so-called
"address space ballooning" technique. XenGT partitions global
graphics memory among multiple VMs, so each VM can directly
access a portion of the memory w/o hypervisor's intervention,
e.g. filling textures and queuing commands. However w/ the
partitioning an unmodified i915 driver would assume a smaller
graphics memory starting from address ZERO, so requires XenGT
core module (vgt) to translate the graphics address between
'guest view' and 'host view', for all registers and command
opcodes which contain a graphics memory address. To reduce the
complexity, XenGT introduces "address space ballooning", by
telling the exact partitioning knowledge to each guest i915
driver, which then reserves and prevents non-allocated portions
from allocation. Then vgt module only needs to scan and validate
graphics addresses w/o complexity of translation.

Note: The partitioning of global graphics memory may break some
applications, with large objects in the aperture, because current
userspace assumes half of the aperture usable. That would need
separate fix either in user space (e.g. remove assumption in mesa)
or in kernel (w/ some faulting mechanism).

The partitioning knowledge is conveyed through a reserved MMIO
range, called PVINFO, which will be architecturally reserved in
future hardware generations. Another information carried through
PVINFO is about the number of fence registers. As a global resource
XenGT also partitions them among VMs.

Other changes are trivial as optimizations, to either reduce the
trap overhead or disable power management features which don't
make sense in a virtualized environment.

Yu Zhang (8):
  drm/i915: Introduce a PV INFO page structure for Intel GVT-g.
  drm/i915: Adds graphic address space ballooning logic
  drm/i915: Partition the fence registers for vgpu in i915 driver
  drm/i915: Disable framebuffer compression for i915 driver in VM
  drm/i915: Add the display switch logic for vgpu in i915 driver
  drm/i915: Disable power management for i915 driver in VM
  drm/i915: Create vgpu specific write MMIO to reduce traps
  drm/i915: Support alias ppgtt in VM if ppgtt is enabled

 drivers/gpu/drm/i915/i915_dma.c      |  22 ++++
 drivers/gpu/drm/i915/i915_drv.h      |  53 +++++++++
 drivers/gpu/drm/i915/i915_gem.c      |   4 +
 drivers/gpu/drm/i915/i915_gem_gtt.c  | 214 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_reg.h      |  20 ++++
 drivers/gpu/drm/i915/intel_display.c |  10 ++
 drivers/gpu/drm/i915/intel_pm.c      |   6 +
 drivers/gpu/drm/i915/intel_uncore.c  |  24 ++++
 8 files changed, 345 insertions(+), 8 deletions(-)

-- 
1.9.1

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

[Jike Song]

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 */
+};
+
 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 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;
+}
+
 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;
+
+	dev_priv->vgpu_active = false;
+
+	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;
+}
+
 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 vgt_info_off(x) \
+	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
+
+
 #endif /* _I915_REG_H_ */
-- 
1.9.1

[PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

In XenGT, the global graphic memory space is partitioned by multiple
vgpu instances in different VMs. The ballooning code is added in
i915_gem_setup_global_gtt(), utilizing the drm mm allocator APIs to
mark the graphic address space which are partitioned out to other
vgpus as reserved.
One special treatment for this scenario is that the guard page is
added at the end of both aperture and non-aperture spaces. This is
to follow the behavior pattern in native case. However here we have
a question: does the prefetch issue mentioned at the begining of
i915_gem_setup_global_gtt() happens for all the GTT entries, or just
at then end of the BAR0 space? If all entries may have prefetch issues,
then this special guard page is necessary, to protect unexpected
accesses into GTT entries partitioned out by other VMs. Otherwise,
we may only need one guard page at the end of the physical GTT space.

Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
Signed-off-by: Eddie Dong <eddie.dong@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 179 ++++++++++++++++++++++++++++++++++--
 1 file changed, 171 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0309a2d..368262d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -30,6 +30,159 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
+struct _balloon_info_ {
+	/*
+	 * There are up to 2 regions per low/high GM that
+	 * might be ballooned. Here, index 0/1 is for low
+	 * GM, 2/3 for high GM.
+	 */
+	struct drm_mm_node space[4];
+} bl_info;
+
+void intel_vgt_deballoon(void)
+{
+	int i;
+
+	DRM_INFO("VGT deballoon.\n");
+
+	for (i = 0; i < 4; i++) {
+		if (bl_info.space[i].allocated)
+			drm_mm_remove_node(&bl_info.space[i]);
+	}
+
+	memset(&bl_info, 0, sizeof(bl_info));
+}
+
+static int vgt_balloon_space(struct drm_mm *mm,
+			     struct drm_mm_node *node,
+			     unsigned long start, unsigned long end)
+{
+	unsigned long size = end - start;
+
+	if (start == end)
+		return -EINVAL;
+
+	DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KB.\n",
+		 start, end, size / 1024);
+
+	return drm_mm_insert_node_in_range_generic(mm, node, size, 0, 0,
+						   start, end,
+						   DRM_MM_SEARCH_DEFAULT,
+						   DRM_MM_CREATE_DEFAULT);
+}
+
+static int intel_vgt_balloon(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_address_space *ggtt_vm = &dev_priv->gtt.base;
+
+	unsigned long low_gm_base, low_gm_size, low_gm_end;
+	unsigned long high_gm_base, high_gm_size, high_gm_end;
+	int ret;
+
+	low_gm_base = I915_READ(vgt_info_off(avail_rs.low_gmadr.my_base));
+	low_gm_size = I915_READ(vgt_info_off(avail_rs.low_gmadr.my_size));
+	high_gm_base = I915_READ(vgt_info_off(avail_rs.high_gmadr.my_base));
+	high_gm_size = I915_READ(vgt_info_off(avail_rs.high_gmadr.my_size));
+
+	low_gm_end = low_gm_base + low_gm_size;
+	high_gm_end = high_gm_base + high_gm_size;
+
+	DRM_INFO("VGT ballooning configuration:\n");
+	DRM_INFO("Low GM: base 0x%lx size %ldKB\n",
+		 low_gm_base, low_gm_size / 1024);
+	DRM_INFO("High GM: base 0x%lx size %ldKB\n",
+		 high_gm_base, high_gm_size / 1024);
+
+	if (low_gm_base < ggtt_vm->start
+	    || low_gm_end > dev_priv->gtt.mappable_end
+	    || high_gm_base < dev_priv->gtt.mappable_end
+	    || high_gm_end > ggtt_vm->start + ggtt_vm->total) {
+		DRM_ERROR("Invalid ballooning configuration!\n");
+		return -EINVAL;
+	}
+
+	memset(&bl_info, 0, sizeof(bl_info));
+
+	/* High GM ballooning */
+	if (high_gm_base > dev_priv->gtt.mappable_end) {
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[2],
+					dev_priv->gtt.mappable_end,
+					high_gm_base);
+
+		if (ret)
+			goto err;
+	}
+
+	if (high_gm_end <= ggtt_vm->start + ggtt_vm->total) {
+		/*
+		 * We need a guard page per low/high GM for each vgpu.
+		 * Here, leave one page at the end of the allocated high
+		 * GM reserved to the scratch page, and balloon it out,
+		 * to serve as guard page.
+		 */
+		if (high_gm_size > PAGE_SIZE) {
+			high_gm_size -= PAGE_SIZE;
+			ggtt_vm->clear_range(ggtt_vm,
+					     high_gm_base + high_gm_size,
+					     PAGE_SIZE, true);
+		} else {
+			BUG_ON(high_gm_size != 0);
+		}
+
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[3],
+					high_gm_base + high_gm_size,
+					ggtt_vm->start + ggtt_vm->total);
+		if (ret)
+			goto err;
+	}
+
+	/* Low GM ballooning */
+	if (low_gm_base > ggtt_vm->start) {
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[0],
+					ggtt_vm->start, low_gm_base);
+
+		if (ret)
+			goto err;
+	}
+
+	if (low_gm_end <= dev_priv->gtt.mappable_end) {
+		/*
+		 * We need a guard page per low/high GM for each vgpu.
+		 * Here, leave one page at the end of the allocated low
+		 * GM reserved to the scratch page, and balloon it out,
+		 * to serve as guard page.
+		 */
+		if (low_gm_size > PAGE_SIZE) {
+			low_gm_size -= PAGE_SIZE;
+			ggtt_vm->clear_range(ggtt_vm,
+					     low_gm_base + low_gm_size,
+					     PAGE_SIZE, true);
+		} else {
+			BUG_ON(low_gm_size != 0);
+		}
+
+		ret = vgt_balloon_space(&ggtt_vm->mm,
+					&bl_info.space[1],
+					low_gm_base + low_gm_size,
+					dev_priv->gtt.mappable_end);
+
+		if (ret)
+			goto err;
+	}
+
+	DRM_INFO("VGT balloon successfully\n");
+	return 0;
+
+err:
+	DRM_ERROR("VGT balloon fail\n");
+	intel_vgt_deballoon();
+	return -ENOMEM;
+}
+
 void i915_check_vgpu(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1682,8 +1835,21 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 
 	BUG_ON(mappable_end > end);
 
-	/* Subtract the guard page ... */
-	drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
+	dev_priv->gtt.base.start = start;
+	dev_priv->gtt.base.total = end - start;
+
+	if (intel_vgpu_active(dev)) {
+		drm_mm_init(&ggtt_vm->mm, start, end - start);
+		ret = intel_vgt_balloon(dev);
+		if (ret)
+			return ret;
+	} else {
+		/* Subtract the guard page ... */
+		drm_mm_init(&ggtt_vm->mm, start, end - start - PAGE_SIZE);
+		/* Clear the reserved guard page */
+		ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
+	}
+
 	if (!HAS_LLC(dev))
 		dev_priv->gtt.base.mm.color_adjust = i915_gtt_color_adjust;
 
@@ -1703,9 +1869,6 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 		obj->has_global_gtt_mapping = 1;
 	}
 
-	dev_priv->gtt.base.start = start;
-	dev_priv->gtt.base.total = end - start;
-
 	/* Clear any non-preallocated blocks */
 	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
 		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
@@ -1714,9 +1877,6 @@ int i915_gem_setup_global_gtt(struct drm_device *dev,
 				     hole_end - hole_start, true);
 	}
 
-	/* And finally clear the reserved guard page */
-	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
-
 	if (USES_PPGTT(dev) && !USES_FULL_PPGTT(dev)) {
 		struct i915_hw_ppgtt *ppgtt;
 
@@ -1757,6 +1917,9 @@ void i915_global_gtt_cleanup(struct drm_device *dev)
 	}
 
 	if (drm_mm_initialized(&vm->mm)) {
+		if (intel_vgpu_active(dev))
+			intel_vgt_deballoon();
+
 		drm_mm_takedown(&vm->mm);
 		list_del(&vm->global_link);
 	}
-- 
1.9.1

[PATCH 3/8] drm/i915: Partition the fence registers for vgpu in i915 driver

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

In XenGT, the fence registers are partitioned by multiple vgpu instances
in different VMs. Routine i915_gem_load() is modified to reset the
num_fence_regs, when the driver detects it's runing in a VM. And the
allocated fence numbers is provided in PV INFO page structure.

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_gem.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2a5351d..3471f15 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5038,6 +5038,10 @@ i915_gem_load(struct drm_device *dev)
 	else
 		dev_priv->num_fence_regs = 8;
 
+	if (intel_vgpu_active(dev))
+		dev_priv->num_fence_regs =
+				I915_READ(vgt_info_off(avail_rs.fence_num));
+
 	/* Initialize fence registers to zero */
 	INIT_LIST_HEAD(&dev_priv->mm.fence_list);
 	i915_gem_restore_fences(dev);
-- 
1.9.1

[PATCH 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

Framebuffer compression is disabled when driver detects it's
running in XenGT VM, because XenGT does not provide emulations
for FBC related operations, and we do not expose stolen memory
to the VM.

Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 927acea..bb4ad52 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1741,6 +1741,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	i915_check_vgpu(dev);
 
+	/* disable framebuffer compression in vgt */
+	if (intel_vgpu_active(dev))
+		i915.enable_fbc = 0;
+
 	intel_irq_init(dev);
 	intel_uncore_sanitize(dev);
 
-- 
1.9.1

[PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

Display switch logic is added to notify the vgt mediator that
current vgpu have a valid surface to show. It does so by writing
the display_ready field in PV INFO page, and then will be handled
in vgt mediator. This is useful to avoid trickiness when the VM's
framebuffer is being accessed in the middle of VM modesetting,
e.g. compositing the framebuffer in the host side

Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
 drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 3 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index bb4ad52..d9462e1 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	} else {
 		/* Start out suspended in ums mode. */
 		dev_priv->ums.mm_suspended = 1;
+		if (intel_vgpu_active(dev)) {
+			/*
+			 * Notify a valid surface after modesetting,
+			 * when running inside a VM.
+			 */
+			I915_WRITE(vgt_info_off(display_ready),
+				VGT_DRV_DISPLAY_READY);
+		}
 	}
 
 	i915_setup_sysfs(dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a70f12e..38d606c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6673,5 +6673,12 @@ enum punit_power_well {
 #define vgt_info_off(x) \
 	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
 
+/*
+ * The information set by the guest gfx driver, through the display_ready field
+ */
+enum vgt_display_status {
+	VGT_DRV_DISPLAY_NOT_READY = 0,
+	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
+};
 
 #endif /* _I915_REG_H_ */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b78f00a..3af9259 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
 			intel_modeset_check_state(set->crtc->dev);
 	}
 
+	if (intel_vgpu_active(dev)) {
+		/*
+		 * Notify a valid surface after modesetting,
+		 * when running inside a VM.
+		 */
+		struct drm_i915_private *dev_priv = dev->dev_private;
+
+		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
+	}
+
 	if (ret) {
 		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
 			      set->crtc->base.id, ret);
-- 
1.9.1

[PATCH 6/8] drm/i915: Disable power management for i915 driver in VM

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

In XenGT, GPU power management is controlled by host i915
driver, so there is no need to provide virtualized GPU PM
support. In the future it might be useful to gather VM
input for freq boost, but now let's disable it simply.

Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 675e8a2..1535fa3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5419,6 +5419,12 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 		mutex_unlock(&dev->struct_mutex);
 	} else if (INTEL_INFO(dev)->gen >= 6) {
 		/*
+		 * Powersaving is disabled when running inside a VM.
+		 */
+		if (intel_vgpu_active(dev))
+			return;
+
+		/*
 		 * PCU communication is slow and this doesn't need to be
 		 * done at any specific time, so do this out of our fast path
 		 * to make resume and init faster.
-- 
1.9.1

[PATCH 7/8] drm/i915: Create vgpu specific write MMIO to reduce traps

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

In the virtualized environment, forcewake operations are not
necessory for the driver, because mmio accesses will be trapped
and emulated by the host side, and real forcewake operations are
also done in the host. New mmio write handlers are added to directly
call the __raw_i915_write, therefore will reduce many traps and
increase the overall performance for drivers runing in the VM
with Intel GVT-g enhancement

Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
Signed-off-by: Kevin Tian <kevin.tian@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c     |  7 +++++--
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c | 24 ++++++++++++++++++++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index d9462e1..a3fe7c0 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1741,10 +1741,13 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 
 	i915_check_vgpu(dev);
 
-	/* disable framebuffer compression in vgt */
-	if (intel_vgpu_active(dev))
+	if (intel_vgpu_active(dev)) {
+		/* disable framebuffer compression in vgt */
 		i915.enable_fbc = 0;
 
+		intel_vgpu_reg_write_init(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 caae6ed..c891638 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2334,6 +2334,7 @@ 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);
+extern void intel_vgpu_reg_write_init(struct drm_device *dev);
 static inline bool intel_vgpu_active(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 918b761..87f2a69 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -722,6 +722,14 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
 	REG_WRITE_FOOTER; \
 }
 
+#define __vgpu_write(x) \
+static void \
+vgpu_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) { \
+	REG_WRITE_HEADER; \
+	__raw_i915_write##x(dev_priv, reg, val); \
+	REG_WRITE_FOOTER; \
+}
+
 static const u32 gen8_shadowed_regs[] = {
 	FORCEWAKE_MT,
 	GEN6_RPNSWREQ,
@@ -816,6 +824,10 @@ __gen4_write(8)
 __gen4_write(16)
 __gen4_write(32)
 __gen4_write(64)
+__vgpu_write(8)
+__vgpu_write(16)
+__vgpu_write(32)
+__vgpu_write(64)
 
 #undef __chv_write
 #undef __gen8_write
@@ -823,9 +835,21 @@ __gen4_write(64)
 #undef __gen6_write
 #undef __gen5_write
 #undef __gen4_write
+#undef __vgpu_write
 #undef REG_WRITE_FOOTER
 #undef REG_WRITE_HEADER
 
+void intel_vgpu_reg_write_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	dev_priv->uncore.funcs.mmio_writeb	= vgpu_write8;
+	dev_priv->uncore.funcs.mmio_writew	= vgpu_write16;
+	dev_priv->uncore.funcs.mmio_writel	= vgpu_write32;
+	dev_priv->uncore.funcs.mmio_writeq	= vgpu_write64;
+
+}
+
 void intel_uncore_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.9.1

[PATCH 8/8] drm/i915: Support alias ppgtt in VM if ppgtt is enabled

[Jike Song]

From: Yu Zhang <yu.c.zhang@intel.com>

The current XenGT only supports alias ppgtt. And the emulation
is done in XenGT host by first trapping PP_DIR_BASE mmio
accesses. Updating PP_DIR_BASE by using instructions such as
MI_LOAD_REGISTER_IMM are hard to emulate and are not supported
in current XenGT. Therefore this patch also added a new callback
routine - vgpu_mm_switch() to set the PP_DIR_BASE by mmio writes.

Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
Signed-off-by: Jike Song <jike.song@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c     |  5 +++++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 14 ++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index a3fe7c0..63dccd2 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1746,6 +1746,11 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		i915.enable_fbc = 0;
 
 		intel_vgpu_reg_write_init(dev);
+
+		if (USES_FULL_PPGTT(dev)) {
+			DRM_INFO("Only support alias ppgtt for now in VM.\n");
+			i915.enable_ppgtt = 1;
+		}
 	}
 
 	intel_irq_init(dev);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 368262d..e61d66f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -898,6 +898,17 @@ static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt,
 	return 0;
 }
 
+static int vgpu_mm_switch(struct i915_hw_ppgtt *ppgtt,
+			 struct intel_engine_cs *ring)
+{
+	struct drm_device *dev = ppgtt->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G);
+	I915_WRITE(RING_PP_DIR_BASE(ring), get_pd_offset(ppgtt));
+	return 0;
+}
+
 static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt,
 			  struct intel_engine_cs *ring)
 {
@@ -1224,6 +1235,9 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	} else
 		BUG();
 
+	if (intel_vgpu_active(dev))
+		ppgtt->switch_mm = vgpu_mm_switch;
+
 	ret = gen6_ppgtt_alloc(ppgtt);
 	if (ret)
 		return ret;
-- 
1.9.1

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Chris Wilson]

On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> > From: Chris Wilson
> > The implementation also looks backwards. To work correctly with the GTT
> > allocator, you need to preallocate the reserved space such that it can
> > only allocate from the allowed ranges. Similarly, it should evict any
> > conflicting nodes when deballooning.
> 
> Could you elaborate a bit for above suggestion?

My expectation was that the dev_priv->gtt.base.vm would contain exactly
two holes after setup (in the mappable and non-mappable range). To do
that you would explicitly reserve everything barred from this client 
using a set of drm_mm_reserve_node()
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Tian, Kevin]

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Daniel Vetter
> 
> > 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 vgt_info_off(x) \
> > +	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> 
> I think a new header file i915_vgt.h for these definitions would be good.
> i915_reg.h is giant already ...
> -Daniel
> 

sure we can follow this suggestion. Actually our original implementation has
the definition separately defined. Just because we saw current i915 has a
single register definition header, so thought we should adapt to that. :-)

Thanks
Kevin

Re: [PATCH 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Jike Song]

On 09/19/2014 04:07 PM, Chris Wilson wrote:
> On Sat, Sep 20, 2014 at 02:47:04AM +0800, Jike Song wrote:
>> From: Yu Zhang <yu.c.zhang@intel.com>
>>
>> Framebuffer compression is disabled when driver detects it's
>> running in XenGT VM, because XenGT does not provide emulations
>> for FBC related operations, and we do not expose stolen memory
>> to the VM.
>>
>> Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_dma.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index 927acea..bb4ad52 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -1741,6 +1741,10 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>
>>   	i915_check_vgpu(dev);
>>
>> +	/* disable framebuffer compression in vgt */
>> +	if (intel_vgpu_active(dev))
>> +		i915.enable_fbc = 0;
>
> This should be done inside intel_enable_fbc() so that the correct reason
> is given as to why it is disabled.

I'm sorry, but do you mean intel_update_fbc()?


> -Chris
>

--
Thanks,
Jike

Re: [PATCH 8/8] drm/i915: Support alias ppgtt in VM if ppgtt is enabled

[Jike Song]

On 09/19/2014 04:25 PM, Chris Wilson wrote:
>
> This should be moved to sanitize_enable_ppgtt(), probably by expanding
> HAS_PPGTT(), e.g.:
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 07dafa2c2d8c..b1fa13942d14 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2139,8 +2139,6 @@ struct drm_i915_cmd_table {
>
>   #define HAS_HW_CONTEXTS(dev)   (INTEL_INFO(dev)->gen >= 6)
>   #define HAS_LOGICAL_RING_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 8)
> -#define HAS_ALIASING_PPGTT(dev)        (INTEL_INFO(dev)->gen >= 6)
> -#define HAS_PPGTT(dev)         (INTEL_INFO(dev)->gen >= 7 && !IS_GEN8(dev))
>   #define USES_PPGTT(dev)                (i915.enable_ppgtt)
>   #define USES_FULL_PPGTT(dev)   (i915.enable_ppgtt == 2)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a234446a8678..3bea0bdfd276 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -35,13 +35,23 @@ static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
>
>   static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>   {
> -       if (enable_ppgtt == 0 || !HAS_ALIASING_PPGTT(dev))
> +       bool has_aliasing_ppgtt;
> +       bool has_full_ppgtt;
> +
> +       has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
> +       has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
> +       if (IS_GEN8(dev))
> +               has_full_ppgtt = false; /* XXX why? */
> +       if (intel_vgpu_active(dev))
> +               has_full_ppgtt = false; /* emulation is too hard */
> +
> +       if (enable_ppgtt == 0 || !has_aliasing_ppgtt)
>                  return 0;
>
>          if (enable_ppgtt == 1)
>                  return 1;
>
> -       if (enable_ppgtt == 2 && HAS_PPGTT(dev))
> +       if (enable_ppgtt == 2 && has_full_ppgtt)
>                  return 2;
>
>   #ifdef CONFIG_INTEL_IOMMU
> @@ -59,7 +69,7 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>                  return 0;
>          }
>
> -       return HAS_PPGTT(dev) ? 2 : HAS_ALIASING_PPGTT(dev) ? 1 : 0;
> +       return has_full_ppgtt ? 2 : has_aliasing_ppgtt ? 1 : 0;
>   }

Thanks for the demo. Currently sanitize_enable_ppgtt() is called by i915_gem_gtt_init(), before
intel_check_vgpu().

I'm trying to detect VGPU as early as possible, maybe between intel_detect_pch() and intel_uncore_init(),
using bare readq/readw instead of I915_READxx.


>
> -Chris
>
--
Thanks,
Jike

Re: [PATCH 8/8] drm/i915: Support alias ppgtt in VM if ppgtt is enabled

[Chris Wilson]

On Mon, Sep 22, 2014 at 07:17:41PM +0800, Jike Song wrote:
> Thanks for the demo. Currently sanitize_enable_ppgtt() is called by i915_gem_gtt_init(), before
> intel_check_vgpu().
> 
> I'm trying to detect VGPU as early as possible, maybe between intel_detect_pch() and intel_uncore_init(),
> using bare readq/readw instead of I915_READxx.

I am thinking that the right level for check_vgpu is uncore_init. Using
raw mmio is acceptable at that point, and once you know you have a vgpu
you can forgo all the mmio emulation.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/8] drm/i915: Disable framebuffer compression for i915 driver in VM

[Chris Wilson]

On Mon, Sep 22, 2014 at 03:10:05PM +0800, Jike Song wrote:
> On 09/19/2014 04:07 PM, Chris Wilson wrote:
> >This should be done inside intel_enable_fbc() so that the correct reason
> >is given as to why it is disabled.
> 
> I'm sorry, but do you mean intel_update_fbc()?

Yes. But don't let me stop you trying to bring sanity to the FBC code
either ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Daniel Vetter]

On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
> On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> > > From: Chris Wilson
> > > The implementation also looks backwards. To work correctly with the GTT
> > > allocator, you need to preallocate the reserved space such that it can
> > > only allocate from the allowed ranges. Similarly, it should evict any
> > > conflicting nodes when deballooning.
> > 
> > Could you elaborate a bit for above suggestion?
> 
> My expectation was that the dev_priv->gtt.base.vm would contain exactly
> two holes after setup (in the mappable and non-mappable range). To do
> that you would explicitly reserve everything barred from this client 
> using a set of drm_mm_reserve_node()

Essentially a reserve_node implements what you open-code with
insert_node_range right now.

One issue aside with both this and with the PDE reservations for gen7 is
that there are now other thins in the ggtt drm_mm allocator than just gem
objects. Which means our debugfs files are now less useful.

It might be useful to augment that dumper with one that dumps everything.
We could add a few bits of driver-private tags in drm_mm_node (there's
space) to figure out what kind of object it is. Would be a great follow-up
task.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Chris Wilson]

On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
> On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
> > On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> > > > From: Chris Wilson
> > > > The implementation also looks backwards. To work correctly with the GTT
> > > > allocator, you need to preallocate the reserved space such that it can
> > > > only allocate from the allowed ranges. Similarly, it should evict any
> > > > conflicting nodes when deballooning.
> > > 
> > > Could you elaborate a bit for above suggestion?
> > 
> > My expectation was that the dev_priv->gtt.base.vm would contain exactly
> > two holes after setup (in the mappable and non-mappable range). To do
> > that you would explicitly reserve everything barred from this client 
> > using a set of drm_mm_reserve_node()
> 
> Essentially a reserve_node implements what you open-code with
> insert_node_range right now.

Heh, there is a big difference. One inserts exactly where you ask and
fails if it conflicts, the other inserts where it feels like within that
range.
 
> One issue aside with both this and with the PDE reservations for gen7 is
> that there are now other thins in the ggtt drm_mm allocator than just gem
> objects. Which means our debugfs files are now less useful.
> 
> It might be useful to augment that dumper with one that dumps everything.
> We could add a few bits of driver-private tags in drm_mm_node (there's
> space) to figure out what kind of object it is. Would be a great follow-up
> task.

I think moving the other way and making them all objects so that we can
tie them into evection and the shrinker, use more interesting allocation
strategies, improve integration with debugging etc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Daniel Vetter]

On Tue, Sep 23, 2014 at 10:19:02AM +0100, Chris Wilson wrote:
> On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
> > On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
> > > On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> > > > > From: Chris Wilson
> > > > > The implementation also looks backwards. To work correctly with the GTT
> > > > > allocator, you need to preallocate the reserved space such that it can
> > > > > only allocate from the allowed ranges. Similarly, it should evict any
> > > > > conflicting nodes when deballooning.
> > > > 
> > > > Could you elaborate a bit for above suggestion?
> > > 
> > > My expectation was that the dev_priv->gtt.base.vm would contain exactly
> > > two holes after setup (in the mappable and non-mappable range). To do
> > > that you would explicitly reserve everything barred from this client 
> > > using a set of drm_mm_reserve_node()
> > 
> > Essentially a reserve_node implements what you open-code with
> > insert_node_range right now.
> 
> Heh, there is a big difference. One inserts exactly where you ask and
> fails if it conflicts, the other inserts where it feels like within that
> range.

Well if the the requested size matches the range exactly then it will be
the same. Which iirc is what's going on here I think.

> > One issue aside with both this and with the PDE reservations for gen7 is
> > that there are now other thins in the ggtt drm_mm allocator than just gem
> > objects. Which means our debugfs files are now less useful.
> > 
> > It might be useful to augment that dumper with one that dumps everything.
> > We could add a few bits of driver-private tags in drm_mm_node (there's
> > space) to figure out what kind of object it is. Would be a great follow-up
> > task.
> 
> I think moving the other way and making them all objects so that we can
> tie them into evection and the shrinker, use more interesting allocation
> strategies, improve integration with debugging etc.

Hm, not sure yet since it will be a lot of work at least. But I guess we
could untangle the meaning of obj->pin a bit and add an unbind vfunc which
adds some magic. But there's a lot of stuff attached to a gem bo that just
doesn't make a lot of sense really, so maybe a better option would be to
subclass a struct i915_ggtt_vma with special magic. Dunno really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Zhang, Yu]

Hi Daniel & Chris,

   Thank you very much for your comments, And sorry for my late reply.:) 
I was focusing on other tasks previously.
   See my questions below:

On 9/23/2014 7:25 PM, Daniel Vetter wrote:
> On Tue, Sep 23, 2014 at 10:19:02AM +0100, Chris Wilson wrote:
>> On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
>>> On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
>>>> On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
>>>>>> From: Chris Wilson
>>>>>> The implementation also looks backwards. To work correctly with the GTT
>>>>>> allocator, you need to preallocate the reserved space such that it can
>>>>>> only allocate from the allowed ranges. Similarly, it should evict any
>>>>>> conflicting nodes when deballooning.
>>>>>
>>>>> Could you elaborate a bit for above suggestion?
>>>>
>>>> My expectation was that the dev_priv->gtt.base.vm would contain exactly
>>>> two holes after setup (in the mappable and non-mappable range). To do
>>>> that you would explicitly reserve everything barred from this client
>>>> using a set of drm_mm_reserve_node()
>>>
>>> Essentially a reserve_node implements what you open-code with
>>> insert_node_range right now.
>>
>> Heh, there is a big difference. One inserts exactly where you ask and
>> fails if it conflicts, the other inserts where it feels like within that
>> range.

Do you mean drm_mm_search_free_in_range_generic() may not get reserve 
the exact range we are expecting to? Is this why you'd prefer the 
drm_mm_reserve_node()?

Besides, the ggtt_vm->mm is just initialized right before the ballooning 
code in routine i915_gem_setup_global_gtt(), so is there any chance the 
range to be partitioned out is already reserved by someone else?

>
> Well if the the requested size matches the range exactly then it will be
> the same. Which iirc is what's going on here I think.
>
>>> One issue aside with both this and with the PDE reservations for gen7 is
>>> that there are now other thins in the ggtt drm_mm allocator than just gem
>>> objects. Which means our debugfs files are now less useful.
>>>
>>> It might be useful to augment that dumper with one that dumps everything.
>>> We could add a few bits of driver-private tags in drm_mm_node (there's
>>> space) to figure out what kind of object it is. Would be a great follow-up
>>> task.
>>
>> I think moving the other way and making them all objects so that we can
>> tie them into evection and the shrinker, use more interesting allocation
>> strategies, improve integration with debugging etc.
>
> Hm, not sure yet since it will be a lot of work at least. But I guess we
> could untangle the meaning of obj->pin a bit and add an unbind vfunc which
> adds some magic. But there's a lot of stuff attached to a gem bo that just
> doesn't make a lot of sense really, so maybe a better option would be to
> subclass a struct i915_ggtt_vma with special magic. Dunno really.

Sorry, not sure what these comments are about. :) I'll need time to read 
the code. Could you please elaborate a bit? Thanks!


P.S. about the guard page: for now, the current logic reserves a guard 
page between different guests and at the very last entry of the whole 
physical GTT. the previous comments says: "The CS prefetcher happens 
everywhere and so can read from the end of one range into the beginning 
of another clients". So I guess the guard page in current patch is 
necessary, right?

> -Daniel
>

Thanks
Yu

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Chris Wilson]

On Wed, Sep 24, 2014 at 08:35:50PM +0800, Zhang, Yu wrote:
> Hi Daniel & Chris,
> 
>   Thank you very much for your comments, And sorry for my late
> reply.:) I was focusing on other tasks previously.
>   See my questions below:
> 
> On 9/23/2014 7:25 PM, Daniel Vetter wrote:
> >On Tue, Sep 23, 2014 at 10:19:02AM +0100, Chris Wilson wrote:
> >>On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
> >>>On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
> >>>>On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> >>>>>>From: Chris Wilson
> >>>>>>The implementation also looks backwards. To work correctly with the GTT
> >>>>>>allocator, you need to preallocate the reserved space such that it can
> >>>>>>only allocate from the allowed ranges. Similarly, it should evict any
> >>>>>>conflicting nodes when deballooning.
> >>>>>
> >>>>>Could you elaborate a bit for above suggestion?
> >>>>
> >>>>My expectation was that the dev_priv->gtt.base.vm would contain exactly
> >>>>two holes after setup (in the mappable and non-mappable range). To do
> >>>>that you would explicitly reserve everything barred from this client
> >>>>using a set of drm_mm_reserve_node()
> >>>
> >>>Essentially a reserve_node implements what you open-code with
> >>>insert_node_range right now.
> >>
> >>Heh, there is a big difference. One inserts exactly where you ask and
> >>fails if it conflicts, the other inserts where it feels like within that
> >>range.
> 
> Do you mean drm_mm_search_free_in_range_generic() may not get
> reserve the exact range we are expecting to? Is this why you'd
> prefer the drm_mm_reserve_node()?
> 
> Besides, the ggtt_vm->mm is just initialized right before the
> ballooning code in routine i915_gem_setup_global_gtt(), so is there
> any chance the range to be partitioned out is already reserved by
> someone else?

No. It is just that drm_mm_reserve_node() was added to have the precise
semantics expected here with the strict checks. And should work better
with dynamic ballooning...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Daniel Vetter]

On Wed, Sep 24, 2014 at 08:35:50PM +0800, Zhang, Yu wrote:
> Hi Daniel & Chris,
> 
>   Thank you very much for your comments, And sorry for my late reply.:) I
> was focusing on other tasks previously.
>   See my questions below:
> 
> On 9/23/2014 7:25 PM, Daniel Vetter wrote:
> >On Tue, Sep 23, 2014 at 10:19:02AM +0100, Chris Wilson wrote:
> >>On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
> >>>On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
> >>>>On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
> >>>>>>From: Chris Wilson
> >>>>>>The implementation also looks backwards. To work correctly with the GTT
> >>>>>>allocator, you need to preallocate the reserved space such that it can
> >>>>>>only allocate from the allowed ranges. Similarly, it should evict any
> >>>>>>conflicting nodes when deballooning.
> >>>>>
> >>>>>Could you elaborate a bit for above suggestion?
> >>>>
> >>>>My expectation was that the dev_priv->gtt.base.vm would contain exactly
> >>>>two holes after setup (in the mappable and non-mappable range). To do
> >>>>that you would explicitly reserve everything barred from this client
> >>>>using a set of drm_mm_reserve_node()
> >>>
> >>>Essentially a reserve_node implements what you open-code with
> >>>insert_node_range right now.
> >>
> >>Heh, there is a big difference. One inserts exactly where you ask and
> >>fails if it conflicts, the other inserts where it feels like within that
> >>range.
> 
> Do you mean drm_mm_search_free_in_range_generic() may not get reserve the
> exact range we are expecting to? Is this why you'd prefer the
> drm_mm_reserve_node()?
> 
> Besides, the ggtt_vm->mm is just initialized right before the ballooning
> code in routine i915_gem_setup_global_gtt(), so is there any chance the
> range to be partitioned out is already reserved by someone else?

Not really, but simply using drm_mm_reserve_node makes the intent much
clearer. Also if we go around to runtime ballooning failing hard is
probably what we want anyway.

> >Well if the the requested size matches the range exactly then it will be
> >the same. Which iirc is what's going on here I think.
> >
> >>>One issue aside with both this and with the PDE reservations for gen7 is
> >>>that there are now other thins in the ggtt drm_mm allocator than just gem
> >>>objects. Which means our debugfs files are now less useful.
> >>>
> >>>It might be useful to augment that dumper with one that dumps everything.
> >>>We could add a few bits of driver-private tags in drm_mm_node (there's
> >>>space) to figure out what kind of object it is. Would be a great follow-up
> >>>task.
> >>
> >>I think moving the other way and making them all objects so that we can
> >>tie them into evection and the shrinker, use more interesting allocation
> >>strategies, improve integration with debugging etc.
> >
> >Hm, not sure yet since it will be a lot of work at least. But I guess we
> >could untangle the meaning of obj->pin a bit and add an unbind vfunc which
> >adds some magic. But there's a lot of stuff attached to a gem bo that just
> >doesn't make a lot of sense really, so maybe a better option would be to
> >subclass a struct i915_ggtt_vma with special magic. Dunno really.
> 
> Sorry, not sure what these comments are about. :) I'll need time to read the
> code. Could you please elaborate a bit? Thanks!

We're discussing different options to unify the objects allocated from the
ggtt drm_mm. Not terribly relevant for your work directly.

> P.S. about the guard page: for now, the current logic reserves a guard page
> between different guests and at the very last entry of the whole physical
> GTT. the previous comments says: "The CS prefetcher happens everywhere and
> so can read from the end of one range into the beginning of another
> clients". So I guess the guard page in current patch is necessary, right?

tbh I'm not really sure whether we even need the guard page at the very
end of the gtt any more on recent platforms. And we definitely don't need
any guard page between different allocations on hsw, so I think we can
just remove that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Zhang, Yu]

Hi Chris & Daniel,

   Thanks for your comments. Following are my understandings about the 
changes needed for this patch:

1> We do not need the guard page anymore between different VMs. For the 
very last physical GTT entry, let's keep it pointing to a guard page.

2> To reserve the GMs in our balloon code, drm_mm_reserve_node() is 
preferred than the current drm_mm_insert_node_in_range_generic().

   Am I correct? Please correct me if anything wrong. Thanks! :)

Yu


On 9/24/2014 9:21 PM, Chris Wilson wrote:
> On Wed, Sep 24, 2014 at 08:35:50PM +0800, Zhang, Yu wrote:
>> Hi Daniel & Chris,
>>
>>    Thank you very much for your comments, And sorry for my late
>> reply.:) I was focusing on other tasks previously.
>>    See my questions below:
>>
>> On 9/23/2014 7:25 PM, Daniel Vetter wrote:
>>> On Tue, Sep 23, 2014 at 10:19:02AM +0100, Chris Wilson wrote:
>>>> On Tue, Sep 23, 2014 at 10:26:26AM +0200, Daniel Vetter wrote:
>>>>> On Fri, Sep 19, 2014 at 09:00:00PM +0100, Chris Wilson wrote:
>>>>>> On Fri, Sep 19, 2014 at 06:21:46PM +0000, Tian, Kevin wrote:
>>>>>>>> From: Chris Wilson
>>>>>>>> The implementation also looks backwards. To work correctly with the GTT
>>>>>>>> allocator, you need to preallocate the reserved space such that it can
>>>>>>>> only allocate from the allowed ranges. Similarly, it should evict any
>>>>>>>> conflicting nodes when deballooning.
>>>>>>>
>>>>>>> Could you elaborate a bit for above suggestion?
>>>>>>
>>>>>> My expectation was that the dev_priv->gtt.base.vm would contain exactly
>>>>>> two holes after setup (in the mappable and non-mappable range). To do
>>>>>> that you would explicitly reserve everything barred from this client
>>>>>> using a set of drm_mm_reserve_node()
>>>>>
>>>>> Essentially a reserve_node implements what you open-code with
>>>>> insert_node_range right now.
>>>>
>>>> Heh, there is a big difference. One inserts exactly where you ask and
>>>> fails if it conflicts, the other inserts where it feels like within that
>>>> range.
>>
>> Do you mean drm_mm_search_free_in_range_generic() may not get
>> reserve the exact range we are expecting to? Is this why you'd
>> prefer the drm_mm_reserve_node()?
>>
>> Besides, the ggtt_vm->mm is just initialized right before the
>> ballooning code in routine i915_gem_setup_global_gtt(), so is there
>> any chance the range to be partitioned out is already reserved by
>> someone else?
>
> No. It is just that drm_mm_reserve_node() was added to have the precise
> semantics expected here with the strict checks. And should work better
> with dynamic ballooning...
> -Chris
>

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Yu, Zhang]

On 9/26/2014 4:48 PM, Chris Wilson wrote:
> On Fri, Sep 26, 2014 at 04:26:20PM +0800, Zhang, Yu wrote:
>> Hi Chris & Daniel,
>>
>>    Thanks for your comments. Following are my understandings about
>> the changes needed for this patch:
>>
>> 1> We do not need the guard page anymore between different VMs. For
>> the very last physical GTT entry, let's keep it pointing to a guard
>> page.
>>
>> 2> To reserve the GMs in our balloon code, drm_mm_reserve_node() is
>> preferred than the current drm_mm_insert_node_in_range_generic().
>>
>>    Am I correct? Please correct me if anything wrong. Thanks! :)
>
> Agreed. If you care to respin the patch, I'll see if anything else
> alarms me ;-)

Got it. Thank you, Chris.
> -Chris
>

Re: [PATCH 2/8] drm/i915: Adds graphic address space ballooning logic

[Chris Wilson]

On Fri, Sep 26, 2014 at 04:26:20PM +0800, Zhang, Yu wrote:
> Hi Chris & Daniel,
> 
>   Thanks for your comments. Following are my understandings about
> the changes needed for this patch:
> 
> 1> We do not need the guard page anymore between different VMs. For
> the very last physical GTT entry, let's keep it pointing to a guard
> page.
> 
> 2> To reserve the GMs in our balloon code, drm_mm_reserve_node() is
> preferred than the current drm_mm_insert_node_in_range_generic().
> 
>   Am I correct? Please correct me if anything wrong. Thanks! :)

Agreed. If you care to respin the patch, I'll see if anything else
alarms me ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Zhiyuan Lv]

Hi Daniel,

On Fri, Sep 19, 2014 at 06:09:37PM +0200, Daniel Vetter wrote:
> On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> > From: Yu Zhang <yu.c.zhang@intel.com>
> > 
> > Display switch logic is added to notify the vgt mediator that
> > current vgpu have a valid surface to show. It does so by writing
> > the display_ready field in PV INFO page, and then will be handled
> > in vgt mediator. This is useful to avoid trickiness when the VM's
> > framebuffer is being accessed in the middle of VM modesetting,
> > e.g. compositing the framebuffer in the host side
> > 
> > Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> > Signed-off-by: Jike Song <jike.song@intel.com>
> > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
> >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
> >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
> >  3 files changed, 25 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index bb4ad52..d9462e1 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	} else {
> >  		/* Start out suspended in ums mode. */
> >  		dev_priv->ums.mm_suspended = 1;
> > +		if (intel_vgpu_active(dev)) {
> > +			/*
> > +			 * Notify a valid surface after modesetting,
> > +			 * when running inside a VM.
> > +			 */
> > +			I915_WRITE(vgt_info_off(display_ready),
> > +				VGT_DRV_DISPLAY_READY);
> > +		}
> >  	}
> >  
> >  	i915_setup_sysfs(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index a70f12e..38d606c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6673,5 +6673,12 @@ enum punit_power_well {
> >  #define vgt_info_off(x) \
> >  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> >  
> > +/*
> > + * The information set by the guest gfx driver, through the display_ready field
> > + */
> > +enum vgt_display_status {
> > +	VGT_DRV_DISPLAY_NOT_READY = 0,
> > +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> > +};
> >  
> >  #endif /* _I915_REG_H_ */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b78f00a..3af9259 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  			intel_modeset_check_state(set->crtc->dev);
> >  	}
> >  
> > +	if (intel_vgpu_active(dev)) {
> > +		/*
> > +		 * Notify a valid surface after modesetting,
> > +		 * when running inside a VM.
> > +		 */
> > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> > +	}
> 
> Since very shortly we now have the frontbuffer tracking support code.
> Should we move it there? See intel_frontbuffer_flush&flip.

Thank you very much for the comments and sorry for the delayed reply!

For virtual machines we only need such notification once for guest system
booting, would that be too heavy to add code into the flush&flip function,
consider that they are to be called many times?

Going forward, we want to use the same "display_ready" for i915 running in host
machines so that we can capture display status changes. Would that be good to
keep the code in "intel_crtc_set_config"? Appreciate your inputs. Thanks!

Regards,
-Zhiyuan

> -Daniel
> 
> > +
> >  	if (ret) {
> >  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> >  			      set->crtc->base.id, ret);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jike Song]

On 09/19/2014 03:25 PM, Chris Wilson wrote:
> 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; }
>
> 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;
>

Hi Chris, sorry that I didn't understand you correctly. After discussion
with Yu today, I realized that unfortunately, the vgt_if can't be
dereferenced directly.

There are several reasons:

	- dereferencing a MMIO address will be complained by sparse(1)

	- for Guest VM, such accesses will be trapped by hypervisor, and
	  hence emulated correctly; However this doesn't work for Host(e.g.
	  Domain 0 of Xen, the Linux host KVM resides in). For host, we used
	  a proactive mechanism to redirect i915 MMIO accesses to vgt,
	  the GPU device-model, for the sake of central management & sharing
	  among VMs, including host.

Given that, though technically your code works for Guest, but after the
integration of host support of iGVT, we still need to use I915_READ/I915_WRITE
then. The host patches is soon to posted for your review :)

I should have realized that earlier, sorry!


> -Chris
>

--
Thanks,
Jike

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

[Yu, Zhang]

On 9/29/2014 7:44 PM, Jike Song wrote:
> On 09/19/2014 03:25 PM, Chris Wilson wrote:
>> 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; }
>>
>> 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;
>>
>
> Hi Chris, sorry that I didn't understand you correctly. After discussion
> with Yu today, I realized that unfortunately, the vgt_if can't be
> dereferenced directly.
>
> There are several reasons:
>
>      - dereferencing a MMIO address will be complained by sparse(1)
>
>      - for Guest VM, such accesses will be trapped by hypervisor, and
>        hence emulated correctly; However this doesn't work for Host(e.g.
>        Domain 0 of Xen, the Linux host KVM resides in). For host, we used
>        a proactive mechanism to redirect i915 MMIO accesses to vgt,
>        the GPU device-model, for the sake of central management & sharing
>        among VMs, including host.
>
> Given that, though technically your code works for Guest, but after the
> integration of host support of iGVT, we still need to use
> I915_READ/I915_WRITE
> then. The host patches is soon to posted for your review :)
>
> I should have realized that earlier, sorry!
>
Hi Chris,

   Sorry, I also should have noticed this issue earlier.
   To my understanding, the reason you proposed to use the "struct 
vgt_if *if" in struct vgpu, to replace the previous vgpu_active, is to 
simplify the mmio accesses in our patches. This suggestion works fine 
from the guest & native point of view. However, just like Jike's mail 
said, this change may not work for the host side, which also need to 
visit the PVINFO page from time to time. So, could we still keep the 
vgpu_active flag when detecting virtual gpu, and read the mmio registers 
in PVINFO structure by I915_READ?

Thanks
Yu
>
>> -Chris
>>
>
> --
> Thanks,
> Jike
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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

[Chris Wilson]

On Mon, Sep 29, 2014 at 07:44:56PM +0800, Jike Song wrote:
> On 09/19/2014 03:25 PM, Chris Wilson wrote:
> >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; }
> >
> >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;
> >
> 
> Hi Chris, sorry that I didn't understand you correctly. After discussion
> with Yu today, I realized that unfortunately, the vgt_if can't be
> dereferenced directly.
> 
> There are several reasons:
> 
> 	- dereferencing a MMIO address will be complained by sparse(1)
> 
> 	- for Guest VM, such accesses will be trapped by hypervisor, and
> 	  hence emulated correctly; However this doesn't work for Host(e.g.
> 	  Domain 0 of Xen, the Linux host KVM resides in). For host, we used
> 	  a proactive mechanism to redirect i915 MMIO accesses to vgt,
> 	  the GPU device-model, for the sake of central management & sharing
> 	  among VMs, including host.

You only need to be careful during vgpu detection. After that you know
everything is safe. If you do the detection during intel_uncore_init(),
or similar, you can use raw mmio access and explict sparse annotations
to do the right thing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Daniel Vetter]

On Mon, Sep 29, 2014 at 02:31:17PM +0800, Zhiyuan Lv wrote:
> Hi Daniel,
> 
> On Fri, Sep 19, 2014 at 06:09:37PM +0200, Daniel Vetter wrote:
> > On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> > > From: Yu Zhang <yu.c.zhang@intel.com>
> > > 
> > > Display switch logic is added to notify the vgt mediator that
> > > current vgpu have a valid surface to show. It does so by writing
> > > the display_ready field in PV INFO page, and then will be handled
> > > in vgt mediator. This is useful to avoid trickiness when the VM's
> > > framebuffer is being accessed in the middle of VM modesetting,
> > > e.g. compositing the framebuffer in the host side
> > > 
> > > Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> > > Signed-off-by: Jike Song <jike.song@intel.com>
> > > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
> > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
> > >  3 files changed, 25 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index bb4ad52..d9462e1 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > >  	} else {
> > >  		/* Start out suspended in ums mode. */
> > >  		dev_priv->ums.mm_suspended = 1;
> > > +		if (intel_vgpu_active(dev)) {
> > > +			/*
> > > +			 * Notify a valid surface after modesetting,
> > > +			 * when running inside a VM.
> > > +			 */
> > > +			I915_WRITE(vgt_info_off(display_ready),
> > > +				VGT_DRV_DISPLAY_READY);
> > > +		}
> > >  	}
> > >  
> > >  	i915_setup_sysfs(dev);
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index a70f12e..38d606c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6673,5 +6673,12 @@ enum punit_power_well {
> > >  #define vgt_info_off(x) \
> > >  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> > >  
> > > +/*
> > > + * The information set by the guest gfx driver, through the display_ready field
> > > + */
> > > +enum vgt_display_status {
> > > +	VGT_DRV_DISPLAY_NOT_READY = 0,
> > > +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> > > +};
> > >  
> > >  #endif /* _I915_REG_H_ */
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index b78f00a..3af9259 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> > >  			intel_modeset_check_state(set->crtc->dev);
> > >  	}
> > >  
> > > +	if (intel_vgpu_active(dev)) {
> > > +		/*
> > > +		 * Notify a valid surface after modesetting,
> > > +		 * when running inside a VM.
> > > +		 */
> > > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> > > +	}
> > 
> > Since very shortly we now have the frontbuffer tracking support code.
> > Should we move it there? See intel_frontbuffer_flush&flip.
> 
> Thank you very much for the comments and sorry for the delayed reply!
> 
> For virtual machines we only need such notification once for guest system
> booting, would that be too heavy to add code into the flush&flip function,
> consider that they are to be called many times?
> 
> Going forward, we want to use the same "display_ready" for i915 running in host
> machines so that we can capture display status changes. Would that be good to
> keep the code in "intel_crtc_set_config"? Appreciate your inputs. Thanks!

I guess the question is what exactly you want to signal to the hyporvisor
with this. I guess I need to dig a bit into the sourcecode for the
hyperviros part, and you need to make a really clear specification of when
guests should call this and what the hyporvisor must to in reaction of
this.

I don't think we'll have any issues with a bit of overhead in the
frontbuffer flip/flush functions, they're not called too often. Aside:
There's no nice kerneldoc for this stuff:

http://people.freedesktop.org/~danvet/drm/drmI915.html#idp54709056

Cheers, Daniel

> 
> Regards,
> -Zhiyuan
> 
> > -Daniel
> > 
> > > +
> > >  	if (ret) {
> > >  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> > >  			      set->crtc->base.id, ret);
> > > -- 
> > > 1.9.1
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Jike Song]

On 09/29/2014 08:16 PM, Chris Wilson wrote:
> On Mon, Sep 29, 2014 at 07:44:56PM +0800, Jike Song wrote:
>> On 09/19/2014 03:25 PM, Chris Wilson wrote:
>>> 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; }
>>>
>>> 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;
>>>
>>
>> Hi Chris, sorry that I didn't understand you correctly. After discussion
>> with Yu today, I realized that unfortunately, the vgt_if can't be
>> dereferenced directly.
>>
>> There are several reasons:
>>
>> 	- dereferencing a MMIO address will be complained by sparse(1)
>>
>> 	- for Guest VM, such accesses will be trapped by hypervisor, and
>> 	  hence emulated correctly; However this doesn't work for Host(e.g.
>> 	  Domain 0 of Xen, the Linux host KVM resides in). For host, we used
>> 	  a proactive mechanism to redirect i915 MMIO accesses to vgt,
>> 	  the GPU device-model, for the sake of central management & sharing
>> 	  among VMs, including host.
>
> You only need to be careful during vgpu detection. After that you know
> everything is safe. If you do the detection during intel_uncore_init(),
> or similar, you can use raw mmio access and explict sparse annotations
> to do the right thing.
> -Chris

Hi Chris,

Thanks for your suggestion, however, it addressed issue #1 but not issue #2.
I'd like to give a detailed explanation :)

For Guest i915 driver, when accessing a MMIO reg, it goes:

	whatever I915_READ/readl or direct dereferencing(addr)
		addr is translated to gpa(guest physical address) by guest paging structure
			hypervisor trapped the gpa access and forward it to vgt
				vgt_emulate_read32(vgt instance of guest, gpa)


The real problem is, when running as the host gpu driver, i915 MMIO/GTT accesses are:

	1) every difficult to trap, say the domain 0 of Xen hypervisor; or
	2) impossible to trap, say KVM(In KVM, the host i915 is running in the very same
	   privilege level and root mode as KVM hypervisor.)

So, for Host i915 driver, when accessing a MMIO reg, it goes:

	I915_READ(addr)
		gen6_read32(addr)
			offset = addr - dev_priv->regs
			vgt_mmio_readl(the vgt instance of host, offset)
				.. some processing ..
				if (passed-throuth)
					readl(offset + dev_priv->regs)
				else
					pa = BAR0 + offset
					vgt_emulate_read32(vgt instance of host, pa)


The vgt_if corresponds to the PVINFO page, not passed-through(actually it doesn't physically exist),
should fall into the "else" above.

Given that ... Yes we can dereference pointers for guest here, but consider that we'll
deal with host in the future ...


--
Thanks,
Jike

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Zhiyuan Lv]

Hi Daniel,

On Mon, Sep 29, 2014 at 02:30:09PM +0200, Daniel Vetter wrote:
> On Mon, Sep 29, 2014 at 02:31:17PM +0800, Zhiyuan Lv wrote:
> > Hi Daniel,
> > 
> > On Fri, Sep 19, 2014 at 06:09:37PM +0200, Daniel Vetter wrote:
> > > On Sat, Sep 20, 2014 at 02:47:05AM +0800, Jike Song wrote:
> > > > From: Yu Zhang <yu.c.zhang@intel.com>
> > > > 
> > > > Display switch logic is added to notify the vgt mediator that
> > > > current vgpu have a valid surface to show. It does so by writing
> > > > the display_ready field in PV INFO page, and then will be handled
> > > > in vgt mediator. This is useful to avoid trickiness when the VM's
> > > > framebuffer is being accessed in the middle of VM modesetting,
> > > > e.g. compositing the framebuffer in the host side
> > > > 
> > > > Signed-off-by: Yu Zhang <yu.c.zhang@intel.com>
> > > > Signed-off-by: Jike Song <jike.song@intel.com>
> > > > Signed-off-by: Zhiyuan Lv <zhiyuan.lv@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_dma.c      |  8 ++++++++
> > > >  drivers/gpu/drm/i915/i915_reg.h      |  7 +++++++
> > > >  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
> > > >  3 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > > index bb4ad52..d9462e1 100644
> > > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > > @@ -1790,6 +1790,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> > > >  	} else {
> > > >  		/* Start out suspended in ums mode. */
> > > >  		dev_priv->ums.mm_suspended = 1;
> > > > +		if (intel_vgpu_active(dev)) {
> > > > +			/*
> > > > +			 * Notify a valid surface after modesetting,
> > > > +			 * when running inside a VM.
> > > > +			 */
> > > > +			I915_WRITE(vgt_info_off(display_ready),
> > > > +				VGT_DRV_DISPLAY_READY);
> > > > +		}
> > > >  	}
> > > >  
> > > >  	i915_setup_sysfs(dev);
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index a70f12e..38d606c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -6673,5 +6673,12 @@ enum punit_power_well {
> > > >  #define vgt_info_off(x) \
> > > >  	(VGT_PVINFO_PAGE + (long)&((struct vgt_if *) NULL)->x)
> > > >  
> > > > +/*
> > > > + * The information set by the guest gfx driver, through the display_ready field
> > > > + */
> > > > +enum vgt_display_status {
> > > > +	VGT_DRV_DISPLAY_NOT_READY = 0,
> > > > +	VGT_DRV_DISPLAY_READY,	/* ready for display switch */
> > > > +};
> > > >  
> > > >  #endif /* _I915_REG_H_ */
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index b78f00a..3af9259 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -11642,6 +11642,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> > > >  			intel_modeset_check_state(set->crtc->dev);
> > > >  	}
> > > >  
> > > > +	if (intel_vgpu_active(dev)) {
> > > > +		/*
> > > > +		 * Notify a valid surface after modesetting,
> > > > +		 * when running inside a VM.
> > > > +		 */
> > > > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +		I915_WRITE(vgt_info_off(display_ready), VGT_DRV_DISPLAY_READY);
> > > > +	}
> > > 
> > > Since very shortly we now have the frontbuffer tracking support code.
> > > Should we move it there? See intel_frontbuffer_flush&flip.
> > 
> > Thank you very much for the comments and sorry for the delayed reply!
> > 
> > For virtual machines we only need such notification once for guest system
> > booting, would that be too heavy to add code into the flush&flip function,
> > consider that they are to be called many times?
> > 
> > Going forward, we want to use the same "display_ready" for i915 running in host
> > machines so that we can capture display status changes. Would that be good to
> > keep the code in "intel_crtc_set_config"? Appreciate your inputs. Thanks!
> 
> I guess the question is what exactly you want to signal to the hyporvisor
> with this. I guess I need to dig a bit into the sourcecode for the
> hyperviros part, and you need to make a really clear specification of when
> guests should call this and what the hyporvisor must to in reaction of
> this.

In this patchset for i915 driver running inside a virtual machine, the code
here is to signal vgt that guest framebuffer address in plane surface register
is ready to be written into physical registers. vgt's reaction is to make
policy change. After that point, VM's write to surface MMIO will go into
physical register directly. Here we do not need notifications for every guest
display surface MMIO write, but just the time we can do the policy change.

The usage of this is for guest system booting. At the very beginning, we block
the guest surface MMIO writes so that the monitor still shows up host
contents, then we rely on this notification to show up guest VM's screen.

We have another change for i915 driver to notify vgt the modesetting. But that
is for i915 running as a host driver. Sorry that it may be confusing to mix
two things together. This can be discussed later.

> 
> I don't think we'll have any issues with a bit of overhead in the
> frontbuffer flip/flush functions, they're not called too often. Aside:
> There's no nice kerneldoc for this stuff:
> 
> http://people.freedesktop.org/~danvet/drm/drmI915.html#idp54709056
>

Thanks for the info! The doc is helpful for me to understand the new feature.
Since intel_frontbuffer_flush() will be called every time there is page flip
request, that may not be what we want: just one time notification to do the
policy change. Thanks!

Regards,
-Zhiyuan

> Cheers, Daniel
> 
> > 
> > Regards,
> > -Zhiyuan
> > 
> > > -Daniel
> > > 
> > > > +
> > > >  	if (ret) {
> > > >  		DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n",
> > > >  			      set->crtc->base.id, ret);
> > > > -- 
> > > > 1.9.1
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > > -- 
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915: Add the display switch logic for vgpu in i915 driver

[Daniel Vetter]

On Tue, Sep 30, 2014 at 06:25:26PM +0800, Zhiyuan Lv wrote:
> On Mon, Sep 29, 2014 at 02:30:09PM +0200, Daniel Vetter wrote:
> > I guess the question is what exactly you want to signal to the hyporvisor
> > with this. I guess I need to dig a bit into the sourcecode for the
> > hyperviros part, and you need to make a really clear specification of when
> > guests should call this and what the hyporvisor must to in reaction of
> > this.
> 
> In this patchset for i915 driver running inside a virtual machine, the code
> here is to signal vgt that guest framebuffer address in plane surface register
> is ready to be written into physical registers. vgt's reaction is to make
> policy change. After that point, VM's write to surface MMIO will go into
> physical register directly. Here we do not need notifications for every guest
> display surface MMIO write, but just the time we can do the policy change.
> 
> The usage of this is for guest system booting. At the very beginning, we block
> the guest surface MMIO writes so that the monitor still shows up host
> contents, then we rely on this notification to show up guest VM's screen.

This sounds more like we need one call at the end of the i915 driver load
sequence to signal to the hypervisor that everything is now down and a
proper driver is in charge of the virtual machine gfx.

> We have another change for i915 driver to notify vgt the modesetting. But that
> is for i915 running as a host driver. Sorry that it may be confusing to mix
> two things together. This can be discussed later.

Yeah, better split that out and move it to the host-side vgt integration
series.

> > I don't think we'll have any issues with a bit of overhead in the
> > frontbuffer flip/flush functions, they're not called too often. Aside:
> > There's no nice kerneldoc for this stuff:
> > 
> > http://people.freedesktop.org/~danvet/drm/drmI915.html#idp54709056
> >
> 
> Thanks for the info! The doc is helpful for me to understand the new feature.
> Since intel_frontbuffer_flush() will be called every time there is page flip
> request, that may not be what we want: just one time notification to do the
> policy change. Thanks!

Always nice when my documentation work proves useful ;-)

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

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

[Yu, Zhang]

On 9/29/2014 8:40 PM, Jike Song wrote:
> On 09/29/2014 08:16 PM, Chris Wilson wrote:
>> On Mon, Sep 29, 2014 at 07:44:56PM +0800, Jike Song wrote:
>>> On 09/19/2014 03:25 PM, Chris Wilson wrote:
>>>> 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; }
>>>>
>>>> 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;
>>>>
>>>
>>> Hi Chris, sorry that I didn't understand you correctly. After discussion
>>> with Yu today, I realized that unfortunately, the vgt_if can't be
>>> dereferenced directly.
>>>
>>> There are several reasons:
>>>
>>>     - dereferencing a MMIO address will be complained by sparse(1)
>>>
>>>     - for Guest VM, such accesses will be trapped by hypervisor, and
>>>       hence emulated correctly; However this doesn't work for Host(e.g.
>>>       Domain 0 of Xen, the Linux host KVM resides in). For host, we used
>>>       a proactive mechanism to redirect i915 MMIO accesses to vgt,
>>>       the GPU device-model, for the sake of central management & sharing
>>>       among VMs, including host.
>>
>> You only need to be careful during vgpu detection. After that you know
>> everything is safe. If you do the detection during intel_uncore_init(),
>> or similar, you can use raw mmio access and explict sparse annotations
>> to do the right thing.
>> -Chris
>
> Hi Chris,
>
> Thanks for your suggestion, however, it addressed issue #1 but not issue
> #2.
> I'd like to give a detailed explanation :)
>
> For Guest i915 driver, when accessing a MMIO reg, it goes:
>
>      whatever I915_READ/readl or direct dereferencing(addr)
>          addr is translated to gpa(guest physical address) by guest
> paging structure
>              hypervisor trapped the gpa access and forward it to vgt
>                  vgt_emulate_read32(vgt instance of guest, gpa)
>
>
> The real problem is, when running as the host gpu driver, i915 MMIO/GTT
> accesses are:
>
>      1) every difficult to trap, say the domain 0 of Xen hypervisor; or
>      2) impossible to trap, say KVM(In KVM, the host i915 is running in
> the very same
>         privilege level and root mode as KVM hypervisor.)
>
> So, for Host i915 driver, when accessing a MMIO reg, it goes:
>
>      I915_READ(addr)
>          gen6_read32(addr)
>              offset = addr - dev_priv->regs
>              vgt_mmio_readl(the vgt instance of host, offset)
>                  .. some processing ..
>                  if (passed-throuth)
>                      readl(offset + dev_priv->regs)
>                  else
>                      pa = BAR0 + offset
>                      vgt_emulate_read32(vgt instance of host, pa)
>
>
> The vgt_if corresponds to the PVINFO page, not passed-through(actually
> it doesn't physically exist),
> should fall into the "else" above.
>
> Given that ... Yes we can dereference pointers for guest here, but
> consider that we'll
> deal with host in the future ...
>
Hi Chris,

   Any comments on this issue?
   The host side patches were sent out by Jike last week. As you can see
in his patch, the I915_READ/WRITEs are redefined, and the MMIO accesses
are mediated to vgt, by offset of the regs, instead of by read/write 
(dev_priv->regs + offset) for host i915.
   So, in order to let i915 in the host side access the PVINFO page
successfully, could we still use the I915_READ/WRTE, and keep the
vgpu_active flag when detecting virtual gpu? :)

Thanks
Yu

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