I was thinking something like this would be more straightforward.
Alex
On Wed, Nov 16, 2022 at 11:01 AM Christian König
<ckoenig.leichtzumerken@xxxxxxxxx> wrote:
>
> That the PSP code tries to free the memory during suspend is quite
> broken and leads to problems during resume.
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 30 ++++++++++---------------
> 1 file changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> index 0a8c30475dda..470cd660c450 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -511,11 +511,10 @@ static int psp_sw_fini(void *handle)
> kfree(cmd);
> cmd = NULL;
>
> - if (psp->km_ring.ring_mem)
> - amdgpu_bo_free_kernel(&adev->firmware.rbuf,
> - &psp->km_ring.ring_mem_mc_addr,
> - (void **)&psp->km_ring.ring_mem);
> -
> + psp_free_shared_bufs(psp);
> + amdgpu_bo_free_kernel(&adev->firmware.rbuf,
> + &psp->km_ring.ring_mem_mc_addr,
> + (void **)&psp->km_ring.ring_mem);
> 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,
> @@ -2635,8 +2634,6 @@ static int psp_hw_fini(void *handle)
>
> psp_ring_destroy(psp, PSP_RING_TYPE__KM);
>
> - psp_free_shared_bufs(psp);
> -
> return 0;
> }
>
> @@ -2651,7 +2648,7 @@ static int psp_suspend(void *handle)
> ret = psp_xgmi_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate xgmi ta\n");
> - goto out;
> + return ret;
> }
> }
>
> @@ -2659,40 +2656,40 @@ static int psp_suspend(void *handle)
> ret = psp_ras_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate ras ta\n");
> - goto out;
> + return ret;
> }
> ret = psp_hdcp_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate hdcp ta\n");
> - goto out;
> + return ret;
> }
> ret = psp_dtm_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate dtm ta\n");
> - goto out;
> + return ret;
> }
> ret = psp_rap_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate rap ta\n");
> - goto out;
> + return ret;
> }
> ret = psp_securedisplay_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate securedisplay ta\n");
> - goto out;
> + return ret;
> }
> }
>
> ret = psp_asd_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate asd\n");
> - goto out;
> + return ret;
> }
>
> ret = psp_tmr_terminate(psp);
> if (ret) {
> DRM_ERROR("Failed to terminate tmr\n");
> - goto out;
> + return ret;
> }
>
> ret = psp_ring_stop(psp, PSP_RING_TYPE__KM);
> @@ -2700,9 +2697,6 @@ static int psp_suspend(void *handle)
> DRM_ERROR("PSP ring stop failed\n");
> }
>
> -out:
> - psp_free_shared_bufs(psp);
> -
> return ret;
> }
>
> --
> 2.34.1
>
From cd73f8a94079290e7c944f3d3105ddf75ac1c43d Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@xxxxxxx>
Date: Wed, 16 Nov 2022 11:26:53 -0500
Subject: [PATCH] drm/amdgpu/psp: don't free PSP buffers on suspend
We can reuse the same buffers on resume.
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 56 +++++++++++++------------
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 0a8c30475dda..d9cb4c4b8289 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -172,6 +172,7 @@ void psp_ta_free_shared_buf(struct ta_mem_context *mem_ctx)
{
amdgpu_bo_free_kernel(&mem_ctx->shared_bo, &mem_ctx->shared_mc_addr,
&mem_ctx->shared_buf);
+ mem_ctx->shared_bo = NULL;
}
static void psp_free_shared_bufs(struct psp_context *psp)
@@ -182,6 +183,7 @@ static void psp_free_shared_bufs(struct psp_context *psp)
/* free TMR memory buffer */
pptr = amdgpu_sriov_vf(psp->adev) ? &tmr_buf : NULL;
amdgpu_bo_free_kernel(&psp->tmr_bo, &psp->tmr_mc_addr, pptr);
+ psp->tmr_bo = NULL;
/* free xgmi shared memory */
psp_ta_free_shared_buf(&psp->xgmi_context.context.mem_context);
@@ -743,37 +745,39 @@ static int psp_load_toc(struct psp_context *psp,
/* Set up Trusted Memory Region */
static int psp_tmr_init(struct psp_context *psp)
{
- int ret;
+ int ret = 0;
int tmr_size;
void *tmr_buf;
void **pptr;
- /*
- * According to HW engineer, they prefer the TMR address be "naturally
- * aligned" , e.g. the start address be an integer divide of TMR size.
- *
- * Note: this memory need be reserved till the driver
- * uninitializes.
- */
- tmr_size = PSP_TMR_SIZE(psp->adev);
-
- /* For ASICs support RLC autoload, psp will parse the toc
- * and calculate the total size of TMR needed */
- if (!amdgpu_sriov_vf(psp->adev) &&
- psp->toc.start_addr &&
- psp->toc.size_bytes &&
- psp->fw_pri_buf) {
- ret = psp_load_toc(psp, &tmr_size);
- if (ret) {
- DRM_ERROR("Failed to load toc\n");
- return ret;
+ if (!psp->tmr_bo) {
+ /*
+ * According to HW engineer, they prefer the TMR address be "naturally
+ * aligned" , e.g. the start address be an integer divide of TMR size.
+ *
+ * Note: this memory need be reserved till the driver
+ * uninitializes.
+ */
+ tmr_size = PSP_TMR_SIZE(psp->adev);
+
+ /* For ASICs support RLC autoload, psp will parse the toc
+ * and calculate the total size of TMR needed */
+ if (!amdgpu_sriov_vf(psp->adev) &&
+ psp->toc.start_addr &&
+ psp->toc.size_bytes &&
+ psp->fw_pri_buf) {
+ ret = psp_load_toc(psp, &tmr_size);
+ if (ret) {
+ DRM_ERROR("Failed to load toc\n");
+ return ret;
+ }
}
- }
- pptr = amdgpu_sriov_vf(psp->adev) ? &tmr_buf : NULL;
- ret = amdgpu_bo_create_kernel(psp->adev, tmr_size, PSP_TMR_ALIGNMENT,
- AMDGPU_GEM_DOMAIN_VRAM,
- &psp->tmr_bo, &psp->tmr_mc_addr, pptr);
+ pptr = amdgpu_sriov_vf(psp->adev) ? &tmr_buf : NULL;
+ ret = amdgpu_bo_create_kernel(psp->adev, tmr_size, PSP_TMR_ALIGNMENT,
+ AMDGPU_GEM_DOMAIN_VRAM,
+ &psp->tmr_bo, &psp->tmr_mc_addr, pptr);
+ }
return ret;
}
@@ -2701,8 +2705,6 @@ static int psp_suspend(void *handle)
}
out:
- psp_free_shared_bufs(psp);
-
return ret;
}
--
2.38.1
