Thanks. Best Regards. Weinan, LI > -----Original Message----- > From: Joonas Lahtinen [mailto:joonas.lahtinen@xxxxxxxxxxxxxxx] > Sent: Thursday, May 11, 2017 8:56 PM > To: Li, Weinan Z <weinan.z.li@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel- > gvt-dev@xxxxxxxxxxxxxxxxxxxxx > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH v4] drm/i915/gvt: return the correct usable aperture size > under gvt environment > > On to, 2017-05-11 at 06:51 +0000, Li, Weinan Z wrote: > > > > > > -----Original Message----- > > > From: Joonas Lahtinen [mailto:joonas.lahtinen@xxxxxxxxxxxxxxx] > > > Sent: Wednesday, May 10, 2017 6:43 PM > > > To: Li, Weinan Z <weinan.z.li@xxxxxxxxx>; > > > intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel- > > > gvt-dev@xxxxxxxxxxxxxxxxxxxxx > > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > > 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(&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. 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(&bl_info.space[i]); + 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)); @@ -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 >= end) @@ -127,9 +128,12 @@ 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; + return ret; } /** @@ -215,14 +219,14 @@ int intel_vgt_balloon(struct drm_i915_private *dev_priv) ggtt->mappable_end, unmappable_base); if (ret) - goto err; + goto out_err; } if (unmappable_end < ggtt_end) { ret = vgt_balloon_space(ggtt, &bl_info.space[3], unmappable_end, ggtt_end); if (ret) - goto err; + goto deballoon_upon_mappable; } /* Mappable graphic memory ballooning */ @@ -231,7 +235,7 @@ int intel_vgt_balloon(struct drm_i915_private *dev_priv) 0, mappable_base); if (ret) - goto err; + goto deballoon_upon_unmappable; } if (mappable_end < ggtt->mappable_end) { @@ -239,14 +243,23 @@ int intel_vgt_balloon(struct drm_i915_private *dev_priv) mappable_end, ggtt->mappable_end); if (ret) - goto err; + goto deballoon_below_mappable; } DRM_INFO("VGT balloon successfully\n"); return 0; -err: +deballoon_below_mappable: + ggtt->base.reserved -= bl_info.space[0].size; + drm_mm_remove_node(&bl_info.space[0]); +deballoon_upon_unmappable: + ggtt->base.reserved -= bl_info.space[3].size; + drm_mm_remove_node(&bl_info.space[3]); +deballoon_upon_mappable: + ggtt->base.reserved -= bl_info.space[2].size; + drm_mm_remove_node(&bl_info.space[2]); +out_err: DRM_ERROR("VGT balloon fail\n"); - intel_vgt_deballoon(dev_priv); + memset(&bl_info, 0, sizeof(bl_info)); return ret; } > > 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
