[Intel-gfx] [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g
Zhi Wang
zhi.a.wang at intel.com
Tue Feb 23 13:23:05 UTC 2016
On 02/23/16 21:16, Joonas Lahtinen wrote:
> On to, 2016-02-18 at 19:42 +0800, Zhi Wang wrote:
>> From: Bing Niu <bing.niu at 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 at intel.com>
>> Signed-off-by: Zhi Wang <zhi.a.wang at 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?
>
The previous version do something similar like that, both i915 and gvt
reads GVT module kernel parameter but considered that GVT modules could
be configured as "n" in kernel configuration, probably we should let
i915 to store this information and GVT to configure it if GVT is active?
>> +
>> 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?
>
Yes, the params need to be checked, indeed.
>> +
>> +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;
More information about the Intel-gfx
mailing list