[PATCH 1/2] drm/amdgpu: Fix a bug on flag table_freed

[Eric Huang]

On 2021-05-31 10:08 a.m., Christian König wrote:
>
>
> Am 30.05.21 um 20:29 schrieb Eric Huang:
>>
>> On 2021-05-30 12:54 p.m., Christian König wrote:
>>>
>>>
>>> Am 30.05.21 um 00:51 schrieb Eric Huang:
>>>> table_freed will be always true when mapping a memory with size
>>>> bigger than 2MB. The problem is page table's entries are always
>>>> existed, but existing mapping depends on page talbe's bo, so
>>>> using a check of page table's bo existed will resolve the issue.
>>>>
>>>> Signed-off-by: Eric Huang <jinhuieric.huang at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 +++-
>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 0dee2e8797c7..95b94c95adac 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1582,9 +1582,11 @@ static int amdgpu_vm_update_ptes(struct 
>>>> amdgpu_vm_update_params *params,
>>>>                * completely covered by the range and so potentially 
>>>> still in use.
>>>>                */
>>>>               while (cursor.pfn < frag_start) {
>>>> +                /* Make sure previous mapping existed */
>>>> +                if (cursor.entry->base.bo)
>>>> +                    params->table_freed = true;
>>>
>>> In general this is the correct approach, but I would push that 
>>> decision into the amdgpu_vm_free_pts() function.
>>>
>>>> amdgpu_vm_free_pts(adev, params->vm, &cursor);
>>>
>>> So that we have here something like
>>>
>>> params->table_freed |= amdgpu_vm_free_pts(..);
>>>
>> Thank you for your review. I was thinking put the check into function 
>> amdgpu_vm_free_pts() since previous review, it will change returns of 
>> two functions amdgpu_vm_free_pts() and amdgpu_vm_free_table(). If the 
>> returns are not used by other functions, it seems make a simple 
>> change complex from my perspective. Can you share the reason of your 
>> suggestion?
>
> Because you can also optimize the bulk_moveable handling in that 
> function.
>
> E.g. bulk_moveable should only be set to false when a table was freed.
Makes sense. In terms of bulk_moveable, how about this:

             while (cursor.pfn < frag_start) {
                 /* Make sure previous mapping existed */
                 if (cursor.entry->base.bo) {
                     params->table_freed = true;
                     amdgpu_vm_free_pts(adev, params->vm, &cursor);
                 }
                 amdgpu_vm_pt_next(adev, &cursor);
             }

It should satisfy bulk_moveable flag, and also save freeing 
cursor.entry->entries, which are pre-allocated in the beginning of this 
function(amdgpu_vm_update_ptes), the pre-allocation can be saved if next 
mapping is a small page as well. It seems the most efficient approach 
for me.

Regards,
Eric
>
> The only case where this doesn't matter is vm_fini and we really don't 
> care for that special one.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> Eric
>>>
>>> Regards,
>>> Christian.
>>>
>>>> amdgpu_vm_pt_next(adev, &cursor);
>>>> -                params->table_freed = true;
>>>>               }
>>>>             } else if (frag >= shift) {
>>>
>>
>