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,
>> 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
>>
>>
