Am 31.10.2017 um 16:14 schrieb Michel Dänzer:
On 31/10/17 04:03 PM, Christian König wrote:
Am 31.10.2017 um 09:43 schrieb Michel Dänzer:
On 31/10/17 09:37 AM, Christian König wrote:
From: Christian König <[email protected]>
The bo structure is freed up in case of an error, so we can't do any
accounting if that happens.
Signed-off-by: Christian König <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index e44b880eefdd..ff6f842655d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -369,6 +369,9 @@ static int amdgpu_bo_do_create(struct
amdgpu_device *adev,
r = ttm_bo_init_reserved(&adev->mman.bdev, &bo->tbo, size, type,
&bo->placement, page_align, !kernel, NULL,
acc_size, sg, resv, &amdgpu_ttm_bo_destroy);
+ if (unlikely(r != 0))
+ return r;
+
bytes_moved = atomic64_read(&adev->num_bytes_moved) -
initial_bytes_moved;
if (adev->mc.visible_vram_size < adev->mc.real_vram_size &&
@@ -378,9 +381,6 @@ static int amdgpu_bo_do_create(struct
amdgpu_device *adev,
else
amdgpu_cs_report_moved_bytes(adev, bytes_moved, 0);
- if (unlikely(r != 0))
- return r;
-
if (kernel)
bo->tbo.priority = 1;
Hmm. If it's at least theoretically possible that ttm_bo_init_reserved
moves some BOs before returning an error, we should instead make the
accounting safe WRT bo having been freed. But assuming it's not possible,
I have made patches for this back in April and send them to the list,
but the value is rather low and you need to change a lot of places in
other drivers as well.
At least in this case, simply moving the
if (adev->mc.visible_vram_size < adev->mc.real_vram_size &&
bo->tbo.mem.mem_type == TTM_PL_VRAM &&
bo->tbo.mem.start < adev->mc.visible_vram_size >> PAGE_SHIFT)
test before the ttm_bo_init_reserved call, and then passing bytes_moved
or 0 for the third parameter of amdgpu_cs_report_moved_bytes depending
on its result, would work as well, wouldn't it?
No, that won't work. Neither bo->tbo.mem.mem_type nor bo->tbo.mem.start
are determined yet before the call to ttm_bo_init().
The root problem is that we have a mis-assumption about how visible VRAM
works here (e.g. it doesn't handle the case where a BO is partially in
visible VRAM) and that we use the global bytes moved counter as
indicator for the local operation (e.g. to know how many bytes moved the
current client).
The only real solution is to give ttm_bo_init_* and ttm_bo_validate a
context parameter where we can note how much work we had to do to
fulfill the request.
Regards,
Christian.
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
