On 11/18/2011 06:32 PM, j.glisse at gmail.com wrote:
> From: Jerome Glisse<jglisse at 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 at 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
