Re: [[PATCH v2]drm/amdgpu/psp: ignore psp reponse status] drm/amdgpu/psp: ignore psp reponse status
Dear Aaron, On 01/15/19 07:30, Liu, Aaron wrote: > The psp code only support ucode loading at the beginning, but forgot > to remove the checking when more and more PSP command get into PSP. I didn’t know that, and missed that your patch is not only about microcode loading. > So some old PSP FW maybe have problem but PSP driver didn’t find it > out. If reverting the patch, we also can’t find out wrong PSP FW and > have big problem when support new PSP command. Understood. > The temporary solution is: do not return error if response is invalid > but add warning message to notify, it will not block old FW and get > attention if new PSP FW have some issue. There is still a small typo in *response* in the summary. Kind regards, Paul smime.p7s Description: S/MIME Cryptographic Signature ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
RE: [[PATCH v2]drm/amdgpu/psp: ignore psp reponse status] drm/amdgpu/psp: ignore psp reponse status
Hi Paul,
The psp code only support ucode loading at the beginning, but forgot to remove
the checking when more and more PSP command get into PSP.
So some old PSP FW maybe have problem but PSP driver didn’t find it out.
If reverting the patch, we also can’t find out wrong PSP FW and have big
problem when support new PSP command.
The temporary solution is: do not return error if response is invalid but add
warning message to notify, it will not block old FW and get attention if new
PSP FW have some issue.
BR,
Aaron Liu
> -Original Message-
> From: Paul Menzel
> Sent: Monday, January 14, 2019 9:24 PM
> To: Liu, Aaron
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [[PATCH v2]drm/amdgpu/psp: ignore psp reponse status]
> drm/amdgpu/psp: ignore psp reponse status
>
> Dear Aaron,
>
>
> Am 14.01.19 um 09:47 schrieb Aaron Liu:
> > In some cases, psp response status is not 0 even there is no problem
> > while the command is submitted. Some version of PSP FW doesn't write 0
> > to that field.
> > So here we would like to only print a warning instead of an error
> > during psp initialization to avoid breaking hw_init and it doesn't
> > return -EINVAL.
> >
> > Change-Id: I680679983f972b6969f4949f1faafaf17fe996a6
> > Signed-off-by: Aaron Liu
> > Reviewed-by: Huang Rui
> > Reviewed-by: Xiangliang Yu
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 13 +
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > index 53c2d60..f26d8fa 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > @@ -140,14 +140,19 @@ psp_cmd_submit_buf(struct psp_context *psp,
> > while (*((unsigned int *)psp->fence_buf) != index)
> > msleep(1);
> >
> > - /* the status field must be 0 after psp command completion */
> > + /* In some cases, psp response status is not 0 even there is no
> > +* problem while the command is submitted. Some version of PSP
> FW
> > +* doesn't write 0 to that field.
> > +* So here we would like to only print a warning instead of an error
> > +* during psp initialization to avoid breaking hw_init and it doesn't
> > +* return -EINVAL.
> > +*/
> > if (psp->cmd_buf_mem->resp.status) {
> > if (ucode)
> > - DRM_ERROR("failed to load ucode id (%d) ",
> > + DRM_WARN("failed to load ucode id (%d) ",
> > ucode->ucode_id);
> > - DRM_ERROR("psp command failed and response status is
> (%d)\n",
> > + DRM_WARN("psp command failed and response status is
> (%d)\n",
> > psp->cmd_buf_mem->resp.status);
> > - return -EINVAL;
> > }
> >
> > /* get xGMI session id from response buffer */
>
> Please describe, why this error can be ignored, and the rest of the function
> be executed. Won’t that introduce other problems?
>
> How can real error situations be determined now?
>
> Also, please extend the commit messages, that this only affects microcode
> update loading (if I understand this correctly).
>
>
> Kind regards,
>
> Paul
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [[PATCH v2]drm/amdgpu/psp: ignore psp reponse status] drm/amdgpu/psp: ignore psp reponse status
Dear Aaron,
Am 14.01.19 um 09:47 schrieb Aaron Liu:
In some cases, psp response status is not 0 even there is no
problem while the command is submitted. Some version of PSP FW
doesn't write 0 to that field.
So here we would like to only print a warning instead of an error
during psp initialization to avoid breaking hw_init and it doesn't
return -EINVAL.
Change-Id: I680679983f972b6969f4949f1faafaf17fe996a6
Signed-off-by: Aaron Liu
Reviewed-by: Huang Rui
Reviewed-by: Xiangliang Yu
---
drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
index 53c2d60..f26d8fa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
@@ -140,14 +140,19 @@ psp_cmd_submit_buf(struct psp_context *psp,
while (*((unsigned int *)psp->fence_buf) != index)
msleep(1);
- /* the status field must be 0 after psp command completion */
+ /* In some cases, psp response status is not 0 even there is no
+* problem while the command is submitted. Some version of PSP FW
+* doesn't write 0 to that field.
+* So here we would like to only print a warning instead of an error
+* during psp initialization to avoid breaking hw_init and it doesn't
+* return -EINVAL.
+*/
if (psp->cmd_buf_mem->resp.status) {
if (ucode)
- DRM_ERROR("failed to load ucode id (%d) ",
+ DRM_WARN("failed to load ucode id (%d) ",
ucode->ucode_id);
- DRM_ERROR("psp command failed and response status is (%d)\n",
+ DRM_WARN("psp command failed and response status is (%d)\n",
psp->cmd_buf_mem->resp.status);
- return -EINVAL;
}
/* get xGMI session id from response buffer */
Please describe, why this error can be ignored, and the rest of the
function be executed. Won’t that introduce other problems?
How can real error situations be determined now?
Also, please extend the commit messages, that this only affects
microcode update loading (if I understand this correctly).
Kind regards,
Paul
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
