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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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.

>
> 3) In the end we'd probably want the kmap iterator methods and the 
> various mapping funtions to be methods of the struct ttm_resource.

Yes moving the iterators into that was also my idea.

>
> So basically here
>
> 1) Would help making range managers with various functionality simple 
> to write and share.

I don't think we want that. Instead allocation backends should be driver 
specific and we should just implement some common ground helpers for 
drm_mm based backends in TTM.

> 2) Would help drivers attach private data to a struct ttm_resource 
> without abusing the manager interfaces,

I don't think that this is abusive in the first place. Drivers should 
append resource specific information by inheriting from the ttm_resource 
object.

Regards,
Christian.

> 3) Would help clean up the mapping code.
>
> But at least 2) here would probably mean that we need a driver 
> callback to allocate a struct ttm_resource rather than having the 
> managers allocate them. Drivers can then embed them in private structs 
> if needed.
>
> Or is there a way to achieve these goals or something similar with the 
> approach you are taking here?
>
> Thanks,
>
> Thomas
>
>
>