[PATCH i-g-t] lib/igt_kmod: Unload/Reload the mei Modules Before Unloading/Reloading the i915/xe Driver
Bommu, Krishnaiah
krishnaiah.bommu at intel.com
Tue Aug 20 05:37:57 UTC 2024
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 <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
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 at DUT6099BMGFRD:/home/gta#
root at 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 at DUT6099BMGFRD:/home/gta# echo -n auto >
/sys/bus/pci/devices/0000\:03\:00.0/power/control
root at DUT6099BMGFRD:/home/gta# echo -n "0000:03:00.0" >
/sys/bus/pci/drivers/xe/unbind
root at 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 at DUT6099BMGFRD:/home/gta#
root at DUT6099BMGFRD:/home/gta# modprobe -r xe
root at DUT6099BMGFRD:/home/gta#
root at DUT6099BMGFRD:/home/gta#
root at 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 at 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
>>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20240820/ab5f76f6/attachment-0001.htm>
More information about the igt-dev
mailing list