| Message ID | 1494385161-25662-1-git-send-email-weinan.z.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
> -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > Sent: Wednesday, May 10, 2017 6:43 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 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(&ggtt->base, node, + ret = i915_gem_gtt_reserve(&ggtt->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
On to, 2017-05-11 at 06:51 +0000, Li, Weinan Z wrote: > > > > -----Original Message----- > > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com] > > Sent: Wednesday, May 10, 2017 6:43 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 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(&ggtt->base, node, > + ret = i915_gem_gtt_reserve(&ggtt->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
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(&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..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..25bed9b 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(&bl_info.space[i]); + } } memset(&bl_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");
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 <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by: Weinan Li <weinan.z.li@intel.com> --- 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(-)