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

[Thomas Hellström]

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?

If the latter, I can agree with that, but could we perhaps then work to 
find a way to turn the common manager (or in the future perhaps 
managers) into helpers that doesn't embed struct ttm_resource rather 
than a full-fledged resource manager. Then the driver will always be 
responsible for embedding the struct ttm_resource and combines helpers 
as it sees fit?

Thanks,
/Thomas