Am 03.04.24 um 09:06 schrieb YiPeng Chai:
[Why]
After calling amdgpu_vram_mgr_reserve_range
multiple times with the same address, calling
amdgpu_vram_mgr_query_page_status will always
return -EBUSY.
From the second call to amdgpu_vram_mgr_reserve_range,
the same address will be added to the reservations_pending
list again and is never moved to the reserved_pages list
because the address had been reserved.
Well that sounds like a really bad idea to me. Why is the function
called multiple times with the same address in the first place ?
Apart from that a note on the coding style below.
[How]
First add the address status check before calling
amdgpu_vram_mgr_do_reserve, if the address is already
reserved, do nothing; If the address is already in the
reservations_pending list, directly reserve memory;
only add new nodes for the addresses that are not in the
reserved_pages list and reservations_pending list.
Signed-off-by: YiPeng Chai <YiPeng.Chai@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 28 +++++++++++++-------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index 1e36c428d254..0bf3f4092900 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -317,7 +317,6 @@ static void amdgpu_vram_mgr_do_reserve(struct ttm_resource_manager *man)
dev_dbg(adev->dev, "Reservation 0x%llx - %lld, Succeeded\n",
rsv->start, rsv->size);
-
vis_usage = amdgpu_vram_mgr_vis_size(adev, block);
atomic64_add(vis_usage, &mgr->vis_usage);
spin_lock(&man->bdev->lru_lock);
@@ -340,19 +339,30 @@ int amdgpu_vram_mgr_reserve_range(struct amdgpu_vram_mgr *mgr,
uint64_t start, uint64_t size)
{
struct amdgpu_vram_reservation *rsv;
+ int ret = 0;
Don't initialize local variables when it isn't necessary.
- rsv = kzalloc(sizeof(*rsv), GFP_KERNEL);
- if (!rsv)
- return -ENOMEM;
+ ret = amdgpu_vram_mgr_query_page_status(mgr, start);
+ if (!ret)
+ return 0;
+
+ if (ret == -ENOENT) {
+ rsv = kzalloc(sizeof(*rsv), GFP_KERNEL);
+ if (!rsv)
+ return -ENOMEM;
- INIT_LIST_HEAD(&rsv->allocated);
- INIT_LIST_HEAD(&rsv->blocks);
+ INIT_LIST_HEAD(&rsv->allocated);
+ INIT_LIST_HEAD(&rsv->blocks);
- rsv->start = start;
- rsv->size = size;
+ rsv->start = start;
+ rsv->size = size;
+
+ mutex_lock(&mgr->lock);
+ list_add_tail(&rsv->blocks, &mgr->reservations_pending);
+ mutex_unlock(&mgr->lock);
+
+ }
You should probably not lock/unlock here.
Regards,
Christian.
mutex_lock(&mgr->lock);
- list_add_tail(&rsv->blocks, &mgr->reservations_pending);
amdgpu_vram_mgr_do_reserve(&mgr->manager);
mutex_unlock(&mgr->lock);
