How about these patches?
Alex
On Fri, Apr 22, 2022 at 5:00 PM Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
>
> On Fri, Apr 22, 2022 at 3:54 PM Wong, Alice <Shiwei.Wong@xxxxxxx> wrote:
> >
> > [AMD Official Use Only]
> >
> > Hi Alex,
> >
> > The attached patch freed most of the allocated memory except for one allocated by psp_tmr_init during psp_load_fw.
> > Combination of the attached patch and calling psp_hw_fini when psp_hw_init failed would fix the issue.
> >
> > As a proper fix, we can call psp_tmr_terminate in psp_load_fw when psp_load_non_psp_fw failed. (attached patch)
> > We can't move psp_tmr_init to sw_init because we need to load toc to get the TMR size.
> > Do you have any concerns with this approach?
>
> That only covers failures in hw_init(). It doesn't handle resume().
> Looks like all of the ta functions also potentially leak. I'm working
> on a cleanup to handle all of these.
>
> Alex
>
> >
> > Alice
> >
> >
> > -----Original Message-----
> > From: Alex Deucher <alexdeucher@xxxxxxxxx>
> > Sent: April 21, 2022 1:31 AM
> > To: Wong, Alice <Shiwei.Wong@xxxxxxx>
> > Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>
> > Subject: Re: [PATCH] drm/amdgpu: clean up psp ip if hw_init failed v2
> >
> > On Wed, Apr 20, 2022 at 5:48 PM Alice Wong <shiwei.wong@xxxxxxx> wrote:
> > >
> > > If at any point psp_hw_init failed, psp_hw_fini would not be called
> > > during unload due to ip_blocks[PSP].status.hw not being set to true.
> > > This could cause a memory leak when the driver unloads.
> > > As a rule of thumb, each IP block should cleanup themselves when their
> > > hw_init fails. Only previously intialized blocks should be cleaned up
> > > by the common framework.
> > >
> > > v1: Call IP blocks' respective hw_fini when hw_init failed from
> > > the common framework
> > > v2: Call psp_hw_fini when psp_hw_init failed.
> > >
> > > Signed-off-by: Alice Wong <shiwei.wong@xxxxxxx>
> >
> > I don't think this is a good idea. The hw programming in hw_fini makes no sense if the driver never set it up in the first place if hw_init failed before initializing the hw. It would be better to just properly unwind the relevant functions. Presumably the problem you are seeing is the failure to free the GPU memory allocated in fw_fini, depending on where it fails. We should just allocate the memory in sw_init; that is what we do in other IPs. Does the attached patch fix the issue you are seeing?
> >
> > Alex
> >
> > > ---
> > > 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;
> > > --
> > > 2.25.1
> > >
From 19b1b0afe03954f45ddf03305b209465121b06a8 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@xxxxxxx>
Date: Fri, 22 Apr 2022 17:19:29 -0400
Subject: [PATCH 4/4] drm/amdgpu/psp: move shared buffer frees into single
function
So we can properly clean up if any of the TAs or TMR fails
to properly initialize or terminate. This avoids any
memory leaks in the error case.
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 114 ++++++++++++------------
1 file changed, 55 insertions(+), 59 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 1ef2aba2ac3f..b1b6f5dd35dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -153,6 +153,36 @@ static int psp_early_init(void *handle)
return 0;
}
+static void psp_free_shared_bufs(struct psp_context *psp)
+{
+ void *tmr_buf;
+ void **pptr;
+
+ /* 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);
+
+ /* free xgmi shared memory */
+ psp_ta_free_shared_buf(&psp->xgmi_context.context.mem_context);
+
+ /* free ras shared memory */
+ psp_ta_free_shared_buf(&psp->ras_context.context.mem_context);
+
+ /* free hdcp shared memory */
+ psp_ta_free_shared_buf(&psp->hdcp_context.context.mem_context);
+
+ /* free dtm shared memory */
+ psp_ta_free_shared_buf(&psp->dtm_context.context.mem_context);
+
+ /* free rap shared memory */
+ psp_ta_free_shared_buf(&psp->rap_context.context.mem_context);
+
+ /* free securedisplay shared memory */
+ psp_ta_free_shared_buf(&psp->securedisplay_context.context.mem_context);
+
+
+}
+
static void psp_memory_training_fini(struct psp_context *psp)
{
struct psp_memory_training_context *ctx = &psp->mem_train_ctx;
@@ -747,17 +777,7 @@ static int psp_tmr_unload(struct psp_context *psp)
static int psp_tmr_terminate(struct psp_context *psp)
{
- int ret;
- void *tmr_buf;
- void **pptr;
-
- ret = psp_tmr_unload(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);
-
- return ret;
+ return psp_tmr_unload(psp);
}
int psp_get_fw_attestation_records_addr(struct psp_context *psp,
@@ -1102,9 +1122,6 @@ int psp_xgmi_terminate(struct psp_context *psp)
psp->xgmi_context.context.initialized = false;
- /* free xgmi shared memory */
- psp_ta_free_shared_buf(&psp->xgmi_context.context.mem_context);
-
return ret;
}
@@ -1465,9 +1482,6 @@ int psp_ras_terminate(struct psp_context *psp)
psp->ras_context.context.initialized = false;
- /* free ras shared memory */
- psp_ta_free_shared_buf(&psp->ras_context.context.mem_context);
-
return ret;
}
@@ -1650,23 +1664,13 @@ static int psp_hdcp_terminate(struct psp_context *psp)
if (amdgpu_sriov_vf(psp->adev))
return 0;
- if (!psp->hdcp_context.context.initialized) {
- if (psp->hdcp_context.context.mem_context.shared_buf) {
- ret = 0;
- goto out;
- } else {
- return 0;
- }
- }
+ if (!psp->hdcp_context.context.initialized)
+ return 0;
ret = psp_ta_unload(psp, &psp->hdcp_context.context);
psp->hdcp_context.context.initialized = false;
-out:
- /* free hdcp shared memory */
- psp_ta_free_shared_buf(&psp->hdcp_context.context.mem_context);
-
return ret;
}
// HDCP end
@@ -1727,23 +1731,13 @@ static int psp_dtm_terminate(struct psp_context *psp)
if (amdgpu_sriov_vf(psp->adev))
return 0;
- if (!psp->dtm_context.context.initialized) {
- if (psp->dtm_context.context.mem_context.shared_buf) {
- ret = 0;
- goto out;
- } else {
- return 0;
- }
- }
+ if (!psp->dtm_context.context.initialized)
+ return 0;
ret = psp_ta_unload(psp, &psp->dtm_context.context);
psp->dtm_context.context.initialized = false;
-out:
- /* free dtm shared memory */
- psp_ta_free_shared_buf(&psp->dtm_context.context.mem_context);
-
return ret;
}
// DTM end
@@ -1785,6 +1779,8 @@ static int psp_rap_initialize(struct psp_context *psp)
ret = psp_rap_invoke(psp, TA_CMD_RAP__INITIALIZE, &status);
if (ret || status != TA_RAP_STATUS__SUCCESS) {
psp_rap_terminate(psp);
+ /* free rap shared memory */
+ psp_ta_free_shared_buf(&psp->rap_context.context.mem_context);
dev_warn(psp->adev->dev, "RAP TA initialize fail (%d) status %d.\n",
ret, status);
@@ -1806,9 +1802,6 @@ static int psp_rap_terminate(struct psp_context *psp)
psp->rap_context.context.initialized = false;
- /* free rap shared memory */
- psp_ta_free_shared_buf(&psp->rap_context.context.mem_context);
-
return ret;
}
@@ -1889,6 +1882,8 @@ static int psp_securedisplay_initialize(struct psp_context *psp)
ret = psp_securedisplay_invoke(psp, TA_SECUREDISPLAY_COMMAND__QUERY_TA);
if (ret) {
psp_securedisplay_terminate(psp);
+ /* free securedisplay shared memory */
+ psp_ta_free_shared_buf(&psp->securedisplay_context.context.mem_context);
dev_err(psp->adev->dev, "SECUREDISPLAY TA initialize fail.\n");
return -EINVAL;
}
@@ -1919,9 +1914,6 @@ static int psp_securedisplay_terminate(struct psp_context *psp)
psp->securedisplay_context.context.initialized = false;
- /* free securedisplay shared memory */
- psp_ta_free_shared_buf(&psp->securedisplay_context.context.mem_context);
-
return ret;
}
@@ -2524,16 +2516,18 @@ static int psp_hw_fini(void *handle)
}
psp_asd_terminate(psp);
-
psp_tmr_terminate(psp);
+
psp_ring_destroy(psp, PSP_RING_TYPE__KM);
+ psp_free_shared_bufs(psp);
+
return 0;
}
static int psp_suspend(void *handle)
{
- int ret;
+ int ret = 0;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
struct psp_context *psp = &adev->psp;
@@ -2542,7 +2536,7 @@ static int psp_suspend(void *handle)
ret = psp_xgmi_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate xgmi ta\n");
- return ret;
+ goto out;
}
}
@@ -2550,49 +2544,51 @@ static int psp_suspend(void *handle)
ret = psp_ras_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate ras ta\n");
- return ret;
+ goto out;
}
ret = psp_hdcp_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate hdcp ta\n");
- return ret;
+ goto out;
}
ret = psp_dtm_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate dtm ta\n");
- return ret;
+ goto out;
}
ret = psp_rap_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate rap ta\n");
- return ret;
+ goto out;
}
ret = psp_securedisplay_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate securedisplay ta\n");
- return ret;
+ goto out;
}
}
ret = psp_asd_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate asd\n");
- return ret;
+ goto out;
}
ret = psp_tmr_terminate(psp);
if (ret) {
DRM_ERROR("Failed to terminate tmr\n");
- return ret;
+ goto out;
}
ret = psp_ring_stop(psp, PSP_RING_TYPE__KM);
if (ret) {
DRM_ERROR("PSP ring stop failed\n");
- return ret;
}
- return 0;
+out:
+ psp_free_shared_bufs(psp);
+
+ return ret;
}
static int psp_resume(void *handle)
--
2.35.1
From e4e0b8d66818cc3250e5373bfeb70dedcc7765c1 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@xxxxxxx>
Date: Fri, 22 Apr 2022 16:51:00 -0400
Subject: [PATCH 3/4] drm/amdgpu/psp: fix memory leak in terminate functions
Make sure we free the memory even if the unload fails.
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 34 ++++++++++---------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index e9dc83641c71..1ef2aba2ac3f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -752,14 +752,12 @@ static int psp_tmr_terminate(struct psp_context *psp)
void **pptr;
ret = psp_tmr_unload(psp);
- if (ret)
- return ret;
/* 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);
- return 0;
+ return ret;
}
int psp_get_fw_attestation_records_addr(struct psp_context *psp,
@@ -1101,15 +1099,13 @@ int psp_xgmi_terminate(struct psp_context *psp)
return 0;
ret = psp_ta_unload(psp, &psp->xgmi_context.context);
- if (ret)
- return ret;
psp->xgmi_context.context.initialized = false;
/* free xgmi shared memory */
psp_ta_free_shared_buf(&psp->xgmi_context.context.mem_context);
- return 0;
+ return ret;
}
int psp_xgmi_initialize(struct psp_context *psp, bool set_extended_data, bool load_ta)
@@ -1466,15 +1462,13 @@ int psp_ras_terminate(struct psp_context *psp)
return 0;
ret = psp_ta_unload(psp, &psp->ras_context.context);
- if (ret)
- return ret;
psp->ras_context.context.initialized = false;
/* free ras shared memory */
psp_ta_free_shared_buf(&psp->ras_context.context.mem_context);
- return 0;
+ return ret;
}
static int psp_ras_initialize(struct psp_context *psp)
@@ -1657,15 +1651,15 @@ static int psp_hdcp_terminate(struct psp_context *psp)
return 0;
if (!psp->hdcp_context.context.initialized) {
- if (psp->hdcp_context.context.mem_context.shared_buf)
+ if (psp->hdcp_context.context.mem_context.shared_buf) {
+ ret = 0;
goto out;
- else
+ } else {
return 0;
+ }
}
ret = psp_ta_unload(psp, &psp->hdcp_context.context);
- if (ret)
- return ret;
psp->hdcp_context.context.initialized = false;
@@ -1673,7 +1667,7 @@ static int psp_hdcp_terminate(struct psp_context *psp)
/* free hdcp shared memory */
psp_ta_free_shared_buf(&psp->hdcp_context.context.mem_context);
- return 0;
+ return ret;
}
// HDCP end
@@ -1734,15 +1728,15 @@ static int psp_dtm_terminate(struct psp_context *psp)
return 0;
if (!psp->dtm_context.context.initialized) {
- if (psp->dtm_context.context.mem_context.shared_buf)
+ if (psp->dtm_context.context.mem_context.shared_buf) {
+ ret = 0;
goto out;
- else
+ } else {
return 0;
+ }
}
ret = psp_ta_unload(psp, &psp->dtm_context.context);
- if (ret)
- return ret;
psp->dtm_context.context.initialized = false;
@@ -1750,7 +1744,7 @@ static int psp_dtm_terminate(struct psp_context *psp)
/* free dtm shared memory */
psp_ta_free_shared_buf(&psp->dtm_context.context.mem_context);
- return 0;
+ return ret;
}
// DTM end
@@ -1922,8 +1916,6 @@ static int psp_securedisplay_terminate(struct psp_context *psp)
return 0;
ret = psp_ta_unload(psp, &psp->securedisplay_context.context);
- if (ret)
- return ret;
psp->securedisplay_context.context.initialized = false;
--
2.35.1
From 16ab75d27de3724ae0ff0e0311e137dc4c5c8ba5 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@xxxxxxx>
Date: Thu, 21 Apr 2022 01:21:52 -0400
Subject: [PATCH 1/4] drm/amdgpu/psp: move PSP memory alloc from hw_init to
sw_init
Memory allocations should be done in sw_init. hw_init should
just be hardware programming needed to initialize the IP block.
This is how most other IP blocks work. Move the GPU memory
allocations from psp hw_init to psp sw_init and move the memory
free to sw_fini. This also fixes a potential GPU memory leak
if psp hw_init fails.
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 95 ++++++++++++-------------
1 file changed, 47 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 0bd22ebcc3d1..0787f2e36f2a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -356,7 +356,39 @@ static int psp_sw_init(void *handle)
}
}
+ ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
+ amdgpu_sriov_vf(adev) ?
+ AMDGPU_GEM_DOMAIN_VRAM : AMDGPU_GEM_DOMAIN_GTT,
+ &psp->fw_pri_bo,
+ &psp->fw_pri_mc_addr,
+ &psp->fw_pri_buf);
+ if (ret)
+ return ret;
+
+ ret = amdgpu_bo_create_kernel(adev, PSP_FENCE_BUFFER_SIZE, PAGE_SIZE,
+ AMDGPU_GEM_DOMAIN_VRAM,
+ &psp->fence_buf_bo,
+ &psp->fence_buf_mc_addr,
+ &psp->fence_buf);
+ if (ret)
+ goto failed1;
+
+ ret = amdgpu_bo_create_kernel(adev, PSP_CMD_BUFFER_SIZE, PAGE_SIZE,
+ AMDGPU_GEM_DOMAIN_VRAM,
+ &psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
+ (void **)&psp->cmd_buf_mem);
+ if (ret)
+ goto failed2;
+
return 0;
+
+failed2:
+ amdgpu_bo_free_kernel(&psp->fw_pri_bo,
+ &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
+failed1:
+ amdgpu_bo_free_kernel(&psp->fence_buf_bo,
+ &psp->fence_buf_mc_addr, &psp->fence_buf);
+ return ret;
}
static int psp_sw_fini(void *handle)
@@ -390,6 +422,13 @@ static int psp_sw_fini(void *handle)
kfree(cmd);
cmd = NULL;
+ 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;
}
@@ -2469,51 +2508,18 @@ static int psp_load_fw(struct amdgpu_device *adev)
struct psp_context *psp = &adev->psp;
if (amdgpu_sriov_vf(adev) && amdgpu_in_reset(adev)) {
- psp_ring_stop(psp, PSP_RING_TYPE__KM); /* should not destroy ring, only stop */
- goto skip_memalloc;
- }
-
- if (amdgpu_sriov_vf(adev)) {
- ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
- AMDGPU_GEM_DOMAIN_VRAM,
- &psp->fw_pri_bo,
- &psp->fw_pri_mc_addr,
- &psp->fw_pri_buf);
+ /* should not destroy ring, only stop */
+ psp_ring_stop(psp, PSP_RING_TYPE__KM);
} else {
- ret = amdgpu_bo_create_kernel(adev, PSP_1_MEG, PSP_1_MEG,
- AMDGPU_GEM_DOMAIN_GTT,
- &psp->fw_pri_bo,
- &psp->fw_pri_mc_addr,
- &psp->fw_pri_buf);
- }
-
- if (ret)
- goto failed;
-
- ret = amdgpu_bo_create_kernel(adev, PSP_FENCE_BUFFER_SIZE, PAGE_SIZE,
- AMDGPU_GEM_DOMAIN_VRAM,
- &psp->fence_buf_bo,
- &psp->fence_buf_mc_addr,
- &psp->fence_buf);
- if (ret)
- goto failed;
-
- ret = amdgpu_bo_create_kernel(adev, PSP_CMD_BUFFER_SIZE, PAGE_SIZE,
- AMDGPU_GEM_DOMAIN_VRAM,
- &psp->cmd_buf_bo, &psp->cmd_buf_mc_addr,
- (void **)&psp->cmd_buf_mem);
- if (ret)
- goto failed;
+ memset(psp->fence_buf, 0, PSP_FENCE_BUFFER_SIZE);
- memset(psp->fence_buf, 0, PSP_FENCE_BUFFER_SIZE);
-
- ret = psp_ring_init(psp, PSP_RING_TYPE__KM);
- if (ret) {
- DRM_ERROR("PSP ring init failed!\n");
- goto failed;
+ ret = psp_ring_init(psp, PSP_RING_TYPE__KM);
+ if (ret) {
+ DRM_ERROR("PSP ring init failed!\n");
+ goto failed;
+ }
}
-skip_memalloc:
ret = psp_hw_start(psp);
if (ret)
goto failed;
@@ -2631,13 +2637,6 @@ static int psp_hw_fini(void *handle)
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;
}
--
2.35.1
From ded8b2c39be83f943c22202b27ac5def0c1c4ff5 Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@xxxxxxx>
Date: Fri, 22 Apr 2022 16:46:24 -0400
Subject: [PATCH 2/4] drm/amdgpu/psp: drop load/unload/init_shared_buf wrappers
Just call the load/unload/init_shared_buf functions
directly. Makes the code easier to follow.
Signed-off-by: Alex Deucher <alexander.deucher@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 143 ++++--------------------
1 file changed, 21 insertions(+), 122 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 0787f2e36f2a..e9dc83641c71 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -866,11 +866,6 @@ static int psp_rl_load(struct amdgpu_device *adev)
return ret;
}
-static int psp_asd_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->asd_context);
-}
-
static int psp_asd_initialize(struct psp_context *psp)
{
int ret;
@@ -886,7 +881,7 @@ static int psp_asd_initialize(struct psp_context *psp)
psp->asd_context.mem_context.shared_mem_size = PSP_ASD_SHARED_MEM_SIZE;
psp->asd_context.ta_load_type = GFX_CMD_ID_LOAD_ASD;
- ret = psp_asd_load(psp);
+ ret = psp_ta_load(psp, &psp->asd_context);
if (!ret)
psp->asd_context.initialized = true;
@@ -914,11 +909,6 @@ int psp_ta_unload(struct psp_context *psp, struct ta_context *context)
return ret;
}
-static int psp_asd_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->asd_context);
-}
-
static int psp_asd_terminate(struct psp_context *psp)
{
int ret;
@@ -929,8 +919,7 @@ static int psp_asd_terminate(struct psp_context *psp)
if (!psp->asd_context.initialized)
return 0;
- ret = psp_asd_unload(psp);
-
+ ret = psp_ta_unload(psp, &psp->asd_context);
if (!ret)
psp->asd_context.initialized = false;
@@ -1002,11 +991,6 @@ void psp_ta_free_shared_buf(struct ta_mem_context *mem_ctx)
&mem_ctx->shared_buf);
}
-static int psp_xgmi_init_shared_buf(struct psp_context *psp)
-{
- return psp_ta_init_shared_buf(psp, &psp->xgmi_context.context.mem_context);
-}
-
static void psp_prep_ta_invoke_indirect_cmd_buf(struct psp_gfx_cmd_resp *cmd,
uint32_t ta_cmd_id,
struct ta_context *context)
@@ -1097,16 +1081,6 @@ int psp_ta_load(struct psp_context *psp, struct ta_context *context)
return ret;
}
-static int psp_xgmi_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->xgmi_context.context);
-}
-
-static int psp_xgmi_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->xgmi_context.context);
-}
-
int psp_xgmi_invoke(struct psp_context *psp, uint32_t ta_cmd_id)
{
return psp_ta_invoke(psp, ta_cmd_id, &psp->xgmi_context.context);
@@ -1126,7 +1100,7 @@ int psp_xgmi_terminate(struct psp_context *psp)
if (!psp->xgmi_context.context.initialized)
return 0;
- ret = psp_xgmi_unload(psp);
+ ret = psp_ta_unload(psp, &psp->xgmi_context.context);
if (ret)
return ret;
@@ -1155,13 +1129,13 @@ int psp_xgmi_initialize(struct psp_context *psp, bool set_extended_data, bool lo
psp->xgmi_context.context.ta_load_type = GFX_CMD_ID_LOAD_TA;
if (!psp->xgmi_context.context.initialized) {
- ret = psp_xgmi_init_shared_buf(psp);
+ ret = psp_ta_init_shared_buf(psp, &psp->xgmi_context.context.mem_context);
if (ret)
return ret;
}
/* Load XGMI TA */
- ret = psp_xgmi_load(psp);
+ ret = psp_ta_load(psp, &psp->xgmi_context.context);
if (!ret)
psp->xgmi_context.context.initialized = true;
else
@@ -1384,21 +1358,6 @@ int psp_xgmi_set_topology_info(struct psp_context *psp,
}
// ras begin
-static int psp_ras_init_shared_buf(struct psp_context *psp)
-{
- return psp_ta_init_shared_buf(psp, &psp->ras_context.context.mem_context);
-}
-
-static int psp_ras_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->ras_context.context);
-}
-
-static int psp_ras_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->ras_context.context);
-}
-
static void psp_ras_ta_check_status(struct psp_context *psp)
{
struct ta_ras_shared_memory *ras_cmd =
@@ -1506,7 +1465,7 @@ int psp_ras_terminate(struct psp_context *psp)
if (!psp->ras_context.context.initialized)
return 0;
- ret = psp_ras_unload(psp);
+ ret = psp_ta_unload(psp, &psp->ras_context.context);
if (ret)
return ret;
@@ -1582,7 +1541,7 @@ static int psp_ras_initialize(struct psp_context *psp)
psp->ras_context.context.ta_load_type = GFX_CMD_ID_LOAD_TA;
if (!psp->ras_context.context.initialized) {
- ret = psp_ras_init_shared_buf(psp);
+ ret = psp_ta_init_shared_buf(psp, &psp->ras_context.context.mem_context);
if (ret)
return ret;
}
@@ -1595,7 +1554,7 @@ static int psp_ras_initialize(struct psp_context *psp)
if (!adev->gmc.xgmi.connected_to_cpu)
ras_cmd->ras_in_message.init_flags.dgpu_mode = 1;
- ret = psp_ras_load(psp);
+ ret = psp_ta_load(psp, &psp->ras_context.context);
if (!ret && !ras_cmd->ras_status)
psp->ras_context.context.initialized = true;
@@ -1642,16 +1601,6 @@ int psp_ras_trigger_error(struct psp_context *psp,
// ras end
// HDCP start
-static int psp_hdcp_init_shared_buf(struct psp_context *psp)
-{
- return psp_ta_init_shared_buf(psp, &psp->hdcp_context.context.mem_context);
-}
-
-static int psp_hdcp_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->hdcp_context.context);
-}
-
static int psp_hdcp_initialize(struct psp_context *psp)
{
int ret;
@@ -1672,12 +1621,12 @@ static int psp_hdcp_initialize(struct psp_context *psp)
psp->hdcp_context.context.ta_load_type = GFX_CMD_ID_LOAD_TA;
if (!psp->hdcp_context.context.initialized) {
- ret = psp_hdcp_init_shared_buf(psp);
+ ret = psp_ta_init_shared_buf(psp, &psp->hdcp_context.context.mem_context);
if (ret)
return ret;
}
- ret = psp_hdcp_load(psp);
+ ret = psp_ta_load(psp, &psp->hdcp_context.context);
if (!ret) {
psp->hdcp_context.context.initialized = true;
mutex_init(&psp->hdcp_context.mutex);
@@ -1686,11 +1635,6 @@ static int psp_hdcp_initialize(struct psp_context *psp)
return ret;
}
-static int psp_hdcp_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->hdcp_context.context);
-}
-
int psp_hdcp_invoke(struct psp_context *psp, uint32_t ta_cmd_id)
{
/*
@@ -1719,7 +1663,7 @@ static int psp_hdcp_terminate(struct psp_context *psp)
return 0;
}
- ret = psp_hdcp_unload(psp);
+ ret = psp_ta_unload(psp, &psp->hdcp_context.context);
if (ret)
return ret;
@@ -1734,16 +1678,6 @@ static int psp_hdcp_terminate(struct psp_context *psp)
// HDCP end
// DTM start
-static int psp_dtm_init_shared_buf(struct psp_context *psp)
-{
- return psp_ta_init_shared_buf(psp, &psp->dtm_context.context.mem_context);
-}
-
-static int psp_dtm_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->dtm_context.context);
-}
-
static int psp_dtm_initialize(struct psp_context *psp)
{
int ret;
@@ -1764,12 +1698,12 @@ static int psp_dtm_initialize(struct psp_context *psp)
psp->dtm_context.context.ta_load_type = GFX_CMD_ID_LOAD_TA;
if (!psp->dtm_context.context.initialized) {
- ret = psp_dtm_init_shared_buf(psp);
+ ret = psp_ta_init_shared_buf(psp, &psp->dtm_context.context.mem_context);
if (ret)
return ret;
}
- ret = psp_dtm_load(psp);
+ ret = psp_ta_load(psp, &psp->dtm_context.context);
if (!ret) {
psp->dtm_context.context.initialized = true;
mutex_init(&psp->dtm_context.mutex);
@@ -1778,11 +1712,6 @@ static int psp_dtm_initialize(struct psp_context *psp)
return ret;
}
-static int psp_dtm_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->dtm_context.context);
-}
-
int psp_dtm_invoke(struct psp_context *psp, uint32_t ta_cmd_id)
{
/*
@@ -1811,7 +1740,7 @@ static int psp_dtm_terminate(struct psp_context *psp)
return 0;
}
- ret = psp_dtm_unload(psp);
+ ret = psp_ta_unload(psp, &psp->dtm_context.context);
if (ret)
return ret;
@@ -1826,21 +1755,6 @@ static int psp_dtm_terminate(struct psp_context *psp)
// DTM end
// RAP start
-static int psp_rap_init_shared_buf(struct psp_context *psp)
-{
- return psp_ta_init_shared_buf(psp, &psp->rap_context.context.mem_context);
-}
-
-static int psp_rap_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->rap_context.context);
-}
-
-static int psp_rap_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->rap_context.context);
-}
-
static int psp_rap_initialize(struct psp_context *psp)
{
int ret;
@@ -1862,12 +1776,12 @@ static int psp_rap_initialize(struct psp_context *psp)
psp->rap_context.context.ta_load_type = GFX_CMD_ID_LOAD_TA;
if (!psp->rap_context.context.initialized) {
- ret = psp_rap_init_shared_buf(psp);
+ ret = psp_ta_init_shared_buf(psp, &psp->rap_context.context.mem_context);
if (ret)
return ret;
}
- ret = psp_rap_load(psp);
+ ret = psp_ta_load(psp, &psp->rap_context.context);
if (!ret) {
psp->rap_context.context.initialized = true;
mutex_init(&psp->rap_context.mutex);
@@ -1894,7 +1808,7 @@ static int psp_rap_terminate(struct psp_context *psp)
if (!psp->rap_context.context.initialized)
return 0;
- ret = psp_rap_unload(psp);
+ ret = psp_ta_unload(psp, &psp->rap_context.context);
psp->rap_context.context.initialized = false;
@@ -1940,22 +1854,6 @@ int psp_rap_invoke(struct psp_context *psp, uint32_t ta_cmd_id, enum ta_rap_stat
// RAP end
/* securedisplay start */
-static int psp_securedisplay_init_shared_buf(struct psp_context *psp)
-{
- return psp_ta_init_shared_buf(
- psp, &psp->securedisplay_context.context.mem_context);
-}
-
-static int psp_securedisplay_load(struct psp_context *psp)
-{
- return psp_ta_load(psp, &psp->securedisplay_context.context);
-}
-
-static int psp_securedisplay_unload(struct psp_context *psp)
-{
- return psp_ta_unload(psp, &psp->securedisplay_context.context);
-}
-
static int psp_securedisplay_initialize(struct psp_context *psp)
{
int ret;
@@ -1978,12 +1876,13 @@ static int psp_securedisplay_initialize(struct psp_context *psp)
psp->securedisplay_context.context.ta_load_type = GFX_CMD_ID_LOAD_TA;
if (!psp->securedisplay_context.context.initialized) {
- ret = psp_securedisplay_init_shared_buf(psp);
+ ret = psp_ta_init_shared_buf(psp,
+ &psp->securedisplay_context.context.mem_context);
if (ret)
return ret;
}
- ret = psp_securedisplay_load(psp);
+ ret = psp_ta_load(psp, &psp->securedisplay_context.context);
if (!ret) {
psp->securedisplay_context.context.initialized = true;
mutex_init(&psp->securedisplay_context.mutex);
@@ -2022,7 +1921,7 @@ static int psp_securedisplay_terminate(struct psp_context *psp)
if (!psp->securedisplay_context.context.initialized)
return 0;
- ret = psp_securedisplay_unload(psp);
+ ret = psp_ta_unload(psp, &psp->securedisplay_context.context);
if (ret)
return ret;
--
2.35.1
