[Intel-gfx] [PATCH v3] drm/i915/gvt: return the actual aperture size under gvt environment

[Joonas Lahtinen]

On ma, 2017-05-08 at 02:49 +0000, Li, Weinan Z wrote:
> Hi Joonas/Chris, do you have any comments?
> I've asked OCL team for this patch, they also agree to use available aperture size
> for max allocation buffer definition, code confirmation ongoing.

The patch title should be corrected to refer to usable aperture size.

> > -----Original Message-----
> > From: Li, Weinan Z
> > Sent: Wednesday, May 3, 2017 8:51 AM
> > > > To: intel-gfx at lists.freedesktop.org; intel-gvt-dev at lists.freedesktop.org
> > > > Cc: Li, Weinan Z <weinan.z.li at intel.com>
> > Subject: [PATCH v3] drm/i915/gvt: return the actual aperture size under gvt
> > environment
> > 
> > I915_GEM_GET_APERTURE ioctl is used to probe aperture size from userspace.
> > In gvt environment, each vm only use the ballooned part of aperture, so we
> > should return the actual available aperture size exclude the reserved part by
> > balloon.
> > 
> > v2: add 'reserved' in struct i915_address_space to record the reserved size in
> > ggtt by balloon.
> > 
> > v3: remain aper_size as total, adjust aper_available_size exclude reserved and
> > pinned. UMD driver need to adjust the max allocation size according to the
> > available aperture size but not total size.
> > 
> > Signed-off-by: Weinan Li <weinan.z.li at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c     | 7 +++----
> >  drivers/gpu/drm/i915/i915_gem_gtt.h | 3 ++-
> >  drivers/gpu/drm/i915/i915_vgpu.c    | 5 ++++-
> >  3 files changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c index 84ea249..e84576c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -145,9 +145,8 @@ int i915_mutex_lock_interruptible(struct drm_device
> > *dev)
> >  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> >  	struct drm_i915_gem_get_aperture *args = data;
> >  	struct i915_vma *vma;
> > -	size_t pinned;
> > +	size_t pinned = 0;

Don't do this unrelated change.

> > 
> > -	pinned = 0;
> >  	mutex_lock(&dev->struct_mutex);
> >  	list_for_each_entry(vma, &ggtt->base.active_list, vm_link)
> >  		if (i915_vma_is_pinned(vma))
> > @@ -158,8 +157,8 @@ int i915_mutex_lock_interruptible(struct drm_device
> > *dev)
> >  	mutex_unlock(&dev->struct_mutex);
> > 
> >  	args->aper_size = ggtt->base.total;
> > -	args->aper_available_size = args->aper_size - pinned;
> > -
> > +	args->aper_available_size = args->aper_size
> > +			- ggtt->base.reserved - pinned;

Wrap the line at '-' mark, just like you would with '+'.

> >  	return 0;
> >  }
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index fb15684..bdf832d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -255,7 +255,8 @@ struct i915_address_space {
> >  	struct drm_i915_file_private *file;
> >  	struct list_head global_link;
> >  	u64 total;		/* size addr space maps (ex. 2GB for ggtt) */
> > -
> > +	/* size addr space reserved by GVT balloon, only used for ggtt */

The comment should not be about GVT at all, just any reserved memory.

> > +	u64 reserved;
> >  	bool closed;

<SNIP>

> > @@ -242,6 +242,9 @@ int intel_vgt_balloon(struct drm_i915_private
> > *dev_priv)
> >  			goto err;
> >  	}
> > 
> > +	for (i = 0; i < ARRAY_SIZE(bl_info.space); i++)
> > +		ggtt->base.reserved += bl_info.space[i].size;
> > +

There should be an equal decrease when deballooning is done. And for
that to be correct, you need to add proper onion teardown to this
function to make sure the count stays correct (can't call deballoon on
failure or the count will become negative which will result in huge
number marked as reserved).

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation