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

[Christian König]

Am 16.04.2018 um 11:21 schrieb zhoucm1:
>
>
> On 2018年04月16日 17:04, zhoucm1 wrote:
>>
>>
>> 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.
> And I'm going to have patch for amdgpu_bo_create paramters, collect 
> many paramters to one param struct like done in vm.

Oh, good idea! That would be a good solution as well I think.

Then we can provide amdgpu_bo_do_create() with both the preferred and 
the allowed domain.

Do you want to keep working on this or should I?

Thanks,
Christian.

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