On 07.03.25 09:39, Christian König wrote:
Am 06.03.25 um 18:01 schrieb Natalie Vock:
When userspace requests buffers to be placed into GTT | VRAM, it is
requesting the buffer to be placed into either of these domains. If the
buffer fits into VRAM but does not fit into GTT, then let the buffer
reside in VRAM instead of failing allocation entirely.
That will completely break suspend/resume on laptops.
we essentially need to always check if a BO can fit into GTT.
Good point, I forgot about suspend. But as you say, we always need to do
this, even for VRAM-only BOs. I'll send a v2 for that in a minute.
Thanks, Natalie
Reported-by: Ivan Avdeev <1@xxxxxxxxx>
Signed-off-by: Natalie Vock <natalie.vock@xxxxxx>
---
This originally came up in https://gitlab.freedesktop.org/mesa/mesa/-/issues/12713:
The crux of the issue is that some applications expect they can allocate
buffers up to the size of VRAM - however, some setups have a
smaller-than-VRAM GTT region. RADV always sets VRAM | GTT for all buffer
allocations, so this causes amdgpu to reject the allocation entirely
because it cannot fit into GTT, even though it could fit into VRAM.
In my opinion, this check doesn't make sense: It is completely valid
behavior for the kernel to always keep a VRAM | GTT buffer in VRAM.
No it isn't. On suspend the power to VRAM is turned off and so we always need to be able to evacuate buffers into GTT.
Regards,
Christian.
The only case where buffers larger than the GTT size are special is that
they cannot be evicted to GTT (or swapped out), but the kernel already
allows VRAM-only buffers to be larger than GTT, and those cannot be
swapped out either. With the check removed, VRAM | GTT buffers larger
than GTT behave exactly like VRAM-only buffers larger than GTT.
The patch adding this check seems to have added it in a v2 - however I
was unable to find any public discussion around the patch with reasoning
in favor of this check.
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 30 ++++++++++------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d09db052e282d..b5e5fea091bf1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -555,27 +555,25 @@ static bool amdgpu_bo_validate_size(struct amdgpu_device *adev,
{
struct ttm_resource_manager *man = NULL;
- /*
- * If GTT is part of requested domains the check must succeed to
- * allow fall back to GTT.
- */
- if (domain & AMDGPU_GEM_DOMAIN_GTT)
- man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
- else if (domain & AMDGPU_GEM_DOMAIN_VRAM)
- man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
- else
+ /* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
+ if (!(domain & (AMDGPU_GEM_DOMAIN_GTT | AMDGPU_GEM_DOMAIN_VRAM)))
return true;
- if (!man) {
- if (domain & AMDGPU_GEM_DOMAIN_GTT)
+ if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
+ man = ttm_manager_type(&adev->mman.bdev, TTM_PL_VRAM);
+ if (size < man->size)
+ return true;
+ }
+ if (domain & AMDGPU_GEM_DOMAIN_GTT) {
+ man = ttm_manager_type(&adev->mman.bdev, TTM_PL_TT);
+ if (!man) {
WARN_ON_ONCE("GTT domain requested but GTT mem manager uninitialized");
- return false;
+ return false;
+ }
+ if (size < man->size)
+ return true;
}
- /* TODO add more domains checks, such as AMDGPU_GEM_DOMAIN_CPU, _DOMAIN_DOORBELL */
- if (size < man->size)
- return true;
-
DRM_DEBUG("BO size %lu > total memory in domain: %llu\n", size, man->size);
return false;
}
--
2.48.1
