Re: [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
> From: Bing Niu <bing.niu@intel.com>
> 
> This patch introduces host graphics memory/fence partition when GVT-g
> is enabled.
> 
> Under GVT-g, i915 host driver only owned limited graphics resources,
> others are managed by GVT-g resource allocator and kept for other vGPUs.
> 
> v2:
> - Address all coding-style comments from Joonas previously.
> - Fix errors and warnning reported by checkpatch.pl. (Joonas)
> - Move the graphs into the header files. (Daniel)
> 
> Signed-off-by: Bing Niu <bing.niu@intel.com>
> Signed-off-by: Zhi Wang <zhi.a.wang@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/gvt.c      |  4 ++++
>  drivers/gpu/drm/i915/gvt/params.c   | 12 ++++++++++++
>  drivers/gpu/drm/i915/gvt/params.h   |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h     | 35 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c     |  4 +++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c |  4 ++--
>  drivers/gpu/drm/i915/i915_vgpu.c    | 21 +++++++++++++++++----
>  7 files changed, 76 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/gvt.c b/drivers/gpu/drm/i915/gvt/gvt.c
> index 52cfa32..2099b7e 100644
> --- a/drivers/gpu/drm/i915/gvt/gvt.c
> +++ b/drivers/gpu/drm/i915/gvt/gvt.c
> @@ -348,6 +348,10 @@ void *gvt_create_pgt_device(struct drm_i915_private *dev_priv)
>  		goto err;
>  	}
>  
> +	dev_priv->gvt.host_fence_sz = gvt.host_fence_sz;
> +	dev_priv->gvt.host_low_gm_sz_in_mb = gvt.host_low_gm_sz;
> +	dev_priv->gvt.host_high_gm_sz_in_mb = gvt.host_high_gm_sz;

I'm thinking, could we expose the pgt_device struct (at least
partially, and then have a PIMPL pattern), to avoid this kind of
duplication of declarations and unnecessary copies between i915 and
i915_gvt modules?

It's little rough that the gvt driver writes to i915_private struct.
I'd rather see that gvt.host_fence_sz and other variables get sanitized
and then written to pgt_device (maybe the public part would be
i915_pgt_device) and used by gvt and i915 code.

Was this ever considered?

> +
>  	gvt_dbg_core("pgt device creation done, id %d", pdev->id);
>  
>  	return pdev;
> diff --git a/drivers/gpu/drm/i915/gvt/params.c b/drivers/gpu/drm/i915/gvt/params.c
> index d381d17..75195fd 100644
> --- a/drivers/gpu/drm/i915/gvt/params.c
> +++ b/drivers/gpu/drm/i915/gvt/params.c
> @@ -26,7 +26,19 @@
>  struct gvt_kernel_params gvt = {
>  	.enable = false,
>  	.debug = 0,
> +	.host_low_gm_sz = 96,	/* in MB */
> +	.host_high_gm_sz = 384, /* in MB */
> +	.host_fence_sz = 4,
>  };
>  
>  module_param_named(gvt_enable, gvt.enable, bool, 0600);
>  MODULE_PARM_DESC(gvt_enable, "Enable Intel GVT-g host support");
> +
> +module_param_named(gvt_host_low_gm_sz, gvt.host_low_gm_sz, int, 0600);
> +MODULE_PARM_DESC(gvt_host_low_gm_sz, "Amount of aperture size of host (in MB)");

As i915_gvt will be the host module, "gvt_host" can be removed so the
module parameters become;

i915_gvt.low_gm_sz instead of i915_gvt.gvt_host_low_gm_sz

Also should these be _unsafe parameters because I bet they can make the
machine go crazy if wrong?

> +
> +module_param_named(gvt_host_high_gm_sz, gvt.host_high_gm_sz, int, 0600);
> +MODULE_PARM_DESC(gvt_host_high_gm_sz, "Amount of high memory size of host (in MB)");
> +
> +module_param_named(gvt_host_fence_sz, gvt.host_fence_sz, int, 0600);
> +MODULE_PARM_DESC(gvt_host_fence_sz, "Amount of fence size of host (in MB)");
> diff --git a/drivers/gpu/drm/i915/gvt/params.h b/drivers/gpu/drm/i915/gvt/params.h
> index d2955b9..f4e9356 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 host_low_gm_sz;
> +	int host_high_gm_sz;
> +	int host_fence_sz;
>  };
>  
>  extern struct gvt_kernel_params gvt;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2f897c3..1fd5575 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1705,8 +1705,43 @@ struct i915_workarounds {
>  	u32 hw_whitelist_count[I915_NUM_RINGS];
>  };
>  
> +/*
> + * Under GVT-g, i915 host driver 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  |
> + * |       |       |       |       |
> + * +-------+-------+-------+-------+
> + */
>  struct i915_gvt {
>  	void *pgt_device;
> +	u32 host_low_gm_sz_in_mb;
> +	u32 host_high_gm_sz_in_mb;
> +	int host_fence_sz;
>  };
>  
>  struct i915_virtual_gpu {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f68f346..1c0006a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5078,7 +5078,9 @@ i915_gem_load_init(struct drm_device *dev)
>  	else
>  		dev_priv->num_fence_regs = 8;
>  
> -	if (intel_vgpu_active(dev))
> +	if (intel_gvt_active(dev))
> +		dev_priv->num_fence_regs = dev_priv->gvt.host_fence_sz;
> +	else if (intel_vgpu_active(dev))
>  		dev_priv->num_fence_regs =
>  				I915_READ(vgtif_reg(avail_rs.fence_num));
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9127f8f..de09dd4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2734,7 +2734,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_active(dev)) {
>  		ret = intel_vgt_balloon(dev);
>  		if (ret)
>  			return ret;
> @@ -2833,7 +2833,7 @@ void i915_global_gtt_cleanup(struct drm_device *dev)
>  	i915_gem_cleanup_stolen(dev);
>  
>  	if (drm_mm_initialized(&vm->mm)) {
> -		if (intel_vgpu_active(dev))
> +		if (intel_vgpu_active(dev) || intel_gvt_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..7be1435 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -188,10 +188,23 @@ 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_active(dev)) {
> +		mappable_base = 0;
> +		mappable_size = dev_priv->gvt.host_low_gm_sz_in_mb << 20;
> +		unmappable_base = dev_priv->gtt.mappable_end;
> +		unmappable_size = dev_priv->gvt.host_high_gm_sz_in_mb << 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;
> +	}
>  
>  	mappable_end = mappable_base + mappable_size;
>  	unmappable_end = unmappable_base + unmappable_size;
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx