[PATCH v2 2/2] drm/amdgpu: Add amdgpu_find_mm_node()

[Kasiviswanathan, Harish]

-----Original Message-----
From: Christian König [mailto:ckoenig.leichtzumerken at gmail.com] 
Sent: Monday, October 09, 2017 5:11 AM
To: Kasiviswanathan, Harish <Harish.Kasiviswanathan at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH v2 2/2] drm/amdgpu: Add amdgpu_find_mm_node()

Am 07.10.2017 um 00:08 schrieb Harish Kasiviswanathan:
> Change-Id: I12231e18bb60152843cd0e0213ddd0d0e04e7497
> Signed-off-by: Harish Kasiviswanathan <Harish.Kasiviswanathan at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 36 +++++++++++++++++++--------------
>   1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> index dda4f08..e9bfe9d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
> @@ -290,6 +290,23 @@ static uint64_t amdgpu_mm_node_addr(struct ttm_buffer_object *bo,
>   }
>   
>   /**
> + * amdgpu_find_mm_node - Helper function finds the drm_mm_node
> + *  corresponding to @offset. It also modifies the offset to be
> + *  within the drm_mm_node returned
> + */
> +static struct drm_mm_node *amdgpu_find_mm_node(struct ttm_mem_reg *mem,
> +					       unsigned long *offset)
> +{
> +	struct drm_mm_node *mm_node = mem->mm_node;
> +
> +	while (*offset >= (mm_node->size << PAGE_SHIFT)) {
> +		*offset -= (mm_node->size << PAGE_SHIFT);
> +		++mm_node;
> +	}
> +	return mm_node;
> +}

Please take a look at amdgpu_ttm_io_mem_pfn().

There we assume that each node has the same size and do:
>         page_offset = do_div(offset, size);
>         mm += offset;

Not 100% sure if that is a good idea, but when we add a common function 
for this we should use it everywhere.

[HK]: Ok I missed this one, probably because it was used in a different way. I will change this function to call amdgpu_find_mm_node().
The optimization (assuming all nodes are same size) will work in the current setup but as you mentioned I am also not sure if it is a good idea. To be safe I will be use the while() loop for now.

Regards,
Christian.

> +
> +/**
>    * amdgpu_copy_ttm_mem_to_mem - Helper function for copy
>    *
>    * The function copies @size bytes from {src->mem + src->offset} to
> @@ -319,21 +336,13 @@ int amdgpu_copy_ttm_mem_to_mem(struct amdgpu_device *adev,
>   		return -EINVAL;
>   	}
>   
> -	src_mm = src->mem->mm_node;
> -	while (src->offset >= (src_mm->size << PAGE_SHIFT)) {
> -		src->offset -= (src_mm->size << PAGE_SHIFT);
> -		++src_mm;
> -	}
> +	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 = dst->mem->mm_node;
> -	while (dst->offset >= (dst_mm->size << PAGE_SHIFT)) {
> -		dst->offset -= (dst_mm->size << PAGE_SHIFT);
> -		++dst_mm;
> -	}
> +	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;
> @@ -1216,7 +1225,7 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo,
>   {
>   	struct amdgpu_bo *abo = container_of(bo, struct amdgpu_bo, tbo);
>   	struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
> -	struct drm_mm_node *nodes = abo->tbo.mem.mm_node;
> +	struct drm_mm_node *nodes;
>   	uint32_t value = 0;
>   	int ret = 0;
>   	uint64_t pos;
> @@ -1225,10 +1234,7 @@ static int amdgpu_ttm_access_memory(struct ttm_buffer_object *bo,
>   	if (bo->mem.mem_type != TTM_PL_VRAM)
>   		return -EIO;
>   
> -	while (offset >= (nodes->size << PAGE_SHIFT)) {
> -		offset -= nodes->size << PAGE_SHIFT;
> -		++nodes;
> -	}
> +	nodes = amdgpu_find_mm_node(&abo->tbo.mem, &offset);
>   	pos = (nodes->start << PAGE_SHIFT) + offset;
>   
>   	while (len && pos < adev->mc.mc_vram_size) {