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@xxxxxxxxx>
Cc: König Christian <Christian.Koenig@xxxxxxx>
Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
---
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;
};
/**
