[PATCH 2/2] drm/amdgpu: allow GTT overcommit during bind

[Christian König]

Am 17.10.2017 um 10:15 schrieb Chunming Zhou:
>
>
> On 2017年10月17日 15:46, Christian König wrote:
>> Am 17.10.2017 um 06:11 schrieb Chunming Zhou:
>>>
>>>
>>> On 2017年10月16日 19:40, Christian König wrote:
>>>> Am 16.10.2017 um 11:42 schrieb Chunming Zhou:
>>>>>
>>>>>
>>>>> On 2017年10月16日 17:26, Christian König wrote:
>>>>>> From: Christian König <christian.koenig at amd.com>
>>>>>>
>>>>>> While binding BOs to GART we need to allow a bit overcommit in 
>>>>>> the GTT
>>>>>> domain.
>>>>> If allowing overcommit, will the new node not over the GART mc 
>>>>> range? Which is also allowed?
>>>> No that is checked separately by drm_mm_insert_node_in_range().
>>>>
>>>> This is just to cover the case when we have a BO in GTT space which 
>>>> needs to be bound into the GART table.
>>> Sorry, I missed that even gart BO is also without node during creating.
>>> One nitpick, atomic64_sub(mem->num_pages, &mgr->available) will be 
>>> calculated twice for one gart bo create and pin, which results in 
>>> available isn't correct.
>>
>> Yeah, that is true but not as problematic as you think.
>>
>> The BO is transfered from the old mem object (without GART mapping) 
>> to the new mem object (with GART mapping). While doing this the old 
>> mem object is released, so we actually don't leak the memory.
>>
>> It's just that for a moment the BO is accounted twice, once for the 
>> old location and once for the new one.
> If in memory pressure, I think we could allocate memory over the mgr 
> limitation.

> For example, the limitation is 1024M, the BO allocation is 800M, when 
> the second counting, the available will be -576M,

Yes, correct so far. That's why I've added the extra check in 
amdgpu_gtt_mgr_usage().

> since available is unsigned

available is an atomic64_t and that is signed IIRC.

We could cast mem->num_pages to signed as well to be double sure, but as 
far as I can see that should work.

Regards,
Christian.

>
> Regards,
> David Zhou
>>
>> But we had that behavior previously as well, so that is not something 
>> introduced with this patch.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> David Zhou
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Regards,
>>>>> David Zhou
>>>>>>   Otherwise we can never use the full GART space when GART 
>>>>>> size=GTT size.
>>>>>>
>>>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c | 8 +++++---
>>>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>>>>>> index 0d15eb7d31d7..33535d347734 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gtt_mgr.c
>>>>>> @@ -169,7 +169,8 @@ static int amdgpu_gtt_mgr_new(struct 
>>>>>> ttm_mem_type_manager *man,
>>>>>>       int r;
>>>>>>         spin_lock(&mgr->lock);
>>>>>> -    if (atomic64_read(&mgr->available) < mem->num_pages) {
>>>>>> +    if ((&tbo->mem == mem || tbo->mem.mem_type != TTM_PL_TT) &&
>>>>>> +        atomic64_read(&mgr->available) < mem->num_pages) {
>>>>>>           spin_unlock(&mgr->lock);
>>>>>>           return 0;
>>>>>>       }
>>>>>> @@ -244,8 +245,9 @@ static void amdgpu_gtt_mgr_del(struct 
>>>>>> ttm_mem_type_manager *man,
>>>>>>   uint64_t amdgpu_gtt_mgr_usage(struct ttm_mem_type_manager *man)
>>>>>>   {
>>>>>>       struct amdgpu_gtt_mgr *mgr = man->priv;
>>>>>> +    s64 result = man->size - atomic64_read(&mgr->available);
>>>>>>   -    return (u64)(man->size - atomic64_read(&mgr->available)) * 
>>>>>> PAGE_SIZE;
>>>>>> +    return (result > 0 ? result : 0) * PAGE_SIZE;
>>>>>>   }
>>>>>>     /**
>>>>>> @@ -265,7 +267,7 @@ static void amdgpu_gtt_mgr_debug(struct 
>>>>>> ttm_mem_type_manager *man,
>>>>>>       drm_mm_print(&mgr->mm, printer);
>>>>>>       spin_unlock(&mgr->lock);
>>>>>>   -    drm_printf(printer, "man size:%llu pages, gtt 
>>>>>> available:%llu pages, usage:%lluMB\n",
>>>>>> +    drm_printf(printer, "man size:%llu pages, gtt available:%lld 
>>>>>> pages, usage:%lluMB\n",
>>>>>>              man->size, (u64)atomic64_read(&mgr->available),
>>>>>>              amdgpu_gtt_mgr_usage(man) >> 20);
>>>>>>   }
>>>>>
>>>>
>>>
>>
>