Am 15.12.2017 um 10:38 schrieb Thomas Hellstrom:
> On 12/15/2017 10:13 AM, Christian König wrote:
>> Hi Thomas,
>>
>> actually I was very happy to get rid of that stuff.
>
> Yes, wrappers that don't do anything don't make much sense, but this
> is a different story.
I was not talking about the wrappers, but rather about having the
locking code in TTM.
>
>>
>> In the long run I indeed wanted to replace ctx->resv with the
>> ww_acquire_ctx to enable eviction of even more things, but that is a
>> different story.
>>
>> Recursive locking is usually something we should try to avoid.
>
> Well this is more or less replicating what you are doing currently but
> instead of spreading the checks and locking state all over the code,
> both as local variables and parameters this is keeping it in a single
> place with correctness checks.
I don't see how that can be correct. As far as I can see it makes TTM
responsible about locking again and to be honest TTM is a bloody mess
regarding this already.
My intention in the long term is rather to remove logic from TTM and
move it back into the drivers. The end result I have in mind is that TTM
becomes a toolbox instead of the midlayer it is currently.
Regards,
Christian.
>
>
> I 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.
>
> /Thomas
>
>
>
>>
>> Regards,
>> 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 of
>>>
>>> int __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 at amd.com>
>>>> ---
>>>> Â 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.c
>>>> index 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
