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!
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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Zhi Wang <zhi.a.wang@xxxxxxxxx>
> > ---
> > 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 "("
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
