Messages in this thread | | | Date | Wed, 15 Dec 2021 10:10:20 +0100 | Subject | Re: [PATCH 00/60] drm: Make all drivers to honour the nomodeset parameter | From | Thomas Zimmermann <> |
| |
Hi
Am 15.12.21 um 09:50 schrieb Javier Martinez Canillas: > Hello Thomas, > > On 12/15/21 09:31, Thomas Zimmermann wrote: >> Hi Javier, >> >> nothing wrong with your patches, but I'd like to propose slightly >> differnt solution. >> >> For many USB drivers, you put the drm_firmware_drivers_only() call into >> the probe function. For registering, these drivers use >> module_usb_driver(), which expands to generic device-register functions. >> >> I'd like to propose a similar macro for DRM drivers that includes the >> test for drm_firmware_drivers_only(). >> >> In drm_drv.h : >> >> #if defined(USB) >> static int drm_usb_register(struct usb_driver *usb) >> { >> if (drm_firmware_drivers_only()) >> return -ENODEV; >> return usb_register_driver(usb); >> } >> #define drm_module_usb_driver(__usb) >> module_driver(drm_usb_register, usb_deregister) >> #endif >> >> In each of the USB-based DRM drivers, replace module_usb_driver with >> drm_module_usb_driver. >> >> And then there's PCI [3] and platform drivers, [4] which can be handled >> similarly. Many PCI drivers open-code the module init and device > > Thanks for the suggestion. I actually thought about this approach as well, > since seems unnecessary to have all that duplicated logic in every driver. > > But at the end decided to just do the less intrusive change, because can't > do any testing for most of the drivers. > >> registering with the driver-specific enable parameter. Maybe adding a >> driver-specific register function would make sense. >> > > If I understood you correctly, suggested something similar in #dri-devel but > danvet's suggestion [0] was to keep it simple: > > 16:32 javierm: danvet: yes, I don't think we would be able to do it at this point. I'll add a function pointer > param to the check function so drivers can also define their own check besides nomodeset > 16:33 danvet: javierm, uh that sounds a bit like overkill? just keep that part of the check in drivers? > 16:33 javierm: danvet: Ok. Wanted to get rid of the duplicated code but I'm indeed over engineering this :)
An extra pointer sounds indeed like over-engineering. If we take ast for example, I'd just do something like this:
static ast_pci_register_driver(struct pci_driver *pci) { if (drm_firmware_drivers_only() && ast_modeset == -1) return -ENODEV; if (ast_modeset == 0) return -ENODEV;
return pci_register_driver(pci); }
module_driver(&ast_pci_driver, ast_pci_register_driver, pci_unregister_driver)
That removes some of the boiler-plate module code without changing the driver's behavior.
Best regards Thomas
> > [0]: https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2021-11-02 > >> Best regards >> Thomas >> > Best regards, >
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev [unhandled content-type:application/pgp-signature] | |