[v3] drm/i915/gvt: return the actual aperture size under gvt environment

Submitted by Weinan Li on May 3, 2017, 12:51 a.m.

Details

Message ID 1493772683-10629-1-git-send-email-weinan.z.li@intel.com
State New
Headers show
Series "drm/i915/gvt: return the actual aperture size under gvt environment" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Weinan Li May 3, 2017, 12:51 a.m.
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@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(-)

Patch hide | download patch | download mbox

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;
 
-	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;
 	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 */
+	u64 reserved;
 	bool closed;
 
 	struct i915_page_dma scratch_page;
diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 4ab8a97..58055a9 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -183,7 +183,7 @@  int intel_vgt_balloon(struct drm_i915_private *dev_priv)
 
 	unsigned long mappable_base, mappable_size, mappable_end;
 	unsigned long unmappable_base, unmappable_size, unmappable_end;
-	int ret;
+	int ret, i;
 
 	if (!intel_vgpu_active(dev_priv))
 		return 0;
@@ -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;
+
 	DRM_INFO("VGT balloon successfully\n");
 	return 0;
 

Comments

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.

> -----Original Message-----
> From: Li, Weinan Z
> Sent: Wednesday, May 3, 2017 8:51 AM
> To: intel-gfx@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org
> Cc: Li, Weinan Z <weinan.z.li@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@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;
> 
> -	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;
>  	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 */
> +	u64 reserved;
>  	bool closed;
> 
>  	struct i915_page_dma scratch_page;
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
> b/drivers/gpu/drm/i915/i915_vgpu.c
> index 4ab8a97..58055a9 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -183,7 +183,7 @@ int intel_vgt_balloon(struct drm_i915_private
> *dev_priv)
> 
>  	unsigned long mappable_base, mappable_size, mappable_end;
>  	unsigned long unmappable_base, unmappable_size, unmappable_end;
> -	int ret;
> +	int ret, i;
> 
>  	if (!intel_vgpu_active(dev_priv))
>  		return 0;
> @@ -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;
> +
>  	DRM_INFO("VGT balloon successfully\n");
>  	return 0;
> 
> --
> 1.9.1
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@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org
> > > > Cc: Li, Weinan Z <weinan.z.li@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@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
On Mon, May 08, 2017 at 01:18:38PM +0300, Joonas Lahtinen wrote:
> 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@lists.freedesktop.org; intel-gvt-dev@lists.freedesktop.org
> > > > > Cc: Li, Weinan Z <weinan.z.li@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.

Only adjusting aper_available_size prevents the immediate breakage wrt
to libdrm_intel, but I'm worried about the fragility of userspace using
this. What is it being used for? I doubt this is the right information
for them, and generally whether it's a correct assumption by userspace.

What happened to reporting the correct size via CONTEXT_PARAM_GTT_SIZE
which doesn't have the same issues with userspace incorrectly using GGTT
for PPGTT?
-Chris
Thanks Joonas. 
> -----Original Message-----

> From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]

> Sent: Monday, May 8, 2017 6:19 PM

> To: Li, Weinan Z <weinan.z.li@intel.com>; intel-gfx@lists.freedesktop.org; intel-

> gvt-dev@lists.freedesktop.org

> Cc: Chris Wilson <chris@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@lists.freedesktop.org;

> > > > > intel-gvt-dev@lists.freedesktop.org

> > > > > Cc: Li, Weinan Z <weinan.z.li@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@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
Thanks Chris.
> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Monday, May 8, 2017 8:11 PM
> To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Li, Weinan Z <weinan.z.li@intel.com>; intel-gfx@lists.freedesktop.org; intel-
> gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH v3] drm/i915/gvt: return the actual aperture size under gvt
> environment
> 
> On Mon, May 08, 2017 at 01:18:38PM +0300, Joonas Lahtinen wrote:
> > 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@lists.freedesktop.org;
> > > > > > intel-gvt-dev@lists.freedesktop.org
> > > > > > Cc: Li, Weinan Z <weinan.z.li@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.
> 
> Only adjusting aper_available_size prevents the immediate breakage wrt to
> libdrm_intel, but I'm worried about the fragility of userspace using this. What is
> it being used for? I doubt this is the right information for them, and generally
> whether it's a correct assumption by userspace.
This issue was found by OCL test in GVT-g Linux guest, actually it uses total aperture
size now, but if we adjust the total size, it may impact some other applications that
need use total size to check available GTT address(talked in the last mail loop).
So I talked with OCL team guys, they also agree to use usable aperture size.
For other applications, I am not sure. Any way we should return correct information
under GVT-g environment I think.
> 
> What happened to reporting the correct size via CONTEXT_PARAM_GTT_SIZE
> which doesn't have the same issues with userspace incorrectly using GGTT for
> PPGTT?
I remove it in this patch, since I haven't found any issues relate to
CONTEXT_PARAM_GTT_SIZE so far.
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
On ti, 2017-05-09 at 03:10 +0000, Li, Weinan Z wrote:

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

Onion teardown should be used according to kernel coding style, there's
really no excuse not to.

Just add to the ggtt->base.reserved in increments, and remove in
increments during teardown or in the deballoon function. ggtt.reserved
is not exclusively for GVT-g to use, so you can't simply zero it. There
needs to be incremental additions and substractions as objects are
added and removed for the variable to stay general.

Regards, Joonas
> -----Original Message-----

> From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]

> Sent: Tuesday, May 9, 2017 8:36 PM

> To: Li, Weinan Z <weinan.z.li@intel.com>; intel-gfx@lists.freedesktop.org; intel-

> gvt-dev@lists.freedesktop.org

> Cc: Chris Wilson <chris@chris-wilson.co.uk>

> Subject: Re: [PATCH v3] drm/i915/gvt: return the actual aperture size under gvt

> environment

> 

> On ti, 2017-05-09 at 03:10 +0000, Li, Weinan Z wrote:

> 

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

> 

> Onion teardown should be used according to kernel coding style, there's really

> no excuse not to.

> 

> Just add to the ggtt->base.reserved in increments, and remove in increments

> during teardown or in the deballoon function. ggtt.reserved is not exclusively

> for GVT-g to use, so you can't simply zero it. There needs to be incremental

> additions and substractions as objects are added and removed for the variable

> to stay general.

Got it, I will refine it and send as patch v4.
> 

> Regards, Joonas

> --

> Joonas Lahtinen

> Open Source Technology Center

> Intel Corporation