On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
> From: Bing Niu <bing.niu@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Signed-off-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
