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

Thomas Zimmermann
2021-11-03T13:02:03Z

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

Hi

Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:
> [ resend with all relevant people as Cc now, sorry to others for the spam ]
>
> There is a lot of historical baggage on this parameter. It's defined in
> the vgacon driver as a "nomodeset" parameter, but it's handler function is
> called text_mode() that sets a variable named vgacon_text_mode_force whose
> value is 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 could 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.
>
> There is also another aspect that could be improved, and is the fact that
> drivers are checking for the nomodeset being set as an indication if have
> to be loaded.
>
> But there may be other reasons why this could be the case, so it is better
> to encapsulate the logic in a separate function to make clear what's about.
>
> Patch #1 is just a trivial fix for a comment that isn't referring to the
> correct kernel parameter.
>
> Patch #2 moves the nomodeset logic to the DRM subsystem.
>
> Patch #3 renames the vgacon_text_force() function and accompaning logic as
> drm_modeset_disabled(), which is what this function is really about.
>
> Patch #4 adds a drm_drv_enabled() function that could be used by drivers
> to check if could be enabled.
>
> Patch #5 uses the drm_drv_enabled() function to check this instead of just
> checking if nomodeset has been set.
>
>
> Javier Martinez Canillas (5):
> drm/i915: Fix comment about modeset parameters
> drm: Move nomodeset kernel parameter handler to the DRM subsystem
> drm: Rename vgacon_text_force() function to drm_modeset_disabled()
> drm: Add a drm_drv_enabled() helper function
> drm: Use drm_drv_enabled() instead of drm_modeset_disabled()

There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And
I'd put patch (4+5) first, so you have the drivers out of the way. After
that patch (2+3) should only modify drm_drv_enabled().

Best regards
Thomas

>
> drivers/gpu/drm/Makefile | 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 5 ++---
> drivers/gpu/drm/ast/ast_drv.c | 3 +--
> drivers/gpu/drm/drm_drv.c | 21 ++++++++++++++++++++
> drivers/gpu/drm/drm_nomodeset.c | 26 +++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_module.c | 10 +++++-----
> drivers/gpu/drm/mgag200/mgag200_drv.c | 3 +--
> drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +--
> drivers/gpu/drm/qxl/qxl_drv.c | 3 +--
> drivers/gpu/drm/radeon/radeon_drv.c | 3 +--
> drivers/gpu/drm/tiny/bochs.c | 3 +--
> drivers/gpu/drm/tiny/cirrus.c | 3 +--
> drivers/gpu/drm/vboxvideo/vbox_drv.c | 5 +----
> drivers/gpu/drm/virtio/virtgpu_drv.c | 3 +--
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 3 +--
> drivers/video/console/vgacon.c | 21 --------------------
> include/drm/drm_drv.h | 1 +
> include/drm/drm_mode_config.h | 6 ++++++
> include/linux/console.h | 6 ------
> 19 files changed, 73 insertions(+), 57 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 --]