[RFC 3/9] drm/xe/late_bind_fw: Introducing late_bind_fw
Nilawar, Badal
badal.nilawar at intel.com
Tue Jun 3 13:52:35 UTC 2025
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 at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20250603/825267a6/attachment-0001.htm>
More information about the Intel-xe
mailing list