On 15-08-2024 22:55, Lucas De Marchi wrote: > 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 >>>> Cc: Kamil Konieczny >>>> Cc: Daniele Ceraolo Spurio >>>> --- >>>> 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_proxy163840 mei_gsc122881 mei_pxp163840 mei_hdcp286720 mei_me491523 mei_gsc mei1679368 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_proxy163840 mei_gsc122880 mei_pxp163840 mei_hdcp286720 mei_me491523 mei_gsc mei1679367 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_proxy163840 mei_gsc122880 mei_pxp163840 mei_hdcp286720 mei_me491523 mei_gsc mei1679367 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 >>>> >>