Re: [PATCH v3 0/6] Cleanups for the nomodeset kernel command line parameter logic

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi

Am 08.11.21 um 13:15 schrieb Javier Martinez Canillas:
> There is a lot of historical baggage on this parameter. It is defined in
> the vgacon driver as nomodeset, but its set function is called text_mode()
> and the value queried with a function named vgacon_text_force().
> 
> All this implies that it's about forcing text mode for VGA, yet it is not
> used in neither vgacon nor other console driver. The only users for these
> are DRM drivers, that check for the vgacon_text_force() return value to
> determine whether the driver should be loaded or not.
> 
> That makes it quite confusing to read the code, because the variables and
> function names don't reflect what they actually do and also are not in the
> same subsystem as the drivers that make use of them.
> 
> This patch-set attempts to cleanup the code by moving the nomodseset param
> to the DRM subsystem and do some renaming to make their intention clearer.
> 
> This is a v3 of the patches, that address issues pointed out by Jani Nikula
> in v2: https://lkml.org/lkml/2021/11/4/594
> 
> Patch #1 and #2 are just trivial cleanups.
> 
> Patch #3 moves the nomodeset boot option to the DRM subsystem and renames
> the variables and functions names.
> 
> Patch #4 removes the relationship between the nomodeset parameter and the
> CONFIG_VGA_CONSOLE Kconfig symbol.

On patches 1 to 4

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> 
> Patch #5 adds nomodeset to the kernel parameters documentation.
> 
> Patch #6 improves the message when nomodeset is enabled to make it more
> accurate and less sensational.

See my comments on these patches.

Best regards
Thomas

> 
> Changes in v3:
> - Drop the drm_drv_enabled() function and just call to drm_get_modeset().
> - Make drm_get_modeset() just a getter function and don't return an error.
> - Move independent cleanups in drivers as separate preparatory patches.
> 
> Changes in v2:
> - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
> - Squash patches to move nomodeset logic to DRM and do the renaming.
> - Name the function drm_check_modeset() and make it return -ENODEV.
> - Squash patch to add drm_drv_enabled() and make drivers use it.
> - Make the drivers changes before moving nomodeset logic to DRM.
> - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
> - Remove debug and error messages in drivers.
> 
> Javier Martinez Canillas (6):
>    drm: Don't print messages if drivers are disabled due nomodeset
>    drm/vboxvideo: Drop CONFIG_VGA_CONSOLE guard to call
>      vgacon_text_force()
>    drm: Move nomodeset kernel parameter to the DRM subsystem
>    drm: Decouple nomodeset from CONFIG_VGA_CONSOLE
>    Documentation/admin-guide: Document nomodeset kernel parameter
>    drm: Make the nomodeset message less sensational
> 
>   .../admin-guide/kernel-parameters.txt         |  4 ++++
>   drivers/gpu/drm/Kconfig                       |  6 +++++
>   drivers/gpu/drm/Makefile                      |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  6 ++---
>   drivers/gpu/drm/ast/ast_drv.c                 |  4 ++--
>   drivers/gpu/drm/drm_nomodeset.c               | 24 +++++++++++++++++++
>   drivers/gpu/drm/i915/i915_module.c            |  4 ++--
>   drivers/gpu/drm/mgag200/mgag200_drv.c         |  4 ++--
>   drivers/gpu/drm/nouveau/nouveau_drm.c         |  4 ++--
>   drivers/gpu/drm/qxl/qxl_drv.c                 |  4 ++--
>   drivers/gpu/drm/radeon/radeon_drv.c           | 10 +++-----
>   drivers/gpu/drm/tiny/bochs.c                  |  4 ++--
>   drivers/gpu/drm/tiny/cirrus.c                 |  5 ++--
>   drivers/gpu/drm/vboxvideo/vbox_drv.c          |  6 ++---
>   drivers/gpu/drm/virtio/virtgpu_drv.c          |  4 ++--
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c           |  4 ++--
>   drivers/video/console/vgacon.c                | 21 ----------------
>   include/drm/drm_mode_config.h                 |  2 ++
>   include/linux/console.h                       |  6 -----
>   19 files changed, 64 insertions(+), 60 deletions(-)
>   create mode 100644 drivers/gpu/drm/drm_nomodeset.c
> 

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