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

[Thomas Zimmermann]

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@xxxxxxxx>
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> ---
>
>   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: OpenPGP_signature
Description: OpenPGP digital signature