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

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

[Weinan Li]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/gvt: return the actual aperture size under gvt environment (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/gvt: return the actual aperture size under gvt environment (rev3)
URL   : https://patchwork.freedesktop.org/series/22910/
State : success

== Summary ==

Series 22910v3 drm/i915/gvt: return the actual aperture size under gvt environment
https://patchwork.freedesktop.org/api/1.0/series/22910/revisions/3/mbox/

Test drv_module_reload:
        Subgroup basic-reload-final:
                dmesg-warn -> PASS       (fi-skl-6770hq) fdo#100248
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007

fdo#100248 https://bugs.freedesktop.org/show_bug.cgi?id=100248
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time:429s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time:425s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time:573s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time:511s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time:555s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time:492s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:477s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:409s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time:407s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time:420s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:496s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:465s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time:458s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:564s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:451s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time:569s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time:463s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time:497s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time:433s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time:532s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time:410s

7f027554339633cb4e6a2d3974510aaef1a9e24c drm-tip: 2017y-05m-02d-18h-48m-28s UTC integration manifest
fafe274 drm/i915/gvt: return the actual aperture size under gvt environment

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4604/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Li, Weinan Z]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Li, Weinan Z]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Li, Weinan Z]

> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx