[PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY

Mon May 31 12:02:14 UTC 2021

Am 31.05.21 um 13:19 schrieb Thomas Hellström (Intel):
>
> On 5/31/21 12:56 PM, Christian König wrote:
>> Am 31.05.21 um 12:46 schrieb Thomas Hellström (Intel):
>>>
>>> On 5/31/21 12:32 PM, Christian König wrote:
>>>> Am 31.05.21 um 11:52 schrieb Thomas Hellström (Intel):
>>>>> Hi, Lang,
>>>>>
>>>>> On 5/31/21 10:19 AM, Yu, Lang wrote:
>>>>>> [Public]
>>>>>>
>>>>>>> Hi,
>>>>>>> On 5/27/21 3:30 AM, Lang Yu wrote:
>>>>>>>> Make TTM_PL_FLAG_* start from zero and add
>>>>>>>> TTM_PL_FLAG_TEMPORARY flag for temporary
>>>>>>>> GTT allocation use.
>>>>>>> GTT is a driver private acronym, right? And it doesn't look like
>>>>>>> TTM_PL_FLAG_TEMPORARY will be used in core TTM, so should we 
>>>>>>> instead set
>>>>>>> aside a mask in the PL flag for driver-private use?
>>>>>> Hi Thomas,
>>>>>>
>>>>>> Thanks for your comments and advice, GTT means Graphics 
>>>>>> Translation Table here, seems
>>>>>> a general acronym. TTM_PL_FLAG_TEMPORARY may also be used by 
>>>>>> other drives.
>>>>>> I have made other patches for this. Please help review.
>>>>>>
>>>>>> Regards,
>>>>>> Lang
>>>>>>
>>>>> My point was really that the flag naming and documentation should 
>>>>> reflect what core ttm is doing with that flag. If there is no 
>>>>> specific core TTM usage, IMO we should move it to a driver 
>>>>> specific flag to avoid future confusion. In particular a writer of 
>>>>> a generic TTM resource manager should be able to know without 
>>>>> looking at an old commit message what the placement flag is 
>>>>> intended for.
>>>>>
>>>>> So here we add a flag where core TTM forces a bo move on validate 
>>>>> and that's it. And that appears to be how it's used when an amdgpu 
>>>>> bo is evicted to GTT, (btw should it be accounted in this situation?)
>>>>>
>>>>> The other use is to force the amdgpu driver to temporarily accept 
>>>>> it into GTT when there is a lack of space, and IMHO that's a 
>>>>> driver specific use and we should allocate a mask for driver 
>>>>> specific flags for that.
>>>>>
>>>>> So shouldn't this be two flags, really?
>>>>
>>>> Well one flag makes sense for the use case at hand that drivers 
>>>> want to signal to TTM that an allocation is only temporary and not 
>>>> considered valid.
>>>>
>>>> That we then use this flag to implement temporary GTT allocations 
>>>> to avoid problems during eviction is just extending that use case.
>>>>
>>> OK, but it looked like there were actually two use-cases. One for 
>>> possibly long-term VRAM evictions to GTT, the other for the 
>>> temporary use-case where the hop resource allocations sometimes 
>>> failed. Or did I misunderstand the code?
>>
>> Ok sounds like we need more documentation here. That's really one use 
>> case.
>>
>> Key point is we need temporary allocation during multihop which 
>> should be handled differently to normal allocations.
>
> Yes, that part is clear from the patches. The part that I can't fit 
> into that pattern is why the evict flags when evicting from visible 
> VRAM to GTT or ordinary VRAM is marked with TTM_PL_FLAG_TEMPORARY. 
> Wouldn't those remain evicted in that placement until re-validated to 
> visible VRAM at an unknown future time?

Not necessarily.

The situation we ran into was the following:
1. OOM on VRAM, we try to evict.

2. GTT space is used up as well, ok evict directly to SYSTEM.

3. For VRAM->SYSTEM eviction we use a temporary bounce buffer.

4. Waiting for the bounce buffer to become idle is interrupted by a 
signal so BO is still backed by bounce buffer.

5. Next CS, BO is validated with VRAM|GTT. TTM sees that it is in GTT 
and doesn't move the buffer.

6. CS goes into nirvana because bounce buffers are not meant to be used 
for CS (we can ignore alignment and accounting for them).

>
> Patch 3/3:
>
>              amdgpu_bo_placement_from_domain(abo, AMDGPU_GEM_DOMAIN_GTT);
>             abo->placements[0].flags |= TTM_PL_FLAG_TEMPORARY;

Thanks for pointing that out, this is indeed still the wrong place. 
Going to fix that.

Christian.

>
>
>
>>
>> Christian.
>>
>>>
>>> /Thomas
>>>
>>>
>>>> Christian.
>>>>
>>>>>
>>>>> TTM_PL_FLAG_FORCE_MOVE
>>>>>
>>>>> and
>>>>>
>>>>> AMDGPU_PL_FLAG_TEMPORARY
>>>>>
>>>>> Thanks,
>>>>>
>>>>> /Thomas
>>>>>
>>>>>>> Thomas
>>>>>>> -----Original Message-----
>>>>>>> From: Yu, Lang <Lang.Yu at amd.com>
>>>>>>> Sent: Thursday, May 27, 2021 9:31 AM
>>>>>>> To: amd-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
>>>>>>> Cc: Koenig, Christian <Christian.Koenig at amd.com>; Huang, Ray
>>>>>>> <Ray.Huang at amd.com>; Deucher, Alexander 
>>>>>>> <Alexander.Deucher at amd.com>;
>>>>>>> Yu, Lang <Lang.Yu at amd.com>
>>>>>>> Subject: [PATCH 1/2] drm/ttm: cleanup and add TTM_PL_FLAG_TEMPORARY
>>>>>>>
>>>>>>> Make TTM_PL_FLAG_* start from zero and add TTM_PL_FLAG_TEMPORARY 
>>>>>>> flag
>>>>>>> for temporary GTT allocation use.
>>>>>>>
>>>>>>> Signed-off-by: Lang Yu <Lang.Yu at amd.com>
>>>>>>> ---
>>>>>>> include/drm/ttm/ttm_placement.h | 5 +++--
>>>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/drm/ttm/ttm_placement.h
>>>>>>> b/include/drm/ttm/ttm_placement.h index 
>>>>>>> aa6ba4d0cf78..9f5cfc7c2d5a 100644
>>>>>>> --- a/include/drm/ttm/ttm_placement.h
>>>>>>> +++ b/include/drm/ttm/ttm_placement.h
>>>>>>> @@ -47,8 +47,9 @@
>>>>>>>   * top of the memory area, instead of the bottom.
>>>>>>>   */
>>>>>>>
>>>>>>> -#define TTM_PL_FLAG_CONTIGUOUS  (1 << 19)
>>>>>>> -#define TTM_PL_FLAG_TOPDOWN     (1 << 22)
>>>>>>> +#define TTM_PL_FLAG_CONTIGUOUS  (1 << 0)
>>>>>>> +#define TTM_PL_FLAG_TOPDOWN     (1 << 1)
>>>>>>> +#define TTM_PL_FLAG_TEMPORARY   (1 << 2)
>>>>>>>
>>>>>>> /**
>>>>>>>   * struct ttm_place
>>>>>>> -- 
>>>>>>> 2.25.1
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx at lists.freedesktop.org
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cchristian.koenig%40amd.com%7C124823f58ee741684d6a08d924260567%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637580568009117976%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=d8k0c8SAcGGp1m4mEmA8nDrM%2FH6b5Mwo9u%2BzR9jiw3A%3D&reserved=0 
>>>>>