Re: [PATCH 05/13] drm/ttm: allocate resource object instead of embedding it

On Fri, 30 Apr 2021 at 10:25, Christian König ckoenig.leichtzumerken@gmail.com wrote:

To improve the handling we want the establish the resource object as base class for the backend allocations.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 54 +++++++-------- drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- drivers/gpu/drm/ttm/ttm_bo.c | 76 +++++++--------------- drivers/gpu/drm/ttm/ttm_bo_util.c | 41 ++++++------ drivers/gpu/drm/ttm/ttm_resource.c | 30 ++++++--- drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c | 2 +- include/drm/ttm/ttm_bo_api.h | 1 - include/drm/ttm/ttm_bo_driver.h | 10 ++- include/drm/ttm/ttm_resource.h | 4 +- 11 files changed, 101 insertions(+), 125 deletions(-)

<snip>

int ttm_resource_alloc(struct ttm_buffer_object *bo, const struct ttm_place *place,

•                  struct ttm_resource *res)
  

•                  struct ttm_resource **res_ptr)
  

{ struct ttm_resource_manager *man =

•           ttm_manager_type(bo->bdev, res->mem_type);
  

•           ttm_manager_type(bo->bdev, place->mem_type);
  

•   struct ttm_resource *res;
  

•   int r;
  
  

•   res = kmalloc(sizeof(*res), GFP_KERNEL);
  

if (!res) return -ENOMEM;

Also there are still some places where we are not checking if ttm_resource_alloc returns an error. I guess those are only for the SYS cases where the ->alloc did nothing so it couldn't really fail, but now there is a big scary kmalloc lurking in here? Is this not a concern?

    res->mm_node = NULL;
    res->start = 0;
    res->num_pages = PFN_UP(bo->base.size);

@@ -41,18 +44,27 @@ int ttm_resource_alloc(struct ttm_buffer_object *bo, res->bus.offset = 0; res->bus.is_iomem = false; res->bus.caching = ttm_cached;

•   r = man->func->alloc(man, bo, place, res);
  

•   if (r) {
  

•           kfree(res);
  

•           return r;
  

•   }
  
  

•   return man->func->alloc(man, bo, place, res);
  

•   *res_ptr = res;
  

•   return 0;
  

}

-void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource *res) +void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res) {

•   struct ttm_resource_manager *man =
  

•           ttm_manager_type(bo->bdev, res->mem_type);
  

•   struct ttm_resource_manager *man;
  
  

•   man->func->free(man, res);
  

•   res->mm_node = NULL;
  

•   res->mem_type = TTM_PL_SYSTEM;
  

•   if (!*res)
  

•           return;
  

•   man = ttm_manager_type(bo->bdev, (*res)->mem_type);
  

•   man->func->free(man, *res);
  

•   kfree(*res);
  

•   *res = NULL;
  

} EXPORT_SYMBOL(ttm_resource_free);

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c index bb1d453f7ca6..4c34216a9ab8 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_buffer.c @@ -746,7 +746,7 @@ static int vmw_move(struct ttm_buffer_object *bo, goto fail;

            vmw_ttm_unbind(bo->bdev, bo->ttm);

•           ttm_resource_free(bo, bo->resource);
  

•           ttm_resource_free(bo, &bo->resource);
            ttm_bo_assign_mem(bo, new_mem);
            return 0;
    } else {
  

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 1876a3d6df73..d8bb46228cc7 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -137,7 +137,6 @@ struct ttm_buffer_object { */

    struct ttm_resource *resource;

•   struct ttm_resource _mem;
    struct ttm_tt *ttm;
    bool deleted;
  
  

diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index 1a9ba0b13622..ead0ef7136c8 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -96,7 +96,7 @@ struct ttm_lru_bulk_move { */ int ttm_bo_mem_space(struct ttm_buffer_object *bo, struct ttm_placement *placement,

•                struct ttm_resource *mem,
  

•                struct ttm_resource **mem,
                 struct ttm_operation_ctx *ctx);
  
  

/** @@ -188,8 +188,8 @@ ttm_bo_move_to_lru_tail_unlocked(struct ttm_buffer_object *bo) static inline void ttm_bo_assign_mem(struct ttm_buffer_object *bo, struct ttm_resource *new_mem) {

•   bo->_mem = *new_mem;
  

•   new_mem->mm_node = NULL;
  

•   WARN_ON(bo->resource);
  

•   bo->resource = new_mem;
  

}

/** @@ -202,9 +202,7 @@ static inline void ttm_bo_assign_mem(struct ttm_buffer_object *bo, static inline void ttm_bo_move_null(struct ttm_buffer_object *bo, struct ttm_resource *new_mem) {

•   struct ttm_resource *old_mem = bo->resource;
  

•   WARN_ON(old_mem->mm_node != NULL);
  

•   ttm_resource_free(bo, &bo->resource);
    ttm_bo_assign_mem(bo, new_mem);
  

}

diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index 890b9d369519..c17c1a52070d 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -225,8 +225,8 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man)

int ttm_resource_alloc(struct ttm_buffer_object *bo, const struct ttm_place *place,

•                  struct ttm_resource *res);
  

-void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource *res);

•                  struct ttm_resource **res);
  

+void ttm_resource_free(struct ttm_buffer_object *bo, struct ttm_resource **res);

void ttm_resource_manager_init(struct ttm_resource_manager *man, unsigned long p_size); -- 2.25.1