[Intel-gfx] [PATCH] drm/i915/ggtt: Move ballooning support to i915_ggtt

[Chris Wilson]

Quoting Michal Wajdeczko (2019-11-21 19:10:46)
> On Thu, 21 Nov 2019 19:05:25 +0100, Chris Wilson  
> <chris at chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-11-21 17:58:50)
> >> Today VGT implements GGTT ballooning on its own, using some private
> >> static structure to hold info about reserved GGTT nodes and then
> >> manually update vm.reserved size owned by i915_ggtt.
> >>
> >> As i915_ggtt already manages some custom reserved nodes (like uc_fw)
> >> move VGT ballooning support to i915_ggtt and make it more generic
> >> for possible future reuse in other scenarios.
> >>
> >> As a bonus we can drop another place in driver that uses static data.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Cc: Xiong Zhang <xiong.y.zhang at intel.com>
> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> Cc: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_gem_gtt.c | 47 +++++++++++++++++++
> >>  drivers/gpu/drm/i915/i915_gem_gtt.h | 18 ++++++++
> >>  drivers/gpu/drm/i915/i915_vgpu.c    | 72 ++++++-----------------------
> >>  3 files changed, 78 insertions(+), 59 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c  
> >> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> index 6239a9adbf14..d8b2084dab7e 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >> @@ -3827,6 +3827,53 @@ int i915_gem_gtt_insert(struct  
> >> i915_address_space *vm,
> >>                                            start, end,  
> >> DRM_MM_INSERT_EVICT);
> >>  }
> >>
> >> +int i915_ggtt_balloon_space(struct i915_ggtt *ggtt,
> >> +                           enum i915_ggtt_balloon_id id,
> >
> > Do we want to use a fixed number of slots?
> 
> Probably not, enum was just used as a starting point to provide explicit
> name each of the available node and to avoid adding GEM_BUG_ON while
> accessing fixed size array.
> 
> >
> > So what exactly is wrong with the clients reserving nodes themselves?
> 
> They still reserve nodes themselves. But now they will not do that
> in a shadow and not use static data to hold actual nodes.

But that's their decision, all we provide is the service to dole out the
ggtt (or not depending on our whim). If they use a static allocation for
their nodes and not kmalloc -- we don't care, why should we as long
as their code is correct?

> > I'm not seeing why exactly we want to move to a much more centralised
> > system
> 
> I wanted to make it clear to all clients that i915_ggtt is a good place
> to keep list of reserved some (instead of hiding them in private places)
> 
> And to provide wrapper function that can be easily reused and that each
> ballooning behaves in the same way.
> 
> Extra bonus is that we have all such balloon tweaks in one place (both
> code and data nodes).

So I think you would be happy with just a 

struct i915_ggtt_balloon (or whatever) {
	struct dma_mm_node node;
	struct list_head link;
	void *private;
} *i915_ggtt_reserve_balloon(i915/ggtt, u64 bottom, u64 top);

... but you are already past the point where you might as well just let
the client allocate their own struct and pass us the drm_mm_node. So

i915_ggtt_reserve(i915/ggtt, drm_mm_node, u64 bottom, u64 top, flags?)

> > and argue over ownership of i915_ggtt_balloon_id.
> 
> Now it's first come, first served order, but we should already know
> all current usages and in case of clash either pick different id or
> add the new one.
> 
> Note that today some scenarios are exclusive (gvt won't work with guc)
> so some node placeholders can be shared - but this can be equally done
> without enums but with union of explicit node definitions.
> 
>         I915_GGTT_BALLOON_GUC_TOP = I915_GGTT_BALLOON3,
>         I915_GGTT_BALLOON_VGT_UNMAP_TOP = I915_GGTT_BALLOON3,
> 
>         union {
>                 struct {
>                         struct drm_mm_node uc_fw;
>                 } guc;
>                 struct {
>                         struct drm_mm_node space[4];
>                 } vgt;
>         } balloons;
> 
> but direct access to nodes can be also wrapped into:
> 
> struct drm_mm_node *i915_ggtt_get_uc_fw_node(i915_ggtt *ggtt)
> {
>         return ggtt->balloon[I915_GGTT_BALLOON_GUC_TOP];
> }

I am not yet sold on putting an array of nodes here, we already have a
list of them all :-p
-Chris