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

[Thomas Zimmermann <tzimmermann@suse.de>]

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