On 16.02.2017 02:00, Michel Dänzer wrote:
On 16/02/17 04:10 AM, Nicolai Hähnle wrote:From: Nicolai Hähnle <nicolai.haehnle@xxxxxxx> Open-code the initial ttm_bo_validate call, so that we can properly unlock the reservation lock when it fails. Also, properly destruct the reservation object when the first part of TTM BO initialization fails. Actual deadlocks caused by the missing unlock should have been fixed by "drm/ttm: never add BO that failed to validate to the LRU list", superseding the flawed fix in commit 38fc4856ad98 ("drm/amdgpu: fix a potential deadlock in amdgpu_bo_create_restricted()"). This change fixes remaining recursive locking errors that can be seen with lock debugging enabled, and avoids the error of freeing a locked mutex. v2: use ttm_bo_unref for error handling, and rebase on latest changes Fixes: 12a852219583 ("drm/amdgpu: improve AMDGPU_GEM_CREATE_VRAM_CLEARED handling (v2)") Signed-off-by: Nicolai Hähnle <nicolai.haehnle@xxxxxxx> --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index c2e57f7..15944ea 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -404,15 +404,31 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev, } initial_bytes_moved = atomic64_read(&adev->num_bytes_moved); - r = ttm_bo_init(&adev->mman.bdev, &bo->tbo, size, type, - &bo->placement, page_align, !kernel, NULL, - acc_size, sg, resv ? resv : &bo->tbo.ttm_resv, - &amdgpu_ttm_bo_destroy); + r = ttm_bo_init_top(&adev->mman.bdev, &bo->tbo, size, type, + page_align, NULL, + acc_size, sg, resv ? resv : &bo->tbo.ttm_resv, + &amdgpu_ttm_bo_destroy); + + if (likely(r == 0)) + r = ttm_bo_validate(&bo->tbo, &bo->placement, !kernel, false); + + if (unlikely(r != 0)) { + struct ttm_buffer_object *tbo = &bo->tbo; + + if (!resv) + ww_mutex_unlock(&bo->tbo.ttm_resv.lock); + ttm_bo_unref(&tbo); + return r; + }I think this would be clearer as if (unlikely(r != 0)) { struct ttm_buffer_object *tbo = &bo->tbo; if (!resv) ww_mutex_unlock(&bo->tbo.ttm_resv.lock); ttm_bo_unref(&tbo); return r; } r = ttm_bo_validate(&bo->tbo, &bo->placement, !kernel, false); [...]amdgpu_cs_report_moved_bytes(adev, atomic64_read(&adev->num_bytes_moved) - initial_bytes_moved); - if (unlikely(r != 0)) - return r; + if (!(bo->tbo.mem.placement & TTM_PL_FLAG_NO_EVICT)) { + spin_lock(&bo->tbo.glob->lru_lock); + ttm_bo_add_to_lru(&bo->tbo); + spin_unlock(&bo->tbo.glob->lru_lock); + }It's a bit ugly to open-code this logic in driver code. Maybe add another TTM helper which calls ttm_bo_validate and ttm_bo_add_to_lru?
On further reflection, maybe a better approach would be to have a function ttm_bo_init_reserved, which returns with the reservation lock held upon success. We can then just change amdgpu_bo_create_restricted to ttm_bo_unreserve instead of ww_mutex_unlock, which does the add-to-LRU.
AFAICT, it doesn't make much sense to add the buffer to the LRU that early anyway, if amdgpu_fill_buffer is used.
Cheers, Nicolai
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
