[PATCH] drm/amdgpu: Moving out the mutex lock and unlcok outside of the "if" statements

[PATCH] drm/amdgpu: Moving out the mutex lock and unlcok outside of the "if" statements

[Alex Jivin]

Moving mutex unlock and lock outside of the "if" statement as it can be shown that
the mutex will be taken and released, regardless of the value checked in the if statement.

Signed-off-by: Alex Jivin <alex.jivin@amd.com>
Suggested-By: Luben Tukov <luben.tuikov@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
index 838d6d51904c..d2401379bd33 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
@@ -3559,16 +3559,14 @@ void amdgpu_dpm_enable_uvd(struct amdgpu_device *adev, bool enable)
 	int ret = 0;
 
 	if (adev->family == AMDGPU_FAMILY_SI) {
+		mutex_lock(&adev->pm.mutex);
 		if (enable) {
-			mutex_lock(&adev->pm.mutex);
 			adev->pm.dpm.uvd_active = true;
 			adev->pm.dpm.state = POWER_STATE_TYPE_INTERNAL_UVD;
-			mutex_unlock(&adev->pm.mutex);
 		} else {
-			mutex_lock(&adev->pm.mutex);
 			adev->pm.dpm.uvd_active = false;
-			mutex_unlock(&adev->pm.mutex);
 		}
+		mutex_unlock(&adev->pm.mutex);
 
 		amdgpu_pm_compute_clocks(adev);
 	} else {
@@ -3596,17 +3594,15 @@ void amdgpu_dpm_enable_vce(struct amdgpu_device *adev, bool enable)
 	int ret = 0;
 
 	if (adev->family == AMDGPU_FAMILY_SI) {
+		mutex_lock(&adev->pm.mutex);
 		if (enable) {
-			mutex_lock(&adev->pm.mutex);
 			adev->pm.dpm.vce_active = true;
 			/* XXX select vce level based on ring/task */
 			adev->pm.dpm.vce_level = AMD_VCE_LEVEL_AC_ALL;
-			mutex_unlock(&adev->pm.mutex);
 		} else {
-			mutex_lock(&adev->pm.mutex);
 			adev->pm.dpm.vce_active = false;
-			mutex_unlock(&adev->pm.mutex);
 		}
+		mutex_unlock(&adev->pm.mutex);
 
 		amdgpu_pm_compute_clocks(adev);
 	} else {
-- 
2.17.1

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

Re: [PATCH] drm/amdgpu: Moving out the mutex lock and unlcok outside of the "if" statements

[Luben Tuikov]

Fix the spelling mistake in the title of the patch,
"unlcok" --> "unlock".

Use present tense in the title and in commit text:
"Moving" --> "Move".

Shorten your title to fit within 80-char limit.
A Git hook checks for this and complains about it
when doing a git-push. I suggest:

	drm/amdgpu: move the mutex lock/unlock out

And then you start your first sentence of your commit
message with the title as a capitalized sentence:

	Move the mutext lock/unlock outside of the if(),
	as the mutex is always taken: either in the if()
	branch or in the else branch.

The commit message text should be 55-65 line width
aligned as Git indents it by 8 spaces when viewed
individually or in a log. (Modern Emacs recognizes
that you're writing a Git commit message and sets
the wrap at 55 chars.)

After all those are fixed, you can add,

Reviewed-by: Luben Tuikov <luben.tuikov@amd.com>

On 2020-07-06 12:10 p.m., Alex Jivin wrote:
> Moving mutex unlock and lock outside of the "if" statement as it can be shown that
> the mutex will be taken and released, regardless of the value checked in the if statement.
> 
> Signed-off-by: Alex Jivin <alex.jivin@amd.com>
> Suggested-By: Luben Tukov <luben.tuikov@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 838d6d51904c..d2401379bd33 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -3559,16 +3559,14 @@ void amdgpu_dpm_enable_uvd(struct amdgpu_device *adev, bool enable)
>  	int ret = 0;
>  
>  	if (adev->family == AMDGPU_FAMILY_SI) {
> +		mutex_lock(&adev->pm.mutex);
>  		if (enable) {
> -			mutex_lock(&adev->pm.mutex);
>  			adev->pm.dpm.uvd_active = true;
>  			adev->pm.dpm.state = POWER_STATE_TYPE_INTERNAL_UVD;
> -			mutex_unlock(&adev->pm.mutex);
>  		} else {
> -			mutex_lock(&adev->pm.mutex);
>  			adev->pm.dpm.uvd_active = false;
> -			mutex_unlock(&adev->pm.mutex);
>  		}
> +		mutex_unlock(&adev->pm.mutex);
>  
>  		amdgpu_pm_compute_clocks(adev);
>  	} else {
> @@ -3596,17 +3594,15 @@ void amdgpu_dpm_enable_vce(struct amdgpu_device *adev, bool enable)
>  	int ret = 0;
>  
>  	if (adev->family == AMDGPU_FAMILY_SI) {
> +		mutex_lock(&adev->pm.mutex);
>  		if (enable) {
> -			mutex_lock(&adev->pm.mutex);
>  			adev->pm.dpm.vce_active = true;
>  			/* XXX select vce level based on ring/task */
>  			adev->pm.dpm.vce_level = AMD_VCE_LEVEL_AC_ALL;
> -			mutex_unlock(&adev->pm.mutex);
>  		} else {
> -			mutex_lock(&adev->pm.mutex);
>  			adev->pm.dpm.vce_active = false;
> -			mutex_unlock(&adev->pm.mutex);
>  		}
> +		mutex_unlock(&adev->pm.mutex);
>  
>  		amdgpu_pm_compute_clocks(adev);
>  	} else {
> 

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