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

Commit Message

Li, Weinan Z May 12, 2017, 3:20 a.m. UTC

Thanks.
Best Regards.
Weinan, LI


> -----Original Message-----
> From: Joonas Lahtinen [mailto:[e-mail]]
> Sent: Thursday, May 11, 2017 8:56 PM
> To: Li, Weinan Z <[e-mail]>; [e-mail]; intel-
> [e-mail]
> Cc: Chris Wilson <[e-mail]>
> 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:[e-mail]]
> > > Sent: Wednesday, May 10, 2017 6:43 PM
> > > To: Li, Weinan Z <[e-mail]>;
> > > [e-mail]; intel-
> > > [e-mail]
> > > Cc: Chris Wilson <[e-mail]>
> > > 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.

> 
> Regards, Joonas
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation

Comments

Joonas Lahtinen May 18, 2017, 8:28 a.m. UTC | #1

On pe, 2017-05-12 at 03:20 +0000, Li, Weinan Z wrote:
> 
> 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.

Yes, there could still be vgt_deballoon_space function to further
reduce code duplication. The labels should begin with err_, like
elsewhere in in the driver.

Regards, Joonas

Patch

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;
 }