[PATCH] drm/amdgpu: Avoid direct cast to amdgpu_ttm_tt

Felix Kuehling felix.kuehling at amd.com
Tue Jul 26 23:55:49 UTC 2022


Am 2022-07-26 um 19:43 schrieb Rajneesh Bhardwaj:
> For typesafety, use container_of() instead of implicit cast from struct
> ttm_tt to struct amdgpu_ttm_tt.
>
> Cc: Christian König <christian.koenig at amd.com>
> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 44 ++++++++++++++-----------
>   1 file changed, 24 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index be0efaae79a9..cd6aa206a59e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -637,6 +637,8 @@ struct amdgpu_ttm_tt {
>   #endif
>   };
>   
> +#define ttm_to_amdgpu_ttm_tt(ttm)	container_of(ttm, struct amdgpu_ttm_tt, ttm)
> +
>   #ifdef CONFIG_DRM_AMDGPU_USERPTR
>   /*
>    * amdgpu_ttm_tt_get_user_pages - get device accessible pages that back user
> @@ -648,7 +650,7 @@ struct amdgpu_ttm_tt {
>   int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages)
>   {
>   	struct ttm_tt *ttm = bo->tbo.ttm;
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	unsigned long start = gtt->userptr;
>   	struct vm_area_struct *vma;
>   	struct mm_struct *mm;
> @@ -702,7 +704,7 @@ int amdgpu_ttm_tt_get_user_pages(struct amdgpu_bo *bo, struct page **pages)
>    */
>   bool amdgpu_ttm_tt_get_user_pages_done(struct ttm_tt *ttm)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	bool r = false;
>   
>   	if (!gtt || !gtt->userptr)
> @@ -751,7 +753,7 @@ static int amdgpu_ttm_tt_pin_userptr(struct ttm_device *bdev,
>   				     struct ttm_tt *ttm)
>   {
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bdev);
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	int write = !(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY);
>   	enum dma_data_direction direction = write ?
>   		DMA_BIDIRECTIONAL : DMA_TO_DEVICE;
> @@ -788,7 +790,7 @@ static void amdgpu_ttm_tt_unpin_userptr(struct ttm_device *bdev,
>   					struct ttm_tt *ttm)
>   {
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bdev);
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	int write = !(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY);
>   	enum dma_data_direction direction = write ?
>   		DMA_BIDIRECTIONAL : DMA_TO_DEVICE;
> @@ -822,7 +824,7 @@ static void amdgpu_ttm_gart_bind(struct amdgpu_device *adev,
>   {
>   	struct amdgpu_bo *abo = ttm_to_amdgpu_bo(tbo);
>   	struct ttm_tt *ttm = tbo->ttm;
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   
>   	if (amdgpu_bo_encrypted(abo))
>   		flags |= AMDGPU_PTE_TMZ;
> @@ -860,7 +862,7 @@ static int amdgpu_ttm_backend_bind(struct ttm_device *bdev,
>   				   struct ttm_resource *bo_mem)
>   {
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bdev);
> -	struct amdgpu_ttm_tt *gtt = (void*)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	uint64_t flags;
>   	int r;
>   
> @@ -927,7 +929,8 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
>   {
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
>   	struct ttm_operation_ctx ctx = { false, false };
> -	struct amdgpu_ttm_tt *gtt = (void *)bo->ttm;
> +	struct ttm_tt *ttm = bo->ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);

Is the extra ttm local variable really needed? It's only used once in 
this function, right here.


>   	struct ttm_placement placement;
>   	struct ttm_place placements;
>   	struct ttm_resource *tmp;
> @@ -998,7 +1001,7 @@ static void amdgpu_ttm_backend_unbind(struct ttm_device *bdev,
>   				      struct ttm_tt *ttm)
>   {
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bdev);
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   
>   	/* if the pages have userptr pinning then clear that first */
>   	if (gtt->userptr) {
> @@ -1025,7 +1028,7 @@ static void amdgpu_ttm_backend_unbind(struct ttm_device *bdev,
>   static void amdgpu_ttm_backend_destroy(struct ttm_device *bdev,
>   				       struct ttm_tt *ttm)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   
>   	if (gtt->usertask)
>   		put_task_struct(gtt->usertask);
> @@ -1079,7 +1082,7 @@ static int amdgpu_ttm_tt_populate(struct ttm_device *bdev,
>   				  struct ttm_operation_ctx *ctx)
>   {
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(bdev);
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	pgoff_t i;
>   	int ret;
>   
> @@ -1113,7 +1116,7 @@ static int amdgpu_ttm_tt_populate(struct ttm_device *bdev,
>   static void amdgpu_ttm_tt_unpopulate(struct ttm_device *bdev,
>   				     struct ttm_tt *ttm)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	struct amdgpu_device *adev;
>   	pgoff_t i;
>   
> @@ -1171,18 +1174,19 @@ int amdgpu_ttm_tt_set_userptr(struct ttm_buffer_object *bo,
>   			      uint64_t addr, uint32_t flags)
>   {
>   	struct amdgpu_ttm_tt *gtt;
> +	struct ttm_tt *ttm = bo->ttm;
>   
> -	if (!bo->ttm) {
> +	if (!ttm) {
>   		/* TODO: We want a separate TTM object type for userptrs */
> -		bo->ttm = amdgpu_ttm_tt_create(bo, 0);

You're removing the assignment to bo->ttm here. I don't see where you're 
adding it back. Basically you're storing the new ttm in a local 
variable, and it will be forgotten when the function returns.

Regards,
   Felix


> -		if (bo->ttm == NULL)
> +		ttm = amdgpu_ttm_tt_create(bo, 0);
> +		if (ttm == NULL)
>   			return -ENOMEM;
>   	}
>   
>   	/* Set TTM_TT_FLAG_EXTERNAL before populate but after create. */
> -	bo->ttm->page_flags |= TTM_TT_FLAG_EXTERNAL;
> +	ttm->page_flags |= TTM_TT_FLAG_EXTERNAL;
>   
> -	gtt = (void *)bo->ttm;
> +	gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	gtt->userptr = addr;
>   	gtt->userflags = flags;
>   
> @@ -1199,7 +1203,7 @@ int amdgpu_ttm_tt_set_userptr(struct ttm_buffer_object *bo,
>    */
>   struct mm_struct *amdgpu_ttm_tt_get_usermm(struct ttm_tt *ttm)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   
>   	if (gtt == NULL)
>   		return NULL;
> @@ -1218,7 +1222,7 @@ struct mm_struct *amdgpu_ttm_tt_get_usermm(struct ttm_tt *ttm)
>   bool amdgpu_ttm_tt_affect_userptr(struct ttm_tt *ttm, unsigned long start,
>   				  unsigned long end, unsigned long *userptr)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   	unsigned long size;
>   
>   	if (gtt == NULL || !gtt->userptr)
> @@ -1241,7 +1245,7 @@ bool amdgpu_ttm_tt_affect_userptr(struct ttm_tt *ttm, unsigned long start,
>    */
>   bool amdgpu_ttm_tt_is_userptr(struct ttm_tt *ttm)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   
>   	if (gtt == NULL || !gtt->userptr)
>   		return false;
> @@ -1254,7 +1258,7 @@ bool amdgpu_ttm_tt_is_userptr(struct ttm_tt *ttm)
>    */
>   bool amdgpu_ttm_tt_is_readonly(struct ttm_tt *ttm)
>   {
> -	struct amdgpu_ttm_tt *gtt = (void *)ttm;
> +	struct amdgpu_ttm_tt *gtt = ttm_to_amdgpu_ttm_tt(ttm);
>   
>   	if (gtt == NULL)
>   		return false;


More information about the amd-gfx mailing list