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

["Kuehling, Felix" <Felix.Kuehling-5C7GfCeVMHo@public.gmane.org>]

On 2019-08-26 1:16 p.m., wrote:
> Am 26.08.19 um 17:45 schrieb 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@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@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.
> Well the job of the VM code is to figure out the flags and location for
> the PTE based on the current placement BO and the flags given for the
> mapping.
>
> And yes that implies that we don't trust upper layers to do this instead.

I consider amdgpu_amdkfd_gpuvm as part of the lower layer. It has 
control over the placement of the buffers.

That said, if the GMC code has to figure out the PTE mapping flags based 
on the location of the buffer and the intended usage, it'll be hard to 
avoid some of the abstractions you criticized in Oak's patch series. You 
can't have it both ways. Either you give user mode the responsibility to 
know all the HW details and let user mode determine the mtype and flags 
(which is what you currently do in the GEM interface), or you let the VM 
code determine the flags based on more abstract information from user mode.

I see this discussion moving towards a more abstract interface. I'll see 
if I can add that into the GMC IP without breaking the existing AMDGPU 
GEM APIs.

Regards,
   Felix


>
>> gmc_v9_0_get_vm_pte_flags doesn't even have all the information it needs to make that determination.
> Well then we probably need to change that.
>
> Regards,
> Christian.
>
>> Regards,
>>      Felix
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx