Am 15.12.2017 um 12:35 schrieb Thomas Hellstrom:
> On 12/15/2017 10:53 AM, Christian König wrote:
>>
>>> 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.
>
> In what sense? It should be doing *exactly* what you're doing now but
> use an abstraction. No more no less. But with correctness checks, and
> less of passing state around. Specifically what I'm referring to are
> things like the locking test this patch is proposing, the "locked"
> variable in ttm_mem_evict_first(), and the parameter special code in
> ttm_mem_cleanup_refs() that I think are hard to follow and error
> prone. Conditional locking like in ttm_mem_clenup_refs() also
> typically trip static lock balance checkers.
It looks like you just have a typo in your example code then
"ctx->reserve_count++;" that should mean "bo->reserved++;", doesn't it?
That makes the idea more clear.
>
>
>> As far as I can see it makes TTM responsible about locking again
>
> No it doesn't. It's a helper. The only TTM state in my suggestion was
> the "bo::reserved"Â debug variable, which could either be ommited or
> count the recursive reservation to aid making sure that all recursive
> reservations you do in TTM are undone in TTM.
Yes, but this bo->reserved counter is what makes me thing that this
isn't a good idea at all.
We need to distinct between BOs which are reserved by the eviction code
and which are already reserved anyway. Hiding that behind an extra layer
makes things more error prone, not less.
>
>> and to be honest TTM is a bloody mess regarding this already.
>
> Hmm. IMO strong unspecific wording like this in a design descussions
> should be avoided. It tends to take away focus from what's actually
> being dicussed by making either part feel bad.
I agree that this is strong wording, but unfortunately regarding locking
TTM is one of the code bases where it really applies in my opinion.
Functions which are entered with locks held and then release and/or
regrab them are really not fun to work with. Took us quite a while to
get a grip on that and understand all the side effects a change could have.
> And it's typically unproductive.
Yeah, agree on that trying to choose my wording more wisely in the future.
>> 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.
>
> 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.
Regards,
Christian.
>
> Thanks,
> Thomas
>
>
>>
>> Regards,
>> Christian. qq
>>
>>>
>>>
>>> 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
>
>
