[PATCH v4 03/10] drm/xe/xe_late_bind_fw: Introducing xe_late_bind_fw
Nilawar, Badal
badal.nilawar at intel.com
Mon Jun 30 13:49:31 UTC 2025
On 28-06-2025 02:34, Rodrigo Vivi wrote:
> On Wed, Jun 25, 2025 at 10:30:08PM +0530, Badal Nilawar wrote:
>> Introducing xe_late_bind_fw to enable firmware loading for the devices,
>> such as the fan controller, during the driver probe. Typically,
>> firmware for such devices are part of IFWI flash image but can be
>> replaced at probe after OEM tuning.
>> This patch binds mei late binding component to enable firmware loading.
>>
>> v2:
>> - Add devm_add_action_or_reset to remove the component (Daniele)
>> - Add INTEL_MEI_GSC check in xe_late_bind_init() (Daniele)
>> v3:
>> - Fail driver probe if late bind initialization fails,
>> add has_late_bind flag (Daniele)
>> v4:
>> - %S/I915_COMPONENT_LATE_BIND/INTEL_COMPONENT_LATE_BIND/
>>
>> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 1 +
>> drivers/gpu/drm/xe/xe_device.c | 5 ++
>> drivers/gpu/drm/xe/xe_device_types.h | 6 ++
>> drivers/gpu/drm/xe/xe_late_bind_fw.c | 90 ++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_late_bind_fw.h | 15 ++++
>> drivers/gpu/drm/xe/xe_late_bind_fw_types.h | 37 +++++++++
>> drivers/gpu/drm/xe/xe_pci.c | 3 +
>> 7 files changed, 157 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/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 7c039caefd00..521547d78fd2 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 cd17c1354ab3..584acd63b0d9 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -44,6 +44,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,10 @@ int xe_device_probe(struct xe_device *xe)
>> if (err)
>> return err;
>>
>> + err = xe_late_bind_init(&xe->late_bind);
>> + if (err && err != -ENODEV)
>> + return err;
>> +
>> 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 6aca4b1a2824..321f9e9a94f6 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"
>> @@ -323,6 +324,8 @@ struct xe_device {
>> u8 has_heci_cscfi:1;
>> /** @info.has_heci_gscfi: device has heci gscfi */
>> u8 has_heci_gscfi:1;
>> + /** @info.has_late_bind: Device has firmware late binding support */
>> + u8 has_late_bind:1;
>> /** @info.has_llc: Device has a shared CPU+GPU last level cache */
>> u8 has_llc:1;
>> /** @info.has_mbx_power_limits: Device has support to manage power limits using
>> @@ -555,6 +558,9 @@ struct xe_device {
>> /** @nvm: discrete graphics non-volatile memory */
>> struct intel_dg_nvm_dev *nvm;
>>
>> + /** @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..eaf12cfec848
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
>> @@ -0,0 +1,90 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#include <linux/component.h>
>> +#include <linux/delay.h>
>> +
>> +#include <drm/drm_managed.h>
>> +#include <drm/intel/i915_component.h>
>> +#include <drm/intel/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;
>> +
>> + mutex_lock(&late_bind->mutex);
>> + late_bind->component.ops = data;
>> + late_bind->component.mei_dev = mei_kdev;
>> + mutex_unlock(&late_bind->mutex);
>> +
>> + 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.ops = 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,
>> +};
>> +
>> +static void xe_late_bind_remove(void *arg)
>> +{
>> + struct xe_late_bind *late_bind = arg;
>> + struct xe_device *xe = late_bind_to_xe(late_bind);
>> +
>> + component_del(xe->drm.dev, &xe_late_bind_component_ops);
>> + mutex_destroy(&late_bind->mutex);
>> +}
>> +
>> +/**
>> + * 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.has_late_bind)
>> + return 0;
>> +
>> + mutex_init(&late_bind->mutex);
>> +
>> + if (!IS_ENABLED(CONFIG_INTEL_MEI_LATE_BIND) || !IS_ENABLED(CONFIG_INTEL_MEI_GSC)) {
>> + 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,
>> + INTEL_COMPONENT_LATE_BIND);
>> + if (err < 0) {
>> + drm_info(&xe->drm, "Failed to add mei late bind component (%pe)\n", ERR_PTR(err));
>> + return err;
>> + }
>> +
>> + return devm_add_action_or_reset(xe->drm.dev, xe_late_bind_remove, late_bind);
>> +}
>> 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..4c73571c3e62
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.h
>> @@ -0,0 +1,15 @@
>> +/* 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);
>> +
>> +#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..1156ef94f0d5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw_types.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2025 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_LATE_BIND_TYPES_H_
>> +#define _XE_LATE_BIND_TYPES_H_
>> +
>> +#include <linux/iosys-map.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct xe_late_bind_component - Late Binding services component
>> + * @mei_dev: device that provide Late Binding service.
>> + * @ops: Ops implemented by Late Binding driver, used by Xe driver.
>> + *
>> + * Communication between Xe and MEI drivers for Late Binding services
>> + */
>> +struct xe_late_bind_component {
>> + /** @late_bind_component.mei_dev: mei device */
>> + struct device *mei_dev;
>> + /** @late_bind_component.ops: late binding ops */
>> + const struct late_bind_component_ops *ops;
>> +};
>> +
>> +/**
>> + * 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.mutex: protects the component binding and usage */
> Please, before submitting another re-spin of this series, refactor
> this mutex. This is absolutely not acceptable.
>
> https://blog.ffwll.ch/2022/07/locking-engineering.html
>
> This is protecting the code and not the data. If binding or usage
> happens you need to have other ways of dealing with it.
>
> The lock needs to be reduced to the data you are trying to protect.
> Perhaps around the state/status or to certain register, but using
> a big mutex like you use in the patch 5 of this series and stating
> that it is to protect the code is not the right way.
>
> Sorry for not having looked at this earlier.
Sure Rodrigo, I will try to refactor mutex. The intention was to
prevent component removal during the push_config process and when a
module unbind operation is performed
This is how it is implemented for xe_gsc_proxy.c and all the components
in i915.
I was considering alternatives like wait_event_timeout or
wait_for_completion. However, since we are already using flushing the
work in xe_late_bind_component_unbind, the above scenario is unlikely to
occur.
Regards,
Badal
>
>> + struct mutex mutex;
>> +};
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index 08e21d4099e0..e5018d3ae74f 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -66,6 +66,7 @@ struct xe_device_desc {
>> u8 has_gsc_nvm:1;
>> u8 has_heci_gscfi:1;
>> u8 has_heci_cscfi:1;
>> + u8 has_late_bind:1;
>> u8 has_llc:1;
>> u8 has_mbx_power_limits:1;
>> u8 has_pxp:1;
>> @@ -355,6 +356,7 @@ static const struct xe_device_desc bmg_desc = {
>> .has_mbx_power_limits = true,
>> .has_gsc_nvm = 1,
>> .has_heci_cscfi = 1,
>> + .has_late_bind = true,
>> .needs_scratch = true,
>> };
>>
>> @@ -600,6 +602,7 @@ static int xe_info_init_early(struct xe_device *xe,
>> xe->info.has_gsc_nvm = desc->has_gsc_nvm;
>> xe->info.has_heci_gscfi = desc->has_heci_gscfi;
>> xe->info.has_heci_cscfi = desc->has_heci_cscfi;
>> + xe->info.has_late_bind = desc->has_late_bind;
>> xe->info.has_llc = desc->has_llc;
>> xe->info.has_pxp = desc->has_pxp;
>> xe->info.has_sriov = desc->has_sriov;
>> --
>> 2.34.1
>>
More information about the dri-devel
mailing list