linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  2:41 Bernard Zhao
  2020-04-21  4:24 ` Felix Kuehling
  2020-04-21  7:15 ` Christian König
  0 siblings, 2 replies; 4+ messages in thread
From: Bernard Zhao @ 2020-04-21  2:41 UTC (permalink / raw)
  To: Felix Kuehling, Alex Deucher, Christian König,
	David (ChunMing) Zhou, David Airlie, Daniel Vetter, amd-gfx,
	dri-devel, linux-kernel
  Cc: opensource.kernel, Bernard Zhao

There is no need to if check again, maybe we could merge
into the above else branch.

Signed-off-by: Bernard Zhao <bernard@vivo.com>

---
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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
  2020-04-21  2:41 [PATCH V2] amdgpu: remove unnecessary condition check Bernard Zhao
@ 2020-04-21  4:24 ` Felix Kuehling
  2020-04-21  4:53   ` 赵军奎
  2020-04-21  7:15 ` Christian König
  1 sibling, 1 reply; 4+ messages in thread
From: Felix Kuehling @ 2020-04-21  4:24 UTC (permalink / raw)
  To: 1587180037-113840-1-git-send-email-bernard, Alex Deucher,
	Christian König, David (ChunMing) Zhou, David Airlie,
	Daniel Vetter, amd-gfx, dri-devel, linux-kernel
  Cc: opensource.kernel, Bernard Zhao

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 <bernard@vivo.com>
>
> ---
> 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;
>  }

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re:Re: [PATCH V2] amdgpu: remove unnecessary condition check
  2020-04-21  4:24 ` Felix Kuehling
@ 2020-04-21  4:53   ` 赵军奎
  0 siblings, 0 replies; 4+ messages in thread
From: 赵军奎 @ 2020-04-21  4:53 UTC (permalink / raw)
  To: Felix Kuehling
  Cc: alexander.deucher, Christian König, David (ChunMing) Zhou,
	David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel,
	opensource.kernel


From: Felix Kuehling <felix.kuehling@amd.com>
Date: 2020-04-21 12:24:19
To:  1587180037-113840-1-git-send-email-bernard@vivo.com,Alex Deucher <alexander.deucher@amd.com>,"Christian König" <christian.koenig@amd.com>,"David (ChunMing) Zhou" <David1.Zhou@amd.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org
Cc:  opensource.kernel@vivo.com,Bernard Zhao <bernard@vivo.com>
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 <bernard@vivo.com>
>>
>> ---
>> 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 <if (! Condition)... else .... > format. 
So in V2, after improve the commit  info, 
I refer to the following code style suggestions and modify it to <if (condition)...else... > 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;
>>  }



^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
  2020-04-21  2:41 [PATCH V2] amdgpu: remove unnecessary condition check Bernard Zhao
  2020-04-21  4:24 ` Felix Kuehling
@ 2020-04-21  7:15 ` Christian König
  1 sibling, 0 replies; 4+ messages in thread
From: Christian König @ 2020-04-21  7:15 UTC (permalink / raw)
  To: 1587180037-113840-1-git-send-email-bernard, Felix Kuehling,
	Alex Deucher, David (ChunMing) Zhou, David Airlie, Daniel Vetter,
	amd-gfx, dri-devel, linux-kernel
  Cc: opensource.kernel, Bernard Zhao

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 <bernard@vivo.com>
>
> ---
> 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&amp;data=02%7C01%7Cchristian.koenig%40amd.com%7C50bb3a13f28b4e5d787508d7e59d9903%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230337359422169&amp;sdata=LXUJgHOxfwSpacdW6suiI00z8egbRC3z3za0H3XtNV4%3D&amp;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;
>   }


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-21  7:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21  2:41 [PATCH V2] amdgpu: remove unnecessary condition check Bernard Zhao
2020-04-21  4:24 ` Felix Kuehling
2020-04-21  4:53   ` 赵军奎
2020-04-21  7:15 ` 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;
as well as URLs for NNTP newsgroup(s).