Re: [PATCH 2/3] drm/xe: declare busted upon GuC load failure

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Mar 15, 2024 at 04:46:16PM +0530, Ghimiray, Himal Prasad wrote:
> 
> On 15-03-2024 06:33, Rodrigo Vivi wrote:
> > Let's block the device upon any GuC load failure.
> > But let's continue with the probe so guc logs can be read
> > from the debugfs.
> > 
> > v2: - s/wedged/busted
> >      - do not block probe or we lose guc_logs in debugfs (Matt)
> > 
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_guc.c | 41 ++++++++++++++++---------------------
> >   1 file changed, 18 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> > index caa86ccbe9e7..85edaaa734a5 100644
> > --- a/drivers/gpu/drm/xe/xe_guc.c
> > +++ b/drivers/gpu/drm/xe/xe_guc.c
> > @@ -461,7 +461,7 @@ static int guc_xfer_rsa(struct xe_guc *guc)
> >   	return 0;
> >   }
> > -static int guc_wait_ucode(struct xe_guc *guc)
> > +static void guc_wait_ucode(struct xe_guc *guc)
> >   {
> >   	struct xe_device *xe = guc_to_xe(guc);
> >   	u32 status;
> > @@ -491,31 +491,28 @@ static int guc_wait_ucode(struct xe_guc *guc)
> >   	if (ret) {
> >   		struct drm_device *drm = &xe->drm;
> > -		drm_info(drm, "GuC load failed: status = 0x%08X\n", status);
> > -		drm_info(drm, "GuC load failed: status: Reset = %d, BootROM = 0x%02X, UKernel = 0x%02X, MIA = 0x%02X, Auth = 0x%02X\n",
> > -			 REG_FIELD_GET(GS_MIA_IN_RESET, status),
> > -			 REG_FIELD_GET(GS_BOOTROM_MASK, status),
> > -			 REG_FIELD_GET(GS_UKERNEL_MASK, status),
> > -			 REG_FIELD_GET(GS_MIA_MASK, status),
> > -			 REG_FIELD_GET(GS_AUTH_STATUS_MASK, status));
> > -
> > -		if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {
> > -			drm_info(drm, "GuC firmware signature verification failed\n");
> > -			ret = -ENOEXEC;
> > -		}
> > +		drm_err(drm, "GuC load failed: status = 0x%08X\n", status);
> > +		drm_err(drm, "GuC load failed: status: Reset = %d, BootROM = 0x%02X, UKernel = 0x%02X, MIA = 0x%02X, Auth = 0x%02X\n",
> > +			REG_FIELD_GET(GS_MIA_IN_RESET, status),
> > +			REG_FIELD_GET(GS_BOOTROM_MASK, status),
> > +			REG_FIELD_GET(GS_UKERNEL_MASK, status),
> > +			REG_FIELD_GET(GS_MIA_MASK, status),
> > +			REG_FIELD_GET(GS_AUTH_STATUS_MASK, status));
> > +
> > +		if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED)
> > +			drm_err(drm, "GuC firmware signature verification failed\n");
> >   		if (REG_FIELD_GET(GS_UKERNEL_MASK, status) ==
> >   		    XE_GUC_LOAD_STATUS_EXCEPTION) {
> > -			drm_info(drm, "GuC firmware exception. EIP: %#x\n",
> > -				 xe_mmio_read32(guc_to_gt(guc),
> > -						SOFT_SCRATCH(13)));
> > -			ret = -ENXIO;
> > +			drm_err(drm, "GuC firmware exception. EIP: %#x\n",
> > +				xe_mmio_read32(guc_to_gt(guc),
> > +					       SOFT_SCRATCH(13)));
> >   		}
> > +
> > +		xe_device_declare_busted(xe);
> >   	} else {
> >   		drm_dbg(&xe->drm, "GuC successfully loaded");
> >   	}
> > -
> > -	return ret;
> >   }
> >   static int __xe_guc_upload(struct xe_guc *guc)
> > @@ -545,16 +542,14 @@ static int __xe_guc_upload(struct xe_guc *guc)
> >   		goto out;
> >   	/* Wait for authentication */
> > -	ret = guc_wait_ucode(guc);
> > -	if (ret)
> > -		goto out;
> > +	guc_wait_ucode(guc);
> >   	xe_uc_fw_change_status(&guc->fw, XE_UC_FIRMWARE_RUNNING);
> >   	return 0;
> >   out:
> >   	xe_uc_fw_change_status(&guc->fw, XE_UC_FIRMWARE_LOAD_FAIL);
> > -	return 0	/* FIXME: ret, don't want to stop load currently */;
> 
> This label is used for other failures too apart from guc_wait_ucode.

I'm afraid I didn't find those here... perhaps I missunderstood the 'label'
you mentioned.

my grep for returs with this comment failed

> 
> Dont we want to ignore ret and return 0 those failures too ?

anyway, perhaps that could be a follow up clean up?

> 
> 
> BR
> 
> Himal Ghimiray
> 
> > +	return ret;
> >   }
> >   /**