Re: [PATCH v8 3/3] drm/buddy: Add user for defragmentation
Hi Christian,
On 3/5/2024 5:41 PM, Christian König wrote:
Am 05.03.24 um 12:14 schrieb Paneer Selvam, Arunpravin:
On 3/5/2024 4:33 PM, Paneer Selvam, Arunpravin wrote:
Hi Christian,
On 3/4/2024 10:09 PM, Christian König wrote:
Am 04.03.24 um 17:32 schrieb Arunpravin Paneer Selvam:
Add amdgpu driver as user for the drm buddy
defragmentation.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++--
drivers/gpu/drm/drm_buddy.c | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index e494f5bf136a..cff8a526c622 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct
ttm_resource_manager *man,
min_block_size,
&vres->blocks,
vres->flags);
- if (unlikely(r))
- goto error_free_blocks;
+ if (unlikely(r)) {
+ if (r == -ENOSPC) {
+ drm_buddy_defrag(mm, min_block_size);
+ r = drm_buddy_alloc_blocks(mm, fpfn,
+ lpfn,
+ size,
+ min_block_size,
+ &vres->blocks,
+ vres->flags);
That doesn't looks like something we should do.
We might fallback when contiguous memory is requested, but
certainly not on normal allocation failure.
yes, defrag here not useful for normal allocations. But worried
about the bigger min_block_size normal allocations.
In such cases, I think we should move this drm_buddy_defrag() call
into buddy allocator file. For example if the required
size is 1024KiB and if min_block_size is 256KiB, the allocator first
tries to find the 1024KiB block, when there is no single 1024KiB block,
the allocator goes one level below in freelist and tries to search
for two 512KiB blocks and goes on. At one point of time if we have
less space,
we might go further levels below to search four 256KiB blocks to
satisfy the request.
Assuming if the allocator cannot find the first 256KiB block, that
time I think we might need to merge the two 128KiB blocks
through defragmentation function. And again for the second 256KiB
block, we might need to call the defragmentation again to
merge two 128KiB blocks or four 64KiB blocks to form minimum
alignment size of 256KiB. This goes on for the third and fourth
256KiB blocks to complete the required size allocation of 1024KiB.
Please let me know if my understanding is not correct.
I don't think we should do that. We essentially have to support two
different use cases:
1. Non contiguous allocation with 2MiB min_block_size for everything
larger than 2MiB. Using a block size as large as possible is
desirable, but not something we enforce.
2. Contiguous allocations for display, firmware etc.. Here we need to
enforce a large block size and can live with the additional overhead
caused by force merging.
Thanks. I will make the changes accordingly.
As you have suggested we can also rename this as force merge or some
other names.
Yeah, but just an suggestion. You are way deeper in the code and
handling than I'm, so feel free to name it whatever you think fits best.
Sure :)
Thanks,
Arun.
Regards,
Christian.
Thanks,
Arun.
Thanks,
Arun.
Regards,
Christian.
+ if (unlikely(r))
+ goto error_free_blocks;
+ } else {
+ goto error_free_blocks;
+ }
+ }
if (size > remaining_size)
remaining_size = 0;
diff --git a/drivers/gpu/drm/drm_buddy.c
b/drivers/gpu/drm/drm_buddy.c
index 40131ed9b0cd..19440f8caec0 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
}
}
}
+EXPORT_SYMBOL(drm_buddy_defrag);
/**
* drm_buddy_free_block - free a block
Re: [PATCH v8 3/3] drm/buddy: Add user for defragmentation
Am 05.03.24 um 12:14 schrieb Paneer Selvam, Arunpravin:
On 3/5/2024 4:33 PM, Paneer Selvam, Arunpravin wrote:
Hi Christian,
On 3/4/2024 10:09 PM, Christian König wrote:
Am 04.03.24 um 17:32 schrieb Arunpravin Paneer Selvam:
Add amdgpu driver as user for the drm buddy
defragmentation.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++--
drivers/gpu/drm/drm_buddy.c | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index e494f5bf136a..cff8a526c622 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct
ttm_resource_manager *man,
min_block_size,
&vres->blocks,
vres->flags);
- if (unlikely(r))
- goto error_free_blocks;
+ if (unlikely(r)) {
+ if (r == -ENOSPC) {
+ drm_buddy_defrag(mm, min_block_size);
+ r = drm_buddy_alloc_blocks(mm, fpfn,
+ lpfn,
+ size,
+ min_block_size,
+ &vres->blocks,
+ vres->flags);
That doesn't looks like something we should do.
We might fallback when contiguous memory is requested, but certainly
not on normal allocation failure.
yes, defrag here not useful for normal allocations. But worried about
the bigger min_block_size normal allocations.
In such cases, I think we should move this drm_buddy_defrag() call
into buddy allocator file. For example if the required
size is 1024KiB and if min_block_size is 256KiB, the allocator first
tries to find the 1024KiB block, when there is no single 1024KiB block,
the allocator goes one level below in freelist and tries to search
for two 512KiB blocks and goes on. At one point of time if we have
less space,
we might go further levels below to search four 256KiB blocks to
satisfy the request.
Assuming if the allocator cannot find the first 256KiB block, that
time I think we might need to merge the two 128KiB blocks
through defragmentation function. And again for the second 256KiB
block, we might need to call the defragmentation again to
merge two 128KiB blocks or four 64KiB blocks to form minimum
alignment size of 256KiB. This goes on for the third and fourth
256KiB blocks to complete the required size allocation of 1024KiB.
Please let me know if my understanding is not correct.
I don't think we should do that. We essentially have to support two
different use cases:
1. Non contiguous allocation with 2MiB min_block_size for everything
larger than 2MiB. Using a block size as large as possible is desirable,
but not something we enforce.
2. Contiguous allocations for display, firmware etc.. Here we need to
enforce a large block size and can live with the additional overhead
caused by force merging.
As you have suggested we can also rename this as force merge or some
other names.
Yeah, but just an suggestion. You are way deeper in the code and
handling than I'm, so feel free to name it whatever you think fits best.
Regards,
Christian.
Thanks,
Arun.
Thanks,
Arun.
Regards,
Christian.
+ if (unlikely(r))
+ goto error_free_blocks;
+ } else {
+ goto error_free_blocks;
+ }
+ }
if (size > remaining_size)
remaining_size = 0;
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 40131ed9b0cd..19440f8caec0 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
}
}
}
+EXPORT_SYMBOL(drm_buddy_defrag);
/**
* drm_buddy_free_block - free a block
Re: [PATCH v8 3/3] drm/buddy: Add user for defragmentation
On 3/5/2024 4:33 PM, Paneer Selvam, Arunpravin wrote:
Hi Christian,
On 3/4/2024 10:09 PM, Christian König wrote:
Am 04.03.24 um 17:32 schrieb Arunpravin Paneer Selvam:
Add amdgpu driver as user for the drm buddy
defragmentation.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++--
drivers/gpu/drm/drm_buddy.c | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index e494f5bf136a..cff8a526c622 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct
ttm_resource_manager *man,
min_block_size,
&vres->blocks,
vres->flags);
- if (unlikely(r))
- goto error_free_blocks;
+ if (unlikely(r)) {
+ if (r == -ENOSPC) {
+ drm_buddy_defrag(mm, min_block_size);
+ r = drm_buddy_alloc_blocks(mm, fpfn,
+ lpfn,
+ size,
+ min_block_size,
+ &vres->blocks,
+ vres->flags);
That doesn't looks like something we should do.
We might fallback when contiguous memory is requested, but certainly
not on normal allocation failure.
yes, defrag here not useful for normal allocations. But worried about
the bigger min_block_size normal allocations.
In such cases, I think we should move this drm_buddy_defrag() call
into buddy allocator file. For example if the required
size is 1024KiB and if min_block_size is 256KiB, the allocator first
tries to find the 1024KiB block, when there is no single 1024KiB block,
the allocator goes one level below in freelist and tries to search for
two 512KiB blocks and goes on. At one point of time if we have less
space,
we might go further levels below to search four 256KiB blocks to
satisfy the request.
Assuming if the allocator cannot find the first 256KiB block, that
time I think we might need to merge the two 128KiB blocks
through defragmentation function. And again for the second 256KiB
block, we might need to call the defragmentation again to
merge two 128KiB blocks or four 64KiB blocks to form minimum alignment
size of 256KiB. This goes on for the third and fourth
256KiB blocks to complete the required size allocation of 1024KiB.
Please let me know if my understanding is not correct.
As you have suggested we can also rename this as force merge or some
other names.
Thanks,
Arun.
Thanks,
Arun.
Regards,
Christian.
+ if (unlikely(r))
+ goto error_free_blocks;
+ } else {
+ goto error_free_blocks;
+ }
+ }
if (size > remaining_size)
remaining_size = 0;
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 40131ed9b0cd..19440f8caec0 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
}
}
}
+EXPORT_SYMBOL(drm_buddy_defrag);
/**
* drm_buddy_free_block - free a block
Re: [PATCH v8 3/3] drm/buddy: Add user for defragmentation
Hi Christian,
On 3/4/2024 10:09 PM, Christian König wrote:
Am 04.03.24 um 17:32 schrieb Arunpravin Paneer Selvam:
Add amdgpu driver as user for the drm buddy
defragmentation.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++--
drivers/gpu/drm/drm_buddy.c | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index e494f5bf136a..cff8a526c622 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct
ttm_resource_manager *man,
min_block_size,
&vres->blocks,
vres->flags);
- if (unlikely(r))
- goto error_free_blocks;
+ if (unlikely(r)) {
+ if (r == -ENOSPC) {
+ drm_buddy_defrag(mm, min_block_size);
+ r = drm_buddy_alloc_blocks(mm, fpfn,
+ lpfn,
+ size,
+ min_block_size,
+ &vres->blocks,
+ vres->flags);
That doesn't looks like something we should do.
We might fallback when contiguous memory is requested, but certainly
not on normal allocation failure.
yes, defrag here not useful for normal allocations. But worried about
the bigger min_block_size normal allocations.
In such cases, I think we should move this drm_buddy_defrag() call into
buddy allocator file. For example if the required
size is 1024KiB and if min_block_size is 256KiB, the allocator first
tries to find the 1024KiB block, when there is no single 1024KiB block,
the allocator goes one level below in freelist and tries to search for
two 512KiB blocks and goes on. At one point of time if we have less space,
we might go further levels below to search four 256KiB blocks to satisfy
the request.
Assuming if the allocator cannot find the first 256KiB block, that time
I think we might need to merge the two 128KiB blocks
through defragmentation function. And again for the second 256KiB block,
we might need to call the defragmentation again to
merge two 128KiB blocks or four 64KiB blocks to form minimum alignment
size of 256KiB. This goes on for the third and fourth
256KiB blocks to complete the required size allocation of 1024KiB.
Please let me know if my understanding is not correct.
Thanks,
Arun.
Regards,
Christian.
+ if (unlikely(r))
+ goto error_free_blocks;
+ } else {
+ goto error_free_blocks;
+ }
+ }
if (size > remaining_size)
remaining_size = 0;
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 40131ed9b0cd..19440f8caec0 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
}
}
}
+EXPORT_SYMBOL(drm_buddy_defrag);
/**
* drm_buddy_free_block - free a block
Re: [PATCH v8 3/3] drm/buddy: Add user for defragmentation
Am 04.03.24 um 17:32 schrieb Arunpravin Paneer Selvam:
Add amdgpu driver as user for the drm buddy
defragmentation.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++--
drivers/gpu/drm/drm_buddy.c | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index e494f5bf136a..cff8a526c622 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager
*man,
min_block_size,
&vres->blocks,
vres->flags);
- if (unlikely(r))
- goto error_free_blocks;
+ if (unlikely(r)) {
+ if (r == -ENOSPC) {
+ drm_buddy_defrag(mm, min_block_size);
+ r = drm_buddy_alloc_blocks(mm, fpfn,
+ lpfn,
+ size,
+ min_block_size,
+ &vres->blocks,
+ vres->flags);
That doesn't looks like something we should do.
We might fallback when contiguous memory is requested, but certainly not
on normal allocation failure.
Regards,
Christian.
+ if (unlikely(r))
+ goto error_free_blocks;
+ } else {
+ goto error_free_blocks;
+ }
+ }
if (size > remaining_size)
remaining_size = 0;
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 40131ed9b0cd..19440f8caec0 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
}
}
}
+EXPORT_SYMBOL(drm_buddy_defrag);
/**
* drm_buddy_free_block - free a block
[PATCH v8 3/3] drm/buddy: Add user for defragmentation
Add amdgpu driver as user for the drm buddy
defragmentation.
Signed-off-by: Arunpravin Paneer Selvam
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 17 +++--
drivers/gpu/drm/drm_buddy.c | 1 +
2 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
index e494f5bf136a..cff8a526c622 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
@@ -533,8 +533,21 @@ static int amdgpu_vram_mgr_new(struct ttm_resource_manager
*man,
min_block_size,
&vres->blocks,
vres->flags);
- if (unlikely(r))
- goto error_free_blocks;
+ if (unlikely(r)) {
+ if (r == -ENOSPC) {
+ drm_buddy_defrag(mm, min_block_size);
+ r = drm_buddy_alloc_blocks(mm, fpfn,
+ lpfn,
+ size,
+ min_block_size,
+ &vres->blocks,
+ vres->flags);
+ if (unlikely(r))
+ goto error_free_blocks;
+ } else {
+ goto error_free_blocks;
+ }
+ }
if (size > remaining_size)
remaining_size = 0;
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 40131ed9b0cd..19440f8caec0 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -396,6 +396,7 @@ void drm_buddy_defrag(struct drm_buddy *mm,
}
}
}
+EXPORT_SYMBOL(drm_buddy_defrag);
/**
* drm_buddy_free_block - free a block
--
2.25.1
