On Thu, Aug 15, 2024 at 09:57:08AM GMT, Daniele Ceraolo Spurio wrote:
On 8/15/2024 8:20 AM, Lucas De Marchi wrote:
On Thu, Aug 15, 2024 at 06:34:23PM GMT, Bommu Krishnaiah wrote:
To ensure stability and proper module management, this update introduces
changes to the handling of MEI modules when loading or unloading
the Intel i915 or Xe drivers.
Key Changes:
- Unload Order: The `mei_gsc_proxy` module is now unloaded before `mei_gsc`,
preventing potential failures during the unloading process.
- Platform-Specific Handling: On platforms where the MEI hardware is integrated
with the graphics device (e.g., DG2/BMG), the MEI modules depend on the i915/Xe driver.
Conversely, on newer platforms like CLS, where MEI hardware is separate,
this dependency does not exist. This update ensures that MEI modules are
unloaded/reloaded in the correct order based on platform-specific dependencies.
These changes address the need for a more robust handling of MEI modules across
different hardware platforms, ensuring that the i915/Xe driver can be
cleanly unloaded and reloaded without issues.
Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
lib/igt_kmod.c | 38 ++++++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 464c0dcf4..6ee2b7afe 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -480,9 +480,29 @@ igt_intel_driver_load(const char *opts, const char *driver)
{
int ret;
+ const char *mei[] = {
+ /* mei_gsc uses an i915 aux dev and the other mei mods depend on it */
+ "mei_pxp",
+ "mei_hdcp",
+ "mei_gsc_proxy",
+ "mei_gsc",
+ NULL,
all this manual approach we have for module unloading should be gone
IMO. Replace that with 1) unbind the driver; 2) unload the module.
There's no need afaict to keep tracking what the module uses.
as for loading, why exactly do we have to do the kernels/depmod
manually? If we have to do anything manually like that, it means
real users won't have that and we are actually missing the proper
integration. I typed something on the different types of
dependencies (harddep, softdep, weakdep):
https://politreco.com/2024/08/linux-module-dependencies/
The mei modules are a bit of an issue because they change behavior between integrated and discrete. In integrated, they're loaded on the CPU MEI device (the CSME) and so they're completely independent from i915/xe and we don't need to unload/load them them to unload/load i915/xe. On discrete however the MEI device is a child device on the graphics card, so the mei modules depend on i915/xe and need to be removed before the graphics driver can be unloaded, but are then auto-reloaded when the i915/xe is re-loaded. So for IGT we have 2 possible solutions:
1) only unload mei modules if we're on discrete, in which case they will correctly auto-reload when we re-export the child device.
2) always unload mei modules before unloading i915/xe, but then we need to manually re-load them because they're not going to be auto-reloaded on integrated.
I'm guessing that if we unplug i915/xe that's going to automatically unplug the mei driver on the child device so maybe that does help in avoiding the unload/reload flow?
yes. Just unbinding the driver should make it stop using it and if
that doesn't work we have something to fix outside of the CI infra.
for the loading part, *if we have no other means*, adding a
MODULE_SOFTDEP("pre: ...") would do it, although a weakdep +
request_module() if/when needed would be better (except that
weakdep is only available on 6.11 and kmod 33)
Lucas De Marchi
Daniele
Lucas De Marchi
thank you for your inputs
I tried with module unbind and unload with this mei modules was not removed, I will add mei_gsc_proxy removal for discrete platforms with MODULE_SOFTDEP
root@DUT6099BMGFRD:/home/gta#
root@DUT6099BMGFRD:/home/gta# lsmod | grep mei
mei_gsc_proxy 16384 0
mei_gsc 12288 1
mei_pxp 16384 0
mei_hdcp 28672 0
mei_me 49152 3 mei_gsc
mei 167936 8 mei_gsc_proxy,mei_gsc,mei_hdcp,mei_pxp,mei_me
root@DUT6099BMGFRD:/home/gta# echo -n auto > /sys/bus/pci/devices/0000\:03\:00.0/power/control
root@DUT6099BMGFRD:/home/gta# echo -n "0000:03:00.0" > /sys/bus/pci/drivers/xe/unbind
root@DUT6099BMGFRD:/home/gta# lsmod | grep mei
mei_gsc_proxy 16384 0
mei_gsc 12288 0
mei_pxp 16384 0
mei_hdcp 28672 0
mei_me 49152 3 mei_gsc
mei 167936 7 mei_gsc_proxy,mei_gsc,mei_hdcp,mei_pxp,mei_me
root@DUT6099BMGFRD:/home/gta#
root@DUT6099BMGFRD:/home/gta# modprobe -r xe
root@DUT6099BMGFRD:/home/gta#
root@DUT6099BMGFRD:/home/gta#
root@DUT6099BMGFRD:/home/gta# lsmod | grep mei
mei_gsc_proxy 16384 0
mei_gsc 12288 0
mei_pxp 16384 0
mei_hdcp 28672 0
mei_me 49152 3 mei_gsc
mei 167936 7 mei_gsc_proxy,mei_gsc,mei_hdcp,mei_pxp,mei_me
root@DUT6099BMGFRD:/home/gta#
Regards,
Krishna.
+ };
+
if (opts)
igt_info("Reloading %s with %s\n\n", driver, opts);
+ for (const char **m = mei; *m; m++) {
+ if (igt_kmod_is_loaded(*m))
+ continue;
+
+ ret = igt_kmod_load(*m, NULL);
+ if (ret) {
+ igt_debug("Could not load %s\n", *m);
+ return ret;
+ }
+ }
+
ret = igt_kmod_load(driver, opts);
if (ret) {
igt_debug("Could not load %s\n", driver);
@@ -620,9 +640,14 @@ int __igt_intel_driver_unload(char **who, const char *driver)
const char *aux[] = {
/* gen5: ips uses symbol_get() so only a soft module dependency */
"intel_ips",
+ NULL,
+ };
+
+ const char *mei[] = {
/* mei_gsc uses an i915 aux dev and the other mei mods depend on it */
"mei_pxp",
"mei_hdcp",
+ "mei_gsc_proxy",
"mei_gsc",
NULL,
};
@@ -647,6 +672,19 @@ int __igt_intel_driver_unload(char **who, const char *driver)
}
}
+ for (const char **m = mei; *m; m++) {
+ if (!igt_kmod_is_loaded(*m))
+ continue;
+
+ ret = igt_kmod_unload(*m);
+ if (ret) {
+ if (who)
+ *who = strdup_realloc(*who, *m);
+
+ return ret;
+ }
+ }
+
if (igt_kmod_is_loaded(driver)) {
ret = igt_kmod_unload(driver);
if (ret) {
--
2.25.1