On Fri, Apr 5, 2024 at 3:57 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> When we removed the hacky start code check we actually didn't took into
> account that *all* VRAM pages needs to be CPU accessible.
>
> Clean up the code and unify the handling into a single helper which
> checks if the whole resource is CPU accessible.
>
> The only place where a partial check would make sense is during
> eviction, but that is neglitible.
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> Fixes: aed01a68047b ("drm/amdgpu: Remove TTM resource->start visible VRAM condition v2")
Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 22 ++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 22 --------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 61 ++++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 3 ++
> 5 files changed, 53 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index bbbd8ad0171f..e9168677ef0a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -819,7 +819,7 @@ static int amdgpu_cs_bo_validate(void *param, struct amdgpu_bo *bo)
>
> p->bytes_moved += ctx.bytes_moved;
> if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> - amdgpu_bo_in_cpu_visible_vram(bo))
> + amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> p->bytes_moved_vis += ctx.bytes_moved;
>
> if (unlikely(r == -ENOMEM) && domain != bo->allowed_domains) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index eb7d824763b9..eff3f9fceada 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -620,8 +620,7 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> return r;
>
> if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> - bo->tbo.resource->mem_type == TTM_PL_VRAM &&
> - amdgpu_bo_in_cpu_visible_vram(bo))
> + amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> amdgpu_cs_report_moved_bytes(adev, ctx.bytes_moved,
> ctx.bytes_moved);
> else
> @@ -1276,18 +1275,20 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
> void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
> struct amdgpu_mem_stats *stats)
> {
> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> + struct ttm_resource *res = bo->tbo.resource;
> uint64_t size = amdgpu_bo_size(bo);
> unsigned int domain;
>
> /* Abort if the BO doesn't currently have a backing store */
> - if (!bo->tbo.resource)
> + if (!res)
> return;
>
> - domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> + domain = amdgpu_mem_type_to_domain(res->mem_type);
> switch (domain) {
> case AMDGPU_GEM_DOMAIN_VRAM:
> stats->vram += size;
> - if (amdgpu_bo_in_cpu_visible_vram(bo))
> + if (amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> stats->visible_vram += size;
> break;
> case AMDGPU_GEM_DOMAIN_GTT:
> @@ -1382,10 +1383,7 @@ vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo)
> /* Remember that this BO was accessed by the CPU */
> abo->flags |= AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
>
> - if (bo->resource->mem_type != TTM_PL_VRAM)
> - return 0;
> -
> - if (amdgpu_bo_in_cpu_visible_vram(abo))
> + if (amdgpu_res_cpu_visible(adev, bo->resource))
> return 0;
>
> /* Can't move a pinned BO to visible VRAM */
> @@ -1409,7 +1407,7 @@ vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo)
>
> /* this should never happen */
> if (bo->resource->mem_type == TTM_PL_VRAM &&
> - !amdgpu_bo_in_cpu_visible_vram(abo))
> + !amdgpu_res_cpu_visible(adev, bo->resource))
> return VM_FAULT_SIGBUS;
>
> ttm_bo_move_to_lru_tail_unlocked(bo);
> @@ -1573,6 +1571,7 @@ uint32_t amdgpu_bo_get_preferred_domain(struct amdgpu_device *adev,
> */
> u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
> {
> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> struct dma_buf_attachment *attachment;
> struct dma_buf *dma_buf;
> const char *placement;
> @@ -1581,10 +1580,11 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
>
> if (dma_resv_trylock(bo->tbo.base.resv)) {
> unsigned int domain;
> +
> domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> switch (domain) {
> case AMDGPU_GEM_DOMAIN_VRAM:
> - if (amdgpu_bo_in_cpu_visible_vram(bo))
> + if (amdgpu_res_cpu_visible(adev, bo->tbo.resource))
> placement = "VRAM VISIBLE";
> else
> placement = "VRAM";
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index d28e21baef16..f8982404da93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -244,28 +244,6 @@ static inline u64 amdgpu_bo_mmap_offset(struct amdgpu_bo *bo)
> return drm_vma_node_offset_addr(&bo->tbo.base.vma_node);
> }
>
> -/**
> - * amdgpu_bo_in_cpu_visible_vram - check if BO is (partly) in visible VRAM
> - */
> -static inline bool amdgpu_bo_in_cpu_visible_vram(struct amdgpu_bo *bo)
> -{
> - struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
> - struct amdgpu_res_cursor cursor;
> -
> - if (!bo->tbo.resource || bo->tbo.resource->mem_type != TTM_PL_VRAM)
> - return false;
> -
> - amdgpu_res_first(bo->tbo.resource, 0, amdgpu_bo_size(bo), &cursor);
> - while (cursor.remaining) {
> - if (cursor.start < adev->gmc.visible_vram_size)
> - return true;
> -
> - amdgpu_res_next(&cursor, cursor.size);
> - }
> -
> - return false;
> -}
> -
> /**
> * amdgpu_bo_explicit_sync - return whether the bo is explicitly synced
> */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 460b23918bfc..6f0cfe66613e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -137,7 +137,7 @@ static void amdgpu_evict_flags(struct ttm_buffer_object *bo,
> amdgpu_bo_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_CPU);
> } else if (!amdgpu_gmc_vram_full_visible(&adev->gmc) &&
> !(abo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) &&
> - amdgpu_bo_in_cpu_visible_vram(abo)) {
> + amdgpu_res_cpu_visible(adev, bo->resource)) {
>
> /* Try evicting to the CPU inaccessible part of VRAM
> * first, but only set GTT as busy placement, so this
> @@ -408,40 +408,55 @@ static int amdgpu_move_blit(struct ttm_buffer_object *bo,
> return r;
> }
>
> -/*
> - * amdgpu_mem_visible - Check that memory can be accessed by ttm_bo_move_memcpy
> +/**
> + * amdgpu_res_cpu_visible - Check that resource can be accessed by CPU
> + * @adev: amdgpu device
> + * @res: the resource to check
> *
> - * Called by amdgpu_bo_move()
> + * Returns: true if the full resource is CPU visible, false otherwise.
> */
> -static bool amdgpu_mem_visible(struct amdgpu_device *adev,
> - struct ttm_resource *mem)
> +bool amdgpu_res_cpu_visible(struct amdgpu_device *adev,
> + struct ttm_resource *res)
> {
> - u64 mem_size = (u64)mem->size;
> struct amdgpu_res_cursor cursor;
> - u64 end;
>
> - if (mem->mem_type == TTM_PL_SYSTEM ||
> - mem->mem_type == TTM_PL_TT)
> + if (!res)
> + return false;
> +
> + if (res->mem_type == TTM_PL_SYSTEM || res->mem_type == TTM_PL_TT ||
> + res->mem_type == AMDGPU_PL_PREEMPT)
> return true;
> - if (mem->mem_type != TTM_PL_VRAM)
> +
> + if (res->mem_type != TTM_PL_VRAM)
> return false;
>
> - amdgpu_res_first(mem, 0, mem_size, &cursor);
> - end = cursor.start + cursor.size;
> + amdgpu_res_first(res, 0, res->size, &cursor);
> while (cursor.remaining) {
> + if ((cursor.start + cursor.size) >= adev->gmc.visible_vram_size)
> + return false;
> amdgpu_res_next(&cursor, cursor.size);
> + }
>
> - if (!cursor.remaining)
> - break;
> + return true;
> +}
>
> - /* ttm_resource_ioremap only supports contiguous memory */
> - if (end != cursor.start)
> - return false;
> +/*
> + * amdgpu_res_copyable - Check that memory can be accessed by ttm_bo_move_memcpy
> + *
> + * Called by amdgpu_bo_move()
> + */
> +static bool amdgpu_res_copyable(struct amdgpu_device *adev,
> + struct ttm_resource *mem)
> +{
> + if (!amdgpu_res_cpu_visible(adev, mem))
> + return false;
>
> - end = cursor.start + cursor.size;
> - }
> + /* ttm_resource_ioremap only supports contiguous memory */
> + if (mem->mem_type == TTM_PL_VRAM &&
> + !(mem->placement & TTM_PL_FLAG_CONTIGUOUS))
> + return false;
>
> - return end <= adev->gmc.visible_vram_size;
> + return true;
> }
>
> /*
> @@ -539,8 +554,8 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo, bool evict,
>
> if (r) {
> /* Check that all memory is CPU accessible */
> - if (!amdgpu_mem_visible(adev, old_mem) ||
> - !amdgpu_mem_visible(adev, new_mem)) {
> + if (!amdgpu_res_copyable(adev, old_mem) ||
> + !amdgpu_res_copyable(adev, new_mem)) {
> pr_err("Move buffer fallback to memcpy unavailable\n");
> return r;
> }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> index 65ec82141a8e..32cf6b6f6efd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
> @@ -139,6 +139,9 @@ int amdgpu_vram_mgr_reserve_range(struct amdgpu_vram_mgr *mgr,
> int amdgpu_vram_mgr_query_page_status(struct amdgpu_vram_mgr *mgr,
> uint64_t start);
>
> +bool amdgpu_res_cpu_visible(struct amdgpu_device *adev,
> + struct ttm_resource *res);
> +
> int amdgpu_ttm_init(struct amdgpu_device *adev);
> void amdgpu_ttm_fini(struct amdgpu_device *adev);
> void amdgpu_ttm_set_buffer_funcs_status(struct amdgpu_device *adev,
> --
> 2.34.1
>
