[[PATCH v2]drm/amdgpu/psp: ignore psp reponse status] drm/amdgpu/psp: ignore psp reponse status

[[PATCH v2]drm/amdgpu/psp: ignore psp reponse status] drm/amdgpu/psp: ignore psp reponse status

[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 <aaron.liu@amd.com>
Reviewed-by: Huang Rui <ray.huang@amd.com>
Reviewed-by: Xiangliang Yu<Xiangliang.Yu@amd.com>
---
 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 */
-- 
2.7.4

_______________________________________________
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

[Paul Menzel]

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 <aaron.liu@amd.com>
> Reviewed-by: Huang Rui <ray.huang@amd.com>
> Reviewed-by: Xiangliang Yu<Xiangliang.Yu@amd.com>
> ---
>   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

[Liu, Aaron]

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 <pmenzel+amd-gfx@molgen.mpg.de>
> Sent: Monday, January 14, 2019 9:24 PM
> To: Liu, Aaron <Aaron.Liu@amd.com>
> 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 <aaron.liu@amd.com>
> > Reviewed-by: Huang Rui <ray.huang@amd.com>
> > Reviewed-by: Xiangliang Yu<Xiangliang.Yu@amd.com>
> > ---
> >   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

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 782 bytes --]

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


[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5174 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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