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

[Li, Weinan Z]

> -----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#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