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

Mon May 31 11:19:51 UTC 2021

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?

Patch 3/3:

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

>
> 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%7C3868af2bd5d94aeda94b08d924215b3a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637580547980190391%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=C7b5wz8Kph5bM8fkFVyXKwSNkSj3lDaxGUnww4jY%2FeM%3D&reserved=0 
>>>>