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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
>>>>>>>
>>>>>>> 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@xxxxxxx>
>>>>>>> ---
>>>>>>>       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
>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux