[Intel-gfx] [PATCH v4] drm/i915/gvt: return the correct usable aperture size under gvt environment
Li, Weinan Z
weinan.z.li at intel.com
Fri May 12 03:20:10 UTC 2017
Thanks.
Best Regards.
Weinan, LI
> -----Original Message-----
> From: Joonas Lahtinen [mailto:joonas.lahtinen at linux.intel.com]
> Sent: Thursday, May 11, 2017 8:56 PM
> To: Li, Weinan Z <weinan.z.li at intel.com>; intel-gfx at lists.freedesktop.org; intel-
> gvt-dev at lists.freedesktop.org
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> 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 at linux.intel.com]
> > > Sent: Wednesday, May 10, 2017 6:43 PM
> > > To: Li, Weinan Z <weinan.z.li at intel.com>;
> > > intel-gfx at lists.freedesktop.org; intel-
> > > gvt-dev at lists.freedesktop.org
> > > Cc: Chris Wilson <chris at 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#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
More information about the Intel-gfx
mailing list