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

[Kuehling, Felix]

On 2019-04-01 7:23 a.m., Christian König wrote:
> 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.

A fence could also indicate a concurrent modification of the page table. 
For example a PTB may be allocated and initialized concurrently, not in 
direct mode. Would direct mode need to wait for a fence that indicates 
completion of the PTB initialization? Or do we have some way to ensure 
such concurrent allocation and initialization of a PTB cannot happen?

Regards,
   Felix


>
> 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;
>