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

[Luben Tuikov <luben.tuikov@amd.com>]

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