On 9/25/2025 4:16 PM, Alex Deucher wrote:
On Thu, Sep 25, 2025 at 3:50 PM Mario Limonciello
<mario.limoncie...@amd.com> wrote:



On 9/25/2025 2:46 PM, Alex Deucher wrote:
On Thu, Sep 25, 2025 at 3:39 PM Mario Limonciello
<mario.limoncie...@amd.com> wrote:

[Why]
Not all renoir hardware supports secure display.  If the TA is present
but the feature isn't supported it will fail to load or send commands.
This shows ERR messages to the user that make it seems like there is
a problem.

[How]
Check the resp_status of the context to see if there was an error
before trying to send any secure display commands.

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/1415
Signed-off-by: Mario Limonciello <mario.limoncie...@amd.com>

I think the tricky part is that we want it to throw an error on a
system where it is supported so the user knows something is wrong.

But a system that it's supported would have loaded the TA correctly, right?

Yes, it should.


This is specifically checking if the TA load failed which is being used
as a proxy to show you shouldn't bother with the other commands.

That makes sense, but I don't think it fixes the bug report.  The
driver will still report the error on TA load.  I'm not sure how we
can avoid that.

AFAICT there should be 4 messages showing up.
 * 2 warning
 * 2 error

It will at least help the two error messages. For the warning ones I guess we could plumb an arugment into psp_ta_load() and psp_cmd_submit_buf() whether it's an optional TA.


Alex


We still show WARN messages from
psp_ta_load()
->psp_cmd_submit_buf()

I guess arguably this change really should be:

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 693357caa9a8..1c790dfccc9f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -2350,7 +2350,7 @@ static int psp_securedisplay_initialize(struct
psp_context *psp)
          }

          ret = psp_ta_load(psp, &psp->securedisplay_context.context);
-       if (!ret) {
+       if (!ret && !psp->securedisplay_context.context.resp_status) {
                  psp->securedisplay_context.context.initialized = true;
                  mutex_init(&psp->securedisplay_context.mutex);
          } else

Alex

---
   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 3 +++
   1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 693357caa9a8..70d4bfb13f46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -2356,6 +2356,9 @@ static int psp_securedisplay_initialize(struct 
psp_context *psp)
          } else
                  return ret;

+       if (psp->securedisplay_context.context.resp_status)
+               return 0;
+
          mutex_lock(&psp->securedisplay_context.mutex);

          psp_prep_securedisplay_cmd_buf(psp, &securedisplay_cmd,
--
2.51.0



Reply via email to