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

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

[Junwei Zhang]

Currently it may miss one page before or after the target mapping

Signed-off-by: Junwei Zhang <Jerry.Zhang@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;
 }
 
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: fix bo_va tracing for bo unmap

[Junwei Zhang]

Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index d5a00f7..c3182e4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -245,7 +245,7 @@
 			     ),
 
 	    TP_fast_assign(
-			   __entry->bo = bo_va->bo;
+			   __entry->bo = bo_va ? bo_va->bo : NULL;
 			   __entry->start = mapping->it.start;
 			   __entry->last = mapping->it.last;
 			   __entry->offset = mapping->offset;
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Christian König]

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.

But what do you mean with miss one page before or after the target mapping?

Christian.

>
> Signed-off-by: Junwei Zhang <Jerry.Zhang@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;
>   }
>   


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhang, Jerry]

> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Thursday, March 16, 2017 15:33
> To: Zhang, Jerry; amd-gfx@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.

Jerry.

> 
> Christian.
> 
> >
> > Signed-off-by: Junwei Zhang <Jerry.Zhang@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;
> >   }
> >
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [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@vodafone.de]
>> Sent: Thursday, March 16, 2017 15:33
>> To: Zhang, Jerry; amd-gfx@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@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;
>>>    }
>>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Zhang, Jerry]

> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Thursday, March 16, 2017 15:59
> To: Zhang, Jerry; amd-gfx@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@vodafone.de]
> >> Sent: Thursday, March 16, 2017 15:33
> >> To: Zhang, Jerry; amd-gfx@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@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;
> >>>    }
> >>>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx