On ti, 2016-02-23 at 15:16 +0200, Joonas Lahtinen wrote:
> >
> > 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.
>
Also, using memparse to handle all kernel memory size parameters is a
good idea (see parse_highmem() or related function). That is what users
expect.
> Was this ever considered?
>
<SNIP>
> > 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;
This could be avoided too.
> > + } 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
