* [PATCH 1/1] drm/amdgpu: release gtt bo after each move test
@ 2021-10-12 12:10 Nirmoy Das
[not found] ` <2be293df-2656-c551-682e-b08955307c0b@163.com>
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Nirmoy Das @ 2021-10-12 12:10 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, Nirmoy Das, zhang
When gart size is < gtt size this test will fail with
-ENOMEM as we are not freeing gtt bo after each move test.
This is generally not an issue when gart size >= gtt size.
Reported-by: zhang <botton_zhang@163.com>
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
index 909d830b513e..0cf2a560d673 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
@@ -212,7 +212,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
gart_addr - adev->gmc.gart_start);
- continue;
out_lclean_unpin:
amdgpu_bo_unpin(gtt_obj[i]);
@@ -220,6 +219,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
amdgpu_bo_unreserve(gtt_obj[i]);
out_lclean_unref:
amdgpu_bo_unref(>t_obj[i]);
+ continue;
out_lclean:
for (--i; i >= 0; --i) {
amdgpu_bo_unpin(gtt_obj[i]);
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread[parent not found: <2be293df-2656-c551-682e-b08955307c0b@163.com>]
* Re: [PATCH 1/1] drm/amdgpu: release gtt bo after each move test
[not found] ` <2be293df-2656-c551-682e-b08955307c0b@163.com>
@ 2021-10-13 10:42 ` Das, Nirmoy
2021-10-13 15:07 ` Das, Nirmoy
0 siblings, 1 reply; 13+ messages in thread
From: Das, Nirmoy @ 2021-10-13 10:42 UTC (permalink / raw)
To: zhang; +Cc: amd-gfx list
[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]
On 10/13/2021 3:22 AM, zhang wrote:
>
> Hi . Nirmoy
>
>
> If you let continue to unpin. this will allways test a same va for gtt
>
> I think we should rafresh calculate the value n
>
Right, I guess then the test should only run till gart size.
Regards,
Nirmoy
>
> On 2021/10/12 20:10, Nirmoy Das wrote:
>> When gart size is < gtt size this test will fail with
>> -ENOMEM as we are not freeing gtt bo after each move test.
>> This is generally not an issue when gart size >= gtt size.
>>
>> Reported-by: zhang<botton_zhang@163.com>
>> Signed-off-by: Nirmoy Das<nirmoy.das@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> index 909d830b513e..0cf2a560d673 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> @@ -212,7 +212,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>>
>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
>> gart_addr - adev->gmc.gart_start);
>> - continue;
>>
>> out_lclean_unpin:
>> amdgpu_bo_unpin(gtt_obj[i]);
>> @@ -220,6 +219,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>> amdgpu_bo_unreserve(gtt_obj[i]);
>> out_lclean_unref:
>> amdgpu_bo_unref(>t_obj[i]);
>> + continue;
>> out_lclean:
>> for (--i; i >= 0; --i) {
>> amdgpu_bo_unpin(gtt_obj[i]);
[-- Attachment #2: Type: text/html, Size: 3075 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: release gtt bo after each move test
2021-10-13 10:42 ` Das, Nirmoy
@ 2021-10-13 15:07 ` Das, Nirmoy
0 siblings, 0 replies; 13+ messages in thread
From: Das, Nirmoy @ 2021-10-13 15:07 UTC (permalink / raw)
To: zhang; +Cc: amd-gfx list
[-- Attachment #1: Type: text/plain, Size: 1807 bytes --]
On 10/13/2021 12:42 PM, Das, Nirmoy wrote:
>
>
> On 10/13/2021 3:22 AM, zhang wrote:
>>
>> Hi . Nirmoy
>>
>>
>> If you let continue to unpin. this will allways test a same va for gtt
>>
>> I think we should rafresh calculate the value n
>>
>
> Right, I guess then the test should only run till gart size.
>
Actually the test size calculation was fine, it is just that we wouldn't
release BO after a successful test as the cleanup code is inside the
test for loop.
Regards,
Nirmoy
>
> Regards,
>
> Nirmoy
>
>>
>> On 2021/10/12 20:10, Nirmoy Das wrote:
>>> When gart size is < gtt size this test will fail with
>>> -ENOMEM as we are not freeing gtt bo after each move test.
>>> This is generally not an issue when gart size >= gtt size.
>>>
>>> Reported-by: zhang<botton_zhang@163.com>
>>> Signed-off-by: Nirmoy Das<nirmoy.das@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> index 909d830b513e..0cf2a560d673 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> @@ -212,7 +212,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>>>
>>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
>>> gart_addr - adev->gmc.gart_start);
>>> - continue;
>>>
>>> out_lclean_unpin:
>>> amdgpu_bo_unpin(gtt_obj[i]);
>>> @@ -220,6 +219,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>>> amdgpu_bo_unreserve(gtt_obj[i]);
>>> out_lclean_unref:
>>> amdgpu_bo_unref(>t_obj[i]);
>>> + continue;
>>> out_lclean:
>>> for (--i; i >= 0; --i) {
>>> amdgpu_bo_unpin(gtt_obj[i]);
[-- Attachment #2: Type: text/html, Size: 3760 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1] drm/amdgpu: release gtt bo after each move test
2021-10-12 12:10 [PATCH 1/1] drm/amdgpu: release gtt bo after each move test Nirmoy Das
[not found] ` <2be293df-2656-c551-682e-b08955307c0b@163.com>
@ 2021-10-13 15:04 ` Nirmoy Das
2021-10-13 15:10 ` Das, Nirmoy
2021-10-13 15:09 ` [PATCH 1/1] drm/amdgpu: fix BO leak after successful " Nirmoy Das
2 siblings, 1 reply; 13+ messages in thread
From: Nirmoy Das @ 2021-10-13 15:04 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, Nirmoy Das, zhang
When gart size is < gtt size this test will fail with
-ENOMEM as we are not freeing gtt bo after each move test.
This is generally not an issue when gart size >= gtt size.
Reported-by: zhang <botton_zhang@163.com>
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
index 909d830b513e..0cf2a560d673 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
@@ -212,7 +212,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
gart_addr - adev->gmc.gart_start);
- continue;
out_lclean_unpin:
amdgpu_bo_unpin(gtt_obj[i]);
@@ -220,6 +219,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
amdgpu_bo_unreserve(gtt_obj[i]);
out_lclean_unref:
amdgpu_bo_unref(>t_obj[i]);
+ continue;
out_lclean:
for (--i; i >= 0; --i) {
amdgpu_bo_unpin(gtt_obj[i]);
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: release gtt bo after each move test
2021-10-13 15:04 ` Nirmoy Das
@ 2021-10-13 15:10 ` Das, Nirmoy
0 siblings, 0 replies; 13+ messages in thread
From: Das, Nirmoy @ 2021-10-13 15:10 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, zhang
Please ignore this!
On 10/13/2021 5:04 PM, Nirmoy Das wrote:
> When gart size is < gtt size this test will fail with
> -ENOMEM as we are not freeing gtt bo after each move test.
> This is generally not an issue when gart size >= gtt size.
>
> Reported-by: zhang <botton_zhang@163.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> index 909d830b513e..0cf2a560d673 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> @@ -212,7 +212,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>
> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
> gart_addr - adev->gmc.gart_start);
> - continue;
>
> out_lclean_unpin:
> amdgpu_bo_unpin(gtt_obj[i]);
> @@ -220,6 +219,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
> amdgpu_bo_unreserve(gtt_obj[i]);
> out_lclean_unref:
> amdgpu_bo_unref(>t_obj[i]);
> + continue;
> out_lclean:
> for (--i; i >= 0; --i) {
> amdgpu_bo_unpin(gtt_obj[i]);
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-12 12:10 [PATCH 1/1] drm/amdgpu: release gtt bo after each move test Nirmoy Das
[not found] ` <2be293df-2656-c551-682e-b08955307c0b@163.com>
2021-10-13 15:04 ` Nirmoy Das
@ 2021-10-13 15:09 ` Nirmoy Das
2021-10-20 8:50 ` Das, Nirmoy
2021-10-20 11:50 ` Christian König
2 siblings, 2 replies; 13+ messages in thread
From: Nirmoy Das @ 2021-10-13 15:09 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, Nirmoy Das, zhang
GTT BO cleanup code is with in the test for loop and
we would skip cleaning up GTT BO on success.
Reported-by: zhang <botton_zhang@163.com>
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25 ++++++++++++------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
index 909d830b513e..5fe7ff680c29 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
@@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
struct amdgpu_bo *vram_obj = NULL;
struct amdgpu_bo **gtt_obj = NULL;
struct amdgpu_bo_param bp;
+ struct dma_fence *fence = NULL;
uint64_t gart_addr, vram_addr;
unsigned n, size;
int i, r;
@@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
void *gtt_map, *vram_map;
void **gart_start, **gart_end;
void **vram_start, **vram_end;
- struct dma_fence *fence = NULL;
bp.domain = AMDGPU_GEM_DOMAIN_GTT;
r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
@@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
gart_addr - adev->gmc.gart_start);
- continue;
+ }
+ --i;
out_lclean_unpin:
- amdgpu_bo_unpin(gtt_obj[i]);
+ amdgpu_bo_unpin(gtt_obj[i]);
out_lclean_unres:
- amdgpu_bo_unreserve(gtt_obj[i]);
+ amdgpu_bo_unreserve(gtt_obj[i]);
out_lclean_unref:
- amdgpu_bo_unref(>t_obj[i]);
+ amdgpu_bo_unref(>t_obj[i]);
out_lclean:
- for (--i; i >= 0; --i) {
- amdgpu_bo_unpin(gtt_obj[i]);
- amdgpu_bo_unreserve(gtt_obj[i]);
- amdgpu_bo_unref(>t_obj[i]);
- }
- if (fence)
- dma_fence_put(fence);
- break;
+ for (--i; i >= 0; --i) {
+ amdgpu_bo_unpin(gtt_obj[i]);
+ amdgpu_bo_unreserve(gtt_obj[i]);
+ amdgpu_bo_unref(>t_obj[i]);
}
+ if (fence)
+ dma_fence_put(fence);
amdgpu_bo_unpin(vram_obj);
out_unres:
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-13 15:09 ` [PATCH 1/1] drm/amdgpu: fix BO leak after successful " Nirmoy Das
@ 2021-10-20 8:50 ` Das, Nirmoy
2021-10-20 11:50 ` Christian König
1 sibling, 0 replies; 13+ messages in thread
From: Das, Nirmoy @ 2021-10-20 8:50 UTC (permalink / raw)
To: amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, zhang
ping.
On 10/13/2021 5:09 PM, Nirmoy Das wrote:
> GTT BO cleanup code is with in the test for loop and
> we would skip cleaning up GTT BO on success.
>
> Reported-by: zhang <botton_zhang@163.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25 ++++++++++++------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> index 909d830b513e..5fe7ff680c29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
> struct amdgpu_bo *vram_obj = NULL;
> struct amdgpu_bo **gtt_obj = NULL;
> struct amdgpu_bo_param bp;
> + struct dma_fence *fence = NULL;
> uint64_t gart_addr, vram_addr;
> unsigned n, size;
> int i, r;
> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
> void *gtt_map, *vram_map;
> void **gart_start, **gart_end;
> void **vram_start, **vram_end;
> - struct dma_fence *fence = NULL;
>
> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>
> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
> gart_addr - adev->gmc.gart_start);
> - continue;
> + }
>
> + --i;
> out_lclean_unpin:
> - amdgpu_bo_unpin(gtt_obj[i]);
> + amdgpu_bo_unpin(gtt_obj[i]);
> out_lclean_unres:
> - amdgpu_bo_unreserve(gtt_obj[i]);
> + amdgpu_bo_unreserve(gtt_obj[i]);
> out_lclean_unref:
> - amdgpu_bo_unref(>t_obj[i]);
> + amdgpu_bo_unref(>t_obj[i]);
> out_lclean:
> - for (--i; i >= 0; --i) {
> - amdgpu_bo_unpin(gtt_obj[i]);
> - amdgpu_bo_unreserve(gtt_obj[i]);
> - amdgpu_bo_unref(>t_obj[i]);
> - }
> - if (fence)
> - dma_fence_put(fence);
> - break;
> + for (--i; i >= 0; --i) {
> + amdgpu_bo_unpin(gtt_obj[i]);
> + amdgpu_bo_unreserve(gtt_obj[i]);
> + amdgpu_bo_unref(>t_obj[i]);
> }
> + if (fence)
> + dma_fence_put(fence);
>
> amdgpu_bo_unpin(vram_obj);
> out_unres:
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-13 15:09 ` [PATCH 1/1] drm/amdgpu: fix BO leak after successful " Nirmoy Das
2021-10-20 8:50 ` Das, Nirmoy
@ 2021-10-20 11:50 ` Christian König
2021-10-20 11:51 ` Christian König
1 sibling, 1 reply; 13+ messages in thread
From: Christian König @ 2021-10-20 11:50 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, zhang
Am 13.10.21 um 17:09 schrieb Nirmoy Das:
> GTT BO cleanup code is with in the test for loop and
> we would skip cleaning up GTT BO on success.
>
> Reported-by: zhang <botton_zhang@163.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25 ++++++++++++------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> index 909d830b513e..5fe7ff680c29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
> struct amdgpu_bo *vram_obj = NULL;
> struct amdgpu_bo **gtt_obj = NULL;
> struct amdgpu_bo_param bp;
> + struct dma_fence *fence = NULL;
> uint64_t gart_addr, vram_addr;
> unsigned n, size;
> int i, r;
> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
> void *gtt_map, *vram_map;
> void **gart_start, **gart_end;
> void **vram_start, **vram_end;
> - struct dma_fence *fence = NULL;
>
> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct amdgpu_device *adev)
>
> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT offset 0x%llx\n",
> gart_addr - adev->gmc.gart_start);
> - continue;
> + }
>
> + --i;
> out_lclean_unpin:
> - amdgpu_bo_unpin(gtt_obj[i]);
> + amdgpu_bo_unpin(gtt_obj[i]);
> out_lclean_unres:
> - amdgpu_bo_unreserve(gtt_obj[i]);
> + amdgpu_bo_unreserve(gtt_obj[i]);
> out_lclean_unref:
> - amdgpu_bo_unref(>t_obj[i]);
> + amdgpu_bo_unref(>t_obj[i]);
> out_lclean:
> - for (--i; i >= 0; --i) {
> - amdgpu_bo_unpin(gtt_obj[i]);
> - amdgpu_bo_unreserve(gtt_obj[i]);
> - amdgpu_bo_unref(>t_obj[i]);
> - }
> - if (fence)
> - dma_fence_put(fence);
> - break;
> + for (--i; i >= 0; --i) {
The usual idiom for cleanups like that is "while (i--)..." because that
also works with an unsigned i.
Apart from that looks good to me.
Christian.
> + amdgpu_bo_unpin(gtt_obj[i]);
> + amdgpu_bo_unreserve(gtt_obj[i]);
> + amdgpu_bo_unref(>t_obj[i]);
> }
> + if (fence)
> + dma_fence_put(fence);
>
> amdgpu_bo_unpin(vram_obj);
> out_unres:
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-20 11:50 ` Christian König
@ 2021-10-20 11:51 ` Christian König
2021-10-20 12:55 ` Das, Nirmoy
2021-10-21 2:07 ` zhang
0 siblings, 2 replies; 13+ messages in thread
From: Christian König @ 2021-10-20 11:51 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, zhang
Am 20.10.21 um 13:50 schrieb Christian König:
>
>
> Am 13.10.21 um 17:09 schrieb Nirmoy Das:
>> GTT BO cleanup code is with in the test for loop and
>> we would skip cleaning up GTT BO on success.
>>
>> Reported-by: zhang <botton_zhang@163.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> index 909d830b513e..5fe7ff680c29 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct
>> amdgpu_device *adev)
>> struct amdgpu_bo *vram_obj = NULL;
>> struct amdgpu_bo **gtt_obj = NULL;
>> struct amdgpu_bo_param bp;
>> + struct dma_fence *fence = NULL;
>> uint64_t gart_addr, vram_addr;
>> unsigned n, size;
>> int i, r;
>> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct
>> amdgpu_device *adev)
>> void *gtt_map, *vram_map;
>> void **gart_start, **gart_end;
>> void **vram_start, **vram_end;
>> - struct dma_fence *fence = NULL;
>> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
>> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct
>> amdgpu_device *adev)
>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT
>> offset 0x%llx\n",
>> gart_addr - adev->gmc.gart_start);
>> - continue;
>> + }
>> + --i;
>> out_lclean_unpin:
>> - amdgpu_bo_unpin(gtt_obj[i]);
>> + amdgpu_bo_unpin(gtt_obj[i]);
>> out_lclean_unres:
>> - amdgpu_bo_unreserve(gtt_obj[i]);
>> + amdgpu_bo_unreserve(gtt_obj[i]);
>> out_lclean_unref:
>> - amdgpu_bo_unref(>t_obj[i]);
>> + amdgpu_bo_unref(>t_obj[i]);
>> out_lclean:
>> - for (--i; i >= 0; --i) {
>> - amdgpu_bo_unpin(gtt_obj[i]);
>> - amdgpu_bo_unreserve(gtt_obj[i]);
>> - amdgpu_bo_unref(>t_obj[i]);
>> - }
>> - if (fence)
>> - dma_fence_put(fence);
>> - break;
>> + for (--i; i >= 0; --i) {
>
> The usual idiom for cleanups like that is "while (i--)..." because
> that also works with an unsigned i.
>
> Apart from that looks good to me.
But I'm not sure that we would want to keep the in kernel tests around
anyway.
We now have my amdgpu_stress tool to test memory bandwidth and mesa has
an option for that for a long time as well.
Christian.
>
> Christian.
>
>> + amdgpu_bo_unpin(gtt_obj[i]);
>> + amdgpu_bo_unreserve(gtt_obj[i]);
>> + amdgpu_bo_unref(>t_obj[i]);
>> }
>> + if (fence)
>> + dma_fence_put(fence);
>> amdgpu_bo_unpin(vram_obj);
>> out_unres:
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-20 11:51 ` Christian König
@ 2021-10-20 12:55 ` Das, Nirmoy
2021-10-21 6:58 ` Christian König
2021-10-21 2:07 ` zhang
1 sibling, 1 reply; 13+ messages in thread
From: Das, Nirmoy @ 2021-10-20 12:55 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, zhang
On 10/20/2021 1:51 PM, Christian König wrote:
> Am 20.10.21 um 13:50 schrieb Christian König:
>>
>>
>> Am 13.10.21 um 17:09 schrieb Nirmoy Das:
>>> GTT BO cleanup code is with in the test for loop and
>>> we would skip cleaning up GTT BO on success.
>>>
>>> Reported-by: zhang <botton_zhang@163.com>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25
>>> ++++++++++++------------
>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> index 909d830b513e..5fe7ff680c29 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct
>>> amdgpu_device *adev)
>>> struct amdgpu_bo *vram_obj = NULL;
>>> struct amdgpu_bo **gtt_obj = NULL;
>>> struct amdgpu_bo_param bp;
>>> + struct dma_fence *fence = NULL;
>>> uint64_t gart_addr, vram_addr;
>>> unsigned n, size;
>>> int i, r;
>>> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct
>>> amdgpu_device *adev)
>>> void *gtt_map, *vram_map;
>>> void **gart_start, **gart_end;
>>> void **vram_start, **vram_end;
>>> - struct dma_fence *fence = NULL;
>>> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>>> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
>>> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct
>>> amdgpu_device *adev)
>>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT
>>> offset 0x%llx\n",
>>> gart_addr - adev->gmc.gart_start);
>>> - continue;
>>> + }
>>> + --i;
>>> out_lclean_unpin:
>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>> out_lclean_unres:
>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>> out_lclean_unref:
>>> - amdgpu_bo_unref(>t_obj[i]);
>>> + amdgpu_bo_unref(>t_obj[i]);
>>> out_lclean:
>>> - for (--i; i >= 0; --i) {
>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>> - amdgpu_bo_unref(>t_obj[i]);
>>> - }
>>> - if (fence)
>>> - dma_fence_put(fence);
>>> - break;
>>> + for (--i; i >= 0; --i) {
>>
>> The usual idiom for cleanups like that is "while (i--)..." because
>> that also works with an unsigned i.
>>
>> Apart from that looks good to me.
>
> But I'm not sure that we would want to keep the in kernel tests around
> anyway.
>
> We now have my amdgpu_stress tool to test memory bandwidth and mesa
> has an option for that for a long time as well.
Shall I then remove amdgpu_test.c ?
Nirmoy
>
> Christian.
>
>>
>> Christian.
>>
>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>> + amdgpu_bo_unref(>t_obj[i]);
>>> }
>>> + if (fence)
>>> + dma_fence_put(fence);
>>> amdgpu_bo_unpin(vram_obj);
>>> out_unres:
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-20 12:55 ` Das, Nirmoy
@ 2021-10-21 6:58 ` Christian König
0 siblings, 0 replies; 13+ messages in thread
From: Christian König @ 2021-10-21 6:58 UTC (permalink / raw)
To: Das, Nirmoy, amd-gfx; +Cc: Christian.Koenig, Alexander.Deucher, zhang
Am 20.10.21 um 14:55 schrieb Das, Nirmoy:
>
> On 10/20/2021 1:51 PM, Christian König wrote:
>> Am 20.10.21 um 13:50 schrieb Christian König:
>>>
>>>
>>> Am 13.10.21 um 17:09 schrieb Nirmoy Das:
>>>> GTT BO cleanup code is with in the test for loop and
>>>> we would skip cleaning up GTT BO on success.
>>>>
>>>> Reported-by: zhang <botton_zhang@163.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25
>>>> ++++++++++++------------
>>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> index 909d830b513e..5fe7ff680c29 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct
>>>> amdgpu_device *adev)
>>>> struct amdgpu_bo *vram_obj = NULL;
>>>> struct amdgpu_bo **gtt_obj = NULL;
>>>> struct amdgpu_bo_param bp;
>>>> + struct dma_fence *fence = NULL;
>>>> uint64_t gart_addr, vram_addr;
>>>> unsigned n, size;
>>>> int i, r;
>>>> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct
>>>> amdgpu_device *adev)
>>>> void *gtt_map, *vram_map;
>>>> void **gart_start, **gart_end;
>>>> void **vram_start, **vram_end;
>>>> - struct dma_fence *fence = NULL;
>>>> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>>>> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
>>>> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct
>>>> amdgpu_device *adev)
>>>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT
>>>> offset 0x%llx\n",
>>>> gart_addr - adev->gmc.gart_start);
>>>> - continue;
>>>> + }
>>>> + --i;
>>>> out_lclean_unpin:
>>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>>> out_lclean_unres:
>>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>>> out_lclean_unref:
>>>> - amdgpu_bo_unref(>t_obj[i]);
>>>> + amdgpu_bo_unref(>t_obj[i]);
>>>> out_lclean:
>>>> - for (--i; i >= 0; --i) {
>>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>>> - amdgpu_bo_unref(>t_obj[i]);
>>>> - }
>>>> - if (fence)
>>>> - dma_fence_put(fence);
>>>> - break;
>>>> + for (--i; i >= 0; --i) {
>>>
>>> The usual idiom for cleanups like that is "while (i--)..." because
>>> that also works with an unsigned i.
>>>
>>> Apart from that looks good to me.
>>
>> But I'm not sure that we would want to keep the in kernel tests
>> around anyway.
>>
>> We now have my amdgpu_stress tool to test memory bandwidth and mesa
>> has an option for that for a long time as well.
>
>
> Shall I then remove amdgpu_test.c ?
Please double check if the amdgpu_stress utility gives you the same
functionality, if yes we should probably remove this test here.
Thanks,
Christian.
>
>
> Nirmoy
>
>
>>
>> Christian.
>>
>>>
>>> Christian.
>>>
>>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>>> + amdgpu_bo_unref(>t_obj[i]);
>>>> }
>>>> + if (fence)
>>>> + dma_fence_put(fence);
>>>> amdgpu_bo_unpin(vram_obj);
>>>> out_unres:
>>>
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-20 11:51 ` Christian König
2021-10-20 12:55 ` Das, Nirmoy
@ 2021-10-21 2:07 ` zhang
2021-10-21 6:58 ` Christian König
1 sibling, 1 reply; 13+ messages in thread
From: zhang @ 2021-10-21 2:07 UTC (permalink / raw)
To: Christian König, Nirmoy Das, amd-gfx
Cc: Christian.Koenig, Alexander.Deucher
On 2021/10/20 19:51, Christian König wrote:
> Am 20.10.21 um 13:50 schrieb Christian König:
>>
>>
>> Am 13.10.21 um 17:09 schrieb Nirmoy Das:
>>> GTT BO cleanup code is with in the test for loop and
>>> we would skip cleaning up GTT BO on success.
>>>
>>> Reported-by: zhang <botton_zhang@163.com>
>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25
>>> ++++++++++++------------
>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> index 909d830b513e..5fe7ff680c29 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct
>>> amdgpu_device *adev)
>>> struct amdgpu_bo *vram_obj = NULL;
>>> struct amdgpu_bo **gtt_obj = NULL;
>>> struct amdgpu_bo_param bp;
>>> + struct dma_fence *fence = NULL;
>>> uint64_t gart_addr, vram_addr;
>>> unsigned n, size;
>>> int i, r;
>>> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct
>>> amdgpu_device *adev)
>>> void *gtt_map, *vram_map;
>>> void **gart_start, **gart_end;
>>> void **vram_start, **vram_end;
>>> - struct dma_fence *fence = NULL;
>>> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>>> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
>>> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct
>>> amdgpu_device *adev)
>>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT
>>> offset 0x%llx\n",
>>> gart_addr - adev->gmc.gart_start);
>>> - continue;
>>> + }
>>> + --i;
>>> out_lclean_unpin:
>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>> out_lclean_unres:
>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>> out_lclean_unref:
>>> - amdgpu_bo_unref(>t_obj[i]);
>>> + amdgpu_bo_unref(>t_obj[i]);
>>> out_lclean:
>>> - for (--i; i >= 0; --i) {
>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>> - amdgpu_bo_unref(>t_obj[i]);
>>> - }
>>> - if (fence)
>>> - dma_fence_put(fence);
>>> - break;
>>> + for (--i; i >= 0; --i) {
>>
>> The usual idiom for cleanups like that is "while (i--)..." because
>> that also works with an unsigned i.
>>
>> Apart from that looks good to me.
>
> But I'm not sure that we would want to keep the in kernel tests around
> anyway.
>
> We now have my amdgpu_stress tool to test memory bandwidth and mesa
> has an option for that for a long time as well.
>
> Christian.
>
I found a testsuit about "bo eviction Test" for amdgpu . in libdrm
tests.
But I couldn't found amdgpu_stress tool to test memory bandwid anywhere
>>
>> Christian.
>>
>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>> + amdgpu_bo_unref(>t_obj[i]);
>>> }
>>> + if (fence)
>>> + dma_fence_put(fence);
>>> amdgpu_bo_unpin(vram_obj);
>>> out_unres:
>>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 1/1] drm/amdgpu: fix BO leak after successful move test
2021-10-21 2:07 ` zhang
@ 2021-10-21 6:58 ` Christian König
0 siblings, 0 replies; 13+ messages in thread
From: Christian König @ 2021-10-21 6:58 UTC (permalink / raw)
To: zhang, Christian König, Nirmoy Das, amd-gfx; +Cc: Alexander.Deucher
Am 21.10.21 um 04:07 schrieb zhang:
>
> On 2021/10/20 19:51, Christian König wrote:
>> Am 20.10.21 um 13:50 schrieb Christian König:
>>>
>>>
>>> Am 13.10.21 um 17:09 schrieb Nirmoy Das:
>>>> GTT BO cleanup code is with in the test for loop and
>>>> we would skip cleaning up GTT BO on success.
>>>>
>>>> Reported-by: zhang <botton_zhang@163.com>
>>>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_test.c | 25
>>>> ++++++++++++------------
>>>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> index 909d830b513e..5fe7ff680c29 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_test.c
>>>> @@ -35,6 +35,7 @@ static void amdgpu_do_test_moves(struct
>>>> amdgpu_device *adev)
>>>> struct amdgpu_bo *vram_obj = NULL;
>>>> struct amdgpu_bo **gtt_obj = NULL;
>>>> struct amdgpu_bo_param bp;
>>>> + struct dma_fence *fence = NULL;
>>>> uint64_t gart_addr, vram_addr;
>>>> unsigned n, size;
>>>> int i, r;
>>>> @@ -82,7 +83,6 @@ static void amdgpu_do_test_moves(struct
>>>> amdgpu_device *adev)
>>>> void *gtt_map, *vram_map;
>>>> void **gart_start, **gart_end;
>>>> void **vram_start, **vram_end;
>>>> - struct dma_fence *fence = NULL;
>>>> bp.domain = AMDGPU_GEM_DOMAIN_GTT;
>>>> r = amdgpu_bo_create(adev, &bp, gtt_obj + i);
>>>> @@ -212,24 +212,23 @@ static void amdgpu_do_test_moves(struct
>>>> amdgpu_device *adev)
>>>> DRM_INFO("Tested GTT->VRAM and VRAM->GTT copy for GTT
>>>> offset 0x%llx\n",
>>>> gart_addr - adev->gmc.gart_start);
>>>> - continue;
>>>> + }
>>>> + --i;
>>>> out_lclean_unpin:
>>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>>> out_lclean_unres:
>>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>>> out_lclean_unref:
>>>> - amdgpu_bo_unref(>t_obj[i]);
>>>> + amdgpu_bo_unref(>t_obj[i]);
>>>> out_lclean:
>>>> - for (--i; i >= 0; --i) {
>>>> - amdgpu_bo_unpin(gtt_obj[i]);
>>>> - amdgpu_bo_unreserve(gtt_obj[i]);
>>>> - amdgpu_bo_unref(>t_obj[i]);
>>>> - }
>>>> - if (fence)
>>>> - dma_fence_put(fence);
>>>> - break;
>>>> + for (--i; i >= 0; --i) {
>>>
>>> The usual idiom for cleanups like that is "while (i--)..." because
>>> that also works with an unsigned i.
>>>
>>> Apart from that looks good to me.
>>
>> But I'm not sure that we would want to keep the in kernel tests
>> around anyway.
>>
>> We now have my amdgpu_stress tool to test memory bandwidth and mesa
>> has an option for that for a long time as well.
>>
>> Christian.
>>
> I found a testsuit about "bo eviction Test" for amdgpu . in
> libdrm tests.
>
> But I couldn't found amdgpu_stress tool to test memory bandwid anywhere
That was merged just yesterday. See upstream libdrm.
Christian.
>
>>>
>>> Christian.
>>>
>>>> + amdgpu_bo_unpin(gtt_obj[i]);
>>>> + amdgpu_bo_unreserve(gtt_obj[i]);
>>>> + amdgpu_bo_unref(>t_obj[i]);
>>>> }
>>>> + if (fence)
>>>> + dma_fence_put(fence);
>>>> amdgpu_bo_unpin(vram_obj);
>>>> out_unres:
>>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-10-21 6:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-10-12 12:10 [PATCH 1/1] drm/amdgpu: release gtt bo after each move test Nirmoy Das
[not found] ` <2be293df-2656-c551-682e-b08955307c0b@163.com>
2021-10-13 10:42 ` Das, Nirmoy
2021-10-13 15:07 ` Das, Nirmoy
2021-10-13 15:04 ` Nirmoy Das
2021-10-13 15:10 ` Das, Nirmoy
2021-10-13 15:09 ` [PATCH 1/1] drm/amdgpu: fix BO leak after successful " Nirmoy Das
2021-10-20 8:50 ` Das, Nirmoy
2021-10-20 11:50 ` Christian König
2021-10-20 11:51 ` Christian König
2021-10-20 12:55 ` Das, Nirmoy
2021-10-21 6:58 ` Christian König
2021-10-21 2:07 ` zhang
2021-10-21 6:58 ` Christian König
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox