<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 15-08-2024 22:55, Lucas De Marchi wrote: </div> <blockquote type="cite" cite="mid:nko5zjbdfw272zgf2di4yzeptujglamsxkryjvicdperonsnqw@ggnhjda4trwj">On Thu, Aug 15, 2024 at 09:57:08AM GMT, Daniele Ceraolo Spurio wrote: <blockquote type="cite"> On 8/15/2024 8:20 AM, Lucas De Marchi wrote: <blockquote type="cite">On Thu, Aug 15, 2024 at 06:34:23PM GMT, Bommu Krishnaiah wrote: <blockquote type="cite">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 <a class="moz-txt-link-rfc2396E" href="mailto:krishnaiah.bommu@intel.com"><krishnaiah.bommu@intel.com></a> Cc: Kamil Konieczny <a class="moz-txt-link-rfc2396E" href="mailto:kamil.konieczny@linux.intel.com"><kamil.konieczny@linux.intel.com></a> Cc: Daniele Ceraolo Spurio <a class="moz-txt-link-rfc2396E" href="mailto:daniele.ceraolospurio@intel.com"><daniele.ceraolospurio@intel.com></a> --- 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, </blockquote> 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): <a class="moz-txt-link-freetext" href="https://politreco.com/2024/08/linux-module-dependencies/">https://politreco.com/2024/08/linux-module-dependencies/</a> </blockquote> 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? </blockquote> 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 <blockquote type="cite"> Daniele <blockquote type="cite"> Lucas De Marchi </blockquote> </blockquote> </blockquote> 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. <blockquote type="cite" cite="mid:nko5zjbdfw272zgf2di4yzeptujglamsxkryjvicdperonsnqw@ggnhjda4trwj"> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite">+ }; + 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 </blockquote> </blockquote> </blockquote> </blockquote> </body> </html>