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

["Christian König" <christian.koenig@amd.com>]

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