| Message ID | 20210910131512.161655-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/ttm: Add a private member to the struct ttm_resource | expand |
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. Regards, Christian. > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > Cc: König Christian <Christian.Koenig@amd.com> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > --- > drivers/gpu/drm/ttm/ttm_resource.c | 10 +++++++--- > include/drm/ttm/ttm_resource.h | 28 ++++++++++++++++++++++++++++ > 2 files changed, 35 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c > index 2431717376e7..973e7c50bfed 100644 > --- a/drivers/gpu/drm/ttm/ttm_resource.c > +++ b/drivers/gpu/drm/ttm/ttm_resource.c > @@ -57,13 +57,17 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo, > void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res) > { > struct ttm_resource_manager *man; > + struct ttm_resource *resource = *res; > > - if (!*res) > + if (!resource) > return; > > - man = ttm_manager_type(bo->bdev, (*res)->mem_type); > - man->func->free(man, *res); > *res = NULL; > + if (resource->priv) > + resource->priv->ops.destroy(resource->priv); > + > + man = ttm_manager_type(bo->bdev, resource->mem_type); > + man->func->free(man, resource); > } > EXPORT_SYMBOL(ttm_resource_free); > > diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h > index 140b6b9a8bbe..5a22c9a29c05 100644 > --- a/include/drm/ttm/ttm_resource.h > +++ b/include/drm/ttm/ttm_resource.h > @@ -44,6 +44,7 @@ struct dma_buf_map; > struct io_mapping; > struct sg_table; > struct scatterlist; > +struct ttm_resource_private; > > struct ttm_resource_manager_func { > /** > @@ -153,6 +154,32 @@ struct ttm_bus_placement { > enum ttm_caching caching; > }; > > +/** > + * struct ttm_resource_private_ops - Operations for a struct > + * ttm_resource_private > + * > + * Not much benefit to keep this as a separate struct with only a single member, > + * but keeping a separate ops struct is the norm. > + */ > +struct ttm_resource_private_ops { > + /** > + * destroy() - Callback to destroy the private data > + * @priv - The private data to destroy > + */ > + void (*destroy) (struct ttm_resource_private *priv); > +}; > + > +/** > + * struct ttm_resource_private - TTM driver private data > + * @ops: Pointer to struct ttm_resource_private_ops with associated operations > + * > + * Intended to be subclassed to hold, for example cached data sharing the > + * lifetime with a struct ttm_resource. > + */ > +struct ttm_resource_private { > + const struct ttm_resource_private_ops ops; > +}; > + > /** > * struct ttm_resource > * > @@ -171,6 +198,7 @@ struct ttm_resource { > uint32_t mem_type; > uint32_t placement; > struct ttm_bus_placement bus; > + struct ttm_resource_private *priv; > }; > > /**
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. I mean, it's a pretty big layer violation to do that for this use-case. 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?) 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? 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? Thanks, Thomas > > Regards, > Christian. > > > > > Cc: Matthew Auld <matthew.william.auld@gmail.com> > > Cc: König Christian <Christian.Koenig@amd.com> > > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> > > --- > > drivers/gpu/drm/ttm/ttm_resource.c | 10 +++++++--- > > include/drm/ttm/ttm_resource.h | 28 > > ++++++++++++++++++++++++++++ > > 2 files changed, 35 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c > > b/drivers/gpu/drm/ttm/ttm_resource.c > > index 2431717376e7..973e7c50bfed 100644 > > --- a/drivers/gpu/drm/ttm/ttm_resource.c > > +++ b/drivers/gpu/drm/ttm/ttm_resource.c > > @@ -57,13 +57,17 @@ int ttm_resource_alloc(struct ttm_buffer_object > > *bo, > > void ttm_resource_free(struct ttm_buffer_object *bo, struct > > ttm_resource **res) > > { > > struct ttm_resource_manager *man; > > + struct ttm_resource *resource = *res; > > > > - if (!*res) > > + if (!resource) > > return; > > > > - man = ttm_manager_type(bo->bdev, (*res)->mem_type); > > - man->func->free(man, *res); > > *res = NULL; > > + if (resource->priv) > > + resource->priv->ops.destroy(resource->priv); > > + > > + man = ttm_manager_type(bo->bdev, resource->mem_type); > > + man->func->free(man, resource); > > } > > EXPORT_SYMBOL(ttm_resource_free); > > > > diff --git a/include/drm/ttm/ttm_resource.h > > b/include/drm/ttm/ttm_resource.h > > index 140b6b9a8bbe..5a22c9a29c05 100644 > > --- a/include/drm/ttm/ttm_resource.h > > +++ b/include/drm/ttm/ttm_resource.h > > @@ -44,6 +44,7 @@ struct dma_buf_map; > > struct io_mapping; > > struct sg_table; > > struct scatterlist; > > +struct ttm_resource_private; > > > > struct ttm_resource_manager_func { > > /** > > @@ -153,6 +154,32 @@ struct ttm_bus_placement { > > enum ttm_caching caching; > > }; > > > > +/** > > + * struct ttm_resource_private_ops - Operations for a struct > > + * ttm_resource_private > > + * > > + * Not much benefit to keep this as a separate struct with only a > > single member, > > + * but keeping a separate ops struct is the norm. > > + */ > > +struct ttm_resource_private_ops { > > + /** > > + * destroy() - Callback to destroy the private data > > + * @priv - The private data to destroy > > + */ > > + void (*destroy) (struct ttm_resource_private *priv); > > +}; > > + > > +/** > > + * struct ttm_resource_private - TTM driver private data > > + * @ops: Pointer to struct ttm_resource_private_ops with > > associated operations > > + * > > + * Intended to be subclassed to hold, for example cached data > > sharing the > > + * lifetime with a struct ttm_resource. > > + */ > > +struct ttm_resource_private { > > + const struct ttm_resource_private_ops ops; > > +}; > > + > > /** > > * struct ttm_resource > > * > > @@ -171,6 +198,7 @@ struct ttm_resource { > > uint32_t mem_type; > > uint32_t placement; > > struct ttm_bus_placement bus; > > + struct ttm_resource_private *priv; > > }; > > > > /** >
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. 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. > 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. Regards, Christian. > > Thanks, > Thomas > > > > > > > > > > >> Regards, >> Christian. >> >>> Cc: Matthew Auld <matthew.william.auld@gmail.com> >>> Cc: König Christian <Christian.Koenig@amd.com> >>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> >>> --- >>> drivers/gpu/drm/ttm/ttm_resource.c | 10 +++++++--- >>> include/drm/ttm/ttm_resource.h | 28 >>> ++++++++++++++++++++++++++++ >>> 2 files changed, 35 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c >>> b/drivers/gpu/drm/ttm/ttm_resource.c >>> index 2431717376e7..973e7c50bfed 100644 >>> --- a/drivers/gpu/drm/ttm/ttm_resource.c >>> +++ b/drivers/gpu/drm/ttm/ttm_resource.c >>> @@ -57,13 +57,17 @@ int ttm_resource_alloc(struct ttm_buffer_object >>> *bo, >>> void ttm_resource_free(struct ttm_buffer_object *bo, struct >>> ttm_resource **res) >>> { >>> struct ttm_resource_manager *man; >>> + struct ttm_resource *resource = *res; >>> >>> - if (!*res) >>> + if (!resource) >>> return; >>> >>> - man = ttm_manager_type(bo->bdev, (*res)->mem_type); >>> - man->func->free(man, *res); >>> *res = NULL; >>> + if (resource->priv) >>> + resource->priv->ops.destroy(resource->priv); >>> + >>> + man = ttm_manager_type(bo->bdev, resource->mem_type); >>> + man->func->free(man, resource); >>> } >>> EXPORT_SYMBOL(ttm_resource_free); >>> >>> diff --git a/include/drm/ttm/ttm_resource.h >>> b/include/drm/ttm/ttm_resource.h >>> index 140b6b9a8bbe..5a22c9a29c05 100644 >>> --- a/include/drm/ttm/ttm_resource.h >>> +++ b/include/drm/ttm/ttm_resource.h >>> @@ -44,6 +44,7 @@ struct dma_buf_map; >>> struct io_mapping; >>> struct sg_table; >>> struct scatterlist; >>> +struct ttm_resource_private; >>> >>> struct ttm_resource_manager_func { >>> /** >>> @@ -153,6 +154,32 @@ struct ttm_bus_placement { >>> enum ttm_caching caching; >>> }; >>> >>> +/** >>> + * struct ttm_resource_private_ops - Operations for a struct >>> + * ttm_resource_private >>> + * >>> + * Not much benefit to keep this as a separate struct with only a >>> single member, >>> + * but keeping a separate ops struct is the norm. >>> + */ >>> +struct ttm_resource_private_ops { >>> + /** >>> + * destroy() - Callback to destroy the private data >>> + * @priv - The private data to destroy >>> + */ >>> + void (*destroy) (struct ttm_resource_private *priv); >>> +}; >>> + >>> +/** >>> + * struct ttm_resource_private - TTM driver private data >>> + * @ops: Pointer to struct ttm_resource_private_ops with >>> associated operations >>> + * >>> + * Intended to be subclassed to hold, for example cached data >>> sharing the >>> + * lifetime with a struct ttm_resource. >>> + */ >>> +struct ttm_resource_private { >>> + const struct ttm_resource_private_ops ops; >>> +}; >>> + >>> /** >>> * struct ttm_resource >>> * >>> @@ -171,6 +198,7 @@ struct ttm_resource { >>> uint32_t mem_type; >>> uint32_t placement; >>> struct ttm_bus_placement bus; >>> + struct ttm_resource_private *priv; >>> }; >>> >>> /** >
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
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 > >
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
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. > 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? Yeah, Arun is already working on that. It's just not finished yet. Regards, Christian. > > > /Thomas > > >
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
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
Am 13.09.21 um 14:41 schrieb Thomas Hellström: > [SNIP] >>>> 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. That diamond inheritance is a bad idea. What I don't understand is why you need that in the first place? Information that you attach to a resource are specific to the domain where the resource is allocated from. So why do you want to attach the same information to a resources from different domains? > > Looking at > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7Cece4bd8aab644feacc1808d976b3ca56%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637671336950757656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LPMnfvC1px0bW8o420vP72oBbkm1v76A%2B0PDUw7urQY%3D&reserved=0 > > > 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? Well we already have that in the form of drm_mm. I mean the ttm_range_manager is just a relatively small glue code which implements the TTMs resource interface using the drm_mm object and a spinlock. IIRC that less than 200 lines of code. So you should already have the necessary helpers and just need to implement the resource manager as far as I can see. I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr and could potentially reuse a bit more of the ttm_range_manager code. But I don't see that as much of an issue, the extra functionality there is just minimal. Regards, Christian. > > Thanks, > > Thomas > > >
On Tue, 2021-09-14 at 09:40 +0200, Christian König wrote: > Am 13.09.21 um 14:41 schrieb Thomas Hellström: > > [SNIP] > > > > > 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. > > That diamond inheritance is a bad idea. What I don't understand is > why > you need that in the first place? > > Information that you attach to a resource are specific to the domain > where the resource is allocated from. So why do you want to attach > the > same information to a resources from different domains? Again, for the same reason that we do that with struct i915_gem_objects and struct ttm_tts, to extend the functionality. I mean information that we attach when we subclass a struct ttm_buffer_object doesn't necessarily care about whether it's a VRAM or a GTT object. In exactly the same way, information that we want to attach to a struct ttm_resource doesn't necessarily care whether it's a system or a VRAM resource, and need not be specific to any of those. In this particular case, as memory management becomes asynchronous, you can't attach things like sg-tables and gpu binding information to the gem object anymore, because the object may have a number of migrations in the pipeline. Such things need to be attached to the structure that abstracts the memory allocation, and which may have a completely different lifetime than the object itself. In our particular case we want to attach information for cached page lookup and and sg-table, and moving forward probably the gpu binding (vma) information, and that is the same information for any ttm_resource regardless where it's allocated from. Typical example: A pipelined GPU operation happening before an async eviction goes wrong. We need to error capture and reset. But if we look at the object for error capturing, it's already updated pointing to an after-eviction resource, and the resource sits on a ghost object (or in the future when ghost objects go away perhaps in limbo somewhere). We need to capture the memory pointed to by the struct ttm_resource the GPU was referencing, and to be able to do that we need to cache driver- specific info on the resource. Typically an sg-list and GPU binding information. Anyway, that cached information needs to be destroyed together with the resource and thus we need to be able to access that information from the resource in some way, regardless whether it's a pointer or whether we embed the struct resource. I think it's pretty important here that we (using the inheritance diagram below) recognize the need for D to inherit from A, just like we do for objects or ttm_tts. > > > > > Looking at > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7Cece4bd8aab644feacc1808d976b3ca56%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637671336950757656%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=LPMnfvC1px0bW8o420vP72oBbkm1v76A%2B0PDUw7urQY%3D&reserved=0 > > > > > > > > 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? > > Well we already have that in the form of drm_mm. I mean the > ttm_range_manager is just a relatively small glue code which > implements > the TTMs resource interface using the drm_mm object and a spinlock. > IIRC > that less than 200 lines of code. > > So you should already have the necessary helpers and just need to > implement the resource manager as far as I can see. > > I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr and > could potentially reuse a bit more of the ttm_range_manager code. But > I > don't see that as much of an issue, the extra functionality there is > just minimal. Sure but that would give up the prereq of having reusable resource manager implementations. What happens if someone would like to reuse the buddy manager? And to complicate things even more, the information we attach to VRAM resources also needs to be attached to system resources. Sure we could probably re-implement a combined system-buddy- range manager, but that seems like something overly complex. The other object examples resolve the diamond inheritance with a pointer to the specialization (BC) and let D derive from A. TTM resources do it backwards. If we can just recognize that and ponder what's the easiest way to resolve this given the current design, I actually think we'd arrive at a backpointer to allow downcasting from A to D. Thanks, Thomas > > Regards, > Christian. > > > > > Thanks, > > > > Thomas > > > > > > >
Am 14.09.21 um 10:27 schrieb Thomas Hellström: > On Tue, 2021-09-14 at 09:40 +0200, Christian König wrote: >> Am 13.09.21 um 14:41 schrieb Thomas Hellström: >>> [SNIP] >>>>>> 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. >> That diamond inheritance is a bad idea. What I don't understand is >> why >> you need that in the first place? >> >> Information that you attach to a resource are specific to the domain >> where the resource is allocated from. So why do you want to attach >> the >> same information to a resources from different domains? > Again, for the same reason that we do that with struct i915_gem_objects > and struct ttm_tts, to extend the functionality. I mean information > that we attach when we subclass a struct ttm_buffer_object doesn't > necessarily care about whether it's a VRAM or a GTT object. In exactly > the same way, information that we want to attach to a struct > ttm_resource doesn't necessarily care whether it's a system or a VRAM > resource, and need not be specific to any of those. > > In this particular case, as memory management becomes asynchronous, you > can't attach things like sg-tables and gpu binding information to the > gem object anymore, because the object may have a number of migrations > in the pipeline. Such things need to be attached to the structure that > abstracts the memory allocation, and which may have a completely > different lifetime than the object itself. > > In our particular case we want to attach information for cached page > lookup and and sg-table, and moving forward probably the gpu binding > (vma) information, and that is the same information for any > ttm_resource regardless where it's allocated from. > > Typical example: A pipelined GPU operation happening before an async > eviction goes wrong. We need to error capture and reset. But if we look > at the object for error capturing, it's already updated pointing to an > after-eviction resource, and the resource sits on a ghost object (or in > the future when ghost objects go away perhaps in limbo somewhere). > > We need to capture the memory pointed to by the struct ttm_resource the > GPU was referencing, and to be able to do that we need to cache driver- > specific info on the resource. Typically an sg-list and GPU binding > information. > > Anyway, that cached information needs to be destroyed together with the > resource and thus we need to be able to access that information from > the resource in some way, regardless whether it's a pointer or whether > we embed the struct resource. > > I think it's pretty important here that we (using the inheritance > diagram below) recognize the need for D to inherit from A, just like we > do for objects or ttm_tts. > > >>> Looking at >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7C268bb562db8548b285b408d977598b2c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672048739103176%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bPyDqiSF%2FHFZbl74ux0vfwh3uma5hZIUf2xbzb9yZz8%3D&reserved=0 >>> >>> >>> >>> 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? >> Well we already have that in the form of drm_mm. I mean the >> ttm_range_manager is just a relatively small glue code which >> implements >> the TTMs resource interface using the drm_mm object and a spinlock. >> IIRC >> that less than 200 lines of code. >> >> So you should already have the necessary helpers and just need to >> implement the resource manager as far as I can see. >> >> I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr and >> could potentially reuse a bit more of the ttm_range_manager code. But >> I >> don't see that as much of an issue, the extra functionality there is >> just minimal. > Sure but that would give up the prereq of having reusable resource > manager implementations. What happens if someone would like to reuse > the buddy manager? And to complicate things even more, the information > we attach to VRAM resources also needs to be attached to system > resources. Sure we could probably re-implement a combined system-buddy- > range manager, but that seems like something overly complex. > > The other object examples resolve the diamond inheritance with a > pointer to the specialization (BC) and let D derive from A. > > TTM resources do it backwards. If we can just recognize that and ponder > what's the easiest way to resolve this given the current design, I > actually think we'd arrive at a backpointer to allow downcasting from A > to D. Yeah, but I think you are approaching that from the wrong side. For use cases like this I think you should probably have the following objects and inheritances: 1. Driver specific objects like i915_sg, i915_vma which don't inherit anything from TTM. 2. i915_vram_node which inherits from ttm_resource or a potential ttm_buddy_allocator. 3. i915_gtt_node which inherits from ttm_range_manger_node. 4. Maybe i915_sys_node which inherits from ttm_resource as well. The managers for the individual domains then provide the glue code to implement both the TTM resource interface as well as a driver specific interface to access the driver objects. Amdgpu just uses a switch/case for now, but you could as well extend the ttm_resource_manager_func table and upcast that inside the driver. Regards, Christian. > > Thanks, > Thomas > > > >> Regards, >> Christian. >> >>> Thanks, >>> >>> Thomas >>> >>> >>> >
On Tue, 2021-09-14 at 10:53 +0200, Christian König wrote: > Am 14.09.21 um 10:27 schrieb Thomas Hellström: > > On Tue, 2021-09-14 at 09:40 +0200, Christian König wrote: > > > Am 13.09.21 um 14:41 schrieb Thomas Hellström: > > > > [SNIP] > > > > > > > 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. > > > That diamond inheritance is a bad idea. What I don't understand > > > is > > > why > > > you need that in the first place? > > > > > > Information that you attach to a resource are specific to the > > > domain > > > where the resource is allocated from. So why do you want to > > > attach > > > the > > > same information to a resources from different domains? > > Again, for the same reason that we do that with struct > > i915_gem_objects > > and struct ttm_tts, to extend the functionality. I mean information > > that we attach when we subclass a struct ttm_buffer_object doesn't > > necessarily care about whether it's a VRAM or a GTT object. In > > exactly > > the same way, information that we want to attach to a struct > > ttm_resource doesn't necessarily care whether it's a system or a > > VRAM > > resource, and need not be specific to any of those. > > > > In this particular case, as memory management becomes asynchronous, > > you > > can't attach things like sg-tables and gpu binding information to > > the > > gem object anymore, because the object may have a number of > > migrations > > in the pipeline. Such things need to be attached to the structure > > that > > abstracts the memory allocation, and which may have a completely > > different lifetime than the object itself. > > > > In our particular case we want to attach information for cached > > page > > lookup and and sg-table, and moving forward probably the gpu > > binding > > (vma) information, and that is the same information for any > > ttm_resource regardless where it's allocated from. > > > > Typical example: A pipelined GPU operation happening before an > > async > > eviction goes wrong. We need to error capture and reset. But if we > > look > > at the object for error capturing, it's already updated pointing to > > an > > after-eviction resource, and the resource sits on a ghost object > > (or in > > the future when ghost objects go away perhaps in limbo somewhere). > > > > We need to capture the memory pointed to by the struct ttm_resource > > the > > GPU was referencing, and to be able to do that we need to cache > > driver- > > specific info on the resource. Typically an sg-list and GPU binding > > information. > > > > Anyway, that cached information needs to be destroyed together with > > the > > resource and thus we need to be able to access that information > > from > > the resource in some way, regardless whether it's a pointer or > > whether > > we embed the struct resource. > > > > I think it's pretty important here that we (using the inheritance > > diagram below) recognize the need for D to inherit from A, just > > like we > > do for objects or ttm_tts. > > > > > > > > Looking at > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7C268bb562db8548b285b408d977598b2c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672048739103176%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bPyDqiSF%2FHFZbl74ux0vfwh3uma5hZIUf2xbzb9yZz8%3D&reserved=0 > > > > > > > > > > > > > > > > 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? > > > Well we already have that in the form of drm_mm. I mean the > > > ttm_range_manager is just a relatively small glue code which > > > implements > > > the TTMs resource interface using the drm_mm object and a > > > spinlock. > > > IIRC > > > that less than 200 lines of code. > > > > > > So you should already have the necessary helpers and just need to > > > implement the resource manager as far as I can see. > > > > > > I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr > > > and > > > could potentially reuse a bit more of the ttm_range_manager code. > > > But > > > I > > > don't see that as much of an issue, the extra functionality there > > > is > > > just minimal. > > Sure but that would give up the prereq of having reusable resource > > manager implementations. What happens if someone would like to > > reuse > > the buddy manager? And to complicate things even more, the > > information > > we attach to VRAM resources also needs to be attached to system > > resources. Sure we could probably re-implement a combined system- > > buddy- > > range manager, but that seems like something overly complex. > > > > The other object examples resolve the diamond inheritance with a > > pointer to the specialization (BC) and let D derive from A. > > > > TTM resources do it backwards. If we can just recognize that and > > ponder > > what's the easiest way to resolve this given the current design, I > > actually think we'd arrive at a backpointer to allow downcasting > > from A > > to D. > > Yeah, but I think you are approaching that from the wrong side. > > For use cases like this I think you should probably have the > following > objects and inheritances: > > 1. Driver specific objects like i915_sg, i915_vma which don't inherit > anything from TTM. > 2. i915_vram_node which inherits from ttm_resource or a potential > ttm_buddy_allocator. > 3. i915_gtt_node which inherits from ttm_range_manger_node. > 4. Maybe i915_sys_node which inherits from ttm_resource as well. > > The managers for the individual domains then provide the glue code to > implement both the TTM resource interface as well as a driver > specific > interface to access the driver objects. Well yes, but this is not really much better than the union thing. More memory efficient but also more duplicated type definitions and manager definitions and in addition overriding the default system resource manager, not counting the kerneldoc needed to explain why all this is necessary. It was this complexity I was trying to get away from in the first place. /Thomas > Amdgpu just uses a switch/case for now, but you could as well extend > the > ttm_resource_manager_func table and upcast that inside the driver. > > Regards, > Christian. > > > > > Thanks, > > Thomas > > > > > > > > > Regards, > > > Christian. > > > > > > > Thanks, > > > > > > > > Thomas > > > > > > > > > > > > > > >
On Tue, Sep 14, 2021 at 12:38:00PM +0200, Thomas Hellström wrote: > On Tue, 2021-09-14 at 10:53 +0200, Christian König wrote: > > Am 14.09.21 um 10:27 schrieb Thomas Hellström: > > > On Tue, 2021-09-14 at 09:40 +0200, Christian König wrote: > > > > Am 13.09.21 um 14:41 schrieb Thomas Hellström: > > > > > [SNIP] > > > > > > > > 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. > > > > That diamond inheritance is a bad idea. What I don't understand > > > > is > > > > why > > > > you need that in the first place? > > > > > > > > Information that you attach to a resource are specific to the > > > > domain > > > > where the resource is allocated from. So why do you want to > > > > attach > > > > the > > > > same information to a resources from different domains? > > > Again, for the same reason that we do that with struct > > > i915_gem_objects > > > and struct ttm_tts, to extend the functionality. I mean information > > > that we attach when we subclass a struct ttm_buffer_object doesn't > > > necessarily care about whether it's a VRAM or a GTT object. In > > > exactly > > > the same way, information that we want to attach to a struct > > > ttm_resource doesn't necessarily care whether it's a system or a > > > VRAM > > > resource, and need not be specific to any of those. > > > > > > In this particular case, as memory management becomes asynchronous, > > > you > > > can't attach things like sg-tables and gpu binding information to > > > the > > > gem object anymore, because the object may have a number of > > > migrations > > > in the pipeline. Such things need to be attached to the structure > > > that > > > abstracts the memory allocation, and which may have a completely > > > different lifetime than the object itself. > > > > > > In our particular case we want to attach information for cached > > > page > > > lookup and and sg-table, and moving forward probably the gpu > > > binding > > > (vma) information, and that is the same information for any > > > ttm_resource regardless where it's allocated from. > > > > > > Typical example: A pipelined GPU operation happening before an > > > async > > > eviction goes wrong. We need to error capture and reset. But if we > > > look > > > at the object for error capturing, it's already updated pointing to > > > an > > > after-eviction resource, and the resource sits on a ghost object > > > (or in > > > the future when ghost objects go away perhaps in limbo somewhere). > > > > > > We need to capture the memory pointed to by the struct ttm_resource > > > the > > > GPU was referencing, and to be able to do that we need to cache > > > driver- > > > specific info on the resource. Typically an sg-list and GPU binding > > > information. > > > > > > Anyway, that cached information needs to be destroyed together with > > > the > > > resource and thus we need to be able to access that information > > > from > > > the resource in some way, regardless whether it's a pointer or > > > whether > > > we embed the struct resource. > > > > > > I think it's pretty important here that we (using the inheritance > > > diagram below) recognize the need for D to inherit from A, just > > > like we > > > do for objects or ttm_tts. > > > > > > > > > > > Looking at > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7C268bb562db8548b285b408d977598b2c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672048739103176%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bPyDqiSF%2FHFZbl74ux0vfwh3uma5hZIUf2xbzb9yZz8%3D&reserved=0 > > > > > > > > > > > > > > > > > > > > 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? > > > > Well we already have that in the form of drm_mm. I mean the > > > > ttm_range_manager is just a relatively small glue code which > > > > implements > > > > the TTMs resource interface using the drm_mm object and a > > > > spinlock. > > > > IIRC > > > > that less than 200 lines of code. > > > > > > > > So you should already have the necessary helpers and just need to > > > > implement the resource manager as far as I can see. > > > > > > > > I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr > > > > and > > > > could potentially reuse a bit more of the ttm_range_manager code. > > > > But > > > > I > > > > don't see that as much of an issue, the extra functionality there > > > > is > > > > just minimal. > > > Sure but that would give up the prereq of having reusable resource > > > manager implementations. What happens if someone would like to > > > reuse > > > the buddy manager? And to complicate things even more, the > > > information > > > we attach to VRAM resources also needs to be attached to system > > > resources. Sure we could probably re-implement a combined system- > > > buddy- > > > range manager, but that seems like something overly complex. > > > > > > The other object examples resolve the diamond inheritance with a > > > pointer to the specialization (BC) and let D derive from A. > > > > > > TTM resources do it backwards. If we can just recognize that and > > > ponder > > > what's the easiest way to resolve this given the current design, I > > > actually think we'd arrive at a backpointer to allow downcasting > > > from A > > > to D. > > > > Yeah, but I think you are approaching that from the wrong side. > > > > For use cases like this I think you should probably have the > > following > > objects and inheritances: > > > > 1. Driver specific objects like i915_sg, i915_vma which don't inherit > > anything from TTM. > > 2. i915_vram_node which inherits from ttm_resource or a potential > > ttm_buddy_allocator. > > 3. i915_gtt_node which inherits from ttm_range_manger_node. > > 4. Maybe i915_sys_node which inherits from ttm_resource as well. > > > > The managers for the individual domains then provide the glue code to > > implement both the TTM resource interface as well as a driver > > specific > > interface to access the driver objects. > > Well yes, but this is not really much better than the union thing. More > memory efficient but also more duplicated type definitions and manager > definitions and in addition overriding the default system resource > manager, not counting the kerneldoc needed to explain why all this is > necessary. > > It was this complexity I was trying to get away from in the first > place. I honestly don't think the union thing is the worst. At least as long as we're reworking i915 at a fairly invasive pace it's probably the lest worst approach. For the specific case of sg list I'm also not sure how great our current i915 design of "everything is an sg" really is. In the wider community there's clear rejection of sg for p2p addresses, so having this as a per-ttm_res_manager kind of situation is probably not the worst. In that world every ttm_res_manager would have it's own implementation of binding into ptes, which then iterate over the pagetables with some common abstraction. So in a way more of a helper approach for the i915 implementations of the various hooks, at the cost of a bit of code duplication. I do agree with Christian that the various backpointers to sort out the diamond inheritence issue isn't not great. The other options aren't pretty either, but at least it's more contained to i915. -Daniel > /Thomas > > > > > > Amdgpu just uses a switch/case for now, but you could as well extend > > the > > ttm_resource_manager_func table and upcast that inside the driver. > > > > Regards, > > Christian. > > > > > > > > Thanks, > > > Thomas > > > > > > > > > > > > > Regards, > > > > Christian. > > > > > > > > > Thanks, > > > > > > > > > > Thomas > > > > > > > > > > > > > > > > > > > > > >
On 9/14/21 4:07 PM, Daniel Vetter wrote: > On Tue, Sep 14, 2021 at 12:38:00PM +0200, Thomas Hellström wrote: >> On Tue, 2021-09-14 at 10:53 +0200, Christian König wrote: >>> Am 14.09.21 um 10:27 schrieb Thomas Hellström: >>>> On Tue, 2021-09-14 at 09:40 +0200, Christian König wrote: >>>>> Am 13.09.21 um 14:41 schrieb Thomas Hellström: >>>>>> [SNIP] >>>>>>>>> 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. >>>>> That diamond inheritance is a bad idea. What I don't understand >>>>> is >>>>> why >>>>> you need that in the first place? >>>>> >>>>> Information that you attach to a resource are specific to the >>>>> domain >>>>> where the resource is allocated from. So why do you want to >>>>> attach >>>>> the >>>>> same information to a resources from different domains? >>>> Again, for the same reason that we do that with struct >>>> i915_gem_objects >>>> and struct ttm_tts, to extend the functionality. I mean information >>>> that we attach when we subclass a struct ttm_buffer_object doesn't >>>> necessarily care about whether it's a VRAM or a GTT object. In >>>> exactly >>>> the same way, information that we want to attach to a struct >>>> ttm_resource doesn't necessarily care whether it's a system or a >>>> VRAM >>>> resource, and need not be specific to any of those. >>>> >>>> In this particular case, as memory management becomes asynchronous, >>>> you >>>> can't attach things like sg-tables and gpu binding information to >>>> the >>>> gem object anymore, because the object may have a number of >>>> migrations >>>> in the pipeline. Such things need to be attached to the structure >>>> that >>>> abstracts the memory allocation, and which may have a completely >>>> different lifetime than the object itself. >>>> >>>> In our particular case we want to attach information for cached >>>> page >>>> lookup and and sg-table, and moving forward probably the gpu >>>> binding >>>> (vma) information, and that is the same information for any >>>> ttm_resource regardless where it's allocated from. >>>> >>>> Typical example: A pipelined GPU operation happening before an >>>> async >>>> eviction goes wrong. We need to error capture and reset. But if we >>>> look >>>> at the object for error capturing, it's already updated pointing to >>>> an >>>> after-eviction resource, and the resource sits on a ghost object >>>> (or in >>>> the future when ghost objects go away perhaps in limbo somewhere). >>>> >>>> We need to capture the memory pointed to by the struct ttm_resource >>>> the >>>> GPU was referencing, and to be able to do that we need to cache >>>> driver- >>>> specific info on the resource. Typically an sg-list and GPU binding >>>> information. >>>> >>>> Anyway, that cached information needs to be destroyed together with >>>> the >>>> resource and thus we need to be able to access that information >>>> from >>>> the resource in some way, regardless whether it's a pointer or >>>> whether >>>> we embed the struct resource. >>>> >>>> I think it's pretty important here that we (using the inheritance >>>> diagram below) recognize the need for D to inherit from A, just >>>> like we >>>> do for objects or ttm_tts. >>>> >>>> >>>>>> Looking at >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wikipedia.org%2Fwiki%2FMultiple_inheritance%23%2Fmedia%2FFile%3ADiamond_inheritance.svg&data=04%7C01%7Cchristian.koenig%40amd.com%7C268bb562db8548b285b408d977598b2c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637672048739103176%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=bPyDqiSF%2FHFZbl74ux0vfwh3uma5hZIUf2xbzb9yZz8%3D&reserved=0 >>>>>> >>>>>> >>>>>> >>>>>> 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? >>>>> Well we already have that in the form of drm_mm. I mean the >>>>> ttm_range_manager is just a relatively small glue code which >>>>> implements >>>>> the TTMs resource interface using the drm_mm object and a >>>>> spinlock. >>>>> IIRC >>>>> that less than 200 lines of code. >>>>> >>>>> So you should already have the necessary helpers and just need to >>>>> implement the resource manager as far as I can see. >>>>> >>>>> I mean I reused the ttm_range_manager_node in for amdgpu_gtt_mgr >>>>> and >>>>> could potentially reuse a bit more of the ttm_range_manager code. >>>>> But >>>>> I >>>>> don't see that as much of an issue, the extra functionality there >>>>> is >>>>> just minimal. >>>> Sure but that would give up the prereq of having reusable resource >>>> manager implementations. What happens if someone would like to >>>> reuse >>>> the buddy manager? And to complicate things even more, the >>>> information >>>> we attach to VRAM resources also needs to be attached to system >>>> resources. Sure we could probably re-implement a combined system- >>>> buddy- >>>> range manager, but that seems like something overly complex. >>>> >>>> The other object examples resolve the diamond inheritance with a >>>> pointer to the specialization (BC) and let D derive from A. >>>> >>>> TTM resources do it backwards. If we can just recognize that and >>>> ponder >>>> what's the easiest way to resolve this given the current design, I >>>> actually think we'd arrive at a backpointer to allow downcasting >>>> from A >>>> to D. >>> Yeah, but I think you are approaching that from the wrong side. >>> >>> For use cases like this I think you should probably have the >>> following >>> objects and inheritances: >>> >>> 1. Driver specific objects like i915_sg, i915_vma which don't inherit >>> anything from TTM. >>> 2. i915_vram_node which inherits from ttm_resource or a potential >>> ttm_buddy_allocator. >>> 3. i915_gtt_node which inherits from ttm_range_manger_node. >>> 4. Maybe i915_sys_node which inherits from ttm_resource as well. >>> >>> The managers for the individual domains then provide the glue code to >>> implement both the TTM resource interface as well as a driver >>> specific >>> interface to access the driver objects. >> Well yes, but this is not really much better than the union thing. More >> memory efficient but also more duplicated type definitions and manager >> definitions and in addition overriding the default system resource >> manager, not counting the kerneldoc needed to explain why all this is >> necessary. >> >> It was this complexity I was trying to get away from in the first >> place. > I honestly don't think the union thing is the worst. At least as long as > we're reworking i915 at a fairly invasive pace it's probably the lest > worst approach. > > For the specific case of sg list I'm also not sure how great our current > i915 design of "everything is an sg" really is. In the wider community > there's clear rejection of sg for p2p addresses, so having this as a > per-ttm_res_manager kind of situation is probably not the worst. OK well, I'm no defender of the usage of sg list itself, but I was under the impression that as long as it was either only visible to the driver code itself or constructed using dma_map_resource() returned addresses for p2p it would be OK? > In that world every ttm_res_manager would have it's own implementation of > binding into ptes, which then iterate over the pagetables with some common > abstraction. So in a way more of a helper approach for the i915 > implementations of the various hooks, at the cost of a bit of code > duplication. > > I do agree with Christian that the various backpointers to sort out the > diamond inheritence issue isn't not great. The other options aren't pretty > either, but at least it's more contained to i915. OK, I guess I will have to implement whatever ends up prettiest without the back pointer then. I wonder whether there is something we can think of in the future to avoid these diamond- or diamond like inheritances. /Thomas > -Daniel > > >> /Thomas >> >> >> >> >>> Amdgpu just uses a switch/case for now, but you could as well extend >>> the >>> ttm_resource_manager_func table and upcast that inside the driver. >>> >>> Regards, >>> Christian. >>> >>>> Thanks, >>>> Thomas >>>> >>>> >>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> Thanks, >>>>>> >>>>>> Thomas >>>>>> >>>>>> >>>>>> >>
diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c index 2431717376e7..973e7c50bfed 100644 --- a/drivers/gpu/drm/ttm/ttm_resource.c +++ b/drivers/gpu/drm/ttm/ttm_resource.c @@ -57,13 +57,17 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo, void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res) { struct ttm_resource_manager *man; + struct ttm_resource *resource = *res; - if (!*res) + if (!resource) return; - man = ttm_manager_type(bo->bdev, (*res)->mem_type); - man->func->free(man, *res); *res = NULL; + if (resource->priv) + resource->priv->ops.destroy(resource->priv); + + man = ttm_manager_type(bo->bdev, resource->mem_type); + man->func->free(man, resource); } EXPORT_SYMBOL(ttm_resource_free); diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index 140b6b9a8bbe..5a22c9a29c05 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -44,6 +44,7 @@ struct dma_buf_map; struct io_mapping; struct sg_table; struct scatterlist; +struct ttm_resource_private; struct ttm_resource_manager_func { /** @@ -153,6 +154,32 @@ struct ttm_bus_placement { enum ttm_caching caching; }; +/** + * struct ttm_resource_private_ops - Operations for a struct + * ttm_resource_private + * + * Not much benefit to keep this as a separate struct with only a single member, + * but keeping a separate ops struct is the norm. + */ +struct ttm_resource_private_ops { + /** + * destroy() - Callback to destroy the private data + * @priv - The private data to destroy + */ + void (*destroy) (struct ttm_resource_private *priv); +}; + +/** + * struct ttm_resource_private - TTM driver private data + * @ops: Pointer to struct ttm_resource_private_ops with associated operations + * + * Intended to be subclassed to hold, for example cached data sharing the + * lifetime with a struct ttm_resource. + */ +struct ttm_resource_private { + const struct ttm_resource_private_ops ops; +}; + /** * struct ttm_resource * @@ -171,6 +198,7 @@ struct ttm_resource { uint32_t mem_type; uint32_t placement; struct ttm_bus_placement bus; + struct ttm_resource_private *priv; }; /**
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. Cc: Matthew Auld <matthew.william.auld@gmail.com> Cc: König Christian <Christian.Koenig@amd.com> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/ttm/ttm_resource.c | 10 +++++++--- include/drm/ttm/ttm_resource.h | 28 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-)