On Thu, Sep 25, 2025 at 5:47 PM Mario Limonciello <[email protected]> wrote: > > > > On 9/25/2025 4:16 PM, Alex Deucher wrote: > > On Thu, Sep 25, 2025 at 3:50 PM Mario Limonciello > > <[email protected]> wrote: > >> > >> > >> > >> On 9/25/2025 2:46 PM, Alex Deucher wrote: > >>> On Thu, Sep 25, 2025 at 3:39 PM Mario Limonciello > >>> <[email protected]> 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 <[email protected]> > >>> > >>> 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.
Ideally we'd have a way to detect which boards have this, but I don't know of a way to do it so maybe we just have to live with the warnings. Patch at least removes the errors so: Reviewed-by: Alex Deucher <[email protected]> Alex > > > > > 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 > >>>> > >> >
