Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 11224 bytes --]

Hi

Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:
> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
> but the exported vgacon_text_force() symbol is only used by DRM drivers.
> 
> It makes much more sense for the parameter logic to be in the subsystem
> of the drivers that are making use of it. Let's move that to DRM.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/Makefile                |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
>   drivers/gpu/drm/ast/ast_drv.c           |  1 -
>   drivers/gpu/drm/drm_nomodeset.c         | 26 +++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_module.c      |  2 --
>   drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
>   drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
>   drivers/gpu/drm/qxl/qxl_drv.c           |  1 -
>   drivers/gpu/drm/radeon/radeon_drv.c     |  1 -
>   drivers/gpu/drm/tiny/bochs.c            |  1 -
>   drivers/gpu/drm/tiny/cirrus.c           |  1 -
>   drivers/gpu/drm/vboxvideo/vbox_drv.c    |  1 -
>   drivers/gpu/drm/virtio/virtgpu_drv.c    |  1 -
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  1 -
>   drivers/video/console/vgacon.c          | 21 --------------------
>   include/drm/drm_mode_config.h           |  6 ++++++
>   include/linux/console.h                 |  6 ------
>   17 files changed, 35 insertions(+), 41 deletions(-)
>   create mode 100644 drivers/gpu/drm/drm_nomodeset.c
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c41156deb5f..0e2d60ea93ca 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o drm_privacy_screen_x86.
>   
>   obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>   
> +obj-y += drm_nomodeset.o

Repeating my other comment, should this rather be protected by a 
separate config symbol that is selected by CONFIG_DRM?

Best regards
Thomas

> +
>   drm_cma_helper-y := drm_gem_cma_helper.o
>   obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index c718fb5f3f8a..2680a2aaa877 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -31,7 +31,6 @@
>   #include "amdgpu_drv.h"
>   
>   #include <drm/drm_pciids.h>
> -#include <linux/console.h>
>   #include <linux/module.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/vga_switcheroo.h>
> @@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
>   	int r;
>   
>   	if (vgacon_text_force()) {
> -		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> +		DRM_ERROR("amdgpu kernel modesetting disabled.\n");
>   		return -EINVAL;
>   	}
>   
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 86d5cd7b6318..048be607b182 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -26,7 +26,6 @@
>    * Authors: Dave Airlie <airlied@redhat.com>
>    */
>   
> -#include <linux/console.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
>   
> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
> new file mode 100644
> index 000000000000..1ac9a8d5a8fe
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_nomodeset.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +static bool vgacon_text_mode_force;
> +
> +bool vgacon_text_force(void)
> +{
> +	return vgacon_text_mode_force;
> +}
> +EXPORT_SYMBOL(vgacon_text_force);
> +
> +static int __init text_mode(char *str)
> +{
> +	vgacon_text_mode_force = true;
> +
> +	pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");
> +	pr_warn("Any video related functionality will be severely degraded, and you may not even be able to suspend the system properly\n");
> +	pr_warn("Unless you actually understand what nomodeset does, you should reboot without enabling it\n");
> +
> +	return 1;
> +}
> +
> +/* force text mode - used by kernel modesetting */
> +__setup("nomodeset", text_mode);
> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
> index c7507266aa83..14a59226519d 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -4,8 +4,6 @@
>    * Copyright © 2021 Intel Corporation
>    */
>   
> -#include <linux/console.h>
> -
>   #include "gem/i915_gem_context.h"
>   #include "gem/i915_gem_object.h"
>   #include "i915_active.h"
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 6b9243713b3c..685e766db6a4 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -6,7 +6,6 @@
>    *          Dave Airlie
>    */
>   
> -#include <linux/console.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
>   #include <linux/vmalloc.h>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 1f828c9f691c..029997f50d1a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -22,7 +22,6 @@
>    * Authors: Ben Skeggs
>    */
>   
> -#include <linux/console.h>
>   #include <linux/delay.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index fc47b0deb021..3cd6bd9f059d 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -29,7 +29,6 @@
>   
>   #include "qxl_drv.h"
>   
> -#include <linux/console.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
>   #include <linux/vgaarb.h>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index b74cebca1f89..9b606c1b11ec 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -31,7 +31,6 @@
>   
>   
>   #include <linux/compat.h>
> -#include <linux/console.h>
>   #include <linux/module.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/vga_switcheroo.h>
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 2ce3bd903b70..04333f78be55 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -1,6 +1,5 @@
>   // SPDX-License-Identifier: GPL-2.0-or-later
>   
> -#include <linux/console.h>
>   #include <linux/pci.h>
>   
>   #include <drm/drm_aperture.h>
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index 4611ec408506..8bd674f0d682 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -16,7 +16,6 @@
>    * Copyright 1999-2001 Jeff Garzik <jgarzik@pobox.com>
>    */
>   
> -#include <linux/console.h>
>   #include <linux/dma-buf-map.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index a6c81af37345..e6d983121d0b 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -7,7 +7,6 @@
>    *          Michael Thayer <michael.thayer@oracle.com,
>    *          Hans de Goede <hdegoede@redhat.com>
>    */
> -#include <linux/console.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
>   #include <linux/vt_kern.h>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 749db18dcfa2..cd4c170236f1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -27,7 +27,6 @@
>    */
>   
>   #include <linux/module.h>
> -#include <linux/console.h>
>   #include <linux/pci.h>
>   #include <linux/poll.h>
>   #include <linux/wait.h>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index ab9a1750e1df..fcc4b5a7f639 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -25,7 +25,6 @@
>    *
>    **************************************************************************/
>   
> -#include <linux/console.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/module.h>
>   #include <linux/pci.h>
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index ef9c57ce0906..d4320b147956 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -97,30 +97,9 @@ static int 		vga_video_font_height;
>   static int 		vga_scan_lines		__read_mostly;
>   static unsigned int 	vga_rolled_over; /* last vc_origin offset before wrap */
>   
> -static bool vgacon_text_mode_force;
>   static bool vga_hardscroll_enabled;
>   static bool vga_hardscroll_user_enable = true;
>   
> -bool vgacon_text_force(void)
> -{
> -	return vgacon_text_mode_force;
> -}
> -EXPORT_SYMBOL(vgacon_text_force);
> -
> -static int __init text_mode(char *str)
> -{
> -	vgacon_text_mode_force = true;
> -
> -	pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");
> -	pr_warn("Any video related functionality will be severely degraded, and you may not even be able to suspend the system properly\n");
> -	pr_warn("Unless you actually understand what nomodeset does, you should reboot without enabling it\n");
> -
> -	return 1;
> -}
> -
> -/* force text mode - used by kernel modesetting */
> -__setup("nomodeset", text_mode);
> -
>   static int __init no_scroll(char *str)
>   {
>   	/*
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 48b7de80daf5..e1d2042a7b77 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -969,4 +969,10 @@ static inline int drm_mode_config_init(struct drm_device *dev)
>   void drm_mode_config_reset(struct drm_device *dev);
>   void drm_mode_config_cleanup(struct drm_device *dev);
>   
> +#ifdef CONFIG_VGA_CONSOLE
> +extern bool vgacon_text_force(void);
> +#else
> +static inline bool vgacon_text_force(void) { return false; }
> +#endif
> +
>   #endif
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 20874db50bc8..d4dd8384898b 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -217,12 +217,6 @@ extern atomic_t ignore_console_lock_warning;
>   #define VESA_HSYNC_SUSPEND      2
>   #define VESA_POWERDOWN          3
>   
> -#ifdef CONFIG_VGA_CONSOLE
> -extern bool vgacon_text_force(void);
> -#else
> -static inline bool vgacon_text_force(void) { return false; }
> -#endif
> -
>   extern void console_init(void);
>   
>   /* For deferred console takeover */
> 

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

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Jani Nikula]

On Wed, 03 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
> The "nomodeset" kernel cmdline parameter is handled by the vgacon driver
> but the exported vgacon_text_force() symbol is only used by DRM drivers.
>
> It makes much more sense for the parameter logic to be in the subsystem
> of the drivers that are making use of it. Let's move that to DRM.
>
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  drivers/gpu/drm/Makefile                |  2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
>  drivers/gpu/drm/ast/ast_drv.c           |  1 -
>  drivers/gpu/drm/drm_nomodeset.c         | 26 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_module.c      |  2 --
>  drivers/gpu/drm/mgag200/mgag200_drv.c   |  1 -
>  drivers/gpu/drm/nouveau/nouveau_drm.c   |  1 -
>  drivers/gpu/drm/qxl/qxl_drv.c           |  1 -
>  drivers/gpu/drm/radeon/radeon_drv.c     |  1 -
>  drivers/gpu/drm/tiny/bochs.c            |  1 -
>  drivers/gpu/drm/tiny/cirrus.c           |  1 -
>  drivers/gpu/drm/vboxvideo/vbox_drv.c    |  1 -
>  drivers/gpu/drm/virtio/virtgpu_drv.c    |  1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  1 -
>  drivers/video/console/vgacon.c          | 21 --------------------
>  include/drm/drm_mode_config.h           |  6 ++++++
>  include/linux/console.h                 |  6 ------
>  17 files changed, 35 insertions(+), 41 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c41156deb5f..0e2d60ea93ca 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o drm_privacy_screen_x86.
>  
>  obj-$(CONFIG_DRM_DP_AUX_BUS) += drm_dp_aux_bus.o
>  
> +obj-y += drm_nomodeset.o

This is a subtle functional change. With this, you'll always have
__setup("nomodeset", text_mode) builtin and the parameter available. And
using nomodeset will print out the pr_warn() splat from text_mode(). But
removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
leads to vgacon_text_force() always returning false.

To not make functional changes, this should be:

obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o

Now, going with the cleanup in this series, maybe we should make the
functional change, and break the connection to CONFIG_VGA_CONSOLE
altogether, also in the header?

(Maybe we'll also need a proxy drm kconfig option to only have
drm_modeset.o builtin when CONFIG_DRM != n.)

> +
>  drm_cma_helper-y := drm_gem_cma_helper.o
>  obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index c718fb5f3f8a..2680a2aaa877 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -31,7 +31,6 @@
>  #include "amdgpu_drv.h"
>  
>  #include <drm/drm_pciids.h>
> -#include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/vga_switcheroo.h>
> @@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
>  	int r;
>  
>  	if (vgacon_text_force()) {
> -		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> +		DRM_ERROR("amdgpu kernel modesetting disabled.\n");
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 86d5cd7b6318..048be607b182 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -26,7 +26,6 @@
>   * Authors: Dave Airlie <airlied@redhat.com>
>   */
>  
> -#include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  
> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
> new file mode 100644
> index 000000000000..1ac9a8d5a8fe
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_nomodeset.c
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/module.h>
> +#include <linux/types.h>
> +
> +static bool vgacon_text_mode_force;
> +
> +bool vgacon_text_force(void)
> +{
> +	return vgacon_text_mode_force;
> +}
> +EXPORT_SYMBOL(vgacon_text_force);
> +
> +static int __init text_mode(char *str)
> +{
> +	vgacon_text_mode_force = true;
> +
> +	pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");
> +	pr_warn("Any video related functionality will be severely degraded, and you may not even be able to suspend the system properly\n");
> +	pr_warn("Unless you actually understand what nomodeset does, you should reboot without enabling it\n");
> +
> +	return 1;
> +}
> +
> +/* force text mode - used by kernel modesetting */
> +__setup("nomodeset", text_mode);
> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
> index c7507266aa83..14a59226519d 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -4,8 +4,6 @@
>   * Copyright © 2021 Intel Corporation
>   */
>  
> -#include <linux/console.h>
> -
>  #include "gem/i915_gem_context.h"
>  #include "gem/i915_gem_object.h"
>  #include "i915_active.h"
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 6b9243713b3c..685e766db6a4 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -6,7 +6,6 @@
>   *          Dave Airlie
>   */
>  
> -#include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/vmalloc.h>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 1f828c9f691c..029997f50d1a 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -22,7 +22,6 @@
>   * Authors: Ben Skeggs
>   */
>  
> -#include <linux/console.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index fc47b0deb021..3cd6bd9f059d 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -29,7 +29,6 @@
>  
>  #include "qxl_drv.h"
>  
> -#include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/vgaarb.h>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index b74cebca1f89..9b606c1b11ec 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -31,7 +31,6 @@
>  
>  
>  #include <linux/compat.h>
> -#include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/vga_switcheroo.h>
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 2ce3bd903b70..04333f78be55 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -1,6 +1,5 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  
> -#include <linux/console.h>
>  #include <linux/pci.h>
>  
>  #include <drm/drm_aperture.h>
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index 4611ec408506..8bd674f0d682 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -16,7 +16,6 @@
>   * Copyright 1999-2001 Jeff Garzik <jgarzik@pobox.com>
>   */
>  
> -#include <linux/console.h>
>  #include <linux/dma-buf-map.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index a6c81af37345..e6d983121d0b 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -7,7 +7,6 @@
>   *          Michael Thayer <michael.thayer@oracle.com,
>   *          Hans de Goede <hdegoede@redhat.com>
>   */
> -#include <linux/console.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/vt_kern.h>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 749db18dcfa2..cd4c170236f1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -27,7 +27,6 @@
>   */
>  
>  #include <linux/module.h>
> -#include <linux/console.h>
>  #include <linux/pci.h>
>  #include <linux/poll.h>
>  #include <linux/wait.h>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index ab9a1750e1df..fcc4b5a7f639 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -25,7 +25,6 @@
>   *
>   **************************************************************************/
>  
> -#include <linux/console.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgacon.c
> index ef9c57ce0906..d4320b147956 100644
> --- a/drivers/video/console/vgacon.c
> +++ b/drivers/video/console/vgacon.c
> @@ -97,30 +97,9 @@ static int 		vga_video_font_height;
>  static int 		vga_scan_lines		__read_mostly;
>  static unsigned int 	vga_rolled_over; /* last vc_origin offset before wrap */
>  
> -static bool vgacon_text_mode_force;
>  static bool vga_hardscroll_enabled;
>  static bool vga_hardscroll_user_enable = true;
>  
> -bool vgacon_text_force(void)
> -{
> -	return vgacon_text_mode_force;
> -}
> -EXPORT_SYMBOL(vgacon_text_force);
> -
> -static int __init text_mode(char *str)
> -{
> -	vgacon_text_mode_force = true;
> -
> -	pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");
> -	pr_warn("Any video related functionality will be severely degraded, and you may not even be able to suspend the system properly\n");
> -	pr_warn("Unless you actually understand what nomodeset does, you should reboot without enabling it\n");
> -
> -	return 1;
> -}
> -
> -/* force text mode - used by kernel modesetting */
> -__setup("nomodeset", text_mode);
> -
>  static int __init no_scroll(char *str)
>  {
>  	/*
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 48b7de80daf5..e1d2042a7b77 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -969,4 +969,10 @@ static inline int drm_mode_config_init(struct drm_device *dev)
>  void drm_mode_config_reset(struct drm_device *dev);
>  void drm_mode_config_cleanup(struct drm_device *dev);
>  
> +#ifdef CONFIG_VGA_CONSOLE
> +extern bool vgacon_text_force(void);
> +#else
> +static inline bool vgacon_text_force(void) { return false; }
> +#endif
> +

As said, maybe the CONFIG_VGA_CONSOLE ifdef should be dropped.

Also, this seems like a completely arbitrary choice of header to place
this.


BR,
Jani.


>  #endif
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 20874db50bc8..d4dd8384898b 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -217,12 +217,6 @@ extern atomic_t ignore_console_lock_warning;
>  #define VESA_HSYNC_SUSPEND      2
>  #define VESA_POWERDOWN          3
>  
> -#ifdef CONFIG_VGA_CONSOLE
> -extern bool vgacon_text_force(void);
> -#else
> -static inline bool vgacon_text_force(void) { return false; }
> -#endif
> -
>  extern void console_init(void);
>  
>  /* For deferred console takeover */

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 10079 bytes --]

Hi

Am 03.11.21 um 13:28 schrieb Javier Martinez Canillas:
> This function is used by some DRM drivers to determine if the "nomodeset"
> kernel command line parameter was set and prevent these drivers to probe.
> 
> But the function name is quite confusing and does not reflect what really
> the drivers are testing when calling it. Use a better naming now that it
> is part of the DRM subsystem.
> 
> Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
> so there is no need to do the same when calling the function.
> 
> Suggested-by: Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
>   drivers/gpu/drm/ast/ast_drv.c           |  2 +-
>   drivers/gpu/drm/drm_nomodeset.c         | 16 ++++++++--------
>   drivers/gpu/drm/i915/i915_module.c      |  2 +-
>   drivers/gpu/drm/mgag200/mgag200_drv.c   |  2 +-
>   drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
>   drivers/gpu/drm/qxl/qxl_drv.c           |  2 +-
>   drivers/gpu/drm/radeon/radeon_drv.c     |  2 +-
>   drivers/gpu/drm/tiny/bochs.c            |  2 +-
>   drivers/gpu/drm/tiny/cirrus.c           |  2 +-
>   drivers/gpu/drm/vboxvideo/vbox_drv.c    |  4 +---
>   drivers/gpu/drm/virtio/virtgpu_drv.c    |  2 +-
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  2 +-
>   include/drm/drm_mode_config.h           |  4 ++--
>   14 files changed, 22 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 2680a2aaa877..f7bd2616cf23 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
>   {
>   	int r;
>   
> -	if (vgacon_text_force()) {
> +	if (drm_modeset_disabled()) {
>   		DRM_ERROR("amdgpu kernel modesetting disabled.\n");

Please remove all such error messages from drivers. 
drm_modeset_disabled() should print a unified message instead.


>   		return -EINVAL;
>   	}
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 048be607b182..6706050414c3 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
>   
>   static int __init ast_init(void)
>   {
> -	if (vgacon_text_force() && ast_modeset == -1)
> +	if (drm_modeset_disabled() && ast_modeset == -1)
>   		return -EINVAL;
>   
>   	if (ast_modeset == 0)
> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
> index 1ac9a8d5a8fe..dfc8b30f0625 100644
> --- a/drivers/gpu/drm/drm_nomodeset.c
> +++ b/drivers/gpu/drm/drm_nomodeset.c
> @@ -3,17 +3,17 @@
>   #include <linux/module.h>
>   #include <linux/types.h>
>   
> -static bool vgacon_text_mode_force;
> +static bool drm_nomodeset;
>   
> -bool vgacon_text_force(void)
> +bool drm_modeset_disabled(void)

I suggest to rename this function to drm_check_modeset() and have it 
return a negative errno code on failure. This gives maximum flexibility 
and reduces errors in drivers. Right now the drivers return something 
like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.

>   {
> -	return vgacon_text_mode_force;
> +	return drm_nomodeset;
>   }
> -EXPORT_SYMBOL(vgacon_text_force);
> +EXPORT_SYMBOL(drm_modeset_disabled);
>   
> -static int __init text_mode(char *str)
> +static int __init disable_modeset(char *str)
>   {
> -	vgacon_text_mode_force = true;
> +	drm_nomodeset = true;
>   
>   	pr_warn("You have booted with nomodeset. This means your GPU drivers are DISABLED\n");
>   	pr_warn("Any video related functionality will be severely degraded, and you may not even be able to suspend the system properly\n");
> @@ -22,5 +22,5 @@ static int __init text_mode(char *str)
>   	return 1;
>   }
>   
> -/* force text mode - used by kernel modesetting */
> -__setup("nomodeset", text_mode);
> +/* Disable kernel modesetting */
> +__setup("nomodeset", disable_modeset);
> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
> index 14a59226519d..3e5531040e4d 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
>   	if (i915_modparams.modeset == 0)
>   		use_kms = false;
>   
> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
> +	if (drm_modeset_disabled() && i915_modparams.modeset == -1)
>   		use_kms = false;
>   
>   	if (!use_kms) {
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 685e766db6a4..7ee87564bade 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
>   
>   static int __init mgag200_init(void)
>   {
> -	if (vgacon_text_force() && mgag200_modeset == -1)
> +	if (drm_modeset_disabled() && mgag200_modeset == -1)
>   		return -EINVAL;
>   
>   	if (mgag200_modeset == 0)
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 029997f50d1a..903d0e626954 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1321,7 +1321,7 @@ nouveau_drm_init(void)
>   	nouveau_display_options();
>   
>   	if (nouveau_modeset == -1) {
> -		if (vgacon_text_force())
> +		if (drm_modeset_disabled())
>   			nouveau_modeset = 0;
>   	}
>   
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index 3cd6bd9f059d..e4ab16837fad 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -294,7 +294,7 @@ static struct drm_driver qxl_driver = {
>   
>   static int __init qxl_init(void)
>   {
> -	if (vgacon_text_force() && qxl_modeset == -1)
> +	if (drm_modeset_disabled() && qxl_modeset == -1)
>   		return -EINVAL;
>   
>   	if (qxl_modeset == 0)
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 9b606c1b11ec..36c8dac68cca 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -636,7 +636,7 @@ static struct pci_driver radeon_kms_pci_driver = {
>   
>   static int __init radeon_module_init(void)
>   {
> -	if (vgacon_text_force() && radeon_modeset == -1) {
> +	if (drm_modeset_disabled() && radeon_modeset == -1) {
>   		DRM_INFO("VGACON disable radeon kernel modesetting.\n");
>   		radeon_modeset = 0;
>   	}
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 04333f78be55..59189f7c1840 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -718,7 +718,7 @@ static struct pci_driver bochs_pci_driver = {
>   
>   static int __init bochs_init(void)
>   {
> -	if (vgacon_text_force() && bochs_modeset == -1)
> +	if (drm_modeset_disabled() && bochs_modeset == -1)
>   		return -EINVAL;
>   
>   	if (bochs_modeset == 0)
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index 8bd674f0d682..fcf98379c641 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -635,7 +635,7 @@ static struct pci_driver cirrus_pci_driver = {
>   
>   static int __init cirrus_init(void)
>   {
> -	if (vgacon_text_force())
> +	if (drm_modeset_disabled())
>   		return -EINVAL;
>   	return pci_register_driver(&cirrus_pci_driver);
>   }
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index e6d983121d0b..09356dbd69b2 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -192,10 +192,8 @@ static const struct drm_driver driver = {
>   
>   static int __init vbox_init(void)
>   {
> -#ifdef CONFIG_VGA_CONSOLE
> -	if (vgacon_text_force() && vbox_modeset == -1)
> +	if (drm_modeset_disabled() && vbox_modeset == -1)
>   		return -EINVAL;
> -#endif
>   
>   	if (vbox_modeset == 0)
>   		return -EINVAL;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index cd4c170236f1..d96797d70fae 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -103,7 +103,7 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
>   	struct drm_device *dev;
>   	int ret;
>   
> -	if (vgacon_text_force() && virtio_gpu_modeset == -1)
> +	if (drm_modeset_disabled() && virtio_gpu_modeset == -1)
>   		return -EINVAL;
>   
>   	if (virtio_gpu_modeset == 0)
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index fcc4b5a7f639..22dab9beea03 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1650,7 +1650,7 @@ static int __init vmwgfx_init(void)
>   {
>   	int ret;
>   
> -	if (vgacon_text_force())
> +	if (drm_modeset_disabled())
>   		return -EINVAL;
>   
>   	ret = pci_register_driver(&vmw_pci_driver);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e1d2042a7b77..a5a2dc02e892 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -970,9 +970,9 @@ void drm_mode_config_reset(struct drm_device *dev);
>   void drm_mode_config_cleanup(struct drm_device *dev);
>   
>   #ifdef CONFIG_VGA_CONSOLE
> -extern bool vgacon_text_force(void);
> +extern bool drm_modeset_disabled(void);
>   #else
> -static inline bool vgacon_text_force(void) { return false; }
> +static inline bool drm_modeset_disabled(void) { return false; }
>   #endif
>   
>   #endif
> 

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

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Thomas Zimmermann]

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

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization