From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH] drm/ttm: callback move_notify any time bo placement change v2 Date: Fri, 18 Nov 2011 23:59:20 +0100 Message-ID: <4EC6E348.3060901@vmware.com> References: <1321637557-8309-1-git-send-email-j.glisse@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-1.vmware.com (smtp-outbound-1.vmware.com [65.115.85.69]) by gabe.freedesktop.org (Postfix) with ESMTP id 9D45E9EDD7 for ; Fri, 18 Nov 2011 15:02:03 -0800 (PST) In-Reply-To: <1321637557-8309-1-git-send-email-j.glisse@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: j.glisse@gmail.com Cc: Jerome Glisse , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 11/18/2011 06:32 PM, j.glisse@gmail.com wrote: > From: Jerome Glisse > > 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 > --- > 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