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

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

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

With your suggestion that combination of prereqs would look like:

struct i915_resource {
	/* Reason why we subclass */
	struct i915_data my_data;

	/* 
         * Uh this is awkward. We need to do this because these       
         * already subclassed struct ttm_resource.
         */
	struct ttm_resource *resource;
	union {
		struct ttm_range_mgr_node range;
		struct i915_ttm_buddy_resource buddy;
        };
};

And I can't make it look like

struct i915_resource {
	struct i915_data my_data;
	struct ttm_resource *resource;
}

Without that private back pointer.
 
But what I'd *really* would want is.

struct i915_resource {
	struct i915_data my_data;
	struct ttm_resource resource;
};

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? 

So basically the question boils down to: Why do we do struct
ttm_resources differently?


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