>>> + 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?
Yes, that would work too.
Regards,
Felix
________________________________________
From: Koenig, Christian
Sent: Thursday, August 31, 2017 3:31:36 AM
To: Kuehling, Felix; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/amdgpu: handle all fragment sizes v3
Am 31.08.2017 um 01:26 schrieb Felix Kuehling:
> One comment inline. With that fixed, this patch is Reviewed-by: Felix
> Kuehling <Felix.Kuehling at amd.com>
>
>
> On 2017-08-30 09:48 AM, Christian König wrote:
>> From: Roger He <Hongbo.He at 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.He at amd.com>
>> Signed-off-by: Christian König <christian.koenig at 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;
>> }
