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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 5213 bytes --]

Hi

Am 07.11.22 um 21:46 schrieb 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"...

I'd like to add this to fbdev or the drivers instead of the video 
helper. On the DRM side, it works a a bit different and I think I have a 
use case for the function that does not directly involve disabling 
drivers. See below for a proposal.

> 
> 
>>> 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)

The term modesetting is misleading and we had this problem with 
'nomodeset' already. There are still plenty of drivers with mode 
setting, such as the USB-based ones.  It's also not so easy on the DRM 
side, where a modesetting operation is one of many effects of loading an 
atomic state.  Maybe let's leave the name is for now? If we ever find 
the perfect name, it's a simple rename with sed.

My proposal would be to add a little helper to fbdev that includes your 
suggestions:

   bool fb_modesetting_disabled(const char *drvname)
   {
      fwonly = video_firmware_drivers_only()
      if (fbonly && drvname)
	pr_warn("")
      return fbonly;
   }

Drivers can use that for the test.

Best regards
Thomas


> 
> Helge

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]