Thanks Joonas. > -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@xxxxxxxxxxxxxxx] > Sent: Monday, May 8, 2017 6:19 PM > To: Li, Weinan Z <weinan.z.li@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel- > gvt-dev@xxxxxxxxxxxxxxxxxxxxx > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxxxxxxxx; > > > > > intel-gvt-dev@xxxxxxxxxxxxxxxxxxxxx > > > > > Cc: Li, Weinan Z <weinan.z.li@xxxxxxxxx> > > > 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@xxxxxxxxx> > > > --- > > > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
