[RESEND,0/5] Cleanups for the nomodeset kernel command line parameter logic

Message

Javier Martinez Canillas Nov. 3, 2021, 12:28 p.m. UTC

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

 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

Comments

Thomas Zimmermann Nov. 3, 2021, 1:01 p.m. UTC | #1

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
>

Javier Martinez Canillas Nov. 3, 2021, 3 p.m. UTC | #2

Hello Thomas,

On 11/3/21 14:01, Thomas Zimmermann wrote:

[snip]

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

Sure, I'm happy with less patches.

Thanks for your feedback.
 
> Best regards
> Thomas
> 
Best regards,