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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

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.

OT:

Having a variable size array as the last member of the range manager resource makes embedding that extremely fragile IMO. Perhaps hide that variable size functionality in the driver rather than in the common code?

/Thomas






[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux