[PATCH 1/8] drm/amdgpu: fix ATC handling for Ryzen

[Christian König]

Am 30.03.19 um 01:41 schrieb Kuehling, Felix:
> Patches 1-3 are Reviewed-by: Felix Kuehling <Felix.Kuehling at amd.com>

Thanks.

>
> About the direct mode, that removes a bunch of synchronization, so it
> must make some assumptions about the state of the page tables. What
> makes that safe?

Direct mode is only supposed to be used during page fault handling.

E.g. we know that the page tables are in the correct place in this 
situation because the hardware is hammering on a PTE and waiting for it 
to become valid.

Christian.

>   Is it safe to use direct-mode on a
> per-page-table-update basis? Or do all page table updates have to go
> through direct mode to avoid hazards? If yes, then maybe this should be
> a property of the VM rather than a parameter that gets passed to a bunch
> of function calls.
>
> Regards,
>     Felix
>
> On 2019-03-29 6:45 a.m., Christian König wrote:
>> Otherwise we don't correctly use translate further.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>    drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 13 ++++++++-----
>>    1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 3d221f044183..059d9802e713 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -767,14 +767,17 @@ static int amdgpu_vm_clear_bo(struct amdgpu_device *adev,
>>    
>>    	addr = 0;
>>    	if (ats_entries) {
>> -		uint64_t ats_value;
>> +		uint64_t value = 0, flags;
>>    
>> -		ats_value = AMDGPU_PTE_DEFAULT_ATC;
>> -		if (level != AMDGPU_VM_PTB)
>> -			ats_value |= AMDGPU_PDE_PTE;
>> +		flags = AMDGPU_PTE_DEFAULT_ATC;
>> +		if (level != AMDGPU_VM_PTB) {
>> +			/* Handle leaf PDEs as PTEs */
>> +			flags |= AMDGPU_PDE_PTE;
>> +			amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
>> +		}
>>    
>>    		r = vm->update_funcs->update(&params, bo, addr, 0, ats_entries,
>> -					     0, ats_value);
>> +					     value, flags);
>>    		if (r)
>>    			return r;
>>