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

["Liu, Aaron" <Aaron.Liu-5C7GfCeVMHo@public.gmane.org>]

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