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

[Chunming Zhou]

On 2017年10月17日 16:34, Chunming Zhou wrote:
>
>
> On 2017年10月17日 16:27, Christian König wrote:
>> 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.
> I agree above, but why you cut some part of mine:" if another process 
> allocation(800M) is coming at this moment, the bo_create is still 
> successful, the binding will be fail. But the total allocation will be 
> over the mgr limitation."
> What do you think of you cut?
Ah, sorry, atomic64_t is singed, so the checking will fail, this case 
will not happen.

Reviewed-by: Chunming Zhou <david1.zhou at amd.com>

>
> Regards,
> David Zhou
>
>>
>> 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);
>>>>>>>>   }
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx