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

[Li, Weinan Z]

Thanks Joonas. 
> -----Original Message-----
> From: Joonas Lahtinen [mailto:joonas.lahtinen at linux.intel.com]
> Sent: Monday, May 8, 2017 6:19 PM
> To: Li, Weinan Z <weinan.z.li at intel.com>; intel-gfx at lists.freedesktop.org; intel-
> gvt-dev at lists.freedesktop.org
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Subject: Re: [PATCH v3] drm/i915/gvt: return the actual aperture size under gvt
> environment
> 
> 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.
Title->return the correct usable aperture size under GVT-g environment
> 
> > > -----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.
+	/* size addr space reserved. */
> 
> > > +	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).
Oh, that's my fault. Should add clean up in intel_vgt_deballoon().
@@ -114,6 +114,7 @@ void intel_vgt_deballoon(struct drm_i915_private *dev_priv)
        }

        memset(&bl_info, 0, sizeof(bl_info));
+       dev_priv->ggtt.reserved = 0;
 }
Since if any steps in intel_vgt_balloon() fail, it will deal as error and run
intel_vgt_deballoon() for clean up, no partial success happen.
So we only calculate the reserved when balloon success, it can ensure it's correct.
> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation