[PATCH 2/2] fbdev: Add support for the nomodeset kernel parameter

[Helge Deller]

On 11/7/22 16:30, Thomas Zimmermann wrote:
> Hi
>
> Am 07.11.22 um 14:57 schrieb Helge Deller:
>> On 11/7/22 11:49, Thomas Zimmermann wrote:
>>> Support the kernel's nomodeset parameter for all PCI-based fbdev
>>> drivers that use aperture helpers to remove other, hardware-agnostic
>>> graphics drivers.
>>>
>>> The parameter is a simple way of using the firmware-provided scanout
>>> buffer if the hardware's native driver is broken.
>>
>> Nah... it's probably not broken, but you want it disabled in order
>> to use the DRM driver instead?
>
> No, it's really for broken native drivers or any kind of problematic
> modesetting. Most DRM drivers already respect the nomodeset option
> and won't load when given. All you'd get are the generic drivers,
> such as simpledrm, efifb or simplefb.
>
> There are better options of configuring video output on the kernel
> command line.  But as graphics output is such a fundamental feature
> to using a computer, we found that a simple and easy option to
> workaround erroneous systems would benefit DRM users; hence the
> nomodeset parameter.
>
> As fbdev drivers also do modesetting, supporting the parameter simply
> unifies the behavior.

Ok.

>>> The same effect
>>> could be achieved with per-driver options, but the importance of the
>>> graphics output for many users makes a single, unified approach
>>> worthwhile.
>>>
>>> With nomodeset specified, the fbdev driver module will not load. This
>>> unifies behavior with similar DRM drivers. In DRM helpers, modules
>>> first check the nomodeset parameter before registering the PCI
>>> driver. As fbdev has no such module helpers, we have to modify each
>>> driver individually.
>>
>> Ok.
>>
>>> The name 'nomodeset' is slightly misleading, but has been chosen for
>>> historical reasons. Several drivers implemented it before it became a
>>> general option for DRM. So keeping the existing name was preferred over
>>> introducing a new one.
>>
>>> diff --git a/drivers/video/fbdev/aty/aty128fb.c b/drivers/video/fbdev/aty/aty128fb.c
>>> index 57e398fe7a81c..1a26ac2865d65 100644
>>> --- a/drivers/video/fbdev/aty/aty128fb.c
>>> +++ b/drivers/video/fbdev/aty/aty128fb.c
>>> @@ -2503,7 +2504,12 @@ static int aty128fb_init(void)
>>>   {
>>>   #ifndef MODULE
>>>       char *option = NULL;
>>> +#endif
>>> +
>>> +    if (video_firmware_drivers_only())
>>> +        return -ENODEV;
>>
>> I think it makes sense to give at least some info, why a specific
>> driver wasn't loaded, e.g. something like this kernel message:
>> aty128fb: Driver disabled due to "nomodeset" kernel parameter.
>>
>> If you e.g. change the function video_firmware_drivers_only()
>> to become video_firmware_drivers_only(const char *drivername)
>> then you could print such a message in video_firmware_drivers_only()
>
> Well, we do have such a message in disable_modeset() already. [1]
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_nomodeset.c#L18

Yes, I saw it, but that message quite generic.
If for example my atyfb doesn't show up, I would grep dmesg for "aty" and
not "nomodeset"...


>> And I don't like very much the name of function video_firmware_drivers_only(),
>> but don't have any other better idea right now either...
>
> It's part of the 'video' module, hence the prefix. The 'nomodeset'
> option used to be implemented in several DRM drivers. It's an awful
> name, but we didn't want to remove it or introduce a new one for the
> same feature. So we kept nomodeset for all of DRM.  Then we started
> bikeshedding the function name that returns the setting. And
> firmware-drivers-only is the best description of what is happening
> here. So that's how the name happend.

video_modesetting_disabled() ?
(Just an idea)

Helge