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

[Christian König]

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?

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
>
>