[PATCH] drm/amdgpu: Don't change preferred domian when fallback GTT v4

[Christian König]

Am 20.03.2018 um 08:55 schrieb Chunming Zhou:
> v2: add sanity checking
> v3: make code open
> v4: also handle visible to invisible fallback
>
> Change-Id: I2cf672ad36b8b4cc1a6b2e704f786bf6a155d9ce
> Signed-off-by: Chunming Zhou <david1.zhou at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 16 ++--------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 19 ++++++++++++++++---
>   2 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 6e6570ff9f8b..8328684aee06 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -76,23 +76,11 @@ int amdgpu_gem_object_create(struct amdgpu_device *adev, unsigned long size,
>   		}
>   	}
>   
> -retry:
>   	r = amdgpu_bo_create(adev, size, alignment, kernel, initial_domain,
>   			     flags, NULL, resv, &bo);
>   	if (r) {
> -		if (r != -ERESTARTSYS) {
> -			if (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> -				flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> -				goto retry;
> -			}
> -
> -			if (initial_domain == AMDGPU_GEM_DOMAIN_VRAM) {
> -				initial_domain |= AMDGPU_GEM_DOMAIN_GTT;
> -				goto retry;
> -			}
> -			DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, %d)\n",
> -				  size, initial_domain, alignment, r);
> -		}
> +		DRM_DEBUG("Failed to allocate GEM object (%ld, %d, %u, %d)\n",
> +			  size, initial_domain, alignment, r);
>   		return r;
>   	}
>   	*obj = &bo->gem_base;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index b3310219e0ac..84c5e9db1b39 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -440,12 +440,25 @@ static int amdgpu_bo_do_create(struct amdgpu_device *adev,
>   #endif
>   
>   	bo->tbo.bdev = &adev->mman.bdev;
> -	amdgpu_ttm_placement_from_domain(bo, domain);
> -
> +retry:
> +	amdgpu_ttm_placement_from_domain(bo, bo->preferred_domains);
>   	r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
>   				 &bo->placement, page_align, &ctx, acc_size,
>   				 sg, resv, &amdgpu_ttm_bo_destroy);
> -	if (unlikely(r != 0))
> +
> +	if (unlikely(r && r != -ERESTARTSYS)) {
> +	    if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> +		    bo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
> +		    goto retry;
> +	    } else if (bo->allowed_domains != bo->preferred_domains) {
> +		    amdgpu_ttm_placement_from_domain(bo, bo->allowed_domains);
> +		    r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size,
> +					     type, &bo->placement, page_align,
> +					     &ctx, acc_size, sg, resv,
> +					     &amdgpu_ttm_bo_destroy);
> +	    }
> +	}
> +	if (unlikely(r))

Mhm, again this ugly retry label. But since we now handled two cases 
open coding this becomes to lengthly as well.

Let's go back to your original approach. How about the following code:

domains = bo->preferred_domains;
retry:
     amdgpu_ttm_placement_from_domain(bo, domains);
     r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
         &bo->placement, page_align, &ctx, acc_size,
         sg, resv, &amdgpu_ttm_bo_destroy);

     if (unlikely(r && r != -ERESTARTSYS)) {
         if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
             bo->flags &= ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED;
             goto retry;
         } else if (domains != bo->preferred_domains) {
             domains = bo->preferred_domains;
             goto retry;
         }
     }
     if (unlikely(r))
...

That shouldn't loop even if it fails with preferred_domains and handle 
the case gracefully that we first try to clear the flag and then the 
move the BO to GTT.

Regards,
Christian.