[PATCH] drm/amdgpu: Refactor code to handle non coherent and uncached

[Bhardwaj, Rajneesh]

On 7/20/2022 7:18 PM, Felix Kuehling wrote:
>
> On 2022-07-18 18:52, Rajneesh Bhardwaj wrote:
>> This simplifies existing coherence handling for Arcturus and Aldabaran
>> to account for !coherent && uncached scenarios.
>>
>> Cc: Joseph Greathouse <joseph.greathouse at amd.com>
>> Cc: Alex Deucher <Alexander.Deucher at amd.com>
>> Signed-off-by: Rajneesh Bhardwaj <rajneesh.bhardwaj at amd.com>
>> ---
>>   .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c  | 53 +++++++++----------
>>   1 file changed, 26 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index d1657de5f875..0fdfd79f69ad 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -471,45 +471,44 @@ static uint64_t get_pte_flags(struct 
>> amdgpu_device *adev, struct kgd_mem *mem)
>>         switch (adev->asic_type) {
>>       case CHIP_ARCTURUS:
>> -        if (mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM) {
>> -            if (bo_adev == adev)
>> -                mapping_flags |= coherent ?
>> -                    AMDGPU_VM_MTYPE_CC : AMDGPU_VM_MTYPE_RW;
>> -            else
>> -                mapping_flags |= coherent ?
>> -                    AMDGPU_VM_MTYPE_UC : AMDGPU_VM_MTYPE_NC;
>> -        } else {
>> -            mapping_flags |= coherent ?
>> -                AMDGPU_VM_MTYPE_UC : AMDGPU_VM_MTYPE_NC;
>> -        }
>> -        break;
>>       case CHIP_ALDEBARAN:
>> -        if (coherent && uncached) {
>> -            if (adev->gmc.xgmi.connected_to_cpu ||
>> -                !(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM))
>> -                snoop = true;
>> -            mapping_flags |= AMDGPU_VM_MTYPE_UC;
>> -        } else if (mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM) {
>> +        if (mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM) {
>>               if (bo_adev == adev) {
>> -                mapping_flags |= coherent ?
>> -                    AMDGPU_VM_MTYPE_CC : AMDGPU_VM_MTYPE_RW;
>> -                if (adev->gmc.xgmi.connected_to_cpu)
>> +                if (uncached)
>> +                    mapping_flags |= AMDGPU_VM_MTYPE_UC;
>> +                else if (coherent)
>> +                    mapping_flags |= AMDGPU_VM_MTYPE_CC;
>> +                else
>> +                    mapping_flags |= AMDGPU_VM_MTYPE_RW;
>> +                if (adev->asic_type == CHIP_ALDEBARAN &&
>> +                    adev->gmc.xgmi.connected_to_cpu)
>>                       snoop = true;
>>               } else {
>> -                mapping_flags |= coherent ?
>> -                    AMDGPU_VM_MTYPE_UC : AMDGPU_VM_MTYPE_NC;
>> +                if (uncached || coherent)
>> +                    mapping_flags |= AMDGPU_VM_MTYPE_UC;
>> +                else
>> +                    mapping_flags |= AMDGPU_VM_MTYPE_NC;
>>                   if (amdgpu_xgmi_same_hive(adev, bo_adev))
>>                       snoop = true;
>>               }
>>           } else {
>> +            if (uncached || coherent)
>> +                mapping_flags |= AMDGPU_VM_MTYPE_UC;
>> +            else
>> +                mapping_flags |= AMDGPU_VM_MTYPE_NC;
>>               snoop = true;
>> -            mapping_flags |= coherent ?
>> -                AMDGPU_VM_MTYPE_UC : AMDGPU_VM_MTYPE_NC;
>>           }
>>           break;
>>       default:
>> -        mapping_flags |= coherent ?
>> -            AMDGPU_VM_MTYPE_UC : AMDGPU_VM_MTYPE_NC;
>> +        if (uncached || coherent)
>> +            mapping_flags |= AMDGPU_VM_MTYPE_UC;
>> +        else
>> +            mapping_flags |= AMDGPU_VM_MTYPE_NC;
>> +
>> +        if (!(mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_VRAM))
>> +            snoop = true;
>> +
>> +
>
> With the two extra blank lines removed, this patch is
>
> Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>
>
> Please check whether a similar cleanup can be made in 
> svm_range_get_pte_flags, or maybe even, whether common code can be 
> factored out of those two functions.


Thanks Felix for the review. Do you want me to send V2 with two lines 
removed or just apply to amd-staging-drm-next after deleting those two 
lines?


I will check svm_range_get_pte_flags and see if I can cleanup the code 
there and get back to you.


>
> Regards,
>   Felix
>
>
>>       }
>>         pte_flags = amdgpu_gem_va_map_flags(adev, mapping_flags);