[PATCH 06/13] drm/ttm: flip over the range manager to self allocated nodes

[Thomas Hellström (Intel)]

On 5/30/21 6:51 PM, Christian König wrote:
> Hi Thomas,
>
> Am 29.05.21 um 17:48 schrieb Thomas Hellström (Intel):
>> Hi, Christian,
>>
>> On 4/30/21 11:25 AM, Christian König wrote:
>>> Start with the range manager to make the resource object the base
>>> class for the allocated nodes.
>>>
>>> While at it cleanup a lot of the code around that.
>>
>> Could you briefly describe the design thoughts around this. While 
>> it's clear to me that we want separately allocated struct 
>> ttm_resource objects, it's not clear why the visibility of those are 
>> pushed down the interfaces to the range managers?
>
> Why do you think the visibility is pushed to the range manger?
>
>>
>> In addition to the need for a separately allocated struct 
>> ttm_resource, when looking at TTM-ify i915 I've come across a couple 
>> of problems.
>>
>> 1) People have started abusing the range manager interface to attach 
>> device private data to the mm_node, or probably really to the struct 
>> ttm_resource. That makes it very unclear what the input needed for 
>> the managers really are. For examle what members of the bo does the 
>> range manager really use and why? Same for the struct ttm_resource. I 
>> think in a perfect world, the interface to these range managers 
>> should be a struct ttm_placement as input and as output an opaque mm 
>> node and perhaps a way to convert that mm node to something useful 
>> like a range or a scatter-gather table.
>
> Well I don't see that as an abuse. The backend allocation are 
> completely driver specific and the range manager is just implementing 
> some common ground for drm_mm based backends.
>
>>
>> 2) But that doesn't really address the problem of drivers wanting to 
>> attach device private data to a struct ttm_resource, which at some 
>> point caused someone to add a bo to the manager interface. The 
>> novueau driver attaches a "kind" member to the mm node that it pulls 
>> out of the bo; The i915 driver would want to cache an st table and a 
>> radix tree to cache index-to-pfn maps.
>
> Driver specific backends should inherit either from the range manager 
> when they want to implement a drm_mm based backend or from 
> ttm_resource directly.

Hmm, OK so in our case a driver that needs a driver-specific struct 
ttm_resource, but still wants to be able to allocate either from drm_mm 
or from the buddy would then either have to re-implement the TTM drm_mm 
allocator or live with a pretty awkward construct?

struct i915_ttm_resource {
     union {
         struct ttm_resource res;
         struct ttm_range_mgr_node range_node; // Let's hope the struct 
ttm_resource remains the first member.
         struct i915_buddy_node buddy_node;
     };
     struct i915_private_stuff common_for_all_backends;
};

/Thomas