On Wed, Sep 11, 2024 at 06:00:47AM +0000, Bommu, Krishnaiah wrote: > > > > -----Original Message----- > > From: De Marchi, Lucas <[email protected]> > > Sent: Tuesday, September 10, 2024 9:13 PM > > To: Vivi, Rodrigo <[email protected]> > > Cc: Bommu, Krishnaiah <[email protected]>; intel- > > [email protected]; [email protected]; Kamil Konieczny > > <[email protected]>; Ceraolo Spurio, Daniele > > <[email protected]>; Upadhyay, Tejas > > <[email protected]>; Tvrtko Ursulin <[email protected]>; Joonas > > Lahtinen <[email protected]>; Nikula, Jani > > <[email protected]>; Thomas Hellström > > <[email protected]>; Teres Alexis, Alan Previn > > <[email protected]>; Winkler, Tomas > > <[email protected]>; Usyskin, Alexander > > <[email protected]> > > Subject: Re: [PATCH v2] drm: Ensure Proper Unload/Reload Order of MEI > > Modules for i915/Xe Driver > > > > On Tue, Sep 10, 2024 at 11:03:30AM GMT, Rodrigo Vivi wrote: > > >On Mon, Sep 09, 2024 at 09:33:17AM +0530, Bommu Krishnaiah wrote: > > >> This update addresses the unload/reload sequence of MEI modules in > > >> relation to the i915/Xe graphics driver. On platforms where the MEI > > >> hardware is integrated with the graphics device (e.g., DG2/BMG), the > > >> i915/xe driver is depend on the MEI modules. Conversely, on newer > > >> platforms like MTL and LNL, where the MEI hardware is separate, this > > dependency does not exist. > > >> > > >> The changes introduced ensure that MEI modules are unloaded and > > >> reloaded in the correct order based on platform-specific > > >> dependencies. This is achieved by adding a MODULE_SOFTDEP directive to > > the i915 and Xe module code. > > > > > > can you explain what causes the modules to be loaded today? Also, is this > > to fix > > anything related to *loading* order or just unload? > > > > >> > > >> These changes enhance the robustness of MEI module handling across > > >> different hardware platforms, ensuring that the i915/Xe driver can be > > >> cleanly unloaded and reloaded without issues. > > >> > > >> v2: updated commit message > > >> > > >> Signed-off-by: Bommu Krishnaiah <[email protected]> > > >> Cc: Kamil Konieczny <[email protected]> > > >> Cc: Daniele Ceraolo Spurio <[email protected]> > > >> Cc: Lucas De Marchi <[email protected]> > > >> Cc: Tejas Upadhyay <[email protected]> > > >> --- > > >> drivers/gpu/drm/i915/i915_module.c | 2 ++ > > >> drivers/gpu/drm/xe/xe_module.c | 2 ++ > > >> 2 files changed, 4 insertions(+) > > >> > > >> diff --git a/drivers/gpu/drm/i915/i915_module.c > > >> b/drivers/gpu/drm/i915/i915_module.c > > >> index 65acd7bf75d0..2ad079ad35db 100644 > > >> --- a/drivers/gpu/drm/i915/i915_module.c > > >> +++ b/drivers/gpu/drm/i915/i915_module.c > > >> @@ -75,6 +75,8 @@ static const struct { }; static int > > >> init_progress; > > >> > > >> +MODULE_SOFTDEP("pre: mei_gsc_proxy mei_gsc"); > > >> + > > >> static int __init i915_init(void) > > >> { > > >> int err, i; > > >> diff --git a/drivers/gpu/drm/xe/xe_module.c > > >> b/drivers/gpu/drm/xe/xe_module.c index bfc3deebdaa2..5633ea1841b7 > > >> 100644 > > >> --- a/drivers/gpu/drm/xe/xe_module.c > > >> +++ b/drivers/gpu/drm/xe/xe_module.c > > >> @@ -127,6 +127,8 @@ static void xe_call_exit_func(unsigned int i) > > >> init_funcs[i].exit(); > > >> } > > >> > > >> +MODULE_SOFTDEP("pre: mei_gsc_proxy mei_gsc"); > > > > > >I'm honestly not very comfortable with this. > > > > > >1. This is not true for every device supported by these modules. > > >2. This is not true for every (and the most basic) functionality of these > > >drivers. > > > > > >Shouldn't this be done in the the mei side? > > > > I don't think it's possible to do from the mei side. Would mei depend on > > both xe > > and i915 (and thus cause both to be loaded regardless of the platform?). > > For a > > runtime dependency like this that depends on the platform, I think the best > > way > > would be a weakdep + either a request_module() or something else that causes > > the module to load (is that what comp_* is doing today?) > > > > > > > >Couldn't at probe we identify the need of them and if needed we return > > >-EPROBE to attempt a retry after the mei drivers were probed? > > > > I'm not sure this is fixing anything for probe. I think we already wait on > > the other > > component to be ready without blocking the rest of the driver functionality. > > > > A weakdep wouldn't cause the module to be loaded where it's not needed, but > > need some clarification if this is trying to fix anything load-related or > > just unload. > > This change is fixing unload. > During xe load I am seeing mei_gsc modules was loaded, but not unloaded > during the unload xe
Is it a problem? > > root@DUT6127BMGFRD:/home/gta# lsmod | grep xe ------>>>just after system > reboot > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# lsmod | grep mei > mei_hdcp 28672 0 > mei_pxp 16384 0 > mei_me 49152 2 > mei 167936 5 mei_hdcp,mei_pxp,mei_me > root@DUT6127BMGFRD:/home/gta# lsmod | grep xe > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# modprobe xe > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# lsmod | grep mei > mei_gsc_proxy 16384 0 > mei_gsc 12288 1 > mei_hdcp 28672 0 > mei_pxp 16384 0 > mei_me 49152 3 mei_gsc > mei 167936 8 mei_gsc_proxy,mei_gsc,mei_hdcp,mei_pxp,mei_me > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# init 3 > root@DUT6127BMGFRD:/home/gta# echo -n auto > > /sys/bus/pci/devices/0000\:03\:00.0/power/control > root@DUT6127BMGFRD:/home/gta# echo -n "0000:03:00.0" > > /sys/bus/pci/drivers/xe/unbind > root@DUT6127BMGFRD:/home/gta# modprobe -r xe > root@DUT6127BMGFRD:/home/gta# > root@DUT6127BMGFRD:/home/gta# lsmod | grep xe > root@DUT6127BMGFRD:/home/gta# lsmod | grep mei > mei_gsc_proxy 16384 0 > mei_gsc 12288 0 > mei_hdcp 28672 0 > mei_pxp 16384 0 > mei_me 49152 3 mei_gsc > mei 167936 7 mei_gsc_proxy,mei_gsc,mei_hdcp,mei_pxp,mei_me > root@DUT6127BMGFRD:/home/gta# > > Regards, > Krishna. > > > > > Lucas De Marchi > > > > > > > >Cc: Alexander Usyskin <[email protected]> > > >Cc: Tomas Winkler <[email protected]> > > >Cc: Alan Previn <[email protected]> > > >Cc: Daniele Ceraolo Spurio <[email protected]> > > >Cc: Lucas De Marchi <[email protected]> > > >Cc: Thomas Hellström <[email protected]> > > >Cc: Jani Nikula <[email protected]> > > >Cc: Joonas Lahtinen <[email protected]> > > >Cc: Tvrtko Ursulin <[email protected]> > > > > > >> + > > >> static int __init xe_init(void) > > >> { > > >> int err, i; > > >> -- > > >> 2.25.1 > > >>
