[PATCH 3/3] drm/amdgpu: set preferred_domain independent of fallback handling

[zhoucm1]

On 2018年04月16日 16:57, Christian König wrote:
> Am 11.04.2018 um 11:22 schrieb Christian König:
>> Am 11.04.2018 um 04:38 schrieb zhoucm1:
>>>
>>>
>>> On 2018年04月10日 21:42, Christian König wrote:
>>>> When GEM needs to fallback to GTT for VRAM BOs we still want the
>>>> preferred domain to be untouched so that the BO has a cance to move 
>>>> back
>>>> to VRAM in the future.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 14 +++++++++++---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 13 ++-----------
>>>>   2 files changed, 13 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> index 46b9ea4e6103..9dc0a190413c 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>>> @@ -47,6 +47,7 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>>> *adev, unsigned long size,
>>>>                    struct reservation_object *resv,
>>>>                    struct drm_gem_object **obj)
>>>>   {
>>>> +    uint32_t domain = initial_domain;
>>>>       struct amdgpu_bo *bo;
>>>>       int r;
>>>>   @@ -57,7 +58,7 @@ int amdgpu_gem_object_create(struct 
>>>> amdgpu_device *adev, unsigned long size,
>>>>       }
>>>>     retry:
>>>> -    r = amdgpu_bo_create(adev, size, alignment, initial_domain,
>>>> +    r = amdgpu_bo_create(adev, size, alignment, domain,
>>>>                    flags, type, resv, &bo);
>>>>       if (r) {
>>>>           if (r != -ERESTARTSYS) {
>>>> @@ -66,8 +67,8 @@ int amdgpu_gem_object_create(struct amdgpu_device 
>>>> *adev, unsigned long size,
>>>>                   goto retry;
>>>>               }
>>>>   -            if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>> -                initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>> +            if (domain == AMDGPU_GEM_DOMAIN_VRAM) {
>>>> +                domain |= AMDGPU_GEM_DOMAIN_GTT;
>>>>                   goto retry;
>>>>               }
>>>>               DRM_DEBUG("Failed to allocate GEM object (%ld, %d, 
>>>> %u, %d)\n",
>>>> @@ -75,6 +76,13 @@ int amdgpu_gem_object_create(struct 
>>>> amdgpu_device *adev, unsigned long size,
>>>>           }
>>>>           return r;
>>>>       }
>>>> +
>>>> +    bo->preferred_domains = initial_domain;
>>>> +    bo->allowed_domains = initial_domain;
>>>> +    if (type != ttm_bo_type_kernel &&
>>>> +        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>> +        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>> It's an ugly change back after bo creation! Do you think it's better 
>>> than previous?
>>
>> Well it's better than moving the fallback handling into the core 
>> functions and then adding a flag to disable it again because we don't 
>> want it in the core function.
>>
>>>
>>> Alternative way, you can add one parameter to amdgpu_bo_create, 
>>> directly pass preferred_domains and allowed_domains to 
>>> amdgpu_bo_create.
>>
>> Then we would need three parameters, one where to create the BO now 
>> and two for preferred/allowed domains.
>>
>> I think that in this case overwriting the placement from the initial 
>> value looks cleaner to me.
>
> Ping? Any more opinions on how to proceed?
No, I keep my opinion on this. Michel already gave your RB I remember.

Regards,
David Zhou
>
> I agree that neither solution is perfect, but updating the placement 
> after we created the BO still sounds like the least painful to me.
>
> Christian.
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> David Zhou
>>>> +
>>>>       *obj = &bo->gem_base;
>>>>         return 0;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index 6d08cde8443c..854d18d5cff4 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -375,17 +375,8 @@ static int amdgpu_bo_do_create(struct 
>>>> amdgpu_device *adev, unsigned long size,
>>>>       drm_gem_private_object_init(adev->ddev, &bo->gem_base, size);
>>>>       INIT_LIST_HEAD(&bo->shadow_list);
>>>>       INIT_LIST_HEAD(&bo->va);
>>>> -    bo->preferred_domains = domain & (AMDGPU_GEM_DOMAIN_VRAM |
>>>> -                     AMDGPU_GEM_DOMAIN_GTT |
>>>> -                     AMDGPU_GEM_DOMAIN_CPU |
>>>> -                     AMDGPU_GEM_DOMAIN_GDS |
>>>> -                     AMDGPU_GEM_DOMAIN_GWS |
>>>> -                     AMDGPU_GEM_DOMAIN_OA);
>>>> -    bo->allowed_domains = bo->preferred_domains;
>>>> -    if (type != ttm_bo_type_kernel &&
>>>> -        bo->allowed_domains == AMDGPU_GEM_DOMAIN_VRAM)
>>>> -        bo->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
>>>> -
>>>> +    bo->preferred_domains = domain;
>>>> +    bo->allowed_domains = domain;
>>>>       bo->flags = flags;
>>>>     #ifdef CONFIG_X86_32
>>>
>>
>