Re: [PATCH 2/2] drm/amdgpu: handle all fragment sizes v3

[Christian König]

Am 31.08.2017 um 01:26 schrieb Felix Kuehling:
One comment inline. With that fixed, this patch is Reviewed-by: Felix
Kuehling <felix.kuehl...@amd.com>

On 2017-08-30 09:48 AM, Christian König wrote:
From: Roger He <hongbo...@amd.com>

This can improve performance for some cases.

v2 (chk): handle all sizes, simplify the patch quite a bit
v3 (chk): adjust dw estimation as well

Signed-off-by: Roger He <hongbo...@amd.com>
Signed-off-by: Christian König <christian.koe...@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 74 ++++++++++++++++++++++------------
  1 file changed, 49 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index b08f031..1575657 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1415,8 +1415,6 @@ static int amdgpu_vm_frag_ptes(struct 
amdgpu_pte_update_params    *params,
                                uint64_t start, uint64_t end,
                                uint64_t dst, uint64_t flags)
  {
-       int r;
-
        /**
         * The MC L1 TLB supports variable sized pages, based on a fragment
         * field in the PTE. When this field is set to a non-zero value, page
@@ -1435,39 +1433,65 @@ static int amdgpu_vm_frag_ptes(struct 
amdgpu_pte_update_params  *params,
         * Userspace can support this by aligning virtual base address and
         * allocation size to the fragment size.
         */
-       unsigned pages_per_frag = params->adev->vm_manager.fragment_size;
-       uint64_t frag_flags = AMDGPU_PTE_FRAG(pages_per_frag);
-       uint64_t frag_align = 1 << pages_per_frag;
+       unsigned max_frag = params->adev->vm_manager.fragment_size;
+       uint64_t frag_flags, frag_end;
+       unsigned frag;
  
-	uint64_t frag_start = ALIGN(start, frag_align);
-       uint64_t frag_end = end & ~(frag_align - 1);
+       int r;
  
  	/* system pages are non continuously */
-       if (params->src || !(flags & AMDGPU_PTE_VALID) ||
-           (frag_start >= frag_end))
+       if (params->src || !(flags & AMDGPU_PTE_VALID))
                return amdgpu_vm_update_ptes(params, start, end, dst, flags);
  
-	/* handle the 4K area at the beginning */
-       if (start != frag_start) {
-               r = amdgpu_vm_update_ptes(params, start, frag_start,
-                                         dst, flags);
+       /* Handle the fragments at the beginning */
+       while (start != end) {
+               /* This intentionally wraps around if no bit is set */
+               frag = min(ffs(start), fls64(end - start)) - 1;
+               if (frag >= max_frag)
+                       break;
+
+               frag_flags = AMDGPU_PTE_FRAG(frag);
+               frag_end = start + (1 << frag);
+
+               r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
+                                         flags | frag_flags);
                if (r)
                        return r;
-               dst += (frag_start - start) * AMDGPU_GPU_PAGE_SIZE;
+
+               dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
+               start = frag_end;
        }
  
  	/* handle the area in the middle */
-       r = amdgpu_vm_update_ptes(params, frag_start, frag_end, dst,
-                                 flags | frag_flags);
-       if (r)
-               return r;
+       if (start != end) {
+               frag_flags = AMDGPU_PTE_FRAG(max_frag);
+               frag_end = end & ~((1 << max_frag) - 1);
You need a cast to uint64_t to make sure your mask is big enough and
doesn't wipe out the high address bits:

     frag_end = end & ~(uint64_t)((1 << max_frag) - 1)

Good point. Using "frag_end = end & ~((1ULL << max_frag) - 1);" should 
work as well, shouldn't it?

Regards,
Christian.


  
-	/* handle the 4K area at the end */
-       if (frag_end != end) {
-               dst += (frag_end - frag_start) * AMDGPU_GPU_PAGE_SIZE;
-               r = amdgpu_vm_update_ptes(params, frag_end, end, dst, flags);
+               r = amdgpu_vm_update_ptes(params, start, frag_end, dst,
+                                         flags | frag_flags);
+               if (r)
+                       return r;
+
+               dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
+               start = frag_end;
        }
-       return r;
+
+       /* Handle the fragments at the end */
+       while (start != end) {
+               frag = fls64(end - start) - 1;
+               frag_flags = AMDGPU_PTE_FRAG(frag);
+               frag_end = start + (1 << frag);
+
+               r = amdgpu_vm_update_ptes(params, start, frag_end,
+                                         dst, flags | frag_flags);
+               if (r)
+                       return r;
+
+               dst += (frag_end - start) * AMDGPU_GPU_PAGE_SIZE;
+               start = frag_end;
+       }
+
+       return 0;
  }
  
  /**
@@ -1557,8 +1581,8 @@ static int amdgpu_vm_bo_update_mapping(struct 
amdgpu_device *adev,
                /* set page commands needed */
                ndw += ncmds * 10;
  
-		/* two extra commands for begin/end of fragment */
-               ndw += 2 * 10;
+               /* extra commands for begin/end fragments */
+               ndw += 2 * 10 * adev->vm_manager.fragment_size;
  
  		params.func = amdgpu_vm_do_set_ptes;
        }


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