Am 16.03.2017 um 08:46 schrieb Zhang, Jerry: >> -----Original Message----- >> From: Christian König [mailto:deathsimple at vodafone.de] >> Sent: Thursday, March 16, 2017 15:33 >> To: Zhang, Jerry; amd-gfx at lists.freedesktop.org >> Subject: Re: [PATCH 1/2] drm/amdgpu: insert partial mappings before and after >> directly >> >> Am 16.03.2017 um 04:44 schrieb Junwei Zhang: >>> Currently it may miss one page before or after the target mapping >> I don't think that this will work correctly. The interval tree still contains the old >> mapping at this point and as far as I know inserting overlapping mappings is not >> allowed here. > Ah, I missed that, so this insert op should be after free up. > I will revise the patch > >> But what do you mean with miss one page before or after the target mapping? > I did a unit test to verify it and found that: > > If the before mapping is 1 page size, so its start and last will be same. > Thus below condition will become false then to free the before mapping. > > if (before->it.start != before->it.last) > But in this case, we need the before mapping of 1 page size. > So does after mapping. > > e.g. > Initialize a mapping: [0, 8] pages size. > A replace mapping: [1, 2] page size. > Before mapping is [0, 1], before.it.start = 0, before.it = 1 - 1 = 0. > > Additionally, if we create a bo as 1 page size, the corresponding mapping also has start==last, I think. Ups, that's indeed not correct. Yeah we need a better criteria if we should insert the ranges or not. Christian. > > Jerry. > >> Christian. >> >>> Signed-off-by: Junwei Zhang <Jerry.Zhang at amd.com> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 26 ++++++++------------------ >>> 1 file changed, 8 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> index f7c02a9..511c6c9 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c >>> @@ -1767,7 +1767,11 @@ int amdgpu_vm_bo_clear_mappings(struct >> amdgpu_device *adev, >>> before->it.last = saddr - 1; >>> before->offset = tmp->offset; >>> before->flags = tmp->flags; >>> + >>> list_add(&before->list, &tmp->list); >>> + interval_tree_insert(&before->it, &vm->va); >>> + if (before->flags & AMDGPU_PTE_PRT) >>> + amdgpu_vm_prt_get(adev); >>> } >>> >>> /* Remember mapping split at the end */ @@ -1777,7 +1781,11 >> @@ int >>> amdgpu_vm_bo_clear_mappings(struct amdgpu_device *adev, >>> after->offset = tmp->offset; >>> after->offset += after->it.start - tmp->it.start; >>> after->flags = tmp->flags; >>> + >>> list_add(&after->list, &tmp->list); >>> + interval_tree_insert(&after->it, &vm->va); >>> + if (after->flags & AMDGPU_PTE_PRT) >>> + amdgpu_vm_prt_get(adev); >>> } >>> >>> list_del(&tmp->list); >>> @@ -1799,24 +1807,6 @@ int amdgpu_vm_bo_clear_mappings(struct >> amdgpu_device *adev, >>> trace_amdgpu_vm_bo_unmap(NULL, tmp); >>> } >>> >>> - /* Insert partial mapping before the range*/ >>> - if (before->it.start != before->it.last) { >>> - interval_tree_insert(&before->it, &vm->va); >>> - if (before->flags & AMDGPU_PTE_PRT) >>> - amdgpu_vm_prt_get(adev); >>> - } else { >>> - kfree(before); >>> - } >>> - >>> - /* Insert partial mapping after the range */ >>> - if (after->it.start != after->it.last) { >>> - interval_tree_insert(&after->it, &vm->va); >>> - if (after->flags & AMDGPU_PTE_PRT) >>> - amdgpu_vm_prt_get(adev); >>> - } else { >>> - kfree(after); >>> - } >>> - >>> return 0; >>> } >>>
