[PATCH 1/3] drm/amdgpu: don't pin kernel objects multiple times

[PATCH 1/3] drm/amdgpu: don't pin kernel objects multiple times

[Christian König]

Some kernel buffers can only be allocated after asking the firmware how
large they should be. But since this happens on every resume don't pin
them multiple times.

Also bail out with an error if the requested size should ever change.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 90eb07106609..d0d53e83a318 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -258,6 +258,9 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
 			return r;
 		}
 		free = true;
+	} else {
+		if ((*bo_ptr)->tbo.base.size != size)
+			return -EINVAL;
 	}
 
 	r = amdgpu_bo_reserve(*bo_ptr, false);
@@ -266,10 +269,12 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
 		goto error_free;
 	}
 
-	r = amdgpu_bo_pin(*bo_ptr, domain);
-	if (r) {
-		dev_err(adev->dev, "(%d) kernel bo pin failed\n", r);
-		goto error_unreserve;
+	if (free) {
+		r = amdgpu_bo_pin(*bo_ptr, domain);
+		if (r) {
+			dev_err(adev->dev, "(%d) kernel bo pin failed\n", r);
+			goto error_unreserve;
+		}
 	}
 
 	r = amdgpu_ttm_alloc_gart(&(*bo_ptr)->tbo);
@@ -292,7 +297,8 @@ int amdgpu_bo_create_reserved(struct amdgpu_device *adev,
 	return 0;
 
 error_unpin:
-	amdgpu_bo_unpin(*bo_ptr);
+	if (free)
+		amdgpu_bo_unpin(*bo_ptr);
 error_unreserve:
 	amdgpu_bo_unreserve(*bo_ptr);
 
-- 
2.34.1

[PATCH 2/3] drm/amdgpu: stop freeing PSP buffers during suspend

[Christian König]

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@amd.com>
---
 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

[PATCH 3/3] drm/amdgpu: WARN when freeing kernel memory during suspend

[Christian König]

When buffers are freed during suspend there is no guarantee that
they can be re-allocated during resume.

The PSP subsystem seems to be quite buggy regarding this, so add
a WARN_ON() to point out those bugs.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d0d53e83a318..063bf6f69918 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -428,6 +428,8 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
 	if (*bo == NULL)
 		return;
 
+	WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend);
+
 	if (likely(amdgpu_bo_reserve(*bo, true) == 0)) {
 		if (cpu_addr)
 			amdgpu_bo_kunmap(*bo);
-- 
2.34.1

Re: [PATCH 2/3] drm/amdgpu: stop freeing PSP buffers during suspend

[Alex Deucher]

[-- Attachment #1: Type: text/plain, Size: 4042 bytes --]

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@gmail.com> 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@amd.com>
> ---
>  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
>

[-- Attachment #2: 0001-drm-amdgpu-psp-don-t-free-PSP-buffers-on-suspend.patch --]
[-- Type: text/x-patch, Size: 3420 bytes --]

From cd73f8a94079290e7c944f3d3105ddf75ac1c43d Mon Sep 17 00:00:00 2001
From: Alex Deucher <alexander.deucher@amd.com>
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@amd.com>
---
 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

Re: [PATCH 2/3] drm/amdgpu: stop freeing PSP buffers during suspend

[Guilherme G. Piccoli]

Hi Christian, thanks for the fix! It worked fine on Steam Deck running
the game Cuphead, feel free to add my:

Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Re: [PATCH 3/3] drm/amdgpu: WARN when freeing kernel memory during suspend

[Guilherme G. Piccoli]

Hey Christian, very interesting idea! I've tested it alone (without
patch 2) and was able to see the PSP code freeing buffers during
suspend, leading to a resume failure later.

Feel free to add my:
Tested-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Cheers,


Guilherme

Re: [PATCH 2/3] drm/amdgpu: stop freeing PSP buffers during suspend

[Alex Deucher]

Hey Christian,

Do you have a preference on which solution we go with?  I'm inclined
to go with my patch just because it's more self-contained and easier
to deal with for stable, but I could be convinced otherwise.

Alex

On Wed, Nov 16, 2022 at 11:38 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> 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@gmail.com> 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@amd.com>
> > ---
> >  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
> >

Re: [PATCH 3/3] drm/amdgpu: WARN when freeing kernel memory during suspend

[Alex Deucher]

On Wed, Nov 16, 2022 at 11:01 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> When buffers are freed during suspend there is no guarantee that
> they can be re-allocated during resume.
>
> The PSP subsystem seems to be quite buggy regarding this, so add
> a WARN_ON() to point out those bugs.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Reviewed-by: Alex Deucher <alexdeucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d0d53e83a318..063bf6f69918 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -428,6 +428,8 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
>         if (*bo == NULL)
>                 return;
>
> +       WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend);
> +
>         if (likely(amdgpu_bo_reserve(*bo, true) == 0)) {
>                 if (cpu_addr)
>                         amdgpu_bo_kunmap(*bo);
> --
> 2.34.1
>