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

[Michal Wajdeczko]

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.

>
> 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).

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

Michal