[PATCH v2] drm: Ensure Proper Unload/Reload Order of MEI Modules for i915/Xe Driver

[Lucas De Marchi]

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 <krishnaiah.bommu at intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>> Cc: Tejas Upadhyay <tejas.upadhyay at intel.com>
>> ---
>>  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.

Lucas De Marchi

>
>Cc: Alexander Usyskin <alexander.usyskin at intel.com>
>Cc: Tomas Winkler <tomas.winkler at intel.com>
>Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>Cc: Jani Nikula <jani.nikula at intel.com>
>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>Cc: Tvrtko Ursulin <tursulin at ursulin.net>
>
>> +
>>  static int __init xe_init(void)
>>  {
>>  	int err, i;
>> --
>> 2.25.1
>>