Re: [PATCH v3 1/3] drm/xe/xe_late_bind_fw: Add support to load Ocode firmware

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 7/9/2026 7:39 AM, Badal Nilawar wrote:
> Enable loading of Ocode, the legacy OOBMSM firmware,
> via the late binding flow.
>
> V2: Added TODO comments for ocode compatibility version check

This needs some text to explain that until we start fetching the 
compatibility version via mailbox this code won't actually load the 
ocode binary.

>
> Assisted-by: GitHub-Copilot:GPT-5.3
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_late_bind_fw.c       | 68 +++++++++++++++++-----
>   drivers/gpu/drm/xe/xe_late_bind_fw_types.h |  2 +
>   2 files changed, 54 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> index 768442ca7da6..1f627cdea24e 100644
> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.c
> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> @@ -33,10 +33,12 @@
>   
>   static const u32 fw_id_to_type[] = {
>   		[XE_LB_FW_FAN_CONTROL] = INTEL_LB_TYPE_FAN_CONTROL,
> +		[XE_LB_FW_OCODE] = INTEL_LB_TYPE_OCODE,
>   	};
>   
>   static const char * const fw_id_to_name[] = {
>   		[XE_LB_FW_FAN_CONTROL] = "fan_control",
> +		[XE_LB_FW_OCODE] = "ocode",
>   	};
>   
>   static struct xe_device *
> @@ -179,6 +181,34 @@ static const char *xe_late_bind_parse_status(uint32_t status)
>   		return "Invalid Payload";
>   	case INTEL_LB_STATUS_TIMEOUT:
>   		return "Timeout";
> +	case INTEL_LB_STATUS_INTERNAL_ERROR:
> +		return "Internal Error";
> +	case INTEL_LB_STATUS_INVALID_FPT_TABLE:
> +		return "Invalid FPT Table";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_VERIFICATION_ERROR:
> +		return "Signed Payload Verification Error";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_INVALID_CPD:
> +		return "Signed Payload Invalid CPD";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_FW_VERSION_MISMATCH:
> +		return "Signed Payload FW Version Mismatch";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_INVALID_MANIFEST:
> +		return "Signed Payload Invalid Manifest";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_INVALID_HASH:
> +		return "Signed Payload Invalid Hash";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_BINDING_TYPE_MISMATCH:
> +		return "Signed Payload Binding type Mismatch";
> +	case INTEL_LB_STATUS_SIGNED_PAYLOAD_HANDLE_SVN_FAILED:
> +		return "Signed Payload Handle SVN Failed";
> +	case INTEL_LB_STATUS_DESTINATION_MBOX_FAILURE:
> +		return "Destination MBOX Failure";
> +	case INTEL_LB_STATUS_MISSING_LOADING_PATCH:
> +		return "Missing Loading Patch";
> +	case INTEL_LB_STATUS_INVALID_COMMAND:
> +		return "Invalid Command";
> +	case INTEL_LB_STATUS_INVALID_HECI_HEADER:
> +		return "Invalid HECI Header";
> +	case INTEL_LB_STATUS_IP_ERROR_START:
> +		return "IP Error Start";
>   	default:
>   		return "Unknown error";
>   	}
> @@ -298,7 +328,7 @@ static int __xe_late_bind_fw_init(struct xe_late_bind *late_bind, u32 fw_id)
>   	struct xe_late_bind_fw *lb_fw;
>   	const struct firmware *fw;
>   	u32 num_fans;
> -	int ret;
> +	int ret = 0;
>   
>   	if (fw_id >= XE_LB_FW_MAX_ID)
>   		return -EINVAL;
> @@ -320,9 +350,16 @@ static int __xe_late_bind_fw_init(struct xe_late_bind *late_bind, u32 fw_id)
>   			return 0;
>   	}
>   
> -	snprintf(lb_fw->blob_path, sizeof(lb_fw->blob_path), "xe/%s_8086_%04x_%04x_%04x.bin",
> -		 fw_id_to_name[lb_fw->id], pdev->device,
> -		 pdev->subsystem_vendor, pdev->subsystem_device);
> +	if (lb_fw->type == INTEL_LB_TYPE_OCODE) {
> +		u32 ocode_comp_v = 0;
> +		/* TODO: Fetch ocode compatibility version via system controller mailbox */

As far as I understand compatibility 0 is not a valid value so we could 
add a:

     if (!ocode_comp_v)
         return 0;

so we don't even attempt to fetch a FW that we know doesn't exist.

> +		snprintf(lb_fw->blob_path, sizeof(lb_fw->blob_path), "xe/%s_8086_%04x_%04x.bin",
> +			 fw_id_to_name[lb_fw->id], pdev->device, ocode_comp_v);
> +	} else {
> +		snprintf(lb_fw->blob_path, sizeof(lb_fw->blob_path), "xe/%s_8086_%04x_%04x_%04x.bin",
> +			 fw_id_to_name[lb_fw->id], pdev->device,
> +			 pdev->subsystem_vendor, pdev->subsystem_device);
> +	}
>   
>   	drm_dbg(&xe->drm, "Request late binding firmware %s\n", lb_fw->blob_path);
>   	ret = firmware_request_nowarn(&fw, lb_fw->blob_path, xe->drm.dev);
> @@ -332,22 +369,15 @@ static int __xe_late_bind_fw_init(struct xe_late_bind *late_bind, u32 fw_id)
>   		return 0;
>   	}
>   
> -	if (fw->size > XE_LB_MAX_PAYLOAD_SIZE) {
> -		drm_err(&xe->drm, "Firmware %s size %zu is larger than max pay load size %u\n",
> -			lb_fw->blob_path, fw->size, XE_LB_MAX_PAYLOAD_SIZE);
> -		release_firmware(fw);
> -		return -ENODATA;
> -	}

IMO we could use some text in the commit message to explain that we can 
now send bigger payloads and so this check is not needed. Also if the 
XE_LB_MAX_PAYLOAD_SIZE define is no longer valid it should probably be 
deleted.

> -
>   	ret = parse_lb_layout(lb_fw, fw->data, fw->size, "LTES");
>   	if (ret)
> -		return ret;
> +		goto release_fw;

Here it seems that you're fixing a bug, because we were returning 
without calling release_firmware. It might be worth splitting this to 
its own patch so it can be backported.

>   
>   	lb_fw->payload_size = fw->size;
>   	lb_fw->payload = drmm_kzalloc(&xe->drm, lb_fw->payload_size, GFP_KERNEL);
>   	if (!lb_fw->payload) {
> -		release_firmware(fw);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto release_fw;
>   	}
>   
>   	drm_info(&xe->drm, "Using %s firmware from %s version %u.%u.%u.%u\n",
> @@ -355,11 +385,17 @@ static int __xe_late_bind_fw_init(struct xe_late_bind *late_bind, u32 fw_id)
>   		 lb_fw->version.major, lb_fw->version.minor,
>   		 lb_fw->version.hotfix, lb_fw->version.build);
>   
> +	/*
> +	 * TODO: Verify compatibility version in manifest header against the version
> +	 * returned by the system controller.
> +	 */
> +
>   	memcpy((void *)lb_fw->payload, fw->data, lb_fw->payload_size);
> -	release_firmware(fw);
>   	INIT_WORK(&lb_fw->work, xe_late_bind_work);
>   
> -	return 0;
> +release_fw:
> +	release_firmware(fw);
> +	return ret;
>   }
>   
>   static int xe_late_bind_fw_init(struct xe_late_bind *late_bind)
> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw_types.h b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
> index ee5efe60774e..e4e5fafa31d7 100644
> --- a/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
> @@ -21,6 +21,8 @@
>   enum xe_late_bind_fw_id {
>   	/** @XE_LB_FW_FAN_CONTROL: Fan control */
>   	XE_LB_FW_FAN_CONTROL = 0,
> +	/** @XE_LB_FW_OCODE: Ocode firmware */
> +	XE_LB_FW_OCODE,

in the review of the RFC version of this patch, we agreed to modify 
has_late_bind to be a mask so that we could set FAN_CONTROL for BMG and 
OCODE for CRI. Is that still planned?

Daniele

>   	/** @XE_LB_FW_MAX_ID: Number of IDs */
>   	XE_LB_FW_MAX_ID
>   };