Re: [PATCH 00/60] drm: Make all drivers to honour the nomodeset parameter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux