Re: [PATCH V2] amdgpu: remove unnecessary condition check
Am 21.04.20 um 04:41 schrieb Bernard Zhao:
There is no need to if check again, maybe we could merge
into the above else branch.
Signed-off-by: Bernard Zhao
---
Changes since V1:
*commit message improve
*code style refactoring
Link for V1:
*
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1226587%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C50bb3a13f28b4e5d787508d7e59d9903%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230337359422169&sdata=LXUJgHOxfwSpacdW6suiI00z8egbRC3z3za0H3XtNV4%3D&reserved=0
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 9dff792c9290..a64eeb07bec4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -660,13 +660,15 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
false, &ctx->duplicates);
- if (!ret)
- ctx->reserved = true;
- else {
+
+ if (ret) {
pr_err("Failed to reserve buffers in ttm\n");
kfree(ctx->vm_pd);
ctx->vm_pd = NULL;
}
+ else {
+ ctx->reserved = true;
+ }
return ret;
}
@@ -733,15 +735,15 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
false, &ctx->duplicates);
- if (!ret)
- ctx->reserved = true;
- else
- pr_err("Failed to reserve buffers in ttm.\n");
if (ret) {
+ pr_err("Failed to reserve buffers in ttm.\n");
kfree(ctx->vm_pd);
ctx->vm_pd = NULL;
}
+ else {
Please use "} else {" here.
Christian.
+ ctx->reserved = true;
+ }
return ret;
}
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH V2] amdgpu: remove unnecessary condition check
There is no need to if check again, maybe we could merge
into the above else branch.
Signed-off-by: Bernard Zhao
---
Changes since V1:
*commit message improve
*code style refactoring
Link for V1:
* https://lore.kernel.org/patchwork/patch/1226587/
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 9dff792c9290..a64eeb07bec4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -660,13 +660,15 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
false, &ctx->duplicates);
- if (!ret)
- ctx->reserved = true;
- else {
+
+ if (ret) {
pr_err("Failed to reserve buffers in ttm\n");
kfree(ctx->vm_pd);
ctx->vm_pd = NULL;
}
+ else {
+ ctx->reserved = true;
+ }
return ret;
}
@@ -733,15 +735,15 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
false, &ctx->duplicates);
- if (!ret)
- ctx->reserved = true;
- else
- pr_err("Failed to reserve buffers in ttm.\n");
if (ret) {
+ pr_err("Failed to reserve buffers in ttm.\n");
kfree(ctx->vm_pd);
ctx->vm_pd = NULL;
}
+ else {
+ ctx->reserved = true;
+ }
return ret;
}
--
2.26.2
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re:Re: [PATCH V2] amdgpu: remove unnecessary condition check
From: Felix Kuehling
Date: 2020-04-21 12:24:19
To: 1587180037-113840-1-git-send-email-bern...@vivo.com,Alex Deucher
,"Christian König" ,"David
(ChunMing) Zhou" ,David Airlie ,Daniel
Vetter
,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org
Cc: opensource.ker...@vivo.com,Bernard Zhao
Subject: Re: [PATCH V2] amdgpu: remove unnecessary condition check>Hi Bernard,
>
>Please see comments inline.
>
>Am 2020-04-20 um 10:41 p.m. schrieb Bernard Zhao:
>> There is no need to if check again, maybe we could merge
>> into the above else branch.
>>
>> Signed-off-by: Bernard Zhao
>>
>> ---
>> Changes since V1:
>> *commit message improve
>> *code style refactoring
>>
>> Link for V1:
>> * https://lore.kernel.org/patchwork/patch/1226587/
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +---
>> 1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> index 9dff792c9290..a64eeb07bec4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -660,13 +660,15 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
>>
>> ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>> false, &ctx->duplicates);
>> -if (!ret)
>> -ctx->reserved = true;
>> -else {
>> +
>> +if (ret) {
>> pr_err("Failed to reserve buffers in ttm\n");
>> kfree(ctx->vm_pd);
>> ctx->vm_pd = NULL;
>> }
>> +else {
>> +ctx->reserved = true;
>> +}
>
>Here you're just reversing the if and else branches. This change looks
>completely superfluous to me.
>
>You're also breaking coding style conventions. The "else" should be on
>the same line as the closing brace "}". I'm pretty sure checkpatch.pl
>will complain about this.
>
In this file, only these two functions are
format.
So in V2, after improve the commit info,
I refer to the following code style suggestions and modify it to format
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=90280eaa88ac1a9140dc759941123530d5545bb6#n191
(refer from Markus Elfring`s suggestion).
>>
>> return ret;
>> }
>> @@ -733,15 +735,15 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
>>
>> ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>> false, &ctx->duplicates);
>> -if (!ret)
>> -ctx->reserved = true;
>> -else
>> -pr_err("Failed to reserve buffers in ttm.\n");
>>
>> if (ret) {
>> +pr_err("Failed to reserve buffers in ttm.\n");
>> kfree(ctx->vm_pd);
>> ctx->vm_pd = NULL;
>> }
>> +else {
>> +ctx->reserved = true;
>> +}
>
>Same as above regarding coding style.
>
>To minimize unnecessary code changes, you can merge the "if (ret) ..."
>code into the else-branch of the previous if.
>
>Regards,
> Felix
>
>
>>
>> return ret;
>> }
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH V2] amdgpu: remove unnecessary condition check
Hi Bernard,
Please see comments inline.
Am 2020-04-20 um 10:41 p.m. schrieb Bernard Zhao:
> There is no need to if check again, maybe we could merge
> into the above else branch.
>
> Signed-off-by: Bernard Zhao
>
> ---
> Changes since V1:
> *commit message improve
> *code style refactoring
>
> Link for V1:
> * https://lore.kernel.org/patchwork/patch/1226587/
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +---
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index 9dff792c9290..a64eeb07bec4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -660,13 +660,15 @@ static int reserve_bo_and_vm(struct kgd_mem *mem,
>
> ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>false, &ctx->duplicates);
> - if (!ret)
> - ctx->reserved = true;
> - else {
> +
> + if (ret) {
> pr_err("Failed to reserve buffers in ttm\n");
> kfree(ctx->vm_pd);
> ctx->vm_pd = NULL;
> }
> + else {
> + ctx->reserved = true;
> + }
Here you're just reversing the if and else branches. This change looks
completely superfluous to me.
You're also breaking coding style conventions. The "else" should be on
the same line as the closing brace "}". I'm pretty sure checkpatch.pl
will complain about this.
>
> return ret;
> }
> @@ -733,15 +735,15 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
>
> ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list,
>false, &ctx->duplicates);
> - if (!ret)
> - ctx->reserved = true;
> - else
> - pr_err("Failed to reserve buffers in ttm.\n");
>
> if (ret) {
> + pr_err("Failed to reserve buffers in ttm.\n");
> kfree(ctx->vm_pd);
> ctx->vm_pd = NULL;
> }
> + else {
> + ctx->reserved = true;
> + }
Same as above regarding coding style.
To minimize unnecessary code changes, you can merge the "if (ret) ..."
code into the else-branch of the previous if.
Regards,
Felix
>
> return ret;
> }
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
