Re: [PATCH v3 04/10] drm/xe/xe_late_bind_fw: Initialize late binding firmware

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

On 6/18/2025 12:00 PM, Badal Nilawar wrote:
> Search for late binding firmware binaries and populate the meta data of
> firmware structures.
>
> v2 (Daniele):
>   - drm_err if firmware size is more than max pay load size
>   - s/request_firmware/firmware_request_nowarn/ as firmware will
>     not be available for all possible cards
> v3 (Daniele):
>   - init firmware from within xe_late_bind_init, propagate error
>   - switch late_bind_fw to array to handle multiple firmware types
>
> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_late_bind_fw.c       | 97 +++++++++++++++++++++-
>   drivers/gpu/drm/xe/xe_late_bind_fw_types.h | 32 +++++++
>   drivers/misc/mei/late_bind/mei_late_bind.c |  1 -
>   3 files changed, 128 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> index 52cb295b7df6..d416d6cc1fa2 100644
> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.c
> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> @@ -5,6 +5,7 @@
>   
>   #include <linux/component.h>
>   #include <linux/delay.h>
> +#include <linux/firmware.h>
>   
>   #include <drm/drm_managed.h>
>   #include <drm/intel/i915_component.h>
> @@ -13,6 +14,16 @@
>   
>   #include "xe_device.h"
>   #include "xe_late_bind_fw.h"
> +#include "xe_pcode.h"
> +#include "xe_pcode_api.h"
> +
> +static const u32 fw_id_to_type[] = {
> +		[FAN_CONTROL_FW_ID] = CSC_LATE_BINDING_TYPE_FAN_CONTROL,
> +	};
> +
> +static const char * const fw_id_to_name[] = {
> +		[FAN_CONTROL_FW_ID] = "fan_control",
> +	};
>   
>   static struct xe_device *
>   late_bind_to_xe(struct xe_late_bind *late_bind)
> @@ -20,6 +31,86 @@ late_bind_to_xe(struct xe_late_bind *late_bind)
>   	return container_of(late_bind, struct xe_device, late_bind);
>   }
>   
> +static int xe_late_bind_fw_num_fans(struct xe_late_bind *late_bind)
> +{
> +	struct xe_device *xe = late_bind_to_xe(late_bind);
> +	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> +	u32 uval;
> +
> +	if (!xe_pcode_read(root_tile,
> +			   PCODE_MBOX(FAN_SPEED_CONTROL, FSC_READ_NUM_FANS, 0), &uval, NULL))
> +		return uval;
> +	else
> +		return 0;
> +}
> +
> +static int __xe_late_bind_fw_init(struct xe_late_bind *late_bind, u32 fw_id)
> +{
> +	struct xe_device *xe = late_bind_to_xe(late_bind);
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	struct xe_late_bind_fw *lb_fw;
> +	const struct firmware *fw;
> +	u32 num_fans;
> +	int ret;
> +
> +	if (fw_id >= MAX_FW_ID)
> +		return -EINVAL;
> +
> +	lb_fw = &late_bind->late_bind_fw[fw_id];
> +
> +	lb_fw->valid = false;
> +	lb_fw->id = fw_id;
> +	lb_fw->type = fw_id_to_type[lb_fw->id];
> +	lb_fw->flags &= ~CSC_LATE_BINDING_FLAGS_IS_PERSISTENT;

nit: lb_fw->flags should already be zero here, so no need to make sure 
that flag is not set. Also, that flag is ignored on BMG, so there is no 
need to make sure it is not set anyway.

> +
> +	if (lb_fw->type == CSC_LATE_BINDING_TYPE_FAN_CONTROL) {
> +		num_fans = xe_late_bind_fw_num_fans(late_bind);
> +		drm_dbg(&xe->drm, "Number of Fans: %d\n", num_fans);
> +		if (!num_fans)
> +			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);
> +
> +	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);
> +	if (ret) {
> +		drm_dbg(&xe->drm, "%s late binding fw not available for current device",
> +			fw_id_to_name[lb_fw->id]);
> +		return 0;
> +	}
> +
> +	if (fw->size > 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, MAX_PAYLOAD_SIZE);
> +		release_firmware(fw);
> +		return -ENODATA;
> +	}
> +
> +	lb_fw->payload_size = fw->size;
> +
> +	memcpy(lb_fw->payload, fw->data, lb_fw->payload_size);
> +	release_firmware(fw);
> +	lb_fw->valid = true;
> +
> +	return 0;
> +}
> +
> +static int xe_late_bind_fw_init(struct xe_late_bind *late_bind)
> +{
> +	int ret;
> +	int fw_id;
> +
> +	for (fw_id = 0; fw_id < MAX_FW_ID; fw_id++) {
> +		ret = __xe_late_bind_fw_init(late_bind, fw_id);
> +		if (ret)
> +			return ret;
> +	}
> +	return ret;

nit: this could be a return 0, since if ret != 0 we return earlier

> +}
> +
>   static int xe_late_bind_component_bind(struct device *xe_kdev,
>   				       struct device *mei_kdev, void *data)
>   {
> @@ -89,5 +180,9 @@ int xe_late_bind_init(struct xe_late_bind *late_bind)
>   
>   	late_bind->component_added = true;
>   
> -	return devm_add_action_or_reset(xe->drm.dev, xe_late_bind_remove, late_bind);
> +	err = devm_add_action_or_reset(xe->drm.dev, xe_late_bind_remove, late_bind);
> +	if (err)
> +		return err;
> +
> +	return xe_late_bind_fw_init(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 ef0a9723bee4..c6fd33fd5484 100644
> --- a/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
> @@ -10,6 +10,36 @@
>   #include <linux/mutex.h>
>   #include <linux/types.h>
>   
> +#define MAX_PAYLOAD_SIZE (1024 * 4)

nit: could use SZ_4K instead of 1024 * 4

> +
> +/**
> + * xe_late_bind_fw_id - enum to determine late binding fw index
> + */
> +enum xe_late_bind_fw_id {
> +	FAN_CONTROL_FW_ID = 0,
> +	MAX_FW_ID

nit: Maybe use a less generic name here, to avoid clashes? something like:

enum xe_late_bind_fw_id {
         XE_LB_FW_FAN_CONTROL = 0,
         XE_LB_FW_MAX_ID
}

> +};
> +
> +/**
> + * struct xe_late_bind_fw
> + */
> +struct xe_late_bind_fw {
> +	/** @late_bind_fw.valid: to check if fw is valid */
> +	bool valid;
> +	/** @late_bind_fw.id: firmware index */
> +	u32 id;
> +	/** @late_bind_fw.blob_path: firmware binary path */
> +	char blob_path[PATH_MAX];
> +	/** @late_bind_fw.type: firmware type */
> +	u32  type;
> +	/** @late_bind_fw.flags: firmware flags */
> +	u32  flags;
> +	/** @late_bind_fw.payload: to store the late binding blob */
> +	u8  payload[MAX_PAYLOAD_SIZE];

Sorry for the late comment on this, just realized that this is a 4K 
allocation, should we alloc this dynamically only if we need it?

> +	/** @late_bind_fw.payload_size: late binding blob payload_size */
> +	size_t payload_size;
> +};
> +
>   /**
>    * struct xe_late_bind_component - Late Binding services component
>    * @mei_dev: device that provide Late Binding service.
> @@ -34,6 +64,8 @@ struct xe_late_bind {
>   	bool component_added;
>   	/** @late_bind.mutex: protects the component binding and usage */
>   	struct mutex mutex;
> +	/** @late_bind.late_bind_fw: late binding firmware array */
> +	struct xe_late_bind_fw late_bind_fw[MAX_FW_ID];
>   };
>   
>   #endif
> diff --git a/drivers/misc/mei/late_bind/mei_late_bind.c b/drivers/misc/mei/late_bind/mei_late_bind.c
> index cb985f32309e..6ea64c44bb94 100644
> --- a/drivers/misc/mei/late_bind/mei_late_bind.c
> +++ b/drivers/misc/mei/late_bind/mei_late_bind.c
> @@ -2,7 +2,6 @@
>   /*
>    * Copyright (C) 2025 Intel Corporation
>    */
> -#include <drm/drm_connector.h>

This change shouldn't be in this patch. If this header is not needed 
just modify the patch that adds it to not do so.

All nits are non blocking.
Daniele

>   #include <drm/intel/i915_component.h>
>   #include <drm/intel/late_bind_mei_interface.h>
>   #include <linux/component.h>