[PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Philip Yang]

Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
the page table, which commonly happens when memory is mapped for the
first time.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philip Yang <Philip.Yang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9596c22fded6..1ea204218903 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 {
 	struct amdgpu_vm_update_params params;
 	struct amdgpu_vm_bo_base *entry;
+	bool flush_tlb_needed = false;
 	int r, idx;
 
 	if (list_empty(&vm->relocated))
@@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 		goto error;
 
 	list_for_each_entry(entry, &vm->relocated, vm_status) {
+		/* vm_flush_needed after updating moved PDEs */
+		flush_tlb_needed |= entry->moved;
+
 		r = amdgpu_vm_pde_update(&params, entry);
 		if (r)
 			goto error;
@@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
 	if (r)
 		goto error;
 
-	/* vm_flush_needed after updating PDEs */
-	atomic64_inc(&vm->tlb_seq);
+	if (flush_tlb_needed)
+		atomic64_inc(&vm->tlb_seq);
 
 	while (!list_empty(&vm->relocated)) {
 		entry = list_first_entry(&vm->relocated,
-- 
2.35.1

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Christian König]

Am 02.06.22 um 15:20 schrieb Philip Yang:
> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
> the page table, which commonly happens when memory is mapped for the
> first time.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9596c22fded6..1ea204218903 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   {
>   	struct amdgpu_vm_update_params params;
>   	struct amdgpu_vm_bo_base *entry;
> +	bool flush_tlb_needed = false;
>   	int r, idx;
>   
>   	if (list_empty(&vm->relocated))
> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   		goto error;
>   
>   	list_for_each_entry(entry, &vm->relocated, vm_status) {
> +		/* vm_flush_needed after updating moved PDEs */
> +		flush_tlb_needed |= entry->moved;
> +
>   		r = amdgpu_vm_pde_update(&params, entry);
>   		if (r)
>   			goto error;
> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   	if (r)
>   		goto error;
>   
> -	/* vm_flush_needed after updating PDEs */
> -	atomic64_inc(&vm->tlb_seq);
> +	if (flush_tlb_needed)
> +		atomic64_inc(&vm->tlb_seq);
>   
>   	while (!list_empty(&vm->relocated)) {
>   		entry = list_first_entry(&vm->relocated,

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Lazar, Lijo]

On 6/2/2022 6:50 PM, Philip Yang wrote:
> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
> the page table, which commonly happens when memory is mapped for the
> first time.
> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9596c22fded6..1ea204218903 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   {
>   	struct amdgpu_vm_update_params params;
>   	struct amdgpu_vm_bo_base *entry;
> +	bool flush_tlb_needed = false;
>   	int r, idx;
>   
>   	if (list_empty(&vm->relocated))
> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   		goto error;
>   
>   	list_for_each_entry(entry, &vm->relocated, vm_status) {
> +		/* vm_flush_needed after updating moved PDEs */
> +		flush_tlb_needed |= entry->moved;

That is a strange thing to do for a bool variable. Why not just assign it?

Thanks,
Lijo

> +
>   		r = amdgpu_vm_pde_update(&params, entry);
>   		if (r)
>   			goto error;
> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   	if (r)
>   		goto error;
>   
> -	/* vm_flush_needed after updating PDEs */
> -	atomic64_inc(&vm->tlb_seq);
> +	if (flush_tlb_needed)
> +		atomic64_inc(&vm->tlb_seq);
>   
>   	while (!list_empty(&vm->relocated)) {
>   		entry = list_first_entry(&vm->relocated,
> 

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Lazar, Lijo]

On 6/2/2022 6:54 PM, Lazar, Lijo wrote:
> 
> 
> On 6/2/2022 6:50 PM, Philip Yang wrote:
>> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
>> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
>> the page table, which commonly happens when memory is mapped for the
>> first time.
>>
>> Suggested-by: Christian König <christian.koenig@amd.com>
>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 9596c22fded6..1ea204218903 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>>   {
>>       struct amdgpu_vm_update_params params;
>>       struct amdgpu_vm_bo_base *entry;
>> +    bool flush_tlb_needed = false;
>>       int r, idx;
>>       if (list_empty(&vm->relocated))
>> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>>           goto error;
>>       list_for_each_entry(entry, &vm->relocated, vm_status) {
>> +        /* vm_flush_needed after updating moved PDEs */
>> +        flush_tlb_needed |= entry->moved;
> 
> That is a strange thing to do for a bool variable. Why not just assign it?
> 

Hmm.. In a loop, perhaps you meant logical OR?

Thanks,
Lijo

> Thanks,
> Lijo
> 
>> +
>>           r = amdgpu_vm_pde_update(&params, entry);
>>           if (r)
>>               goto error;
>> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>>       if (r)
>>           goto error;
>> -    /* vm_flush_needed after updating PDEs */
>> -    atomic64_inc(&vm->tlb_seq);
>> +    if (flush_tlb_needed)
>> +        atomic64_inc(&vm->tlb_seq);
>>       while (!list_empty(&vm->relocated)) {
>>           entry = list_first_entry(&vm->relocated,
>>

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Christian König]

Am 02.06.22 um 15:26 schrieb Lazar, Lijo:
>
>
> On 6/2/2022 6:54 PM, Lazar, Lijo wrote:
>>
>>
>> On 6/2/2022 6:50 PM, Philip Yang wrote:
>>> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
>>> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
>>> the page table, which commonly happens when memory is mapped for the
>>> first time.
>>>
>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 9596c22fded6..1ea204218903 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>> *adev,
>>>   {
>>>       struct amdgpu_vm_update_params params;
>>>       struct amdgpu_vm_bo_base *entry;
>>> +    bool flush_tlb_needed = false;
>>>       int r, idx;
>>>       if (list_empty(&vm->relocated))
>>> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>> *adev,
>>>           goto error;
>>>       list_for_each_entry(entry, &vm->relocated, vm_status) {
>>> +        /* vm_flush_needed after updating moved PDEs */
>>> +        flush_tlb_needed |= entry->moved;
>>
>> That is a strange thing to do for a bool variable. Why not just 
>> assign it?
>>
>
> Hmm.. In a loop, perhaps you meant logical OR?

Well IIRC C doesn't have a logical or assignment operator "||=", so "|=" 
is used instead which also gets the job done.

Christian.

>
> Thanks,
> Lijo
>
>> Thanks,
>> Lijo
>>
>>> +
>>>           r = amdgpu_vm_pde_update(&params, entry);
>>>           if (r)
>>>               goto error;
>>> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>> *adev,
>>>       if (r)
>>>           goto error;
>>> -    /* vm_flush_needed after updating PDEs */
>>> -    atomic64_inc(&vm->tlb_seq);
>>> +    if (flush_tlb_needed)
>>> +        atomic64_inc(&vm->tlb_seq);
>>>       while (!list_empty(&vm->relocated)) {
>>>           entry = list_first_entry(&vm->relocated,
>>>

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Lazar, Lijo]

On 6/2/2022 7:06 PM, Christian König wrote:
> Am 02.06.22 um 15:26 schrieb Lazar, Lijo:
>>
>>
>> On 6/2/2022 6:54 PM, Lazar, Lijo wrote:
>>>
>>>
>>> On 6/2/2022 6:50 PM, Philip Yang wrote:
>>>> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
>>>> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
>>>> the page table, which commonly happens when memory is mapped for the
>>>> first time.
>>>>
>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 9596c22fded6..1ea204218903 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>> *adev,
>>>>   {
>>>>       struct amdgpu_vm_update_params params;
>>>>       struct amdgpu_vm_bo_base *entry;
>>>> +    bool flush_tlb_needed = false;
>>>>       int r, idx;
>>>>       if (list_empty(&vm->relocated))
>>>> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>> *adev,
>>>>           goto error;
>>>>       list_for_each_entry(entry, &vm->relocated, vm_status) {
>>>> +        /* vm_flush_needed after updating moved PDEs */
>>>> +        flush_tlb_needed |= entry->moved;
>>>
>>> That is a strange thing to do for a bool variable. Why not just 
>>> assign it?
>>>
>>
>> Hmm.. In a loop, perhaps you meant logical OR?
> 
> Well IIRC C doesn't have a logical or assignment operator "||=", so "|=" 
> is used instead which also gets the job done.
> 

var = var || value;  also will work.

BTW, v1 of this patch was incrementing vm->tlb_seq for every entry 
moved. This one increments only once. So is this vm->tlb_seq required 
only to be a bool?

Thanks,
Lijo

> Christian.
> 
>>
>> Thanks,
>> Lijo
>>
>>> Thanks,
>>> Lijo
>>>
>>>> +
>>>>           r = amdgpu_vm_pde_update(&params, entry);
>>>>           if (r)
>>>>               goto error;
>>>> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>> *adev,
>>>>       if (r)
>>>>           goto error;
>>>> -    /* vm_flush_needed after updating PDEs */
>>>> -    atomic64_inc(&vm->tlb_seq);
>>>> +    if (flush_tlb_needed)
>>>> +        atomic64_inc(&vm->tlb_seq);
>>>>       while (!list_empty(&vm->relocated)) {
>>>>           entry = list_first_entry(&vm->relocated,
>>>>
> 

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Christian König]

Am 02.06.22 um 15:44 schrieb Lazar, Lijo:
>
>
> On 6/2/2022 7:06 PM, Christian König wrote:
>> Am 02.06.22 um 15:26 schrieb Lazar, Lijo:
>>>
>>>
>>> On 6/2/2022 6:54 PM, Lazar, Lijo wrote:
>>>>
>>>>
>>>> On 6/2/2022 6:50 PM, Philip Yang wrote:
>>>>> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
>>>>> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
>>>>> the page table, which commonly happens when memory is mapped for the
>>>>> first time.
>>>>>
>>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>>> Signed-off-by: Philip Yang <Philip.Yang@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 9596c22fded6..1ea204218903 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>>> *adev,
>>>>>   {
>>>>>       struct amdgpu_vm_update_params params;
>>>>>       struct amdgpu_vm_bo_base *entry;
>>>>> +    bool flush_tlb_needed = false;
>>>>>       int r, idx;
>>>>>       if (list_empty(&vm->relocated))
>>>>> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>>> *adev,
>>>>>           goto error;
>>>>>       list_for_each_entry(entry, &vm->relocated, vm_status) {
>>>>> +        /* vm_flush_needed after updating moved PDEs */
>>>>> +        flush_tlb_needed |= entry->moved;
>>>>
>>>> That is a strange thing to do for a bool variable. Why not just 
>>>> assign it?
>>>>
>>>
>>> Hmm.. In a loop, perhaps you meant logical OR?
>>
>> Well IIRC C doesn't have a logical or assignment operator "||=", so 
>> "|=" is used instead which also gets the job done.
>>
>
> var = var || value;  also will work.
>
> BTW, v1 of this patch was incrementing vm->tlb_seq for every entry 
> moved. This one increments only once. So is this vm->tlb_seq required 
> only to be a bool?

No, it is required to only increment once. It's a sequence number and 
changing it signals that a TLB flush is necessary.

Regards,
Christian.

>
> Thanks,
> Lijo
>
>> Christian.
>>
>>>
>>> Thanks,
>>> Lijo
>>>
>>>> Thanks,
>>>> Lijo
>>>>
>>>>> +
>>>>>           r = amdgpu_vm_pde_update(&params, entry);
>>>>>           if (r)
>>>>>               goto error;
>>>>> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device 
>>>>> *adev,
>>>>>       if (r)
>>>>>           goto error;
>>>>> -    /* vm_flush_needed after updating PDEs */
>>>>> -    atomic64_inc(&vm->tlb_seq);
>>>>> +    if (flush_tlb_needed)
>>>>> +        atomic64_inc(&vm->tlb_seq);
>>>>>       while (!list_empty(&vm->relocated)) {
>>>>>           entry = list_first_entry(&vm->relocated,
>>>>>
>>

Re: [PATCH v2 1/1] drm/amdgpu: Update PDEs flush TLB if PTB/PDB moved

[Felix Kuehling]

Am 2022-06-02 um 09:20 schrieb Philip Yang:
> Flush TLBs when existing PDEs are updated because a PTB or PDB moved,
> but avoids unnecessary TLB flushes when new PDBs or PTBs are added to
> the page table, which commonly happens when memory is mapped for the
> first time.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philip Yang <Philip.Yang@amd.com>

Reviewed-by: Felix Kuehling <Felix.Kuehling@amd.com>


> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9596c22fded6..1ea204218903 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -737,6 +737,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   {
>   	struct amdgpu_vm_update_params params;
>   	struct amdgpu_vm_bo_base *entry;
> +	bool flush_tlb_needed = false;
>   	int r, idx;
>   
>   	if (list_empty(&vm->relocated))
> @@ -755,6 +756,9 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   		goto error;
>   
>   	list_for_each_entry(entry, &vm->relocated, vm_status) {
> +		/* vm_flush_needed after updating moved PDEs */
> +		flush_tlb_needed |= entry->moved;
> +
>   		r = amdgpu_vm_pde_update(&params, entry);
>   		if (r)
>   			goto error;
> @@ -764,8 +768,8 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>   	if (r)
>   		goto error;
>   
> -	/* vm_flush_needed after updating PDEs */
> -	atomic64_inc(&vm->tlb_seq);
> +	if (flush_tlb_needed)
> +		atomic64_inc(&vm->tlb_seq);
>   
>   	while (!list_empty(&vm->relocated)) {
>   		entry = list_first_entry(&vm->relocated,