[PATCH 3/4] drm/amdgpu: Support snooped PTE flag

[Kuehling, Felix]

On 2019-08-24 2:59 p.m., Christian König wrote:
> Am 24.08.19 um 18:24 schrieb Kuehling, Felix:
>> On 2019-08-24 7:13, Christian König wrote:
>>> Am 23.08.19 um 23:33 schrieb Kuehling, Felix:
>>>> From: Oak Zeng <Oak.Zeng at amd.com>
>>>>
>>>> Set snooped PTE flag according to mapping flag. Write request to a
>>>> page with snooped bit set, will send out invalidate probe request
>>>> to TCC of the remote GPU where the vram page resides.
>>>>
>>>> Change-Id: I799f68ec7a5a1abf32075f5ef31051641a0b3736
>>>> Signed-off-by: Oak Zeng <Oak.Zeng at amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> index 9aafcda6c488..8a7c4ec69ae8 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> @@ -604,6 +604,9 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct
>>>> amdgpu_device *adev,
>>>>        if (flags & AMDGPU_VM_PAGE_PRT)
>>>>            pte_flag |= AMDGPU_PTE_PRT;
>>>>    +    if (flags & AMDGPU_VM_PAGE_INVALIDATE_PROBE)
>>>> +        pte_flag |= AMDGPU_PTE_SNOOPED;
>>>> +
>>> That is still a NAK without further checks. We need to make absolutely
>>> sure that we don't set this when PCIe routing is in use.
>> The only place where AMDGPU_VM_... flags are accepted from user mode
>> seems to be amdgpu_gem_va_ioctl. It has an explicit set of valid_flags
>> it accepts. The new INVALIDATE_PROBE flag is not part of it. That means
>> user mode will  not be able to set it directly. If we added it to the
>> set of valid_flags in amdgpu_gem_va_ioctl, we'd need to add appropriate
>> checks at the same time.
>>
>> KFD does not expose AMDGPU_VM_... flags directly to user mode. It only
>> sets the INVALIDATE_PROBE flag in kernel mode for mappings in the same
>> XGMI hive on Arturus (in patch 4).
>>
>> If there is something I'm missing, please point it out. But AFAICT the
>> checking that is currently done should satisfy your requirements.
>
> The hardware behavior depends on the placement of the buffer, so at 
> bare minimum we need to check if it's pointing to PCIe or local (check 
> the system bit).
>
> But even if it's local what is the behavior for local memory? E.g. not 
> accessed through XGMI?
>
> As far as I can see what we need to check here is that this is a 
> remote access over XGMI and then (and only then!) we are allowed to 
> set the snoop bit on the PTE.

My point is, the only place where this bit can get set right now is in 
kernel mode in amdgpu_amdkfd_gpuvm.c. See patch 4 of my series. That 
code already has all the right conditions to make sure the 
INVALIDATE_PROBE bit is only set in the correct circumstances (remote 
XGMI mappings in the same hive):

> +	switch (adev->asic_type) {
> +	case CHIP_ARCTURUS:
> +		if (mem->alloc_flags & ALLOC_MEM_FLAGS_VRAM) {
> +			if (bo_adev == adev) {
> +				...
> +			} else {
> +				...
> +				if (amdgpu_xgmi_same_hive(adev, bo_adev))
> +					mapping_flags |=
> +						AMDGPU_VM_PAGE_INVALIDATE_PROBE;
> +			}
> +		} else {
I think you're asking me to add an extra check for the same conditions 
in the GMC code? Like GMC doesn't trust amdgpu_amdkfd. It seems 
completely redundant and a bit paranoid to me. gmc_v9_0_get_vm_pte_flags 
doesn't even have all the information it needs to make that determination.

Regards,
   Felix


>
> Regards,
> Christian.
>
>>
>> Regards,
>>     Felix
>>
>>> Christian.
>>>
>>>>        return pte_flag;
>>>>    }
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>