All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  2:41 ` Bernard Zhao
  0 siblings, 0 replies; 12+ 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] 12+ messages in thread
* [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  2:41 ` Bernard Zhao
  0 siblings, 0 replies; 12+ 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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  2:41 ` Bernard Zhao
  0 siblings, 0 replies; 12+ 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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
  2020-04-21  2:41 ` Bernard Zhao
  (?)
@ 2020-04-21  4:24   ` Felix Kuehling
  -1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  4:24   ` Felix Kuehling
  0 siblings, 0 replies; 12+ 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;
>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  4:24   ` Felix Kuehling
  0 siblings, 0 replies; 12+ 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;
>  }
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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


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


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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


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


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
  2020-04-21  2:41 ` Bernard Zhao
  (?)
@ 2020-04-21  7:15   ` Christian König
  -1 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  7:15   ` Christian König
  0 siblings, 0 replies; 12+ 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;
>   }

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH V2] amdgpu: remove unnecessary condition check
@ 2020-04-21  7:15   ` Christian König
  0 siblings, 0 replies; 12+ 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;
>   }

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

Thread overview: 12+ 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  2:41 ` Bernard Zhao
2020-04-21  2:41 ` Bernard Zhao
2020-04-21  4:24 ` Felix Kuehling
2020-04-21  4:24   ` Felix Kuehling
2020-04-21  4:24   ` Felix Kuehling
2020-04-21  4:53   ` 赵军奎
2020-04-21  4:53     ` 赵军奎
2020-04-21  4:53     ` 赵军奎
2020-04-21  7:15 ` Christian König
2020-04-21  7:15   ` Christian König
2020-04-21  7:15   ` Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.