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

Submitted by Kuehling, Felix on Aug. 23, 2019, 9:33 p.m.

Details

Message ID 20190823213249.10749-4-Felix.Kuehling@amd.com
State New
Headers show
Series "KFD: Mapping-specific MTYPEs on Arcturus" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Kuehling, Felix Aug. 23, 2019, 9:33 p.m.
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(+)

Patch hide | download patch | download mbox

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;
+
 	return pte_flag;
 }
 

Comments

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.

Christian.

>   	return pte_flag;
>   }
>
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.

Regards,
   Felix

>

> Christian.

>

>>       return pte_flag;

>>   }

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

Regards,
Christian.

>
> Regards,
>     Felix
>
>> Christian.
>>
>>>        return pte_flag;
>>>    }
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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. 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@lists.freedesktop.org

>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

> 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

>
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

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


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

>>>
On 2019-08-26 2:12 p.m., Koenig, Christian wrote:
>

> 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 :)


OK, and that does look like the right place to set the PTE_SNOOPED bit 
for XGMI mappings on Arcturus. I'll send out an updated patch series 
that no longer needs the AMDGPU_VM_PAGE_INVALIDATE_PROBE bit in the UAPI.

Thanks for the pointer,
   Felix


>

> Regards,

> Christian.

>