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
