[PATCH 1/2] drm/amdgpu: insert partial mappings before and after directly

[Christian König]

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;
>>>    }
>>>