[Intel-gfx] [RFC 02/29] drm/i915: Introduce host graphics memory balloon for gvt
Zhiyuan Lv
zhiyuan.lv at intel.com
Fri Feb 5 14:16:29 UTC 2016
Hi Joonas,
On Fri, Feb 05, 2016 at 03:40:49PM +0200, Joonas Lahtinen wrote:
> Hi,
>
> On pe, 2016-02-05 at 18:03 +0800, Zhiyuan Lv wrote:
> > Hi Joonas,
> >
> > Thanks much for the review! We will incorporate those review comments!
> >
> > Meanwhile, is it good enough to do the host ballooning like below? The
> > pros is that it is very simple, especially consider that guest
> > ballooning logic has been there. Thanks!
> >
>
> I think slicing it down like that is fine. Is there going to be real
> ballooning happening, as in, is the available memory dynamically going
> to change during runtime, or just be fixed size since the beginning?
In our current design, once the gvt is enabled, it is fixed size of graphics
memory for host since beginning. The reserved memory is all for virtual
machines.
We ever considered to support dynamically enabling/disabling gvt, that host
i915 could boot without gvt enabled, and can use all the hardware resources.
When there is need to creat VM, we can reload i915 driver to reserve the
resources. Does that sound good? Thanks!
Regards,
-Zhiyuan
>
> Regards, Joonas
>
> > Regards,
> > -Zhiyuan
> >
> > On Thu, Feb 04, 2016 at 01:27:08PM +0200, Joonas Lahtinen wrote:
> > > Hi,
> > >
> > > On to, 2016-01-28 at 18:21 +0800, Zhi Wang wrote:
> > > > From: Bing Niu <bing.niu at 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 at intel.com>
> > > > Signed-off-by: Zhi Wang <zhi.a.wang at 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;
>
> Some comment on the unit would be nice here, as there is a shift with
> << 20 later on.
>
> Is "unsigned long" type and count in pages not acceptable? I think
> that's the assumption one is going to make.
>
> > > > };
> > > >
> > > > 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 "("
> >
> > Thanks! Will correct it.
> >
> > >
> > > > + 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?
> >
> > That looks better!
> >
> > >
> > > > 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.
> >
> > That's right. Thanks for pointing it out!
> >
> > >
> > > > 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.
> > >
> >
> > Will delete. Thanks!
> >
> > > 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
>
More information about the Intel-gfx
mailing list