[PATCH i-g-t] lib/igt_kmod: Unload/Reload the mei Modules Before Unloading/Reloading the i915/xe Driver

[Lucas De Marchi]

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 at intel.com>
>>>Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>>>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at 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
>>
>>>+    };
>>>+
>>>    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
>>>
>