[PATCH] drm/ttm: callback move_notify any time bo placement change v2

[PATCH] drm/ttm: callback move_notify any time bo placement change v2

[j.glisse]

From: Jerome Glisse <jglisse@redhat.com>

Previously we were calling back move_notify in error path when the
bo is returned to it's original position or when destroy the bo.
When destroying the bo set the new mem placement as NULL when calling
back in the driver.

Updating nouveau to deal with NULL placement properly.

v2: reserve the object before calling move_notify in bo destroy path
    at that point ttm should be the only piece of code interacting
    with the object so atomic_set is safe here.

Reviewed-by: Jerome Glisse <jglisse@redhat.com>
---
 drivers/gpu/drm/nouveau/nouveau_bo.c |    4 ++--
 drivers/gpu/drm/ttm/ttm_bo.c         |    9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
index 857bca4..f12dd0f 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -815,10 +815,10 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
 	struct nouveau_vma *vma;
 
 	list_for_each_entry(vma, &nvbo->vma_list, head) {
-		if (new_mem->mem_type == TTM_PL_VRAM) {
+		if (new_mem && new_mem->mem_type == TTM_PL_VRAM) {
 			nouveau_vm_map(vma, new_mem->mm_node);
 		} else
-		if (new_mem->mem_type == TTM_PL_TT &&
+		if (new_mem && new_mem->mem_type == TTM_PL_TT &&
 		    nvbo->page_shift == vma->vm->spg_shift) {
 			nouveau_vm_map_sg(vma, 0, new_mem->
 					  num_pages << PAGE_SHIFT,
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index de7ad99..c15bcc3 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -147,6 +147,12 @@ static void ttm_bo_release_list(struct kref *list_kref)
 	BUG_ON(!list_empty(&bo->lru));
 	BUG_ON(!list_empty(&bo->ddestroy));
 
+	/* force bo to reserved, at this point we should be the only owner */
+	atomic_set(&bo->reserved, 1);
+	if (bdev->driver->move_notify)
+		bdev->driver->move_notify(bo, NULL);
+	atomic_set(&bo->reserved, 0);
+
 	if (bo->ttm)
 		ttm_tt_destroy(bo->ttm);
 	atomic_dec(&bo->glob->bo_count);
@@ -437,6 +443,9 @@ moved:
 	return 0;
 
 out_err:
+	if (bdev->driver->move_notify)
+		bdev->driver->move_notify(bo, &bo->mem);
+
 	new_man = &bdev->man[bo->mem.mem_type];
 	if ((new_man->flags & TTM_MEMTYPE_FLAG_FIXED) && bo->ttm) {
 		ttm_tt_unbind(bo->ttm);
-- 
1.7.7.1

Re: [PATCH] drm/ttm: callback move_notify any time bo placement change v2

[Thomas Hellstrom]

On 11/18/2011 06:32 PM, j.glisse@gmail.com wrote:
> From: Jerome Glisse<jglisse@redhat.com>
>
> Previously we were calling back move_notify in error path when the
> bo is returned to it's original position or when destroy the bo.
> When destroying the bo set the new mem placement as NULL when calling
> back in the driver.
>
> Updating nouveau to deal with NULL placement properly.
>
> v2: reserve the object before calling move_notify in bo destroy path
>      at that point ttm should be the only piece of code interacting
>      with the object so atomic_set is safe here.
>
> Reviewed-by: Jerome Glisse<jglisse@redhat.com>
> ---
>   drivers/gpu/drm/nouveau/nouveau_bo.c |    4 ++--
>   drivers/gpu/drm/ttm/ttm_bo.c         |    9 +++++++++
>   2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 857bca4..f12dd0f 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -815,10 +815,10 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
>   	struct nouveau_vma *vma;
>
>   	list_for_each_entry(vma,&nvbo->vma_list, head) {
> -		if (new_mem->mem_type == TTM_PL_VRAM) {
> +		if (new_mem&&  new_mem->mem_type == TTM_PL_VRAM) {
>   			nouveau_vm_map(vma, new_mem->mm_node);
>   		} else
> -		if (new_mem->mem_type == TTM_PL_TT&&
> +		if (new_mem&&  new_mem->mem_type == TTM_PL_TT&&
>   		nvbo->page_shift == vma->vm->spg_shift) {
>   			nouveau_vm_map_sg(vma, 0, new_mem->
>   					num_pages<<  PAGE_SHIFT,
>    

Is Nouveau the only user of move_notify.


> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index de7ad99..c15bcc3 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -147,6 +147,12 @@ static void ttm_bo_release_list(struct kref *list_kref)
>   	BUG_ON(!list_empty(&bo->lru));
>   	BUG_ON(!list_empty(&bo->ddestroy));
>
> +	/* force bo to reserved, at this point we should be the only owner */
> +	atomic_set(&bo->reserved, 1);
> +	if (bdev->driver->move_notify)
> +		bdev->driver->move_notify(bo, NULL);
> +	atomic_set(&bo->reserved, 0);
> +
>    

I think this is good for now, but not for generic use. Multiple GPU maps 
*may* have their own LRU lists and if they do, this needs to be done 
earlier, like in ttm_bo_cleanup_refs_or_queue.


>   	if (bo->ttm)
>   		ttm_tt_destroy(bo->ttm);
>   	atomic_dec(&bo->glob->bo_count);
> @@ -437,6 +443,9 @@ moved:
>   	return 0;
>
>   out_err:
> +	if (bdev->driver->move_notify)
> +		bdev->driver->move_notify(bo,&bo->mem);
> +
>    

Looks OK, but see the mail just sent to Ben. I don't think move_notify 
should set up virtual GPU mappings. Only tear them down, and the driver 
should set them up if needed once placement is finally determined.

>   	new_man =&bdev->man[bo->mem.mem_type];
>   	if ((new_man->flags&  TTM_MEMTYPE_FLAG_FIXED)&&  bo->ttm) {
>   		ttm_tt_unbind(bo->ttm);
>    

What about ttm_bo_swapout? Should definietely tear down all GPU mappings.

/Thomas