Am 11.09.21 um 08:07 schrieb Thomas Hellström:
On Fri, 2021-09-10 at 19:03 +0200, Christian König wrote:
Am 10.09.21 um 17:30 schrieb Thomas Hellström:
On Fri, 2021-09-10 at 16:40 +0200, Christian König wrote:
Am 10.09.21 um 15:15 schrieb Thomas Hellström:
Both the provider (resource manager) and the consumer (the TTM driver) want to subclass struct ttm_resource. Since this is left for the resource manager, we need to provide a private pointer for the TTM driver.
Provide a struct ttm_resource_private for the driver to subclass for data with the same lifetime as the struct ttm_resource: In the i915 case it will, for example, be an sg-table and radix tree into the LMEM /VRAM pages that currently are awkwardly attached to the GEM object.
Provide an ops structure for associated ops (Which is only destroy() ATM) It might seem pointless to provide a separate ops structure, but Linus has previously made it clear that that's the norm.
After careful audit one could perhaps also on a per-driver basis replace the delete_mem_notify() TTM driver callback with the above destroy function.
Well this is a really big NAK to this approach.
If you need to attach some additional information to the resource then implement your own resource manager like everybody else does.
Well this was the long discussion we had back then when the resource mangagers started to derive from struct resource and I was under the impression that we had come to an agreement about the different use- cases here, and this was my main concern.
Ok, then we somehow didn't understood each other.
I mean, it's a pretty big layer violation to do that for this use- case.
Well exactly that's the point. TTM should not have a layer design in the first place.
Devices, BOs, resources etc.. are base classes which should implement a base functionality which is then extended by the drivers to implement the driver specific functionality.
That is a component based approach, and not layered at all.
The TTM resource manager doesn't want to know about this data at all, it's private to the ttm resource user layer and the resource manager works perfectly well without it. (I assume the other drivers that implement their own resource managers need the data that the subclassing provides?)
Yes, that's exactly why we have the subclassing.
The fundamental problem here is that there are two layers wanting to subclass struct ttm_resource. That means one layer gets to do that, the second gets to use a private pointer, (which in turn can provide yet another private pointer to a potential third layer). With your suggestion, the second layer instead is forced to subclass each subclassed instance it uses from the first layer provides?
Well completely drop the layer approach/thinking here.
The resource is an object with a base class. The base class implements the interface TTM needs to handle the object, e.g. create/destroy/debug etc...
Then we need to subclass this object because without any additional information the object is pretty pointless.
One possibility for this is to use the range manager to implement something drm_mm based. BTW: We should probably rename that to something like ttm_res_drm_mm or similar.
Sure I'm all in on that, but my point is this becomes pretty awkward because the reusable code already subclasses struct ttm_resource. Let me give you an example:
Prereqs:
- We want to be able to re-use resource manager implementations among
drivers. 2) A driver might want to re-use multiple implementations and have identical data "struct i915_data" attached to both
Well that's the point I don't really understand. Why would a driver want to do this?
It's perfectly possible that you have ttm_range_manager extended and a potential ttm_page_manager, but that are two different objects then which also need different handling.
.... This would be identical to how we subclass a struct ttm_buffer_object or a struct ttm_tt. But It can't look like this because then we can't reuse exising implementations that *already subclass* struct ttm_resource.
What we have currently ttm_resource-wise is like having a struct tt_bo_vram, a struct ttm_bo_system, a struct ttm_bo_gtt and trying to subclass them all combined into a struct i915_bo. It would become awkward without a dynamic backend that facilitates subclassing a single struct ttm_buffer_object?
Why? They all implement different handling.
When you add a private point to ttm_resource you allow common handling which doesn't take into account that this ttm_resource object is subclassed.
So basically the question boils down to: Why do we do struct ttm_resources differently?
ttm_buffer_object is a subclass of drm_gem_object and I hope to make ttm_device a subclass of drm_device in the near term.
I really try to understand what you mean hear, but I even after reading that multiple times I absolutely don't get it.
Regards, Christian.
What we should avoid is to abuse TTM resource interfaces in the driver, e.g. what i915 is currently doing. This is a TTM->resource mgr interface and should not be used by drivers at all.
Yes I guess that can be easily fixed when whatever we end up with above lands.
Ofc we can do that, but it does indeed feel pretty awkward.
In any case, if you still think that's the approach we should go for, I'd need to add init() and fini() members to the ttm_range_manager_func struct to allow subclassing without having to unnecessarily copy the full code?
Yes, exporting the ttm_range_manager functions as needed is one thing I wanted to do for the amdgpu_gtt_mgr.c code as well.
Just don't extend the function table but rather directly export the necessary functions.
Sure. /Thomas