Re: [RFC 3/9] drm/xe/late_bind_fw: Introducing late_bind_fw

["Nilawar, Badal" <badal.nilawar@intel.com>]

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


On 08-05-2025 03:08, Daniele Ceraolo Spurio wrote:
>
>
> On 4/29/2025 9:09 AM, Badal Nilawar wrote:
>> Introducing late_bind_fw to enable firmware loading for the devices,
>> such as the fan controller and voltage regulator, during the driver 
>> probe.
>> Typically, firmware for these devices are part of IFWI flash image but
>> can be replaced at probe after OEM tuning.
>
> This description does not fully match what's happening in the patch, 
> as the main thing happening is the addition of the mei component.
Sure I will update the description.
>
>>
>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>> ---
>>   drivers/gpu/drm/xe/Kconfig                 |   1 +
>>   drivers/gpu/drm/xe/Makefile                |   1 +
>>   drivers/gpu/drm/xe/xe_device.c             |   3 +
>>   drivers/gpu/drm/xe/xe_device_types.h       |   4 +
>>   drivers/gpu/drm/xe/xe_late_bind_fw.c       | 104 +++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_late_bind_fw.h       |  16 ++++
>>   drivers/gpu/drm/xe/xe_late_bind_fw_types.h |  95 +++++++++++++++++++
>>   7 files changed, 224 insertions(+)
>>   create mode 100644 drivers/gpu/drm/xe/xe_late_bind_fw.c
>>   create mode 100644 drivers/gpu/drm/xe/xe_late_bind_fw.h
>>   create mode 100644 drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>>
>> diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>> index 9bce047901b2..a8cc1876a24f 100644
>> --- a/drivers/gpu/drm/xe/Kconfig
>> +++ b/drivers/gpu/drm/xe/Kconfig
>> @@ -44,6 +44,7 @@ config DRM_XE
>>       select WANT_DEV_COREDUMP
>>       select AUXILIARY_BUS
>>       select HMM_MIRROR
>> +    select INTEL_MEI_LATE_BIND
>
> I'm not sure this is enough to guarantee that late bind will work. 
> This selects the component, but the MEI_GSC child driver also needs to 
> be built for the component to bind into it on dGPU. We can't select 
> INTEL_MEI_GSC from here because that depends on the graphics driver, 
> so we'd go circular. For other components (PXP, HDCP, SW proxy) what 
> we did was notify the distros that they needed to enable the new 
> config for the feature to work instead of selecting it from the Kconfig.
I will follow the approach used for other components. Is it ok to enable 
this feature for CI in a separate patch labeled "CI_only, do not review"?
>
>>       help
>>         Experimental driver for Intel Xe series GPUs
>>   diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index c5d6681645ed..6de291a21965 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -76,6 +76,7 @@ xe-y += xe_bb.o \
>>       xe_hw_fence.o \
>>       xe_irq.o \
>>       xe_lrc.o \
>> +    xe_late_bind_fw.o \
>>       xe_migrate.o \
>>       xe_mmio.o \
>>       xe_mocs.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device.c 
>> b/drivers/gpu/drm/xe/xe_device.c
>> index 75e753e0a682..86a7b7065122 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -42,6 +42,7 @@
>>   #include "xe_hw_engine_group.h"
>>   #include "xe_hwmon.h"
>>   #include "xe_irq.h"
>> +#include "xe_late_bind_fw.h"
>>   #include "xe_memirq.h"
>>   #include "xe_mmio.h"
>>   #include "xe_module.h"
>> @@ -889,6 +890,8 @@ int xe_device_probe(struct xe_device *xe)
>>       if (err)
>>           return err;
>>   +    xe_late_bind_init(&xe->late_bind);
>> +
>>       err = xe_oa_init(xe);
>>       if (err)
>>           return err;
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h 
>> b/drivers/gpu/drm/xe/xe_device_types.h
>> index 495bc00ebed4..57b63cc9b8ac 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -16,6 +16,7 @@
>>   #include "xe_devcoredump_types.h"
>>   #include "xe_heci_gsc.h"
>>   #include "xe_lmtt_types.h"
>> +#include "xe_late_bind_fw_types.h"
>>   #include "xe_memirq_types.h"
>>   #include "xe_oa_types.h"
>>   #include "xe_platform_types.h"
>> @@ -543,6 +544,9 @@ struct xe_device {
>>       /** @heci_gsc: graphics security controller */
>>       struct xe_heci_gsc heci_gsc;
>>   +    /** @late_bind: xe mei late bind interface */
>> +    struct xe_late_bind late_bind;
>> +
>>       /** @oa: oa observation subsystem */
>>       struct xe_oa oa;
>>   diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c 
>> b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> new file mode 100644
>> index 000000000000..7981fc500a78
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> @@ -0,0 +1,104 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>
> 2025?
Ok
>
>> + */
>> +
>> +#include <linux/component.h>
>> +#include <linux/delay.h>
>> +
>> +#include <drm/drm_managed.h>
>> +#include <drm/intel/i915_component.h>
>> +#include <drm/intel/xe_late_bind_mei_interface.h>
>> +#include <drm/drm_print.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_late_bind_fw.h"
>> +
>> +static struct xe_device *
>> +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_component_bind(struct device *xe_kdev,
>> +                       struct device *mei_kdev, void *data)
>> +{
>> +    struct xe_device *xe = kdev_to_xe_device(xe_kdev);
>> +    struct xe_late_bind *late_bind = &xe->late_bind;
>> +    struct xe_late_bind_component *component;
>> +
>> +    component = drmm_kzalloc(&xe->drm, sizeof(*component), GFP_KERNEL);
>
> The component is unbound and re-bound on every suspend/resume, so if 
> you do allocs in the bind function without freeing them in the unbind 
> you'll keep the old allocations around. Why do you need this to be 
> dynamically allocated to begin with?
Dynamic allocation is not needed, I will drop the dynamic allocation and 
fix below comments.
>
>> +
>> +    mutex_lock(&late_bind->mutex);
>> +    component->mei_dev = mei_kdev;
>> +    component->ops = data;
>> +    mutex_unlock(&late_bind->mutex);
>
> This is a local variable right now, so locking around it doesn't do 
> anything.
>
>> +
>> +    late_bind->component = component;
>
> This assignment instead you might want to protect.
>
>> +
>> +    return 0;
>> +}
>> +
>> +static void xe_late_bind_component_unbind(struct device *xe_kdev,
>> +                      struct device *mei_kdev, void *data)
>> +{
>> +    struct xe_device *xe = kdev_to_xe_device(xe_kdev);
>> +    struct xe_late_bind *late_bind = &xe->late_bind;
>> +
>> +    mutex_lock(&late_bind->mutex);
>> +    late_bind->component = NULL;
>> +    mutex_unlock(&late_bind->mutex);
>> +}
>> +
>> +static const struct component_ops xe_late_bind_component_ops = {
>> +    .bind   = xe_late_bind_component_bind,
>> +    .unbind = xe_late_bind_component_unbind,
>> +};
>> +
>> +/**
>> + * xe_late_bind_init() - add xe mei late binding component
>> + *
>> + * Return: 0 if the initialization was successful, a negative errno 
>> otherwise.
>> + */
>> +int xe_late_bind_init(struct xe_late_bind *late_bind)
>> +{
>> +    struct xe_device *xe = late_bind_to_xe(late_bind);
>> +    int err;
>> +
>> +    if (xe->info.platform != XE_BATTLEMAGE)
>> +        return 0;
>> +
>> +    mutex_init(&late_bind->mutex);
>> +
>> +    if (!IS_ENABLED(CONFIG_INTEL_MEI_LATE_BIND)) {
>
> also need INTEL_MEI_GSC for BMG as mentioned above
Ok.
>
>> +        drm_info(&xe->drm, "Can't init xe mei late bind missing mei 
>> component\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    err = component_add_typed(xe->drm.dev, &xe_late_bind_component_ops,
>> +                  I915_COMPONENT_LATE_BIND);
>> +    if (err < 0) {
>> +        drm_info(&xe->drm, "Failed to add mei late bind component 
>> (%pe)\n", ERR_PTR(err));
>> +        return err;
>> +    }
>> +
>> +    late_bind->component_added = true;
>> +
>> +    /* the component must be removed before unload, so can't use 
>> drmm for cleanup */
>
> this has now changed (see 8e1ddfada453 ("drivers: base: devres: Allow 
> to release group on device release") ), so you can use a devm action 
> here.
Ok.
>
>> +
>> +    return 0;
>> +}
>> +
>> +/**
>> + * xe_late_bind_remove() - remove the xe mei late binding component
>> + */
>> +void xe_late_bind_remove(struct xe_late_bind *late_bind)
>> +{
>> +    struct xe_device *xe = late_bind_to_xe(late_bind);
>> +
>> +    if (!late_bind->component_added)
>> +        return;
>> +
>> +    component_del(xe->drm.dev, &xe_late_bind_component_ops);
>> +    late_bind->component_added = false;
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.h 
>> b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>> new file mode 100644
>> index 000000000000..21299de54b47
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>> @@ -0,0 +1,16 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_LATE_BIND_FW_H_
>> +#define _XE_LATE_BIND_FW_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct xe_late_bind;
>> +
>> +int xe_late_bind_init(struct xe_late_bind *late_bind);
>> +void xe_late_bind_remove(struct xe_late_bind *late_bind);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw_types.h 
>> b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>> new file mode 100644
>> index 000000000000..ea11061ce556
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>> @@ -0,0 +1,95 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_MEI_LATE_BIND_TYPES_H_
>> +#define _XE_MEI_LATE_BIND_TYPES_H_
>> +
>> +#include <linux/iosys-map.h>
>> +#include <linux/mutex.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/types.h>
>> +#include <linux/workqueue.h>
>> +
>> +#define MAX_PAYLOAD_SIZE (1024 * 4)
>> +
>> +struct xe_bo;
>> +struct xe_late_bind_component;
>> +
>> +/**
>> + * xe_late_bind_fw_type - enum to determine late binding fw type
>> + */
>> +enum xe_late_bind_type {
>> +    CSC_LATE_BINDING_TYPE_FAN_CONTROL = 1,
>> +    CSC_LATE_BINDING_TYPE_VOLTAGE_REGULATOR
>> +};
>> +
>> +/**
>> + * Late Binding flags
>> + */
>> +enum csc_late_binding_flags {
>> +    /** Persistent across warm reset */
>> +    CSC_LATE_BINDING_FLAGS_IS_PERSISTENT = 0x1
>> +};
>> +
>> +/**
>> + * xe_late_bind_fw_id - enum to determine late binding fw index
>> + */
>> +enum xe_late_bind_fw_id {
>> +    FAN_CONTROL_ID = 0,
>> +    VOLTAGE_REGULATOR_ID,
>> +    MAX_ID
>> +};
>> +
>> +/**
>> + * struct xe_late_bind_fw
>> + */
>> +struct xe_late_bind_fw {
>> +    /** @late_bind_fw.valid */
>> +    bool valid;
>> +
>> +    /** @late_bind_fw.id */
>> +    u32 id;
>> +
>> +    /** @late_bind_fw.blob_path: late binding fw blob path */
>> +    char blob_path[PATH_MAX];
>> +
>> +    /** @late_bind_fw.type */
>> +    u32  type;
>> +
>> +    /** @late_bind_fw.type */
>> +    u32  flags;
>> +
>> +    /** @late_bind_fw.payload: to store the late binding blob */
>> +    u8  payload[MAX_PAYLOAD_SIZE];
>> +
>> +    /** @late_bind_fw.payload_size: late binding blob payload_size */
>> +    size_t payload_size;
>> +
>> +    /** @late_bind_fw.work: worker to upload latebind blob */
>> +    struct work_struct work;
>> +};
>> +
>> +/**
>> + * struct xe_late_bind
>> + */
>> +struct xe_late_bind {
>> +    /** @late_bind.component: struct for communication with mei 
>> component */
>> +    struct xe_late_bind_component *component;
>> +
>> +    /** @late_bind.component_added: whether the component has been 
>> added */
>> +    bool component_added;
>> +
>> +    /** @late_bind.wq: workqueue to submit request to download late 
>> bind blob */
>> +    struct workqueue_struct *wq;
>> +
>> +    /** @late_bind.mutex: protects the component binding and usage */
>> +    struct mutex mutex;
>> +
>> +    /** @late_bind.late_bind_fw: late binding firmwares */
>> +    struct xe_late_bind_fw late_bind_fw[MAX_ID];
>> +
>> +};
>> +
>
> A lot of the variables/enums in this file are unused in this patch. 
> Can you move the additions to the patch in which they're actually used?

Sure, I will add variables/enums to this structure incrementally in 
subsequent patches.

Thanks,
Badal

>
> Daniele
>
>> +#endif
>

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