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

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

 





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;
  };
/**




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

  Powered by Linux