On 12/15/2017 01:05 PM, Christian König wrote:
I'm in favour of that. But I don't think what I proposed is a step away from that direction. On the contrary. I've attached a POC patch with the correctness checks stripped, not compile-tested. Much easier to follow if you ask me, but if you feel so strongly against it, never mind.Exactly that's what I've meant with that this won't work. See you loose the extra check "!ctx->allow_reserved_eviction && list_empty(&bo->ddestroy))" here and that one is really important for correct operation.
Um, yes. The list_empty(&bo->ddestroy) can't be guaranteed to be preserved against a lock - unlock sequence, that would indeed need an extra state variable bo->shared_reserved, but still allow for locking outside of TTM.
But apparently we have different opinions what's clean and what's not, and you're the maintainer now, so you decide :).
/Thomas
Regards, Christian.Thanks, ThomasRegards, Christian. qqI agree recursive locking is generally frowned upon, but you're already doing it, not by using recursive locks, but by passing locking state around which IMO is worse.Collecting the state in a the operation_ctx will make that usage-pattern more obvious but will help make the code cleaner and less error prone./ThomasRegards, Christian. Am 15.12.2017 um 08:01 schrieb Thomas Hellstrom:Roger and Chrisitian,Correct me if I'm wrong, but It seems to me like a lot of the recent changes to ttm_bo.c are to allow recursive reservation object locking in the case of shared reservation objects, but only in certain functions and with special arguments so it doesn't look like recursive locking to the lockdep checker. Wouldn't it be a lot cleaner if we were to hide all this in a resurrected __ttm_bo_reserve something along the lines ofint __ttm_bo_reserve(struct ttm_bo *bo, struct ttm_operation_ctx *ctx) {if (ctx && ctx->resv == bo->resv) { #ifdef CONFIG_LOCKDEP WARN_ON(bo->reserved); lockdep_assert_held(&ctx->resv); ctx->reserve_count++; bo->reserved = true; #endif return0; } else {int ret = reservation_object_lock(bo->resv, NULL) ? 0:-EBUSY;if (ret) return ret; #ifdef CONFIG_LOCKDEP WARN_ON(bo->reserved); bo->reserved = true; #endif return 0; }And similar for tryreserve and unreserve? Perhaps with a ww_acquire_ctx included somewhere as well.../Thomas On 12/14/2017 09:10 AM, Roger He wrote:Change-Id: I0c6ece0decd18d30ccc94e5c7ca106d351941c62 Signed-off-by: Roger He <Hongbo.He@xxxxxxx> --- drivers/gpu/drm/ttm/ttm_bo.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.cindex 098b22e..ba5b486 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c@@ -707,7 +707,6 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,EXPORT_SYMBOL(ttm_bo_eviction_valuable); static int ttm_mem_evict_first(struct ttm_bo_device *bdev, - struct reservation_object *resv, uint32_t mem_type, const struct ttm_place *place, struct ttm_operation_ctx *ctx)@@ -722,8 +721,9 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,spin_lock(&glob->lru_lock); for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { list_for_each_entry(bo, &man->lru[i], lru) { - if (bo->resv == resv) { - if (list_empty(&bo->ddestroy)) + if (bo->resv == ctx->resv) { + if (!ctx->allow_reserved_eviction && + list_empty(&bo->ddestroy)) continue; } else { locked = reservation_object_trylock(bo->resv);@@ -835,7 +835,7 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,return ret; if (mem->mm_node) break;- ret = ttm_mem_evict_first(bdev, bo->resv, mem_type, place, ctx);+ ret = ttm_mem_evict_first(bdev, mem_type, place, ctx); if (unlikely(ret != 0)) return ret; } while (1);@@ -1332,8 +1332,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) { while (!list_empty(&man->lru[i])) { spin_unlock(&glob->lru_lock); - ret = ttm_mem_evict_first(bdev, NULL, mem_type, - NULL, &ctx); + ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx); if (ret) return ret; spin_lock(&glob->lru_lock);_______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
