[PATCH 1/2] drm/amdgpu: cleanup amdgpu_ttm_copy_mem_to_mem and amdgpu_map_buffer
Felix Kuehling
felix.kuehling at amd.com
Thu Mar 19 20:04:43 UTC 2020
That looks like a nice cleanup. Some nit-picks inline ...
On 2020-03-19 9:41, Christian König wrote:
> Cleanup amdgpu_ttm_copy_mem_to_mem by using fewer variables
> for the same value.
>
> Rename amdgpu_map_buffer to amdgpu_ttm_map_buffer, move it
> to avoid the forward decleration, cleanup by moving the map
> decission into the function and add some documentation.
>
> No functional change.
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 244 ++++++++++++------------
> 1 file changed, 118 insertions(+), 126 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index 665db2353a78..2b5974268e63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -62,12 +62,6 @@
>
> #define AMDGPU_TTM_VRAM_MAX_DW_READ (size_t)128
>
> -static int amdgpu_map_buffer(struct ttm_buffer_object *bo,
> - struct ttm_mem_reg *mem, unsigned num_pages,
> - uint64_t offset, unsigned window,
> - struct amdgpu_ring *ring, bool tmz,
> - uint64_t *addr);
> -
> static int amdgpu_invalidate_caches(struct ttm_bo_device *bdev, uint32_t flags)
> {
> return 0;
> @@ -293,6 +287,92 @@ static struct drm_mm_node *amdgpu_find_mm_node(struct ttm_mem_reg *mem,
> return mm_node;
> }
>
> +/**
> + * amdgpu_ttm_map_buffer - Map memory into the GART windows
> + * @bo: buffer object to map
> + * @mem: memory object to map
Missing documentation for @mm_node.
> + * @num_pages: number of pages to map
> + * @offset: offset into @mem where to start
> + * @windows: which GART window to use
@window (no s)
> + * @ring: DMA ring to use for the copy
> + * @tmz: if we should setup a TMZ enabled mapping
> + * @addr: resulting address inside the MC address space
> + *
> + * Setup one of the GART windows to access a specific piece of memory.
... or return the physical address for local memory.
> + */
> +static int amdgpu_ttm_map_buffer(struct ttm_buffer_object *bo,
> + struct ttm_mem_reg *mem,
> + struct drm_mm_node *mm_node,
> + unsigned num_pages, uint64_t offset,
> + unsigned window, struct amdgpu_ring *ring,
> + bool tmz, uint64_t *addr)
> +{
> + struct ttm_dma_tt *dma = container_of(bo->ttm, struct ttm_dma_tt, ttm);
> + struct amdgpu_device *adev = ring->adev;
> + struct amdgpu_job *job;
> + unsigned num_dw, num_bytes;
> + dma_addr_t *dma_address;
> + struct dma_fence *fence;
> + uint64_t src_addr, dst_addr;
> + uint64_t flags;
> + int r;
> +
> + BUG_ON(adev->mman.buffer_funcs->copy_max_bytes <
> + AMDGPU_GTT_MAX_TRANSFER_SIZE * 8);
> +
> + /* Map only what can't be accessed directly */
> + if (mem->start != AMDGPU_BO_INVALID_OFFSET) {
> + *addr = amdgpu_mm_node_addr(bo, mm_node, mem) + offset;
> + return 0;
> + }
> +
> + *addr = adev->gmc.gart_start;
> + *addr += (u64)window * AMDGPU_GTT_MAX_TRANSFER_SIZE *
> + AMDGPU_GPU_PAGE_SIZE;
> + *addr += offset & ~PAGE_MASK;
> +
> + num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw, 8);
> + num_bytes = num_pages * 8;
> +
> + r = amdgpu_job_alloc_with_ib(adev, num_dw * 4 + num_bytes, &job);
> + if (r)
> + return r;
> +
> + src_addr = num_dw * 4;
> + src_addr += job->ibs[0].gpu_addr;
> +
> + dst_addr = amdgpu_bo_gpu_offset(adev->gart.bo);
> + dst_addr += window * AMDGPU_GTT_MAX_TRANSFER_SIZE * 8;
> + amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr,
> + dst_addr, num_bytes, false);
> +
> + amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> + WARN_ON(job->ibs[0].length_dw > num_dw);
> +
> + dma_address = &dma->dma_address[offset >> PAGE_SHIFT];
> + flags = amdgpu_ttm_tt_pte_flags(adev, bo->ttm, mem);
> + if (tmz)
> + flags |= AMDGPU_PTE_TMZ;
> +
> + r = amdgpu_gart_map(adev, 0, num_pages, dma_address, flags,
> + &job->ibs[0].ptr[num_dw]);
> + if (r)
> + goto error_free;
> +
> + r = amdgpu_job_submit(job, &adev->mman.entity,
> + AMDGPU_FENCE_OWNER_UNDEFINED, &fence);
> + if (r)
> + goto error_free;
> +
> + dma_fence_put(fence);
> +
> + return r;
> +
> +error_free:
> + amdgpu_job_free(job);
> + return r;
> +}
> +
> /**
> * amdgpu_copy_ttm_mem_to_mem - Helper function for copy
> * @adev: amdgpu device
> @@ -315,14 +395,14 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
> struct dma_resv *resv,
> struct dma_fence **f)
> {
> + const uint32_t GTT_MAX_BYTES = (AMDGPU_GTT_MAX_TRANSFER_SIZE *
> + AMDGPU_GPU_PAGE_SIZE);
> +
> struct amdgpu_ring *ring = adev->mman.buffer_funcs_ring;
> + uint64_t src_node_size, dst_node_size;
> struct drm_mm_node *src_mm, *dst_mm;
> - uint64_t src_node_start, dst_node_start, src_node_size,
> - dst_node_size, src_page_offset, dst_page_offset;
> struct dma_fence *fence = NULL;
> int r = 0;
> - const uint64_t GTT_MAX_BYTES = (AMDGPU_GTT_MAX_TRANSFER_SIZE *
> - AMDGPU_GPU_PAGE_SIZE);
>
> if (!adev->mman.buffer_funcs_enabled) {
> DRM_ERROR("Trying to move memory with ring turned off.\n");
> @@ -330,58 +410,39 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
> }
>
> src_mm = amdgpu_find_mm_node(src->mem, &src->offset);
> - src_node_start = amdgpu_mm_node_addr(src->bo, src_mm, src->mem) +
> - src->offset;
> src_node_size = (src_mm->size << PAGE_SHIFT) - src->offset;
> - src_page_offset = src_node_start & (PAGE_SIZE - 1);
>
> dst_mm = amdgpu_find_mm_node(dst->mem, &dst->offset);
> - dst_node_start = amdgpu_mm_node_addr(dst->bo, dst_mm, dst->mem) +
> - dst->offset;
> dst_node_size = (dst_mm->size << PAGE_SHIFT) - dst->offset;
> - dst_page_offset = dst_node_start & (PAGE_SIZE - 1);
>
> mutex_lock(&adev->mman.gtt_window_lock);
>
> while (size) {
> - unsigned long cur_size;
> - uint64_t from = src_node_start, to = dst_node_start;
> + uint32_t src_page_offset = src->offset & ~PAGE_MASK;
> + uint32_t dst_page_offset = dst->offset & ~PAGE_MASK;
> struct dma_fence *next;
> + uint32_t cur_size;
> + uint64_t from, to;
>
> /* Copy size cannot exceed GTT_MAX_BYTES. So if src or dst
> * begins at an offset, then adjust the size accordingly
> */
> - cur_size = min3(min(src_node_size, dst_node_size), size,
> - GTT_MAX_BYTES);
> - if (cur_size + src_page_offset > GTT_MAX_BYTES ||
> - cur_size + dst_page_offset > GTT_MAX_BYTES)
> - cur_size -= max(src_page_offset, dst_page_offset);
> -
> - /* Map only what needs to be accessed. Map src to window 0 and
> - * dst to window 1
> - */
> - if (src->mem->start == AMDGPU_BO_INVALID_OFFSET) {
> - r = amdgpu_map_buffer(src->bo, src->mem,
> - PFN_UP(cur_size + src_page_offset),
> - src_node_start, 0, ring, tmz,
> - &from);
> - if (r)
> - goto error;
> - /* Adjust the offset because amdgpu_map_buffer returns
> - * start of mapped page
> - */
> - from += src_page_offset;
> - }
> + cur_size = min3(src_node_size, dst_node_size, size);
> + cur_size = min(GTT_MAX_BYTES - src_page_offset, cur_size);
> + cur_size = min(GTT_MAX_BYTES - dst_page_offset, cur_size);
> +
> + /* Map src to window 0 and dst to window 1. */
> + r = amdgpu_ttm_map_buffer(src->bo, src->mem, src_mm,
> + PFN_UP(cur_size + src_page_offset),
> + src->offset, 0, ring, tmz, &from);
> + if (r)
> + goto error;
>
> - if (dst->mem->start == AMDGPU_BO_INVALID_OFFSET) {
> - r = amdgpu_map_buffer(dst->bo, dst->mem,
> - PFN_UP(cur_size + dst_page_offset),
> - dst_node_start, 1, ring, tmz,
> - &to);
> - if (r)
> - goto error;
> - to += dst_page_offset;
> - }
> + r = amdgpu_ttm_map_buffer(dst->bo, dst->mem, dst_mm,
> + PFN_UP(cur_size + dst_page_offset),
> + dst->offset, 1, ring, tmz, &to);
> + if (r)
> + goto error;
>
> r = amdgpu_copy_buffer(ring, from, to, cur_size,
> resv, &next, false, true, tmz);
> @@ -397,23 +458,20 @@ int amdgpu_ttm_copy_mem_to_mem(struct amdgpu_device *adev,
>
> src_node_size -= cur_size;
> if (!src_node_size) {
> - src_node_start = amdgpu_mm_node_addr(src->bo, ++src_mm,
> - src->mem);
> - src_node_size = (src_mm->size << PAGE_SHIFT);
> - src_page_offset = 0;
> + ++src_mm;
> + src_node_size = src_mm->size << PAGE_SHIFT;
> + src->offset = 0;
This is a bit of an ugly side effect. I don't think it matters for the
existing callers. But it would be cleaner to use a local variable and
declare the src and dst parameters as const struct amdgpu_copy_mem *.
Regards,
Felix
> } else {
> - src_node_start += cur_size;
> - src_page_offset = src_node_start & (PAGE_SIZE - 1);
> + src->offset += cur_size;
> }
> +
> dst_node_size -= cur_size;
> if (!dst_node_size) {
> - dst_node_start = amdgpu_mm_node_addr(dst->bo, ++dst_mm,
> - dst->mem);
> - dst_node_size = (dst_mm->size << PAGE_SHIFT);
> - dst_page_offset = 0;
> + ++dst_mm;
> + dst_node_size = dst_mm->size << PAGE_SHIFT;
> + dst->offset = 0;
> } else {
> - dst_node_start += cur_size;
> - dst_page_offset = dst_node_start & (PAGE_SIZE - 1);
> + dst->offset += cur_size;
> }
> }
> error:
> @@ -2033,72 +2091,6 @@ int amdgpu_mmap(struct file *filp, struct vm_area_struct *vma)
> return ttm_bo_mmap(filp, vma, &adev->mman.bdev);
> }
>
> -static int amdgpu_map_buffer(struct ttm_buffer_object *bo,
> - struct ttm_mem_reg *mem, unsigned num_pages,
> - uint64_t offset, unsigned window,
> - struct amdgpu_ring *ring, bool tmz,
> - uint64_t *addr)
> -{
> - struct amdgpu_ttm_tt *gtt = (void *)bo->ttm;
> - struct amdgpu_device *adev = ring->adev;
> - struct ttm_tt *ttm = bo->ttm;
> - struct amdgpu_job *job;
> - unsigned num_dw, num_bytes;
> - dma_addr_t *dma_address;
> - struct dma_fence *fence;
> - uint64_t src_addr, dst_addr;
> - uint64_t flags;
> - int r;
> -
> - BUG_ON(adev->mman.buffer_funcs->copy_max_bytes <
> - AMDGPU_GTT_MAX_TRANSFER_SIZE * 8);
> -
> - *addr = adev->gmc.gart_start;
> - *addr += (u64)window * AMDGPU_GTT_MAX_TRANSFER_SIZE *
> - AMDGPU_GPU_PAGE_SIZE;
> -
> - num_dw = ALIGN(adev->mman.buffer_funcs->copy_num_dw, 8);
> - num_bytes = num_pages * 8;
> -
> - r = amdgpu_job_alloc_with_ib(adev, num_dw * 4 + num_bytes, &job);
> - if (r)
> - return r;
> -
> - src_addr = num_dw * 4;
> - src_addr += job->ibs[0].gpu_addr;
> -
> - dst_addr = amdgpu_bo_gpu_offset(adev->gart.bo);
> - dst_addr += window * AMDGPU_GTT_MAX_TRANSFER_SIZE * 8;
> - amdgpu_emit_copy_buffer(adev, &job->ibs[0], src_addr,
> - dst_addr, num_bytes, false);
> -
> - amdgpu_ring_pad_ib(ring, &job->ibs[0]);
> - WARN_ON(job->ibs[0].length_dw > num_dw);
> -
> - dma_address = >t->ttm.dma_address[offset >> PAGE_SHIFT];
> - flags = amdgpu_ttm_tt_pte_flags(adev, ttm, mem);
> - if (tmz)
> - flags |= AMDGPU_PTE_TMZ;
> -
> - r = amdgpu_gart_map(adev, 0, num_pages, dma_address, flags,
> - &job->ibs[0].ptr[num_dw]);
> - if (r)
> - goto error_free;
> -
> - r = amdgpu_job_submit(job, &adev->mman.entity,
> - AMDGPU_FENCE_OWNER_UNDEFINED, &fence);
> - if (r)
> - goto error_free;
> -
> - dma_fence_put(fence);
> -
> - return r;
> -
> -error_free:
> - amdgpu_job_free(job);
> - return r;
> -}
> -
> int amdgpu_copy_buffer(struct amdgpu_ring *ring, uint64_t src_offset,
> uint64_t dst_offset, uint32_t byte_count,
> struct dma_resv *resv,
More information about the amd-gfx
mailing list