[PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Christian König]

Staring at the function for six hours, just to essentially move one line
of code.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 35d53d81f486..138c98902033 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
 static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
 				    struct list_head *lru, bool is_swap)
 {
+	struct list_head *list;
 	LIST_HEAD(entries);
 	LIST_HEAD(before);
-	struct list_head *list1, *list2;
 
-	list1 = is_swap ? &pos->last->swap : &pos->last->lru;
-	list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
+	reservation_object_assert_held(pos->last->resv);
+	list = is_swap ? &pos->last->swap : &pos->last->lru;
+	list_cut_position(&entries, lru, list);
+
+	reservation_object_assert_held(pos->first->resv);
+	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
+	list_cut_position(&before, &entries, list);
 
-	list_cut_position(&entries, lru, list1);
-	list_cut_position(&before, &entries, list2);
 	list_splice(&before, lru);
 	list_splice_tail(&entries, lru);
 }
-- 
2.14.1

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

[PATCH 2/3] drm/amdgpu: fix "use bulk moves for efficient VM LRU handling" v2

[Christian König]

First step to fix the LRU corruption, we accidentially tried to move things
on the LRU after dropping the lock.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index dd734970e167..349dcc37ee64 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1237,6 +1237,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	ring = to_amdgpu_ring(entity->rq->sched);
 	amdgpu_ring_priority_get(ring, priority);
 
+	amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
+
 	ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
 	amdgpu_mn_unlock(p->mn);
 
@@ -1258,7 +1260,6 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	union drm_amdgpu_cs *cs = data;
 	struct amdgpu_cs_parser parser = {};
 	bool reserved_buffers = false;
-	struct amdgpu_fpriv *fpriv;
 	int i, r;
 
 	if (!adev->accel_working)
@@ -1303,8 +1304,6 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 
 	r = amdgpu_cs_submit(&parser, cs);
 
-	fpriv = filp->driver_priv;
-	amdgpu_vm_move_to_lru_tail(adev, &fpriv->vm);
 out:
 	amdgpu_cs_parser_fini(&parser, r, reserved_buffers);
 	return r;
-- 
2.14.1

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

[PATCH 3/3] drm/amdgpu: fix idle state and bulk_moveavle flag

[Christian König]

Add BOs to the idle state again and correctly clear the flag when
new BOs are added.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index f31fa351caba..d59222fb5931 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -156,12 +156,15 @@ static void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
 		return;
 	list_add_tail(&base->bo_list, &bo->va);
 
-	if (bo->tbo.type == ttm_bo_type_kernel)
-		list_move(&base->vm_status, &vm->relocated);
-
 	if (bo->tbo.resv != vm->root.base.bo->tbo.resv)
 		return;
 
+	vm->bulk_moveable = false;
+	if (bo->tbo.type == ttm_bo_type_kernel)
+		list_move(&base->vm_status, &vm->relocated);
+	else
+		list_move(&base->vm_status, &vm->idle);
+
 	if (bo->preferred_domains &
 	    amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type))
 		return;
@@ -1121,7 +1124,7 @@ int amdgpu_vm_update_directories(struct amdgpu_device *adev,
 					   struct amdgpu_vm_bo_base,
 					   vm_status);
 		bo_base->moved = false;
-		list_del_init(&bo_base->vm_status);
+		list_move(&bo_base->vm_status, &vm->idle);
 
 		bo = bo_base->bo->parent;
 		if (!bo)
@@ -2646,7 +2649,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		return r;
 
 	vm->pte_support_ats = false;
-	vm->bulk_moveable = true;
 
 	if (vm_context == AMDGPU_VM_CONTEXT_COMPUTE) {
 		vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
-- 
2.14.1

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

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Michel Dänzer]

On 2018-08-31 3:10 p.m., Christian König wrote:
> Staring at the function for six hours, just to essentially move one line
> of code.

That sucks, but the commit log should describe what the problem was and
how this patch solves it.


> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 35d53d81f486..138c98902033 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>  static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>  				    struct list_head *lru, bool is_swap)
>  {
> +	struct list_head *list;
>  	LIST_HEAD(entries);
>  	LIST_HEAD(before);
> -	struct list_head *list1, *list2;
>  
> -	list1 = is_swap ? &pos->last->swap : &pos->last->lru;
> -	list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> +	reservation_object_assert_held(pos->last->resv);
> +	list = is_swap ? &pos->last->swap : &pos->last->lru;
> +	list_cut_position(&entries, lru, list);
> +
> +	reservation_object_assert_held(pos->first->resv);
> +	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> +	list_cut_position(&before, &entries, list);

So the problem was that the first list_cut_position call could result in
list2 pointing to la-la-land? Good catch!


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Christian König]

Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
> On 2018-08-31 3:10 p.m., Christian König wrote:
>> Staring at the function for six hours, just to essentially move one line
>> of code.
> That sucks, but the commit log should describe what the problem was and
> how this patch solves it.
>
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index 35d53d81f486..138c98902033 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>   static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>>   				    struct list_head *lru, bool is_swap)
>>   {
>> +	struct list_head *list;
>>   	LIST_HEAD(entries);
>>   	LIST_HEAD(before);
>> -	struct list_head *list1, *list2;
>>   
>> -	list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>> -	list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>> +	reservation_object_assert_held(pos->last->resv);
>> +	list = is_swap ? &pos->last->swap : &pos->last->lru;
>> +	list_cut_position(&entries, lru, list);
>> +
>> +	reservation_object_assert_held(pos->first->resv);
>> +	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>> +	list_cut_position(&before, &entries, list);
> So the problem was that the first list_cut_position call could result in
> list2 pointing to la-la-land? Good catch!

Yes, exactly. Thought that would be obvious, but going to add that to 
the commit log.

Can I get a tested-by? You where much better at reproducing that than I'm.

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

Re: [PATCH 3/3] drm/amdgpu: fix idle state and bulk_moveavle flag

[Michel Dänzer]

On 2018-08-31 3:10 p.m., Christian König wrote:
> Add BOs to the idle state again and correctly clear the flag when
> new BOs are added.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Typo in the shortlog: bulk_moveavle -> bulk_moveable


The series is

Tested-by: Michel Dänzer <michel.daenzer@amd.com>

Thanks Christian!


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Zhang, Jerry (Junwei)]

On 08/31/2018 09:10 PM, Christian König wrote:
> Staring at the function for six hours, just to essentially move one line
> of code.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 35d53d81f486..138c98902033 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>   static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>   				    struct list_head *lru, bool is_swap)
>   {
> +	struct list_head *list;
>   	LIST_HEAD(entries);
>   	LIST_HEAD(before);
> -	struct list_head *list1, *list2;
>
> -	list1 = is_swap ? &pos->last->swap : &pos->last->lru;
> -	list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> +	reservation_object_assert_held(pos->last->resv);
> +	list = is_swap ? &pos->last->swap : &pos->last->lru;
> +	list_cut_position(&entries, lru, list);
> +
> +	reservation_object_assert_held(pos->first->resv);
> +	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> +	list_cut_position(&before, &entries, list);
>
> -	list_cut_position(&entries, lru, list1);
> -	list_cut_position(&before, &entries, list2);
>   	list_splice(&before, lru);
>   	list_splice_tail(&entries, lru);
>   }
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/3] drm/amdgpu: fix "use bulk moves for efficient VM LRU handling" v2

[Zhang, Jerry (Junwei)]

On 08/31/2018 09:10 PM, Christian König wrote:
> First step to fix the LRU corruption, we accidentially tried to move things
> on the LRU after dropping the lock.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index dd734970e167..349dcc37ee64 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1237,6 +1237,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>   	ring = to_amdgpu_ring(entity->rq->sched);
>   	amdgpu_ring_priority_get(ring, priority);
>
> +	amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);
> +
>   	ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence);
>   	amdgpu_mn_unlock(p->mn);
>
> @@ -1258,7 +1260,6 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	union drm_amdgpu_cs *cs = data;
>   	struct amdgpu_cs_parser parser = {};
>   	bool reserved_buffers = false;
> -	struct amdgpu_fpriv *fpriv;
>   	int i, r;
>
>   	if (!adev->accel_working)
> @@ -1303,8 +1304,6 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>
>   	r = amdgpu_cs_submit(&parser, cs);
>
> -	fpriv = filp->driver_priv;
> -	amdgpu_vm_move_to_lru_tail(adev, &fpriv->vm);
>   out:
>   	amdgpu_cs_parser_fini(&parser, r, reserved_buffers);
>   	return r;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Huang Rui]

On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote:
> Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
> >On 2018-08-31 3:10 p.m., Christian König wrote:
> >>Staring at the function for six hours, just to essentially move one line
> >>of code.
> >That sucks, but the commit log should describe what the problem was and
> >how this patch solves it.
> >
> >
> >>Signed-off-by: Christian König <christian.koenig@amd.com>
> >>---
> >>  drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> >>index 35d53d81f486..138c98902033 100644
> >>--- a/drivers/gpu/drm/ttm/ttm_bo.c
> >>+++ b/drivers/gpu/drm/ttm/ttm_bo.c
> >>@@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
> >>  static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
> >>  				    struct list_head *lru, bool is_swap)
> >>  {
> >>+	struct list_head *list;
> >>  	LIST_HEAD(entries);
> >>  	LIST_HEAD(before);
> >>-	struct list_head *list1, *list2;
> >>-	list1 = is_swap ? &pos->last->swap : &pos->last->lru;
> >>-	list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> >>+	reservation_object_assert_held(pos->last->resv);
> >>+	list = is_swap ? &pos->last->swap : &pos->last->lru;
> >>+	list_cut_position(&entries, lru, list);
> >>+
> >>+	reservation_object_assert_held(pos->first->resv);
> >>+	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> >>+	list_cut_position(&before, &entries, list);
> >So the problem was that the first list_cut_position call could result in
> >list2 pointing to la-la-land? Good catch!
> 
> Yes, exactly. Thought that would be obvious, but going to add that
> to the commit log.
> 
> Can I get a tested-by? You where much better at reproducing that than I'm.
> 

Michel, Christian, thanks so much to take care of this when I was on
vacation. And sorry to let you take a long time for finding the cause.

Is that because I didn't hold the resveration before cut the list from
position "first" and "last"? May I know in which cases, we must hold the
bo's reservation firstly?

Thanks,
Ray


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

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Christian König]

Am 06.09.2018 um 12:02 schrieb Huang Rui:
> On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote:
>> Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
>>> On 2018-08-31 3:10 p.m., Christian König wrote:
>>>> Staring at the function for six hours, just to essentially move one line
>>>> of code.
>>> That sucks, but the commit log should describe what the problem was and
>>> how this patch solves it.
>>>
>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>>>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index 35d53d81f486..138c98902033 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>>>   static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>>>>   				    struct list_head *lru, bool is_swap)
>>>>   {
>>>> +	struct list_head *list;
>>>>   	LIST_HEAD(entries);
>>>>   	LIST_HEAD(before);
>>>> -	struct list_head *list1, *list2;
>>>> -	list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>>>> -	list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>>> +	reservation_object_assert_held(pos->last->resv);
>>>> +	list = is_swap ? &pos->last->swap : &pos->last->lru;
>>>> +	list_cut_position(&entries, lru, list);
>>>> +
>>>> +	reservation_object_assert_held(pos->first->resv);
>>>> +	list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>>> +	list_cut_position(&before, &entries, list);
>>> So the problem was that the first list_cut_position call could result in
>>> list2 pointing to la-la-land? Good catch!
>> Yes, exactly. Thought that would be obvious, but going to add that
>> to the commit log.
>>
>> Can I get a tested-by? You where much better at reproducing that than I'm.
>>
> Michel, Christian, thanks so much to take care of this when I was on
> vacation. And sorry to let you take a long time for finding the cause.
>
> Is that because I didn't hold the resveration before cut the list from
> position "first" and "last"?

Yes, that was one problem. Another was that the cutting code was buggy 
and determined one of the positions to cut at the wrong time.

>   May I know in which cases, we must hold the
> bo's reservation firstly?

BOs are reserved to prevent moving them. E.g. when the BO isn't reserved 
it can move around and so the LRU where you want to add/remove it could 
change.

Christian.

>
> Thanks,
> Ray
>
>

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

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Huang, Ray]

[-- Attachment #1: Type: text/plain, Size: 4061 bytes --]


>> Yes, that was one problem. Another was that the cutting code was buggy 
>>  and determined one of the positions to cut at the wrong time.

I went through again about the list cutting behavior and wrote down with the attached picture.
After do the second list_cut_position, the list2 should be point the end of "before" list. And list2 won't be used anymore after list cutting. May I know am I something missed?  

Thanks,
Ray

From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
Sent: Thursday, September 6, 2018 6:06 PM
To: Huang, Ray
Cc: Michel Dänzer; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper
  

Am 06.09.2018 um 12:02 schrieb Huang Rui:
> On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote:
>> Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
>>> On 2018-08-31 3:10 p.m., Christian König wrote:
>>>> Staring at the function for six hours, just to essentially move one line
>>>> of code.
>>> That sucks, but the commit log should describe what the problem was and
>>> how this patch solves it.
>>>
>>>
>>>> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>>>> ---
>>>>   drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>>>>   1 file changed, 8 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> index 35d53d81f486..138c98902033 100644
>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>> @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>>>   static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>>>>                                 struct list_head *lru, bool is_swap)
>>>>   {
>>>> +  struct list_head *list;
>>>>     LIST_HEAD(entries);
>>>>     LIST_HEAD(before);
>>>> -  struct list_head *list1, *list2;
>>>> -  list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>>>> -  list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>>> +  reservation_object_assert_held(pos->last->resv);
>>>> +  list = is_swap ? &pos->last->swap : &pos->last->lru;
>>>> +  list_cut_position(&entries, lru, list);
>>>> +
>>>> +  reservation_object_assert_held(pos->first->resv);
>>>> +  list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>>> +  list_cut_position(&before, &entries, list);
>>> So the problem was that the first list_cut_position call could result in
>>> list2 pointing to la-la-land? Good catch!
>> Yes, exactly. Thought that would be obvious, but going to add that
>> to the commit log.
>>
>> Can I get a tested-by? You where much better at reproducing that than I'm.
>>
> Michel, Christian, thanks so much to take care of this when I was on
> vacation. And sorry to let you take a long time for finding the cause.
>
> Is that because I didn't hold the resveration before cut the list from
> position "first" and "last"?

Yes, that was one problem. Another was that the cutting code was buggy 
and determined one of the positions to cut at the wrong time.

>   May I know in which cases, we must hold the
> bo's reservation firstly?

BOs are reserved to prevent moving them. E.g. when the BO isn't reserved 
it can move around and so the LRU where you want to add/remove it could 
change.

Christian.

>
> Thanks,
> Ray
>
>

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


amd-gfx Info Page - freedesktop.org
lists.freedesktop.org
To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to amd-gfx-PD4FTy7X32lNgt0PjOBp9xlNPtJONSTn@public.gmane.org You can subscribe to the list, or change your existing subscription, in the sections below.

    

[-- Attachment #2: list_cutting.jpg --]
[-- Type: image/jpeg, Size: 107697 bytes --]

[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Christian König]

[-- Attachment #1.1: Type: text/plain, Size: 4793 bytes --]

Am 07.09.2018 um 13:02 schrieb Huang, Ray:
>>>   Yes, that was one problem. Another was that the cutting code was buggy
>>>    and determined one of the positions to cut at the wrong time.
> I went through again about the list cutting behavior and wrote down with the attached picture.
> After do the second list_cut_position, the list2 should be point the end of "before" list. And list2 won't be used anymore after list cutting. May I know am I something missed?

Let's take a look at the original code:
> list1 = is_swap ? &pos->last->swap : &pos->last->lru;
> list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>
> list_cut_position(&entries, lru, list1);
> list_cut_position(&before, &entries, list2);

As far as I can see the problem is that the first list_cur_position 
could modify the value of pos->first->lru.prev and so make the second 
list_cut_position work on the wrong position.

Regards,
Christian.

>
> Thanks,
> Ray
>
> From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
> Sent: Thursday, September 6, 2018 6:06 PM
> To: Huang, Ray
> Cc: Michel Dänzer; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> Subject: Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper
>    
>
> Am 06.09.2018 um 12:02 schrieb Huang Rui:
>> On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote:
>>> Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
>>>> On 2018-08-31 3:10 p.m., Christian König wrote:
>>>>> Staring at the function for six hours, just to essentially move one line
>>>>> of code.
>>>> That sucks, but the commit log should describe what the problem was and
>>>> how this patch solves it.
>>>>
>>>>
>>>>> Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>>>>> ---
>>>>>     drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>>>>>     1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>> index 35d53d81f486..138c98902033 100644
>>>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>>>> @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>>>>     static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>>>>>                                   struct list_head *lru, bool is_swap)
>>>>>     {
>>>>> +  struct list_head *list;
>>>>>       LIST_HEAD(entries);
>>>>>       LIST_HEAD(before);
>>>>> -  struct list_head *list1, *list2;
>>>>> -  list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>>>>> -  list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>>>> +  reservation_object_assert_held(pos->last->resv);
>>>>> +  list = is_swap ? &pos->last->swap : &pos->last->lru;
>>>>> +  list_cut_position(&entries, lru, list);
>>>>> +
>>>>> +  reservation_object_assert_held(pos->first->resv);
>>>>> +  list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>>>> +  list_cut_position(&before, &entries, list);
>>>> So the problem was that the first list_cut_position call could result in
>>>> list2 pointing to la-la-land? Good catch!
>>> Yes, exactly. Thought that would be obvious, but going to add that
>>> to the commit log.
>>>
>>> Can I get a tested-by? You where much better at reproducing that than I'm.
>>>
>> Michel, Christian, thanks so much to take care of this when I was on
>> vacation. And sorry to let you take a long time for finding the cause.
>>
>> Is that because I didn't hold the resveration before cut the list from
>> position "first" and "last"?
> Yes, that was one problem. Another was that the cutting code was buggy
> and determined one of the positions to cut at the wrong time.
>
>>     May I know in which cases, we must hold the
>> bo's reservation firstly?
> BOs are reserved to prevent moving them. E.g. when the BO isn't reserved
> it can move around and so the LRU where you want to add/remove it could
> change.
>
> Christian.
>
>> Thanks,
>> Ray
>>
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
> amd-gfx Info Page - freedesktop.org
> lists.freedesktop.org
> To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to amd-gfx-PD4FTy7X32lNgt0PjOBp9xlNPtJONSTn@public.gmane.org You can subscribe to the list, or change your existing subscription, in the sections below.
>
>      
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 7434 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Huang Rui]

[-- Attachment #1: Type: text/plain, Size: 6406 bytes --]

On Fri, Sep 07, 2018 at 07:18:59PM +0800, Christian K�nig wrote:
> Am 07.09.2018 um 13:02 schrieb Huang, Ray:
> 
>              Yes, that was one problem. Another was that the cutting code was buggy 
>               and determined one of the positions to cut at the wrong time.
> 
>     I went through again about the list cutting behavior and wrote down with the attached picture.
>     After do the second list_cut_position, the list2 should be point the end of "before" list. And list2 won't be used anymore after list cutting. May I know am I something missed? 
> 
> 
> Let's take a look at the original code:
> 
>     list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>     list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>      
>     list_cut_position(&entries, lru, list1);
>     list_cut_position(&before, &entries, list2);
> 
> 
> As far as I can see the problem is that the first list_cur_position could
> modify the value of pos->first->lru.prev and so make the second
> list_cut_position work on the wrong position.
> 

I think I understood. In this case (the first element of LRU ==
pos->first->lru), please see the picture. If we store first->lru.prev as
list2(LRU head), after do list cutting, the first->lru.prev will overwrite
as new head (entries), however, the orignal list2 will still point previous
head (that is the wrong position now). We actually expected to use the
latest first->lru.prev as the second cutting position. So we should adjust
code sequence like below:

list1 = is_swap ? &pos->last->swap : &pos->last->lru;
list_cut_position(&entries, lru, list1);

list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
list_cut_position(&before, &entries, list2);

Am I understanding right?

Thanks,
Ray

> Regards,
> Christian.
> 
> 
> 
>     Thanks,
>     Ray
> 
>     From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>     Sent: Thursday, September 6, 2018 6:06 PM
>     To: Huang, Ray
>     Cc: Michel Dänzer; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>     Subject: Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper
>      
> 
>     Am 06.09.2018 um 12:02 schrieb Huang Rui:
> 
>         On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote:
> 
>             Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
> 
>                 On 2018-08-31 3:10 p.m., Christian König wrote:
> 
>                     Staring at the function for six hours, just to essentially move one line
>                     of code.
> 
>                 That sucks, but the commit log should describe what the problem was and
>                 how this patch solves it.
> 
> 
> 
>                     Signed-off-by: Christian König <christian.koenig-5C7GfCeVMHo@public.gmane.org>
>                     ---
>                        drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>                        1 file changed, 8 insertions(+), 5 deletions(-)
> 
>                     diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>                     index 35d53d81f486..138c98902033 100644
>                     --- a/drivers/gpu/drm/ttm/ttm_bo.c
>                     +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>                     @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>                        static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>                                                      struct list_head *lru, bool is_swap)
>                        {
>                     +  struct list_head *list;
>                          LIST_HEAD(entries);
>                          LIST_HEAD(before);
>                     -  struct list_head *list1, *list2;
>                     -  list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>                     -  list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>                     +  reservation_object_assert_held(pos->last->resv);
>                     +  list = is_swap ? &pos->last->swap : &pos->last->lru;
>                     +  list_cut_position(&entries, lru, list);
>                     +
>                     +  reservation_object_assert_held(pos->first->resv);
>                     +  list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>                     +  list_cut_position(&before, &entries, list);
> 
>                 So the problem was that the first list_cut_position call could result in
>                 list2 pointing to la-la-land? Good catch!
> 
>             Yes, exactly. Thought that would be obvious, but going to add that
>             to the commit log.
> 
>             Can I get a tested-by? You where much better at reproducing that than I'm.
> 
> 
>         Michel, Christian, thanks so much to take care of this when I was on
>         vacation. And sorry to let you take a long time for finding the cause.
> 
>         Is that because I didn't hold the resveration before cut the list from
>         position "first" and "last"?
> 
>     Yes, that was one problem. Another was that the cutting code was buggy
>     and determined one of the positions to cut at the wrong time.
> 
> 
>            May I know in which cases, we must hold the
>         bo's reservation firstly?
> 
>     BOs are reserved to prevent moving them. E.g. when the BO isn't reserved
>     it can move around and so the LRU where you want to add/remove it could
>     change.
> 
>     Christian.
> 
> 
>         Thanks,
>         Ray
> 
> 
> 
>     _______________________________________________
>     amd-gfx mailing list
>     amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> 
>     amd-gfx Info Page - freedesktop.org
>     lists.freedesktop.org
>     To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to amd-gfx-PD4FTy7X32lNgt0PjOBp9xlNPtJONSTn@public.gmane.org You can subscribe to the list, or change your existing subscription, in the sections below.
> 
> 
> 
> 
>    
>     _______________________________________________
>     amd-gfx mailing list
>     amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
> 

[-- Attachment #2: Wrong_case.jpg --]
[-- Type: image/jpeg, Size: 70757 bytes --]

[-- Attachment #3: Type: text/plain, Size: 154 bytes --]

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

Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper

[Christian König]

Am 08.09.2018 um 09:54 schrieb Huang Rui:
> On Fri, Sep 07, 2018 at 07:18:59PM +0800, Christian K�nig wrote:
>> Am 07.09.2018 um 13:02 schrieb Huang, Ray:
>>
>>               Yes, that was one problem. Another was that the cutting code was buggy
>>                and determined one of the positions to cut at the wrong time.
>>
>>      I went through again about the list cutting behavior and wrote down with the attached picture.
>>      After do the second list_cut_position, the list2 should be point the end of "before" list. And list2 won't be used anymore after list cutting. May I know am I something missed?
>>
>>
>> Let's take a look at the original code:
>>
>>      list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>>      list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>       
>>      list_cut_position(&entries, lru, list1);
>>      list_cut_position(&before, &entries, list2);
>>
>>
>> As far as I can see the problem is that the first list_cur_position could
>> modify the value of pos->first->lru.prev and so make the second
>> list_cut_position work on the wrong position.
>>
> I think I understood. In this case (the first element of LRU ==
> pos->first->lru), please see the picture. If we store first->lru.prev as
> list2(LRU head), after do list cutting, the first->lru.prev will overwrite
> as new head (entries), however, the orignal list2 will still point previous
> head (that is the wrong position now). We actually expected to use the
> latest first->lru.prev as the second cutting position. So we should adjust
> code sequence like below:
>
> list1 = is_swap ? &pos->last->swap : &pos->last->lru;
> list_cut_position(&entries, lru, list1);
>
> list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
> list_cut_position(&before, &entries, list2);
>
> Am I understanding right?

Yes, exactly. And the fix looks like what I did as well. Just only used 
one variable and added some extra BUG_ON().

Christian.

>
> Thanks,
> Ray
>
>> Regards,
>> Christian.
>>
>>
>>
>>      Thanks,
>>      Ray
>>
>>      From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Christian König <christian.koenig@amd.com>
>>      Sent: Thursday, September 6, 2018 6:06 PM
>>      To: Huang, Ray
>>      Cc: Michel Dänzer; amd-gfx@lists.freedesktop.org
>>      Subject: Re: [PATCH 1/3] drm/ttm: fix ttm_bo_bulk_move_helper
>>       
>>
>>      Am 06.09.2018 um 12:02 schrieb Huang Rui:
>>
>>          On Fri, Aug 31, 2018 at 05:17:33PM +0200, Christian König wrote:
>>
>>              Am 31.08.2018 um 17:15 schrieb Michel Dänzer:
>>
>>                  On 2018-08-31 3:10 p.m., Christian König wrote:
>>
>>                      Staring at the function for six hours, just to essentially move one line
>>                      of code.
>>
>>                  That sucks, but the commit log should describe what the problem was and
>>                  how this patch solves it.
>>
>>
>>
>>                      Signed-off-by: Christian König <christian.koenig@amd.com>
>>                      ---
>>                         drivers/gpu/drm/ttm/ttm_bo.c | 13 ++++++++-----
>>                         1 file changed, 8 insertions(+), 5 deletions(-)
>>
>>                      diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>>                      index 35d53d81f486..138c98902033 100644
>>                      --- a/drivers/gpu/drm/ttm/ttm_bo.c
>>                      +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>>                      @@ -250,15 +250,18 @@ EXPORT_SYMBOL(ttm_bo_move_to_lru_tail);
>>                         static void ttm_bo_bulk_move_helper(struct ttm_lru_bulk_move_pos *pos,
>>                                                       struct list_head *lru, bool is_swap)
>>                         {
>>                      +  struct list_head *list;
>>                           LIST_HEAD(entries);
>>                           LIST_HEAD(before);
>>                      -  struct list_head *list1, *list2;
>>                      -  list1 = is_swap ? &pos->last->swap : &pos->last->lru;
>>                      -  list2 = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>                      +  reservation_object_assert_held(pos->last->resv);
>>                      +  list = is_swap ? &pos->last->swap : &pos->last->lru;
>>                      +  list_cut_position(&entries, lru, list);
>>                      +
>>                      +  reservation_object_assert_held(pos->first->resv);
>>                      +  list = is_swap ? pos->first->swap.prev : pos->first->lru.prev;
>>                      +  list_cut_position(&before, &entries, list);
>>
>>                  So the problem was that the first list_cut_position call could result in
>>                  list2 pointing to la-la-land? Good catch!
>>
>>              Yes, exactly. Thought that would be obvious, but going to add that
>>              to the commit log.
>>
>>              Can I get a tested-by? You where much better at reproducing that than I'm.
>>
>>
>>          Michel, Christian, thanks so much to take care of this when I was on
>>          vacation. And sorry to let you take a long time for finding the cause.
>>
>>          Is that because I didn't hold the resveration before cut the list from
>>          position "first" and "last"?
>>
>>      Yes, that was one problem. Another was that the cutting code was buggy
>>      and determined one of the positions to cut at the wrong time.
>>
>>
>>             May I know in which cases, we must hold the
>>          bo's reservation firstly?
>>
>>      BOs are reserved to prevent moving them. E.g. when the BO isn't reserved
>>      it can move around and so the LRU where you want to add/remove it could
>>      change.
>>
>>      Christian.
>>
>>
>>          Thanks,
>>          Ray
>>
>>
>>
>>      _______________________________________________
>>      amd-gfx mailing list
>>      amd-gfx@lists.freedesktop.org
>>      https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
>>      amd-gfx Info Page - freedesktop.org
>>      lists.freedesktop.org
>>      To see the collection of prior postings to the list, visit the amd-gfx Archives.. Using amd-gfx: To post a message to all the list members, send email to amd-gfx@lists.freedesktop.org. You can subscribe to the list, or change your existing subscription, in the sections below.
>>
>>
>>
>>
>>     
>>      _______________________________________________
>>      amd-gfx mailing list
>>      amd-gfx@lists.freedesktop.org
>>      https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>

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