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

[Christian König]

Am 31.05.21 um 16:30 schrieb 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.

Works for me as well, I would just update the comment to read something 
like "Make sure previous mapping is freed".

Christian.

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