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

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

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
>