[PATCH] drm/nouveau: fix takedown in move_notify

[PATCH] drm/nouveau: fix takedown in move_notify

[Maarten Lankhorst]

move_notify is called by ttm after the the object is idle and about
to be destroyed. Clean up the vm list properly in that case.

This is not a problem right now, since the list should already be
empty, but if it wasn't empty, vm_put was not called which leads to
random corruption later.

With this fix, nouveau_gem_object_close can be safely changed to a noop,
forcing the vm bindings to be removed when the original object is. This
is not done in this patch since it may lead to the object staying mapped
in the vm space until the gem object refcount drops to 0. This shouldn't
be a big issue however.

If we choose to do so does allow us to use ttm's delayed destruction
mechanism to unmap vm after object is idle, resulting in ioread32 no
longer taking up 30% of cpu in Team Fortress 2 if I don't do the vma
unmap in nouveau_gem_object_close.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>

---
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 35ac57f..e8a47f0 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1139,12 +1139,22 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
 	if (bo->destroy != nouveau_bo_del_ttm)
 		return;
 
+	if (!new_mem) {
+		while (!list_empty(&nvbo->vma_list)) {
+			vma = list_first_entry(&nvbo->vma_list, struct nouveau_vma, head);
+
+			nouveau_vm_unmap(vma);
+			nouveau_vm_put(vma);
+			list_del(&vma->head);
+		}
+		return;
+	}
+
 	list_for_each_entry(vma, &nvbo->vma_list, head) {
-		if (new_mem && new_mem->mem_type == TTM_PL_VRAM) {
+		if (new_mem->mem_type == TTM_PL_VRAM) {
 			nouveau_vm_map(vma, new_mem->mm_node);
-		} else
-		if (new_mem && new_mem->mem_type == TTM_PL_TT &&
-		    nvbo->page_shift == vma->vm->vmm->spg_shift) {
+		} else if (new_mem->mem_type == TTM_PL_TT &&
+		           nvbo->page_shift == vma->vm->vmm->spg_shift) {
 			if (((struct nouveau_mem *)new_mem->mm_node)->sg)
 				nouveau_vm_map_sg_table(vma, 0, new_mem->
 						  num_pages << PAGE_SHIFT,
@@ -1153,8 +1163,6 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
 				nouveau_vm_map_sg(vma, 0, new_mem->
 						  num_pages << PAGE_SHIFT,
 						  new_mem->mm_node);
-		} else {
-			nouveau_vm_unmap(vma);
 		}
 	}
 }

Re: [PATCH] drm/nouveau: fix takedown in move_notify

[Maarten Lankhorst]

Op 21-11-12 14:15, Maarten Lankhorst schreef:
> With this fix, nouveau_gem_object_close can be safely changed to a noop,
> forcing the vm bindings to be removed when the original object is. This
> is not done in this patch since it may lead to the object staying mapped
> in the vm space until the gem object refcount drops to 0. This shouldn't
> be a big issue however.
>
> If we choose to do so does allow us to use ttm's delayed destruction
> mechanism to unmap vm after object is idle, resulting in ioread32 no
> longer taking up 30% of cpu in Team Fortress 2 if I don't do the vma
> unmap in nouveau_gem_object_close.

And for those crazy enough to try, deliberately in the most hacky form possible:

drm/nouveau: use ttm delayed destroy for unmapping vm

According to perf top, drops ioread32 from taking 30% cpu to 3% cpu..

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c
index 5e2f521..bdb99ab 100644
--- a/drivers/gpu/drm/nouveau/nouveau_gem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
@@ -111,7 +111,7 @@ nouveau_gem_object_close(struct drm_gem_object *gem, struct drm_file *file_priv)
 	struct nouveau_vma *vma;
 	int ret;
 
-	if (!cli->base.vm)
+//	if (!cli->base.vm)
 		return;
 
 	ret = ttm_bo_reserve(&nvbo->bo, false, false, false, 0);

Re: [PATCH] drm/nouveau: fix takedown in move_notify

[Emil Velikov]

On 21/11/12 13:15, Maarten Lankhorst wrote:
> move_notify is called by ttm after the the object is idle and about
> to be destroyed. Clean up the vm list properly in that case.
> 
> This is not a problem right now, since the list should already be
> empty, but if it wasn't empty, vm_put was not called which leads to
> random corruption later.
> 
> With this fix, nouveau_gem_object_close can be safely changed to a noop,
> forcing the vm bindings to be removed when the original object is. This
> is not done in this patch since it may lead to the object staying mapped
> in the vm space until the gem object refcount drops to 0. This shouldn't
> be a big issue however.
> 
> If we choose to do so does allow us to use ttm's delayed destruction
> mechanism to unmap vm after object is idle, resulting in ioread32 no
> longer taking up 30% of cpu in Team Fortress 2 if I don't do the vma
> unmap in nouveau_gem_object_close.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> 
Hi Maarten

I've noticed ioread32 of up-to 40% on of cpu my system. With your patch
if goes down to ~3% with no side effects. Frame-rate appears to be
slightly improved, although it's hard to say.

Is there any reason for holding this patch ?
For what it's worth you can add

Tested-by: Emil Velikov <emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> ---
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 35ac57f..e8a47f0 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -1139,12 +1139,22 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
>  	if (bo->destroy != nouveau_bo_del_ttm)
>  		return;
>  
> +	if (!new_mem) {
> +		while (!list_empty(&nvbo->vma_list)) {
> +			vma = list_first_entry(&nvbo->vma_list, struct nouveau_vma, head);
> +
> +			nouveau_vm_unmap(vma);
> +			nouveau_vm_put(vma);
> +			list_del(&vma->head);
> +		}
> +		return;
> +	}
> +
>  	list_for_each_entry(vma, &nvbo->vma_list, head) {
> -		if (new_mem && new_mem->mem_type == TTM_PL_VRAM) {
> +		if (new_mem->mem_type == TTM_PL_VRAM) {
>  			nouveau_vm_map(vma, new_mem->mm_node);
> -		} else
> -		if (new_mem && new_mem->mem_type == TTM_PL_TT &&
> -		    nvbo->page_shift == vma->vm->vmm->spg_shift) {
> +		} else if (new_mem->mem_type == TTM_PL_TT &&
> +		           nvbo->page_shift == vma->vm->vmm->spg_shift) {
>  			if (((struct nouveau_mem *)new_mem->mm_node)->sg)
>  				nouveau_vm_map_sg_table(vma, 0, new_mem->
>  						  num_pages << PAGE_SHIFT,
> @@ -1153,8 +1163,6 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
>  				nouveau_vm_map_sg(vma, 0, new_mem->
>  						  num_pages << PAGE_SHIFT,
>  						  new_mem->mm_node);
> -		} else {
> -			nouveau_vm_unmap(vma);
>  		}
>  	}
>  }
> 
> _______________________________________________
> Nouveau mailing list
> Nouveau-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau
> 

Re: [Nouveau] [PATCH] drm/nouveau: fix takedown in move_notify

[Maarten Lankhorst]

Op 10-12-12 17:39, Emil Velikov schreef:
> On 21/11/12 13:15, Maarten Lankhorst wrote:
>> move_notify is called by ttm after the the object is idle and about
>> to be destroyed. Clean up the vm list properly in that case.
>>
>> This is not a problem right now, since the list should already be
>> empty, but if it wasn't empty, vm_put was not called which leads to
>> random corruption later.
>>
>> With this fix, nouveau_gem_object_close can be safely changed to a noop,
>> forcing the vm bindings to be removed when the original object is. This
>> is not done in this patch since it may lead to the object staying mapped
>> in the vm space until the gem object refcount drops to 0. This shouldn't
>> be a big issue however.
>>
>> If we choose to do so does allow us to use ttm's delayed destruction
>> mechanism to unmap vm after object is idle, resulting in ioread32 no
>> longer taking up 30% of cpu in Team Fortress 2 if I don't do the vma
>> unmap in nouveau_gem_object_close.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
>>
> Hi Maarten
>
> I've noticed ioread32 of up-to 40% on of cpu my system. With your patch
> if goes down to ~3% with no side effects. Frame-rate appears to be
> slightly improved, although it's hard to say.
>
> Is there any reason for holding this patch ?
> For what it's worth you can add
>
> Tested-by: Emil Velikov <emil.l.velikov@gmail.com>
Well 2 reasons..

1) In the current form, you're guaranteed to cause memory corruption on forced client takedown. Locking needs more thought.
The warns you get are very real errors. ;-)

>> ---
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
>> index 35ac57f..e8a47f0 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
>> @@ -1139,12 +1139,22 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
>>  	if (bo->destroy != nouveau_bo_del_ttm)
>>  		return;
>>  
>> +	if (!new_mem) {
>> +		while (!list_empty(&nvbo->vma_list)) {
>> +			vma = list_first_entry(&nvbo->vma_list, struct nouveau_vma, head);
>> +
>> +			nouveau_vm_unmap(vma);
>> +			nouveau_vm_put(vma);
>> +			list_del(&vma->head);
2. I missed a kfree here ;-)

~Maarten