[PATCH] drm/amdgpu: adjust get preferred domain policy

[Zhang, Yifan]

[Public]

Hi Christian,

Actually we met some issues when tried to enable multiple 4k displays in some APU platforms. With current code logic, framebuffers are always allocated from VRAM and eventually pin failed after vram is used up. Although system is able to support S/G framebuffer, it never gets  a chance to do it. This patch is  to enable S/G system memory framebuffer in these corner cases. I agree the designs need to be changed. And can we shrink AMDGPU_SG_THRESHOLD to avoid enabling S/G framebuffer unnecessarily ? or do you have to any suggestion to fix this issue ? Thanks !

static int dm_plane_helper_prepare_fb(struct drm_plane *plane,
                       struct drm_plane_state *new_state)
 {
    if (plane->type != DRM_PLANE_TYPE_CURSOR)
         domain = amdgpu_display_supported_domains(adev, rbo->flags);
     else
         domain = AMDGPU_GEM_DOMAIN_VRAM;
 
// We'd like to pin framebuffer to system memory when vram is used up.
     r = amdgpu_bo_pin(rbo, domain);  
     if (unlikely(r != 0)) {
         if (r != -ERESTARTSYS)
             DRM_ERROR("Failed to pin framebuffer with error %d\n", r);
         ttm_eu_backoff_reservation(&ticket, &list);
         return r;
     }
}


BRs,
Yifan

-----Original Message-----
From: Koenig, Christian <Christian.Koenig at amd.com> 
Sent: Friday, August 27, 2021 4:51 PM
To: Zhang, Yifan <Yifan1.Zhang at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Koenig, Christian <Christian.Koenig at amd.com>
Subject: Re: [PATCH] drm/amdgpu: adjust get preferred domain policy

Am 27.08.21 um 10:45 schrieb Yifan Zhang:
> current preferred domain policy is static, which makes vram > 256M APU 
> never get a chance to allocate system S/G framebuffer. Change this 
> policy to take vram memory pressure into accout. If system support 
> both vram/system memory, return the preferred domain based on vram 
> usage.
>
> Signed-off-by: Yifan Zhang <yifan1.zhang at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 65a9b23f0a46..ea3b084e3c3f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1530,7 +1530,9 @@ uint32_t amdgpu_bo_get_preferred_domain(struct amdgpu_device *adev,
>   {
>   	if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) {
>   		domain = AMDGPU_GEM_DOMAIN_VRAM;
> -		if (adev->gmc.real_vram_size <= AMDGPU_SG_THRESHOLD)
> +		if (adev->gmc.real_vram_size -
> +				atomic64_read(&adev->mman.vram_mgr.vis_usage) <=
> +				AMDGPU_SG_THRESHOLD)

Well apart from that you are accessing a private member of the vram manager and have messed up the coding style I don't think that this is a good idea at all from the design point of view.

Scanning out from SG has some huge disadvantages (unstable on some HW generations, more walker overhead etc) that we certainly don't want to enable it unless we don't absolutely need it.

Christian.

>   			domain = AMDGPU_GEM_DOMAIN_GTT;
>   	}
>   	return domain;