From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH] drm/radeon: fence virtual address and free it once idle v3 Date: Mon, 06 Aug 2012 18:06:00 +0200 Message-ID: <501FEB68.5030605@vodafone.de> References: <1344029208-10069-1-git-send-email-j.glisse@gmail.com> <501CE652.7080509@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 856F69E94A for ; Mon, 6 Aug 2012 09:06:06 -0700 (PDT) In-Reply-To: <501CE652.7080509@vodafone.de> 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 04.08.2012 11:07, Christian K=F6nig wrote: > On 03.08.2012 23:26, j.glisse@gmail.com wrote: >> From: Jerome Glisse >> >> Virtual address need to be fenced to know when we can safely remove it. >> This patch also properly clear the pagetable. Previously it was >> serouisly broken. >> >> Kernel 3.5/3.4 need a similar patch but adapted for difference in = >> mutex locking. >> >> v2: For to update pagetable when unbinding bo (don't bailout if >> bo_va->valid is true). >> v3: Add kernel 3.5/3.4 comment. >> >> Signed-off-by: Jerome Glisse > It should fix the problem, going to test it as soon as I find some = > 5min spare in my vacation. Till then it is: > > Reviewed-by: Christian K=F6nig > > In the future I would really like to make the updating of the PTE also = > async, that should save us allot of problems here. Had time today to test that a bit more. Found two minor notes in the = patch, see below. > > >> --- >> drivers/gpu/drm/radeon/radeon.h | 1 + >> drivers/gpu/drm/radeon/radeon_cs.c | 32 = >> +++++++++++++++++++++++++++++--- >> drivers/gpu/drm/radeon/radeon_gart.c | 24 ++++++++++++++++++++++-- >> drivers/gpu/drm/radeon/radeon_gem.c | 13 ++----------- >> drivers/gpu/drm/radeon/radeon_object.c | 6 +----- >> 5 files changed, 55 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/radeon.h = >> b/drivers/gpu/drm/radeon/radeon.h >> index 5431af2..8d75c65 100644 >> --- a/drivers/gpu/drm/radeon/radeon.h >> +++ b/drivers/gpu/drm/radeon/radeon.h >> @@ -300,6 +300,7 @@ struct radeon_bo_va { >> uint64_t soffset; >> uint64_t eoffset; >> uint32_t flags; >> + struct radeon_fence *fence; >> bool valid; >> }; >> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c = >> b/drivers/gpu/drm/radeon/radeon_cs.c >> index 8a4c49e..995f3ab 100644 >> --- a/drivers/gpu/drm/radeon/radeon_cs.c >> +++ b/drivers/gpu/drm/radeon/radeon_cs.c >> @@ -278,6 +278,30 @@ int radeon_cs_parser_init(struct = >> radeon_cs_parser *p, void *data) >> return 0; >> } >> +static void radeon_bo_vm_fence_va(struct radeon_cs_parser *parser, >> + struct radeon_fence *fence) >> +{ >> + struct radeon_fpriv *fpriv =3D parser->filp->driver_priv; >> + struct radeon_vm *vm =3D &fpriv->vm; >> + struct radeon_bo_list *lobj; >> + int r; "r" is unused here, please remove. >> + >> + if (parser->chunk_ib_idx =3D=3D -1) >> + return; >> + if ((parser->cs_flags & RADEON_CS_USE_VM) =3D=3D 0) >> + return; >> + >> + list_for_each_entry(lobj, &parser->validated, tv.head) { >> + struct radeon_bo_va *bo_va; >> + struct radeon_bo *rbo =3D lobj->bo; >> + >> + bo_va =3D radeon_bo_va(rbo, vm); >> + radeon_fence_unref(&bo_va->fence); >> + bo_va->fence =3D radeon_fence_ref(fence); >> + } >> + return 0; The function doesn't return an error value, so this should just be a = "return" without value. >> +} >> + >> /** >> * cs_parser_fini() - clean parser states >> * @parser: parser structure holding parsing context. >> @@ -290,11 +314,14 @@ static void radeon_cs_parser_fini(struct = >> radeon_cs_parser *parser, int error) >> { >> unsigned i; >> - if (!error) >> + if (!error) { >> + /* fence all bo va before ttm_eu_fence_buffer_objects so bo = >> are still reserved */ >> + radeon_bo_vm_fence_va(parser, parser->ib.fence); >> ttm_eu_fence_buffer_objects(&parser->validated, >> parser->ib.fence); >> - else >> + } else { >> ttm_eu_backoff_reservation(&parser->validated); >> + } >> if (parser->relocs !=3D NULL) { >> for (i =3D 0; i < parser->nrelocs; i++) { >> @@ -388,7 +415,6 @@ static int radeon_cs_ib_vm_chunk(struct = >> radeon_device *rdev, >> if (parser->chunk_ib_idx =3D=3D -1) >> return 0; >> - >> if ((parser->cs_flags & RADEON_CS_USE_VM) =3D=3D 0) >> return 0; >> diff --git a/drivers/gpu/drm/radeon/radeon_gart.c = >> b/drivers/gpu/drm/radeon/radeon_gart.c >> index b372005..9912182 100644 >> --- a/drivers/gpu/drm/radeon/radeon_gart.c >> +++ b/drivers/gpu/drm/radeon/radeon_gart.c >> @@ -814,7 +814,7 @@ int radeon_vm_bo_update_pte(struct radeon_device = >> *rdev, >> return -EINVAL; >> } >> - if (bo_va->valid) >> + if (bo_va->valid && mem) >> return 0; >> ngpu_pages =3D radeon_bo_ngpu_pages(bo); >> @@ -859,11 +859,27 @@ int radeon_vm_bo_rmv(struct radeon_device *rdev, >> struct radeon_bo *bo) >> { >> struct radeon_bo_va *bo_va; >> + int r; >> bo_va =3D radeon_bo_va(bo, vm); >> if (bo_va =3D=3D NULL) >> return 0; >> + /* wait for va use to end */ >> + while (bo_va->fence) { >> + r =3D radeon_fence_wait(bo_va->fence, false); >> + if (r) { >> + DRM_ERROR("error while waiting for fence: %d\n", r); >> + } >> + if (r =3D=3D -EDEADLK) { >> + r =3D radeon_gpu_reset(rdev); >> + if (!r) >> + continue; >> + } >> + break; >> + } >> + radeon_fence_unref(&bo_va->fence); >> + >> mutex_lock(&rdev->vm_manager.lock); >> mutex_lock(&vm->mutex); >> radeon_vm_bo_update_pte(rdev, vm, bo, NULL); >> @@ -952,12 +968,15 @@ void radeon_vm_fini(struct radeon_device *rdev, = >> struct radeon_vm *vm) >> radeon_vm_unbind_locked(rdev, vm); >> mutex_unlock(&rdev->vm_manager.lock); >> - /* remove all bo */ >> + /* remove all bo at this point non are busy any more because unbind >> + * waited for the last vm fence to signal >> + */ >> r =3D radeon_bo_reserve(rdev->ring_tmp_bo.bo, false); >> if (!r) { >> bo_va =3D radeon_bo_va(rdev->ring_tmp_bo.bo, vm); >> list_del_init(&bo_va->bo_list); >> list_del_init(&bo_va->vm_list); >> + radeon_fence_unref(&bo_va->fence); >> radeon_bo_unreserve(rdev->ring_tmp_bo.bo); >> kfree(bo_va); >> } >> @@ -969,6 +988,7 @@ void radeon_vm_fini(struct radeon_device *rdev, = >> struct radeon_vm *vm) >> r =3D radeon_bo_reserve(bo_va->bo, false); >> if (!r) { >> list_del_init(&bo_va->bo_list); >> + radeon_fence_unref(&bo_va->fence); >> radeon_bo_unreserve(bo_va->bo); >> kfree(bo_va); >> } >> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c = >> b/drivers/gpu/drm/radeon/radeon_gem.c >> index 84d0452..1b57b00 100644 >> --- a/drivers/gpu/drm/radeon/radeon_gem.c >> +++ b/drivers/gpu/drm/radeon/radeon_gem.c >> @@ -134,25 +134,16 @@ void radeon_gem_object_close(struct = >> drm_gem_object *obj, >> struct radeon_device *rdev =3D rbo->rdev; >> struct radeon_fpriv *fpriv =3D file_priv->driver_priv; >> struct radeon_vm *vm =3D &fpriv->vm; >> - struct radeon_bo_va *bo_va, *tmp; >> if (rdev->family < CHIP_CAYMAN) { >> return; >> } >> if (radeon_bo_reserve(rbo, false)) { >> + dev_err(rdev->dev, "leaking bo va because we fail to reserve = >> bo\n"); >> return; >> } >> - list_for_each_entry_safe(bo_va, tmp, &rbo->va, bo_list) { >> - if (bo_va->vm =3D=3D vm) { >> - /* remove from this vm address space */ >> - mutex_lock(&vm->mutex); >> - list_del(&bo_va->vm_list); >> - mutex_unlock(&vm->mutex); >> - list_del(&bo_va->bo_list); >> - kfree(bo_va); >> - } >> - } >> + radeon_vm_bo_rmv(rdev, vm, rbo); >> radeon_bo_unreserve(rbo); >> } >> diff --git a/drivers/gpu/drm/radeon/radeon_object.c = >> b/drivers/gpu/drm/radeon/radeon_object.c >> index 1f1a4c8..1cb014b 100644 >> --- a/drivers/gpu/drm/radeon/radeon_object.c >> +++ b/drivers/gpu/drm/radeon/radeon_object.c >> @@ -52,11 +52,7 @@ void radeon_bo_clear_va(struct radeon_bo *bo) >> list_for_each_entry_safe(bo_va, tmp, &bo->va, bo_list) { >> /* remove from all vm address space */ >> - mutex_lock(&bo_va->vm->mutex); >> - list_del(&bo_va->vm_list); >> - mutex_unlock(&bo_va->vm->mutex); >> - list_del(&bo_va->bo_list); >> - kfree(bo_va); >> + radeon_vm_bo_rmv(bo->rdev, bo_va->vm, bo); >> } >> } > Additional to that patch we still need a minor fix to mesa (just move = freeing the VM range after closing the handle). Going to send that in = the next minute to the mesa-dev list. Otherwise the patch indeed fixes the problem, but also isn't the best = idea for performance. Cheers, Christian.