Re: [PATCH V2] drm/amdgpu: Destroy psp ring in hw_fini

[Huang Rui <ray.huang-5C7GfCeVMHo@public.gmane.org>]

On Tue, Apr 18, 2017 at 03:04:45PM +0800, Trigger Huang wrote:
> Fix issue that PSP initialization will fail if reload amdgpu module.
> That's because the PSP ring must be destroyed to be ready for the
> next time PSP initialization.
> 
> Changes in v2:
>         - Move psp_ring_destroy before all BOs free according to
>           Ray Huang's suggestion.
> 
> Signed-off-by: Trigger Huang <trigger.huang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c |  3 +++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_psp.h |  3 +++
>  drivers/gpu/drm/amd/amdgpu/psp_v3_1.c   | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/psp_v3_1.h   |  2 ++
>  4 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/
> amdgpu/amdgpu_psp.c
> index 19180aa..73ddf4b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> @@ -56,6 +56,7 @@ static int psp_sw_init(void *handle)
>                  psp->prep_cmd_buf = psp_v3_1_prep_cmd_buf;
>                  psp->ring_init = psp_v3_1_ring_init;
>                  psp->ring_create = psp_v3_1_ring_create;
> +               psp->ring_destroy = psp_v3_1_ring_destroy;
>                  psp->cmd_submit = psp_v3_1_cmd_submit;
>                  psp->compare_sram_data = psp_v3_1_compare_sram_data;
>                  psp->smu_reload_quirk = psp_v3_1_smu_reload_quirk;
> @@ -411,6 +412,8 @@ static int psp_hw_fini(void *handle)
>          struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>          struct psp_context *psp = &adev->psp;
>  
> +       psp_ring_destroy(psp, PSP_RING_TYPE__KM);
> +
>          if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP)
>                  amdgpu_ucode_fini_bo(adev);

If adev->firmware.load_type is AMDGPU_FW_LOAD_DIRECT, we should not call
psp_ring_destroy, right?

I already told you in last mail:
if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP)
        return 0;
amdgpu_ucode_fini_bo(adev);
psp_ring_destroy(psp, PSP_RING_TYPE__KM);
...

>  
> +int psp_v3_1_ring_destroy(struct psp_context *psp, enum psp_ring_type
> ring_type)
> +{
> +       int ret = 0;
> +       struct psp_ring *ring;
> +       unsigned int psp_ring_reg = 0;
> +       struct amdgpu_device *adev = psp->adev;
> +
> +       ring = &psp->km_ring;
> +
> +       /* Write the ring destroy command to C2PMSG_64 */
> +       psp_ring_reg = 3 << 16;
> +       WREG32(SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_64), psp_ring_reg);
> +
> +       /* there might be handshake issue with hardware which needs delay */
> +       mdelay(20);
> +
> +       /* Wait for response flag (bit 31) in C2PMSG_64 */
> +       ret = psp_wait_for(psp, SOC15_REG_OFFSET(MP0, 0, mmMP0_SMN_C2PMSG_64),
> +                          0x80000000, 0x8000FFFF, false);
> +

> +       if (ring->ring_mem)
> +               amdgpu_bo_free_kernel(&adev->firmware.rbuf,
> +                                     &ring->ring_mem_mc_addr,
> +                                     (void **)&ring->ring_mem);

adev->firmware.rbuf is entired memory space for storing firmware data which
inited by amdgpu_ucode_init_bo, and it already teared down via
amdgpu_ucode_fini_bo. You cannot free it again.

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