Re: [PATCH] drm/amdgpu: clean up psp ip if hw_init failed v2

[Paul Menzel <pmenzel@xxxxxxxxxxxxx>]

Dear Alice,


Thank you for your patch.

Am 20.04.22 um 21:37 schrieb Alice Wong:
> Call psp_hw_fini when psp_hw_init failed.

Please amend the commit message, and add the motivation/reasoning too [1].

I think it’s common, if a patch (series) is rerolled to list the changes 
between the versions.


Kind regards,

Paul


[1]: https://cbea.ms/git-commit/

> Signed-off-by: Alice Wong <shiwei.wong@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 57 +++++++++++++------------
>   1 file changed, 29 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 5b9e48d44f5b..52b14efa848a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -2537,6 +2537,34 @@ static int psp_load_fw(struct amdgpu_device *adev)
>   	return ret;
>   }
>   
> +static int psp_hw_fini(void *handle)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +	struct psp_context *psp = &adev->psp;
> +
> +	if (psp->ta_fw) {
> +		psp_ras_terminate(psp);
> +		psp_securedisplay_terminate(psp);
> +		psp_rap_terminate(psp);
> +		psp_dtm_terminate(psp);
> +		psp_hdcp_terminate(psp);
> +	}
> +
> +	psp_asd_terminate(psp);
> +
> +	psp_tmr_terminate(psp);
> +	psp_ring_destroy(psp, PSP_RING_TYPE__KM);
> +
> +	amdgpu_bo_free_kernel(&psp->fw_pri_bo,
> +			      &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
> +	amdgpu_bo_free_kernel(&psp->fence_buf_bo,
> +			      &psp->fence_buf_mc_addr, &psp->fence_buf);
> +	amdgpu_bo_free_kernel(&psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
> +			      (void **)&psp->cmd_buf_mem);
> +
> +	return 0;
> +}
> +
>   static int psp_hw_init(void *handle)
>   {
>   	int ret;
> @@ -2563,37 +2591,10 @@ static int psp_hw_init(void *handle)
>   failed:
>   	adev->firmware.load_type = AMDGPU_FW_LOAD_DIRECT;
>   	mutex_unlock(&adev->firmware.mutex);
> +	psp_hw_fini(handle);
>   	return -EINVAL;
>   }
>   
> -static int psp_hw_fini(void *handle)
> -{
> -	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> -	struct psp_context *psp = &adev->psp;
> -
> -	if (psp->ta_fw) {
> -		psp_ras_terminate(psp);
> -		psp_securedisplay_terminate(psp);
> -		psp_rap_terminate(psp);
> -		psp_dtm_terminate(psp);
> -		psp_hdcp_terminate(psp);
> -	}
> -
> -	psp_asd_terminate(psp);
> -
> -	psp_tmr_terminate(psp);
> -	psp_ring_destroy(psp, PSP_RING_TYPE__KM);
> -
> -	amdgpu_bo_free_kernel(&psp->fw_pri_bo,
> -			      &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
> -	amdgpu_bo_free_kernel(&psp->fence_buf_bo,
> -			      &psp->fence_buf_mc_addr, &psp->fence_buf);
> -	amdgpu_bo_free_kernel(&psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
> -			      (void **)&psp->cmd_buf_mem);
> -
> -	return 0;
> -}
> -
>   static int psp_suspend(void *handle)
>   {
>   	int ret;