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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On to, 2017-05-11 at 06:51 +0000, Li, Weinan Z wrote:
> > 
> > -----Original Message-----
> > From: Joonas Lahtinen [mailto:joonas.lahtinen@linux.intel.com]
> > Sent: Wednesday, May 10, 2017 6:43 PM
> > To: Li, Weinan Z <weinan.z.li@intel.com>; intel-gfx@lists.freedesktop.org; intel-
> > gvt-dev@lists.freedesktop.org
> > Cc: Chris Wilson <chris@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.

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.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx