[Intel-gfx] [RFCv2 03/14] drm/i915: Introduce host graphics memory/fence partition for GVT-g

[Joonas Lahtinen]

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 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.
> 

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