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

[Thomas Hellstrom <thellstrom@vmware.com>]

On 11/19/2011 12:32 AM, 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.
> v3: callback move notify only once the bo is in its new position
>      call move notify want swaping out the buffer
>
> Reviewed-by: Jerome Glisse<jglisse@redhat.com>
> ---
>   drivers/gpu/drm/nouveau/nouveau_bo.c |    4 ++--
>   drivers/gpu/drm/ttm/ttm_bo.c         |   17 +++++++++++++----
>   2 files changed, 15 insertions(+), 6 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..0c1d821 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);
>    

We can actually do this from the top of ttm_bo_cleanup_memtype_use(). 
Then we should catch all current and future use-cases and you wouldn't 
need the fake reserving, because at that point, we're already reserved.

> +
>   	if (bo->ttm)
>   		ttm_tt_destroy(bo->ttm);
>   	atomic_dec(&bo->glob->bo_count);
> @@ -404,9 +410,6 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>   		}
>   	}
>
> -	if (bdev->driver->move_notify)
> -		bdev->driver->move_notify(bo, mem);
> -
>   	if (!(old_man->flags&  TTM_MEMTYPE_FLAG_FIXED)&&
>   	!(new_man->flags&  TTM_MEMTYPE_FLAG_FIXED))
>   		ret = ttm_bo_move_ttm(bo, evict, no_wait_reserve, no_wait_gpu, mem);
> @@ -419,6 +422,9 @@ static int ttm_bo_handle_move_mem(struct ttm_buffer_object *bo,
>   	if (ret)
>   		goto out_err;
>
> +	if (bdev->driver->move_notify)
> +		bdev->driver->move_notify(bo, mem);
> +
>    

>   moved:
>   	if (bo->evicted) {
>   		ret = bdev->driver->invalidate_caches(bdev, bo->mem.placement);
> @@ -1872,9 +1878,12 @@ static int ttm_bo_swapout(struct ttm_mem_shrink *shrink)
>   	if (bo->bdev->driver->swap_notify)
>   		bo->bdev->driver->swap_notify(bo);
>
> +	if (bo->bdev->driver->move_notify)
> +		bo->bdev->driver->move_notify(bo, NULL);
> +
>    

Hmm. On second thought, we could use swap_notify() for this, I missed we 
already had that and that's what vmwgfx once used for exactly the same 
purpose.


>   	ret = ttm_tt_swapout(bo->ttm, bo->persistent_swap_storage);
> -out:
>
> +out:
>    

Whitespace.

/Thomas