[PATCH 3/4] drm/amdgpu: Support snooped PTE flag
Koenig, Christian
Christian.Koenig at amd.com
Mon Aug 26 18:12:25 UTC 2019
Am 26.08.19 um 20:03 schrieb Kuehling, Felix:
> 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 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.
>> 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.
Good point, and yes I actually think as well that we shouldn't have had
the gmc_v9_0_get_vm_pte_flags callback in the first place.
> 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.
We should probably just revert back the to doing most of the mapping in
amdgpu_vm_bo_split_mapping().
Here we already have a whole bunch of ASIC specific handling anyway:
> if (!(mapping->flags & AMDGPU_PTE_READABLE))
> flags &= ~AMDGPU_PTE_READABLE;
> if (!(mapping->flags & AMDGPU_PTE_WRITEABLE))
> flags &= ~AMDGPU_PTE_WRITEABLE;
>
> flags &= ~AMDGPU_PTE_EXECUTABLE;
> flags |= mapping->flags & AMDGPU_PTE_EXECUTABLE;
>
> if (adev->asic_type == CHIP_NAVI10) {
> flags &= ~AMDGPU_PTE_MTYPE_NV10_MASK;
> flags |= (mapping->flags & AMDGPU_PTE_MTYPE_NV10_MASK);
> } else {
> flags &= ~AMDGPU_PTE_MTYPE_VG10_MASK;
> flags |= (mapping->flags & AMDGPU_PTE_MTYPE_VG10_MASK);
> }
>
> if ((mapping->flags & AMDGPU_PTE_PRT) &&
> (adev->asic_type >= CHIP_VEGA10)) {
> flags |= AMDGPU_PTE_PRT;
> if (adev->asic_type >= CHIP_NAVI10) {
> flags |= AMDGPU_PTE_SNOOPED;
> flags |= AMDGPU_PTE_LOG;
> flags |= AMDGPU_PTE_SYSTEM;
> }
> flags &= ~AMDGPU_PTE_VALID;
> }
And now that you mentioned it, the code you proposed wouldn't have
worked anyway because the AMDGPU_PTE_SNOOPED would have been filtered
out here :)
Regards,
Christian.
>
> 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
>>>
More information about the amd-gfx
mailing list