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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

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?

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
>>