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

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

 





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

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



_______________________________________________
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