Message-ID: <6a9075f7-bfde-4856-a1c8-ecc0103c75f8@intel.com>
Date: Thu, 19 Jun 2025 10:27:48 +0530
From: "Nilawar, Badal" <badal.nilawar@...el.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@...el.com>,
	<intel-xe@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>,
	<linux-kernel@...r.kernel.org>
CC: <anshuman.gupta@...el.com>, <rodrigo.vivi@...el.com>,
	<alexander.usyskin@...el.com>, <gregkh@...uxfoundation.org>, <jgg@...dia.com>
Subject: Re: [PATCH v3 04/10] drm/xe/xe_late_bind_fw: Initialize late binding
 firmware


On 19-06-2025 02:16, Daniele Ceraolo Spurio wrote:
>
>
> 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@...el.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 I discard this, I will need to remove the definition of 
CSC_LATE_BINDING_FLAGS_IS_PERSISTENT. I kept this for future use. I 
would prefer this way.
I will fix rest of the nits and comments in next rev.

Thanks,
Badal

>> +
>> +    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>
>

Powered by blists - more mailing lists