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

[Thomas Hellström]

On 9/13/21 12:16 PM, Thomas Hellström wrote:
>
> On 9/13/21 11:41 AM, Christian König wrote:
>> Am 13.09.21 um 11:36 schrieb Thomas Hellström:
>>> On 9/13/21 8:17 AM, Christian König wrote:
>>>> 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:
>>>>> 1) 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?
>>>
>>> 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.

Looking at
https://en.wikipedia.org/wiki/Multiple_inheritance#/media/File:Diamond_inheritance.svg

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?

Thanks,

Thomas