Re: [RFC 02/29] drm/i915: Introduce host graphics memory balloon for gvt

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Zhi Wang <zhi.a.wang@intel.com>,
	intel-gfx@lists.freedesktop.org, igvt-g@lists.01.org
Cc: daniel.vetter@ffwll.ch, david.j.cowperthwaite@intel.com
Subject: Re: [RFC 02/29] drm/i915: Introduce host graphics memory balloon for gvt
Date: Thu, 04 Feb 2016 13:27:08 +0200	[thread overview]
Message-ID: <1454585228.9041.24.camel@linux.intel.com> (raw)
In-Reply-To: <1453976511-27322-3-git-send-email-zhi.a.wang@intel.com>

Hi,

On to, 2016-01-28 at 18:21 +0800, Zhi Wang wrote:
> From: Bing Niu <bing.niu@intel.com>
> 
> This patch introduces host graphics memory ballon when GVT-g is enabled.
> 
> As under GVT-g, i915 only owned limited graphics resources, others are
> managed by GVT-g resource allocator and kept for other vGPUs.
> 
> For graphics memory space partition, a typical layout looks like:
> 
> +-------+-----------------------+------+-----------------------+
> > * Host |   *GVT-g Resource     |* Host|   *GVT-g Resource     |
> > Owned |   Allocator Managed   | Owned|   Allocator Managed   |
> >       |                       |      |                       |
> +---------------+-------+----------------------+-------+-------+
> >       |       |       |       |      |       |       |       |
> > i915  | vm 1  | vm 2  | vm 3  | i915 | vm 1  | vm 2  | vm 3  |
> >       |       |       |       |      |       |       |       |
> +-------+-------+-------+--------------+-------+-------+-------+
> >           Aperture            |            Hidden            |
> +-------------------------------+------------------------------+
> >                       GGTT memory space                      |
> +--------------------------------------------------------------+
> 
> Similar with fence registers partition:
> 
>  +------ +-----------------------+
>  | * Host|    GVT-g Resource     |
>  | Owned |   Allocator Managed   +
>  0       |                       31
>  +---------------+-------+-------+
>  |       |       |       |       |
>  | i915  | vm 1  | vm 2  | vm 3  |
>  |       |       |       |       |
>  +-------+-------+-------+-------+
> 
> i915 host will read the amount of allocated resources via GVT-g kernel parameters.
> 
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/params.h   |  3 +++
>  drivers/gpu/drm/i915/i915_gem.c     |  3 +++
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  4 ++--
>  drivers/gpu/drm/i915/i915_vgpu.c    | 16 ++++++++++++----
>  drivers/gpu/drm/i915/i915_vgpu.h    |  1 +
>  5 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
> index d2955b9..0656a98 100644
> --- a/drivers/gpu/drm/i915/gvt/params.h
> +++ b/drivers/gpu/drm/i915/gvt/params.h
> @@ -27,6 +27,9 @@
>  struct gvt_kernel_params {
>  	bool enable;
>  	int debug;
> +	int dom0_low_gm_sz;
> +	int dom0_high_gm_sz;
> +	int dom0_fence_sz;
>  };
>  
>  extern struct gvt_kernel_params gvt;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 799a53a..e916e43 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5080,6 +5080,9 @@ i915_gem_load(struct drm_device *dev)
>  	else
>  		dev_priv->num_fence_regs = 8;
>  
> +	if(intel_gvt_host_active(dev))

Space between "if" and "("

> +		dev_priv->num_fence_regs = gvt.dom0_fence_sz;
> +
>  	if (intel_vgpu_active(dev))
>  		dev_priv->num_fence_regs =
>  				I915_READ(vgtif_reg(avail_rs.fence_num));

I'd like to see the above as "else if" construct just like you have
below with the intel_vgt_balloon(). Could even do

	if (gvt_host)  {
		...
	} else if (vgpu_active) {
		...
	} else {
		per machine detection
	}

Right?

> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7377b67..0540de2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2713,7 +2713,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  	i915_address_space_init(ggtt_vm, dev_priv);
>  	ggtt_vm->total += PAGE_SIZE;
>  
> -	if (intel_vgpu_active(dev)) {
> +	if (intel_vgpu_active(dev) || intel_gvt_host_active(dev)) {
>  		ret = intel_vgt_balloon(dev);
>  		if (ret)
>  			return ret;
> @@ -2810,7 +2810,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev)
>  	}
>  
>  	if (drm_mm_initialized(&vm->mm)) {
> -		if (intel_vgpu_active(dev))
> +		if (intel_vgpu_active(dev) || intel_gvt_host_active(dev))
>  			intel_vgt_deballoon();
>  
>  		drm_mm_takedown(&vm->mm);
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
> index dea7429..fbe6114 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -188,10 +188,18 @@ int intel_vgt_balloon(struct drm_device *dev)
>  	unsigned long unmappable_base, unmappable_size, unmappable_end;
>  	int ret;
>  
> -	mappable_base = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.base));
> -	mappable_size = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.size));
> -	unmappable_base = I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.base));
> -	unmappable_size = I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.size));
> +	if(intel_gvt_host_active(dev)) {
> +		mappable_base = 0;
> +		mappable_size = gvt.dom0_low_gm_sz << 20;
> +		unmappable_base = dev_priv->gtt.mappable_end;
> +		unmappable_size = gvt.dom0_high_gm_sz << 20;
> +	} else if (intel_vgpu_active(dev)) {
> +		mappable_base = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.base));
> +		mappable_size = I915_READ(vgtif_reg(avail_rs.mappable_gmadr.size));
> +		unmappable_base = I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.base));
> +		unmappable_size = I915_READ(vgtif_reg(avail_rs.nonmappable_gmadr.size));
> +	} else
> +		return -ENODEV;
>  
	} else {
		return -ENODEV;
	}

This must use braces too, according to the Kernel Coding Style.

>  	mappable_end = mappable_base + mappable_size;
>  	unmappable_end = unmappable_base + unmappable_size;
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h
> index 942490a..b8a49e6 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.h
> +++ b/drivers/gpu/drm/i915/i915_vgpu.h
> @@ -24,6 +24,7 @@
>  #ifndef _I915_VGPU_H_
>  #define _I915_VGPU_H_
>  
> +#include "gvt/params.h"

This is not a good way of including headers, as this header itself
doesn't need it. Including unnecessary stuff in often included headers
contributes to increasing project compile times and makes it harder to
solve cross dependencies later. So put it into the implementation file
that needs it.

Regards, Joonas

>  /* The MMIO offset of the shared info between guest and host emulator */
>  #define VGT_PVINFO_PAGE	0x78000
>  #define VGT_PVINFO_SIZE	0x1000
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx