Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-18 Thread Joonas Lahtinen 
On pe, 2017-05-12 at 03:20 +, Li, Weinan Z wrote:
> 
> Thanks Joonas' guidance.
> I add 4 labels in intel_vgt_balloon for cleaning up ballooned space, the 
> reserved size
> will increase when one balloon space reserve success, and will clean up if 
> anyone reserve
> fail step by step.

Yes, there could still be vgt_deballoon_space function to further
reduce code duplication. The labels should begin with err_, like
elsewhere in in the driver.

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: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-11 Thread Li, Weinan Z 


Thanks.
Best Regards.
Weinan, LI


> -Original Message-
> From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> Sent: Thursday, May 11, 2017 8:56 PM
> To: Li, Weinan Z ; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org
> Cc: Chris Wilson 
> Subject: Re: [PATCH v4] drm/i915/gvt: return the correct usable aperture size
> under gvt environment
> 
> On to, 2017-05-11 at 06:51 +, Li, Weinan Z wrote:
> > >
> > > -Original Message-
> > > From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> > > Sent: Wednesday, May 10, 2017 6:43 PM
> > > To: Li, Weinan Z ;
> > > intel-gfx@lists.freedesktop.org; intel-
> > > gvt-...@lists.freedesktop.org
> > > Cc: Chris Wilson 
> > > Subject: Re: [PATCH v4] drm/i915/gvt: return the correct usable
> > > aperture size under gvt environment
> > >
> > > On ke, 2017-05-10 at 10:59 +0800, Weinan Li wrote:
> > > >
> > > > 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 correct available aperture size
> > > > exclude the reserved part by balloon.
> > > >
> > > > v2: add 'reserved' in struct i915_address_space to record the
> > > > reserved size in ggtt.
> > > >
> > > > 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.
> > > > KMD return the correct usable aperture size any time.
> > > >
> > > > v4: add onion teardown to balloon and deballoon to make sure the
> > > > reserved stays correct. Code style refine.
> > >
> > > There's no onion teardown seen yet, please read:
> > >
> > > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#cent
> > > ral
> > > ized-exiting-of-functions
> > >
> > > Please incorporate the addition to vgt_balloon_space function to
> > > reduce code duplication.
> > >
> > > Once the proper teardown is implemented, intel_vgt_deballoon
> > > function should unconditionally remove the drm_mm nodes as there's
> > > no condition when only one of them would be allocated. And
> > > intel_vgt_balloon definitely should not call intel_vgt_deballoon in error
> path as per the coding style above.
> >
> > Thanks Joonas. A little change is brought in if move the fail checking
> > into balloon_space() There are 4 balloon spaces, current policy is if
> > any one fail clean up all the success ones, with this change it won't clean 
> > up
> the success ones. It should not impact the driver's behavior.
> 
> Please re-read the "Centralized exiting of function", in this case you'd have
> three labels for onion teardown if any of the space reservations fails, you 
> jump
> to the right one. Please take a look in the i915 to similar initialization 
> functions
> for examples.
> 
> > @@ -127,9 +130,14 @@ static int vgt_balloon_space(struct i915_ggtt
> > *ggtt,
> >
> > DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KiB.\n",
> >  start, end, size / 1024);
> > -   return i915_gem_gtt_reserve(>base, node,
> > +   ret = i915_gem_gtt_reserve(>base, node,
> > size, start,
> > I915_COLOR_UNEVICTABLE,
> > 0);
> > +   if (!ret)
> > +   ggtt->base.reserved += size;
> > +   else
> > +   memset(node, 0, sizeof(*node));
> 
> This should not be needed. Either all of the nodes have been successfully
> initialize or not. The only partial states are in the intel_vgt_balloon 
> function,
> which should use different labels to back off from different stages of
> initialization.
Thanks Joonas' guidance.
I add 4 labels in intel_vgt_balloon for cleaning up ballooned space, the 
reserved size
will increase when one balloon space reserve success, and will clean up if 
anyone reserve
fail step by step.

diff --git a/drivers/gpu/drm/i915/i915_vgpu.c b/drivers/gpu/drm/i915/i915_vgpu.c
index 4ab8a97..e21cfff 100644
--- a/drivers/gpu/drm/i915/i915_vgpu.c
+++ b/drivers/gpu/drm/i915/i915_vgpu.c
@@ -109,8 +109,8 @@ void intel_vgt_deballoon(struct drm_i915_private *dev_priv)
DRM_DEBUG("VGT deballoon.\n");

for (i = 0; i < 4; i++) {
-   if (bl_info.space[i].allocated)
-   drm_mm_remove_node(_info.space[i]);
+   dev_priv->ggtt.base.reserved -= bl_info.space[i].size;
+   drm_mm_remove_node(_info.space[i]);
}

memset(_info, 0, sizeof(bl_info));
@@ -120,6 +120,7 @@ static int vgt_balloon_space(struct i915_ggtt *ggtt,
 struct drm_mm_node *node,
 unsigned long start, unsigned long end)
 {
+   int ret;
unsigned long size = end - start;

if (start

Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-11 Thread kbuild test robot 
Hi Weinan,

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.11 next-20170511]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Weinan-Li/drm-i915-gvt-return-the-correct-usable-aperture-size-under-gvt-environment/20170511-170356
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu//drm/i915/i915_vgpu.c: In function 'intel_vgt_deballoon':
>> drivers/gpu//drm/i915/i915_vgpu.c:113:18: error: invalid type argument of 
>> '->' (have 'struct i915_ggtt')
   dev_priv->ggtt->base.reserved -= bl_info.space[i].size;
 ^~

vim +113 drivers/gpu//drm/i915/i915_vgpu.c

97   * @dev_priv: i915 device private data
98   *
99   * This function is called to deallocate the ballooned-out graphic 
memory, when
   100   * driver is unloaded or when ballooning fails.
   101   */
   102  void intel_vgt_deballoon(struct drm_i915_private *dev_priv)
   103  {
   104  int i;
   105  
   106  if (!intel_vgpu_active(dev_priv))
   107  return;
   108  
   109  DRM_DEBUG("VGT deballoon.\n");
   110  
   111  for (i = 0; i < 4; i++) {
   112  if (bl_info.space[i].allocated) {
 > 113  dev_priv->ggtt->base.reserved -= 
 > bl_info.space[i].size;
   114  drm_mm_remove_node(_info.space[i]);
   115  }
   116  }
   117  
   118  memset(_info, 0, sizeof(bl_info));
   119  }
   120  
   121  static int vgt_balloon_space(struct i915_ggtt *ggtt,

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-11 Thread Joonas Lahtinen 
On to, 2017-05-11 at 06:51 +, Li, Weinan Z wrote:
> > 
> > -Original Message-
> > From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> > Sent: Wednesday, May 10, 2017 6:43 PM
> > To: Li, Weinan Z ; intel-gfx@lists.freedesktop.org; 
> > intel-
> > gvt-...@lists.freedesktop.org
> > Cc: Chris Wilson 
> > Subject: Re: [PATCH v4] drm/i915/gvt: return the correct usable aperture 
> > size
> > under gvt environment
> > 
> > On ke, 2017-05-10 at 10:59 +0800, Weinan Li wrote:
> > > 
> > > 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 correct available aperture size exclude the
> > > reserved part by balloon.
> > > 
> > > v2: add 'reserved' in struct i915_address_space to record the reserved
> > > size in ggtt.
> > > 
> > > 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. KMD
> > > return the correct usable aperture size any time.
> > > 
> > > v4: add onion teardown to balloon and deballoon to make sure the
> > > reserved stays correct. Code style refine.
> > 
> > There's no onion teardown seen yet, please read:
> > 
> > https://www.kernel.org/doc/html/v4.10/process/coding-style.html#central
> > ized-exiting-of-functions
> > 
> > Please incorporate the addition to vgt_balloon_space function to reduce code
> > duplication.
> > 
> > Once the proper teardown is implemented, intel_vgt_deballoon function should
> > unconditionally remove the drm_mm nodes as there's no condition when only
> > one of them would be allocated. And intel_vgt_balloon definitely should not 
> > call
> > intel_vgt_deballoon in error path as per the coding style above.
> 
> Thanks Joonas. A little change is brought in if move the fail checking into 
> balloon_space()
> There are 4 balloon spaces, current policy is if any one fail clean up all 
> the success ones, with
> this change it won't clean up the success ones. It should not impact the 
> driver's behavior.

Please re-read the "Centralized exiting of function", in this case
you'd have three labels for onion teardown if any of the space
reservations fails, you jump to the right one. Please take a look in
the i915 to similar initialization functions for examples.

> @@ -127,9 +130,14 @@ static int vgt_balloon_space(struct i915_ggtt *ggtt,
> 
> DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KiB.\n",
>  start, end, size / 1024);
> -   return i915_gem_gtt_reserve(>base, node,
> +   ret = i915_gem_gtt_reserve(>base, node,
> size, start, I915_COLOR_UNEVICTABLE,
> 0);
> +   if (!ret)
> +   ggtt->base.reserved += size;
> +   else
> +   memset(node, 0, sizeof(*node));

This should not be needed. Either all of the nodes have been
successfully initialize or not. The only partial states are in the
intel_vgt_balloon function, which should use different labels to back
off from different stages of initialization.

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: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-11 Thread Li, Weinan Z 
> -Original Message-
> From: Joonas Lahtinen [mailto:joonas.lahti...@linux.intel.com]
> Sent: Wednesday, May 10, 2017 6:43 PM
> To: Li, Weinan Z ; intel-gfx@lists.freedesktop.org; 
> intel-
> gvt-...@lists.freedesktop.org
> Cc: Chris Wilson 
> Subject: Re: [PATCH v4] drm/i915/gvt: return the correct usable aperture size
> under gvt environment
> 
> On ke, 2017-05-10 at 10:59 +0800, Weinan Li wrote:
> > 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 correct available aperture size exclude the
> > reserved part by balloon.
> >
> > v2: add 'reserved' in struct i915_address_space to record the reserved
> > size in ggtt.
> >
> > 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. KMD
> > return the correct usable aperture size any time.
> >
> > v4: add onion teardown to balloon and deballoon to make sure the
> > reserved stays correct. Code style refine.
> 
> There's no onion teardown seen yet, please read:
> 
> https://www.kernel.org/doc/html/v4.10/process/coding-style.html#central
> ized-exiting-of-functions
> 
> Please incorporate the addition to vgt_balloon_space function to reduce code
> duplication.
> 
> Once the proper teardown is implemented, intel_vgt_deballoon function should
> unconditionally remove the drm_mm nodes as there's no condition when only
> one of them would be allocated. And intel_vgt_balloon definitely should not 
> call
> intel_vgt_deballoon in error path as per the coding style above.

Thanks Joonas. A little change is brought in if move the fail checking into 
balloon_space()
There are 4 balloon spaces, current policy is if any one fail clean up all the 
success ones, with
this change it won't clean up the success ones. It should not impact the 
driver's behavior.

@@ -120,6 +122,7 @@ static int vgt_balloon_space(struct i915_ggtt *ggtt,
 struct drm_mm_node *node,
 unsigned long start, unsigned long end)
 {
+   int ret;
unsigned long size = end - start;

if (start >= end)
@@ -127,9 +130,14 @@ static int vgt_balloon_space(struct i915_ggtt *ggtt,

DRM_INFO("balloon space: range [ 0x%lx - 0x%lx ] %lu KiB.\n",
 start, end, size / 1024);
-   return i915_gem_gtt_reserve(>base, node,
+   ret = i915_gem_gtt_reserve(>base, node,
size, start, I915_COLOR_UNEVICTABLE,
0);
+   if (!ret)
+   ggtt->base.reserved += size;
+   else
+   memset(node, 0, sizeof(*node));
+   return ret;
 }

 /**
@@ -247,6 +255,5 @@ int intel_vgt_balloon(struct drm_i915_private *dev_priv)

 err:
DRM_ERROR("VGT balloon fail\n");
-   intel_vgt_deballoon(dev_priv);
return ret;
 }
> 
> 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: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-10 Thread Joonas Lahtinen 
On ke, 2017-05-10 at 10:59 +0800, Weinan Li wrote:
> 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 correct available aperture size exclude the reserved part
> by balloon.
> 
> v2: add 'reserved' in struct i915_address_space to record the reserved size
> in ggtt.
> 
> 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. KMD return the correct
> usable aperture size any time.
> 
> v4: add onion teardown to balloon and deballoon to make sure the reserved
> stays correct. Code style refine.

There's no onion teardown seen yet, please read:

https://www.kernel.org/doc/html/v4.10/process/coding-style.html#central
ized-exiting-of-functions

Please incorporate the addition to vgt_balloon_space function to reduce
code duplication.

Once the proper teardown is implemented, intel_vgt_deballoon function
should unconditionally remove the drm_mm nodes as there's no condition
when only one of them would be allocated. And intel_vgt_balloon
definitely should not call intel_vgt_deballoon in error path as per the
coding style above.

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: [Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment

2017-05-09 Thread Li, Weinan Z 
Really sorry, please ignore this mail with wrong patch. Will send the correct 
one then.

Thanks.
Best Regards.
Weinan, LI


> -Original Message-
> From: Li, Weinan Z
> Sent: Wednesday, May 10, 2017 10:48 AM
> To: intel-gfx@lists.freedesktop.org; intel-gvt-...@lists.freedesktop.org
> Cc: Li, Weinan Z ; Chris Wilson  wilson.co.uk>; Joonas Lahtinen 
> Subject: [PATCH v4] drm/i915/gvt: return the correct usable 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 correct available aperture size exclude the reserved part by
> balloon.
> 
> v2: add 'reserved' in struct i915_address_space to record the reserved size in
> ggtt.
> 
> 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. KMD return the correct usable
> aperture size any time.
> 
> v4: add onion teardown to balloon and deballoon to make sure the reserved
> stays correct. Code style refine.
> 
> Cc: Chris Wilson 
> Cc: Joonas Lahtinen 
> Signed-off-by: Weinan Li 
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 ++--
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
>  drivers/gpu/drm/i915/i915_vgpu.c| 8 +++-
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 33fb11c..8d8d9c0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -156,8 +156,8 @@ int i915_mutex_lock_interruptible(struct drm_device
> *dev)
>   mutex_unlock(>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..da9aa9f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -255,6 +255,7 @@ 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) */
> + u64 reserved;   /* size addr space reserved */
> 
>   bool closed;
> 
> diff --git a/drivers/gpu/drm/i915/i915_vgpu.c
> b/drivers/gpu/drm/i915/i915_vgpu.c
> index 4ab8a97..b144cf6 100644
> --- a/drivers/gpu/drm/i915/i915_vgpu.c
> +++ b/drivers/gpu/drm/i915/i915_vgpu.c
> @@ -109,8 +109,10 @@ void intel_vgt_deballoon(struct drm_i915_private
> *dev_priv)
>   DRM_DEBUG("VGT deballoon.\n");
> 
>   for (i = 0; i < 4; i++) {
> - if (bl_info.space[i].allocated)
> + if (bl_info.space[i].allocated) {
> + dev_priv->ggtt->base.reserved -= bl_info.space[i].size;
>   drm_mm_remove_node(_info.space[i]);
> + }
>   }
> 
>   memset(_info, 0, sizeof(bl_info));
> @@ -216,6 +218,7 @@ int intel_vgt_balloon(struct drm_i915_private
> *dev_priv)
> 
>   if (ret)
>   goto err;
> + ggtt->base.reserved += bl_info.space[2].size;
>   }
> 
>   if (unmappable_end < ggtt_end) {
> @@ -223,6 +226,7 @@ int intel_vgt_balloon(struct drm_i915_private
> *dev_priv)
>   unmappable_end, ggtt_end);
>   if (ret)
>   goto err;
> + ggtt->base.reserved += bl_info.space[3].size;
>   }
> 
>   /* Mappable graphic memory ballooning */ @@ -232,6 +236,7 @@ int
> intel_vgt_balloon(struct drm_i915_private *dev_priv)
> 
>   if (ret)
>   goto err;
> + ggtt->base.reserved += bl_info.space[0].size;
>   }
> 
>   if (mappable_end < ggtt->mappable_end) { @@ -240,6 +245,7 @@ int
> intel_vgt_balloon(struct drm_i915_private *dev_priv)
> 
>   if (ret)
>   goto err;
> + ggtt->base.reserved += bl_info.space[1].size;
>   }
> 
>   DRM_INFO("VGT balloon successfully\n");
> --
> 1.9.1

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