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

From: "Thomas Hellström (Intel)" <thomas_os@shipmail.org>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>,
	dri-devel@lists.freedesktop.org
Cc: daniel.vetter@ffwll.ch, matthew.william.auld@gmail.com
Subject: Re: [PATCH 06/13] drm/ttm: flip over the range manager to self allocated nodes
Date: Sat, 29 May 2021 17:48:16 +0200	[thread overview]
Message-ID: <f792308b-389d-67da-0cf8-667c1e6ac96d@shipmail.org> (raw)
In-Reply-To: <20210430092508.60710-6-christian.koenig@amd.com>

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?

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.

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.

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.

So basically here

1) Would help making range managers with various functionality simple to 
write and share.
2) Would help drivers attach private data to a struct ttm_resource 
without abusing the manager interfaces,
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