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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3587 bytes --]


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 */;
> +	return ret;

In current implementation of __xe_guc_upload

incase of failure in  guc_xfer_rsa(guc) , xe_uc_fw_upload or 
guc_wait_ucode(guc);

We goto out (Label) and still return 0.

With above changes incase of failure in guc_xfer_rsa(guc)  or 
xe_uc_fw_upload we we will return error codes .

Is this intentional ?

>   }
>   
>   /**

[-- Attachment #2: Type: text/html, Size: 4363 bytes --]