Re: [PATCH 5/6] drm/amdgpu: always enable move threshold for BOs

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

>>           if (domain & abo->preferred_domains & 
>> AMDGPU_GEM_DOMAIN_VRAM &&
>> -            !(adev->flags & AMD_IS_APU))
>> -            places[c].flags |= TTM_PL_FLAG_FALLBACK;
>> +            !(adev->flags & AMD_IS_APU)) {
>> +            /*
>> +             * When GTT is just an alternative to VRAM make sure 
>> that we
>> +             * only use it as fallback and still try to fill up VRAM 
>> first.
>> +            */
>> +            if (abo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT)
>> +                places[c].flags |= TTM_PL_FLAG_FALLBACK;
>> +
>> +            /*
>> +             * Enable GTT when the threshold of moved bytes is
>> +             * reached. This prevents any non essential buffer move
>> +             * when the links are already saturated.
>> +             */
>> +            places[c].flags |= TTM_PL_FLAG_MOVE_THRESHOLD;
>> +        }
>
> For the APU case I *think* this works, but for discrete I am not sure 
> yet.

Agree, APUs are basically already fine as they are. VRAM is just used so 
that it isn't wasted there.

>
> As a side note and disclaimer, the TTM "resource compatible" logic has 
> a half-life of about one week in my brain until I need to almost 
> re-figure it all out. I don't know if it just me, but I find it really 
> non-intuitive and almost like double, triple, or even quadruple 
> negation way of thinking about things.

Yeah I was also going back and forth between the different approaches 
multiple times and just ended up in this implementation because it 
seemed to do what I wanted to have.

It's certainly not very intuitive what's going on here.

>
> It is not helping that with this proposal you set threshold on just 
> one of the possible object placements which further increases the 
> asymmetry. For me intuitive thing would be that thresholds apply to 
> the act of changing the current placement directly. Not indirectly via 
> playing with one of the placement flags dynamically.

Interesting idea, how would the handling then be? Currently we have only 
the stages - 'don't evict' and 'evict'. Should we make it something more 
like 'don't move', 'move', 'evict' ?

>
>
> Anyway, lets see.. So you set TTM_PL_FLAG_MOVE_THRESHOLD and 
> TTM_PL_FLAG_FALLBACK on the GTT placement, with the logic that it will 
> be considered compatible while under the migration budget?
>
> (Side note, the fact both flags are set I also find very difficult to 
> mentally model.)
>
> Say a buffer was evicted to GTT already. What then brings it back to 
> VRAM?
>
> The first subsequent ttm_bo_validate pass (!evicting) says GTT is fine 
> (applicable) while ctx->bytes_moved < ctx->move_threshold, no? Isn't 
> that the opposite of what would be required and causes nothing to be 
> migrated back in? What am I missing?

The flag says that GTT is fine when ctx->bytes_moved >= 
ctx->move_threshold. The logic is exactly inverted to what you described.

This way a BO will be moved back into VRAM as long as bytes moved 
doesn't exceed the threshold.

Setting both flags has the effect of saying: It's ok that the BO stays 
in GTT when you either above the move threshold or would have to evict 
something.

Regards,
Christian.

>
> Regards,
>
> Tvrtko