> -----Original Message----- > From: Christian König [mailto:deathsimple at vodafone.de] > Sent: Thursday, March 16, 2017 15:59 > 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 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. How about check the init before->list and after->list, then check if the list is not empty? Otherwise, it may add a bool flag for each of them, I don't like that. I have sent the patch as v2. Please check it. Thanks. Jerry > > 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; > >>> } > >>>
