[Intel-gfx] [RFC PATCH] drm/ttm: Add a private member to the struct ttm_resource

[Christian König]

Am 13.09.21 um 14:41 schrieb Thomas Hellström:
> [SNIP]
>>>> Let's say you have a struct ttm_object_vram and a struct 
>>>> ttm_object_gtt, both subclassing drm_gem_object. Then I'd say a 
>>>> driver would want to subclass those to attach identical data, 
>>>> extend functionality and provide a single i915_gem_object to the 
>>>> rest of the driver, which couldn't care less whether it's vram or 
>>>> gtt? Wouldn't you say having separate struct ttm_object_vram and a 
>>>> struct ttm_object_gtt in this case would be awkward?. We *want* to 
>>>> allow common handling.
>>>
>>> Yeah, but that's a bad idea. This is like diamond inheritance in C++.
>>>
>>> When you need the same functionality in different backends you 
>>> implement that as separate object and then add a parent class.
>>>
>>>>
>>>> It's the exact same situation here. With struct ttm_resource you 
>>>> let *different* implementation flavours subclass it, which makes it 
>>>> awkward for the driver to extend the functionality in a common way 
>>>> by subclassing, unless the driver only uses a single implementation.
>>>
>>> Well the driver should use separate implementations for their 
>>> different domains as much as possible.
>>>
>> Hmm, Now you lost me a bit. Are you saying that the way we do dynamic 
>> backends in the struct ttm_buffer_object to facilitate driver 
>> subclassing is a bad idea or that the RFC with backpointer is a bad 
>> idea?
>>
>>
> Or if you mean diamond inheritance is bad, yes that's basically my point.

That diamond inheritance is a bad idea. What I don't understand is why 
you need that in the first place?

Information that you attach to a resource are specific to the domain 
where the resource is allocated from. So why do you want to attach the 
same information to a resources from different domains?

>
> Looking at
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7Cece4bd8aab644feacc1808d976b3ca56%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637671336950757656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LPMnfvC1px0bW8o420vP72oBbkm1v76A%2B0PDUw7urQY%3D&reserved=0 
>
>
> 1)
>
> A would be the struct ttm_resource itself,
> D would be struct i915_resource,
> B would be struct ttm_range_mgr_node,
> C would be struct i915_ttm_buddy_resource
>
> And we need to resolve the ambiguity using the awkward union 
> construct, iff we need to derive from both B and C.
>
> Struct ttm_buffer_object and struct ttm_tt instead have B) and C) 
> being dynamic backends of A) or a single type derived from A) Hence 
> the problem doesn't exist for these types.
>
> So the question from last email remains, if ditching this RFC, can we 
> have B) and C) implemented by helpers that can be used from D) and 
> that don't derive from A?

Well we already have that in the form of drm_mm. I mean the 
ttm_range_manager is just a relatively small glue code which implements 
the TTMs resource interface using the drm_mm object and a spinlock. IIRC 
that less than 200 lines of code.

So you should already have the necessary helpers and just need to 
implement the resource manager as far as I can see.

I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr and 
could potentially reuse a bit more of the ttm_range_manager code. But I 
don't see that as much of an issue, the extra functionality there is 
just minimal.

Regards,
Christian.

>
> Thanks,
>
> Thomas
>
>
>