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

[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?

/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://lists.freedesktop.org/mailman/listinfo/amd-gfx