Re: [RFC PATCH v2] drm/amdgpu: move ttm bo->offset to amdgpu_bo

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

Am 11.02.20 um 15:06 schrieb Nirmoy Das:
> GPU address should belong to driver not in memory management.
> This patch moves ttm bo.offset and gpu_offset calculation to amdgpu driver.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 22 +++++++++++++++++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     |  8 +-------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c |  4 ++--
>   4 files changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6f60a581e3ba..e25924be33ca 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -917,7 +917,7 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain,
>   		bo->pin_count++;
>   
>   		if (max_offset != 0) {
> -			u64 domain_start = bo->tbo.bdev->man[mem_type].gpu_offset;
> +			u64 domain_start = amdgpu_bo_gpu_start(adev, mem_type);
>   			WARN_ON_ONCE(max_offset <
>   				     (amdgpu_bo_gpu_offset(bo) - domain_start));
>   		}
> @@ -1248,6 +1248,18 @@ int amdgpu_bo_get_metadata(struct amdgpu_bo *bo, void *buffer,
>   	return 0;
>   }
>   
> +uint64_t amdgpu_bo_gpu_start(struct amdgpu_device *adev, uint32_t type)

Probably better to put that into amdgpu_ttm.[ch] or amdgpu_gmc.[ch] and 
name it an amdgpu_ttm_domain_start.

> +{
> +	switch(type) {
> +		case TTM_PL_TT:

Coding style here is of, what editor and setting do you use?

> +			return adev->gmc.gart_start;
> +		case TTM_PL_VRAM:
> +			return adev->gmc.vram_start;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * amdgpu_bo_move_notify - notification about a memory move
>    * @bo: pointer to a buffer object
> @@ -1460,6 +1472,9 @@ int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr)
>    */
>   u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo)
>   {
> +	uint64_t offset;
> +	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);

Better put adev first.

> +
>   	WARN_ON_ONCE(bo->tbo.mem.mem_type == TTM_PL_SYSTEM);
>   	WARN_ON_ONCE(!dma_resv_is_locked(bo->tbo.base.resv) &&
>   		     !bo->pin_count && bo->tbo.type != ttm_bo_type_kernel);
> @@ -1467,7 +1482,10 @@ u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo)
>   	WARN_ON_ONCE(bo->tbo.mem.mem_type == TTM_PL_VRAM &&
>   		     !(bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS));
>   
> -	return amdgpu_gmc_sign_extend(bo->tbo.offset);
> +        offset = (bo->tbo.mem.start << PAGE_SHIFT) +
> +		 amdgpu_bo_gpu_start(adev, bo->tbo.mem.mem_type);
> +
> +	return amdgpu_gmc_sign_extend(offset);
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 96d805889e8d..dfa4e75b95e2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -294,6 +294,7 @@ int amdgpu_bo_restore_shadow(struct amdgpu_bo *shadow,
>   uint32_t amdgpu_bo_get_preferred_pin_domain(struct amdgpu_device *adev,
>   					    uint32_t domain);
>   
> +uint64_t amdgpu_bo_gpu_start(struct amdgpu_device *adev, uint32_t type);
>   /*
>    * sub allocation
>    */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 2c1d1eb1a7e1..3e65bdfca94f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -103,7 +103,6 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>   	case TTM_PL_TT:
>   		/* GTT memory  */
>   		man->func = &amdgpu_gtt_mgr_func;
> -		man->gpu_offset = adev->gmc.gart_start;
>   		man->available_caching = TTM_PL_MASK_CACHING;
>   		man->default_caching = TTM_PL_FLAG_CACHED;
>   		man->flags = TTM_MEMTYPE_FLAG_MAPPABLE | TTM_MEMTYPE_FLAG_CMA;
> @@ -111,7 +110,6 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>   	case TTM_PL_VRAM:
>   		/* "On-card" video ram */
>   		man->func = &amdgpu_vram_mgr_func;
> -		man->gpu_offset = adev->gmc.vram_start;
>   		man->flags = TTM_MEMTYPE_FLAG_FIXED |
>   			     TTM_MEMTYPE_FLAG_MAPPABLE;
>   		man->available_caching = TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_WC;
> @@ -122,7 +120,6 @@ static int amdgpu_init_mem_type(struct ttm_bo_device *bdev, uint32_t type,
>   	case AMDGPU_PL_OA:
>   		/* On-chip GDS memory*/
>   		man->func = &ttm_bo_manager_func;
> -		man->gpu_offset = 0;
>   		man->flags = TTM_MEMTYPE_FLAG_FIXED | TTM_MEMTYPE_FLAG_CMA;
>   		man->available_caching = TTM_PL_FLAG_UNCACHED;
>   		man->default_caching = TTM_PL_FLAG_UNCACHED;
> @@ -270,7 +267,7 @@ static uint64_t amdgpu_mm_node_addr(struct ttm_buffer_object *bo,
>   
>   	if (mm_node->start != AMDGPU_BO_INVALID_OFFSET) {
>   		addr = mm_node->start << PAGE_SHIFT;
> -		addr += bo->bdev->man[mem->mem_type].gpu_offset;
> +		addr += amdgpu_bo_gpu_start(amdgpu_ttm_adev(bo->bdev), mem->mem_type);
>   	}
>   	return addr;
>   }
> @@ -1136,9 +1133,6 @@ int amdgpu_ttm_alloc_gart(struct ttm_buffer_object *bo)
>   		bo->mem = tmp;
>   	}
>   
> -	bo->offset = (bo->mem.start << PAGE_SHIFT) +
> -		bo->bdev->man[bo->mem.mem_type].gpu_offset;
> -
>   	return 0;
>   }
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 4cc7881f438c..e7a383134521 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -140,7 +140,7 @@ static void amdgpu_vm_sdma_copy_ptes(struct amdgpu_vm_update_params *p,
>   
>   	src += p->num_dw_left * 4;
>   
> -	pe += amdgpu_gmc_sign_extend(bo->tbo.offset);
> +	pe += amdgpu_bo_gpu_offset(bo);

That won't work. We need to add something like 
amdgpu_bo_gpu_offset_no_check() or adjust the WARN_ONs in 
amdgpu_bo_gpu_offset() a bit.

>   	trace_amdgpu_vm_copy_ptes(pe, src, count, p->direct);
>   
>   	amdgpu_vm_copy_pte(p->adev, ib, pe, src, count);
> @@ -167,7 +167,7 @@ static void amdgpu_vm_sdma_set_ptes(struct amdgpu_vm_update_params *p,
>   {
>   	struct amdgpu_ib *ib = p->job->ibs;
>   
> -	pe += amdgpu_gmc_sign_extend(bo->tbo.offset);
> +	pe += amdgpu_bo_gpu_offset(bo);

Same here.

Apart from those minor issue looks good to me.

Christian.

>   	trace_amdgpu_vm_set_ptes(pe, addr, count, incr, flags, p->direct);
>   	if (count < 3) {
>   		amdgpu_vm_write_pte(p->adev, ib, pe, addr | flags,

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx