Hi,
On 4/15/20 1:39 PM, Hans de Goede wrote:
<snip>
/* Add comment explaining why we need this messy stuff here */ const char * const shadow_providers[] = { #ifdef CONFIG_THINKPAD_ACPI_MODULE "thinkpad_acpi", #endif #ifdef CONFIG_OTHER_MODULE "other", #endif NULL };
int module_init(void) { /* do actual setup of the ?class? */
for (i = 0; shadow_providers[i]; i++) request_module(shadow_providers[i]);
return 0; }
Hm I think explicitly loading drivers feels very much not device model like. Don't these drivers auto-load, matching on acpi functions?
thinkpad_acpi does autoload based on a number of ACPI device-ids, the idea behind the above request_module is to avoid the need to have the acpi-match function you mentioned above.
Basically what would happen is e.g. :
- i915 loads, calls lcdshadow_get(dev, "eDP-1");
- if this is the first lcdshadow_get() call then
the lcdshadow core will do the request_module calls, allowing any of these modules to get loaded + probed and call e.g. lcdshadow_register(&mylcdshadowdev, <gfx-adapter-dev-name>, "eDP-1"); 3. After the request modules the lcdshadow_get() will walk over the list of registered devices, including the ones just registered by the request_module calls and then hopefully find a match
So by doing the request-module calls before checking for a matching lcdshadow dev, we avoid the need of having some of the knowledge currently abstracted away in the thinkpad_acpi driver duplicated inside the drm code somewhere.
I guess if that doesn't exist, then we'd need to fix that one first :-/ In general no request_module please, that shouldn't be needed either.
The trouble with request_module is also that (afaiui) it doesn't really work well with parallel module load and all that, for EPROBE_DEFER to work we do need to be able to answer "should we have a driver?" independently of whether that driver has loaded already or not.
The idea here is to avoid using EPROBE_DEFER (on x86 at least) and either directly return the lcdshadow_dev or ENOENT. Also see below.
<snip>
Assuming we are going to add some device/model specific lcdshadow knowledge inside the lcdshadow core as you suggested with your "small acpi match function" above, we could do something similar to what the vga_switcheroo code is doing for this and have a lcdshadow_defer_probe() helper and call that really early in i915_pci_probe(), which currently already has this for the vgaswitcheroo case:
if (vga_switcheroo_client_probe_defer(pdev)) return -EPROBE_DEFER;
So thinking more about this and given the total lack of EPROBE_DEFER handling in the 3 major X86 GPU/kms drivers I think that adding a lcdshadow_defer_probe() helper is the way to go. This will also avoid the need for duplicating the lcdshadow detect functionality in the small ACPI-match functions you mentioned (although that might still be interesting to speedup the boot).
When everything is builtin then each enabled "module"-s module_init function will get called, we can call a lcdshadow_probe_done("module-name") function from those and the lcdshadow core can then track if all potential lcdhadow providers have initialized before it stops returning non 0 from lcdshadow_defer_probe().
Or if we still do the small match functions it could be even smarter with this...
And for the modular case it can call request_module on all (enabled as module) potential lcdhadow providers (or again we could rely on the small match function instead).
Then (on x86 at least) we can have lcdshadow_get never return -EPROBE_DEFER and avoid the need to solve the lack of EPROBE_DEFER support in the 3 major x86 drivers.
And this is all kernel internal, so if that lack of EPROBE_DEFER support ever gets fixed then we can drop the lcdshadow_defer_probe() hack and make lcdshadow_get also return -EPROBE_DEFER on x86 in some cases.
Regards,
Hans