There is a lot of historical baggage on this parameter. It is defined in
the vgacon driver as nomodeset, but its set function is called text_mode()
and the value 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 should 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.
This is a v4 of the patches, that address issues pointed out by Thomas
Zimmermann in v3: https://lkml.org/lkml/2021/11/8/384
Patch #1 and #2 are just trivial cleanups.
Patch #3 moves the nomodeset boot option to the DRM subsystem and renames
the variables and functions names.
Patch #4 removes the relationship between the nomodeset parameter and the
CONFIG_VGA_CONSOLE Kconfig symbol.
Patch #5 adds nomodeset to the kernel parameters documentation.
Patch #6 improves the message when nomodeset is enabled to make it more
accurate and less sensational.
Changes in v4:
- Don't mention the simpledrm driver and instead explain in high level
terms what the nomodeset option is about.
- Don't mention DRM drivers in the kernel message and instead explain
that only the system framebuffer will be available.
Changes in v3:
- Drop the drm_drv_enabled() function and just call to drm_get_modeset().
- Make drm_get_modeset() just a getter function and don't return an error.
- Move independent cleanups in drivers as separate preparatory patches.
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
- Squash patch to add drm_drv_enabled() and make drivers use it.
- Make the drivers changes before moving nomodeset logic to DRM.
- Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
- Remove debug and error messages in drivers.
Javier Martinez Canillas (6):
drm: Don't print messages if drivers are disabled due nomodeset
drm/vboxvideo: Drop CONFIG_VGA_CONSOLE guard to call
vgacon_text_force()
drm: Move nomodeset kernel parameter to the DRM subsystem
drm: Decouple nomodeset from CONFIG_VGA_CONSOLE
Documentation/admin-guide: Document nomodeset kernel parameter
drm: Make the nomodeset message less sensational
.../admin-guide/kernel-parameters.txt | 7 ++++++
drivers/gpu/drm/Kconfig | 6 +++++
drivers/gpu/drm/Makefile | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++---
drivers/gpu/drm/ast/ast_drv.c | 4 ++--
drivers/gpu/drm/drm_nomodeset.c | 24 +++++++++++++++++++
drivers/gpu/drm/i915/i915_module.c | 4 ++--
drivers/gpu/drm/mgag200/mgag200_drv.c | 4 ++--
drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++--
drivers/gpu/drm/qxl/qxl_drv.c | 4 ++--
drivers/gpu/drm/radeon/radeon_drv.c | 10 +++-----
drivers/gpu/drm/tiny/bochs.c | 4 ++--
drivers/gpu/drm/tiny/cirrus.c | 5 ++--
drivers/gpu/drm/vboxvideo/vbox_drv.c | 6 ++---
drivers/gpu/drm/virtio/virtgpu_drv.c | 4 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
drivers/video/console/vgacon.c | 21 ----------------
include/drm/drm_mode_config.h | 2 ++
include/linux/console.h | 6 -----
19 files changed, 67 insertions(+), 60 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
--
2.33.1
The nomodeset kernel parameter handler already prints a message that the
DRM drivers will be disabled, so there's no need for drivers to do that.
Suggested-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 +---
drivers/gpu/drm/radeon/radeon_drv.c | 8 ++------
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..289d04999ced 100644
--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2514,10 +2514,8 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force()) {
- DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+ if (vgacon_text_force())
return -EINVAL;
- }
r = amdgpu_sync_init();
if (r)
diff --git drivers/gpu/drm/radeon/radeon_drv.c drivers/gpu/drm/radeon/radeon_drv.c
index b74cebca1f89..380adc61e71c 100644
--- drivers/gpu/drm/radeon/radeon_drv.c
+++ drivers/gpu/drm/radeon/radeon_drv.c
@@ -637,15 +637,11 @@ static struct pci_driver radeon_kms_pci_driver = {
static int __init radeon_module_init(void)
{
- if (vgacon_text_force() && radeon_modeset == -1) {
- DRM_INFO("VGACON disable radeon kernel modesetting.\n");
+ if (vgacon_text_force() && radeon_modeset == -1)
radeon_modeset = 0;
- }
- if (radeon_modeset == 0) {
- DRM_ERROR("No UMS support in radeon module!\n");
+ if (radeon_modeset == 0)
return -EINVAL;
- }
DRM_INFO("radeon kernel modesetting enabled.\n");
radeon_register_atpx_handler();
--
2.33.1
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 the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename the function to drm_get_modeset() and
make it return true if modesetting is enabled and false otherwise. This
better reflects the condition that drivers are actually testing for.
Suggested-by: Daniel Vetter <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
---
(no changes since v3)
Changes in v3:
- Drop the drm_drv_enabled() function and just call to drm_get_modeset().
- Make drm_get_modeset() just a getter function and don't return an error.
- Move independent cleanups in drivers as separate preparatory patches.
Changes in v2:
- Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
- Squash patches to move nomodeset logic to DRM and do the renaming.
- Name the function drm_check_modeset() and make it return -ENODEV.
- Squash patch to add drm_drv_enabled() and make drivers use it.
- Make the drivers changes before moving nomodeset logic to DRM.
- Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
- Remove debug and error messages in drivers.
drivers/gpu/drm/Makefile | 2 ++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 4 ++--
drivers/gpu/drm/ast/ast_drv.c | 4 ++--
drivers/gpu/drm/drm_nomodeset.c | 26 +++++++++++++++++++++++++
drivers/gpu/drm/i915/i915_module.c | 4 ++--
drivers/gpu/drm/mgag200/mgag200_drv.c | 4 ++--
drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++--
drivers/gpu/drm/qxl/qxl_drv.c | 4 ++--
drivers/gpu/drm/radeon/radeon_drv.c | 4 ++--
drivers/gpu/drm/tiny/bochs.c | 4 ++--
drivers/gpu/drm/tiny/cirrus.c | 5 +++--
drivers/gpu/drm/vboxvideo/vbox_drv.c | 4 ++--
drivers/gpu/drm/virtio/virtgpu_drv.c | 4 ++--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
drivers/video/console/vgacon.c | 21 --------------------
include/drm/drm_mode_config.h | 6 ++++++
include/linux/console.h | 6 ------
17 files changed, 59 insertions(+), 51 deletions(-)
create mode 100644 drivers/gpu/drm/drm_nomodeset.c
diff --git drivers/gpu/drm/Makefile drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 100644
--- drivers/gpu/drm/Makefile
+++ 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-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 289d04999ced..0da7f0b14700 100644
--- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -28,10 +28,10 @@
#include <drm/drm_gem.h>
#include <drm/drm_vblank.h>
#include <drm/drm_managed.h>
+#include <drm/drm_mode_config.h>
#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>
@@ -2514,7 +2514,7 @@ static int __init amdgpu_init(void)
{
int r;
- if (vgacon_text_force())
+ if (!drm_get_modeset())
return -EINVAL;
r = amdgpu_sync_init();
diff --git drivers/gpu/drm/ast/ast_drv.c drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..e788deac15cb 100644
--- drivers/gpu/drm/ast/ast_drv.c
+++ drivers/gpu/drm/ast/ast_drv.c
@@ -26,7 +26,6 @@
* Authors: Dave Airlie <[email protected]>
*/
-#include <linux/console.h>
#include <linux/module.h>
#include <linux/pci.h>
@@ -35,6 +34,7 @@
#include <drm/drm_crtc_helper.h>
#include <drm/drm_drv.h>
#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_probe_helper.h>
#include "ast_drv.h"
@@ -233,7 +233,7 @@ static struct pci_driver ast_pci_driver = {
static int __init ast_init(void)
{
- if (vgacon_text_force() && ast_modeset == -1)
+ if (!drm_get_modeset() && ast_modeset == -1)
return -EINVAL;
if (ast_modeset == 0)
diff --git drivers/gpu/drm/drm_nomodeset.c drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index 000000000000..fc3acf3ab2e2
--- /dev/null
+++ 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 drm_nomodeset;
+
+bool drm_get_modeset(void)
+{
+ return !drm_nomodeset;
+}
+EXPORT_SYMBOL(drm_get_modeset);
+
+static int __init disable_modeset(char *str)
+{
+ 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");
+ pr_warn("Unless you actually understand what nomodeset does, you should reboot without enabling it\n");
+
+ return 1;
+}
+
+/* Disable kernel modesetting */
+__setup("nomodeset", disable_modeset);
diff --git drivers/gpu/drm/i915/i915_module.c drivers/gpu/drm/i915/i915_module.c
index ab2295dd4500..f952e8b69465 100644
--- drivers/gpu/drm/i915/i915_module.c
+++ drivers/gpu/drm/i915/i915_module.c
@@ -4,7 +4,7 @@
* Copyright © 2021 Intel Corporation
*/
-#include <linux/console.h>
+#include <drm/drm_mode_config.h>
#include "gem/i915_gem_context.h"
#include "gem/i915_gem_object.h"
@@ -31,7 +31,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_get_modeset() && i915_modparams.modeset == -1)
use_kms = false;
if (!use_kms) {
diff --git drivers/gpu/drm/mgag200/mgag200_drv.c drivers/gpu/drm/mgag200/mgag200_drv.c
index 6b9243713b3c..3df5a608d0e7 100644
--- drivers/gpu/drm/mgag200/mgag200_drv.c
+++ 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>
@@ -15,6 +14,7 @@
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
#include <drm/drm_ioctl.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_pciids.h>
#include "mgag200_drv.h"
@@ -378,7 +378,7 @@ static struct pci_driver mgag200_pci_driver = {
static int __init mgag200_init(void)
{
- if (vgacon_text_force() && mgag200_modeset == -1)
+ if (!drm_get_modeset() && mgag200_modeset == -1)
return -EINVAL;
if (mgag200_modeset == 0)
diff --git drivers/gpu/drm/nouveau/nouveau_drm.c drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..d42e07de6460 100644
--- drivers/gpu/drm/nouveau/nouveau_drm.c
+++ 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>
@@ -34,6 +33,7 @@
#include <drm/drm_crtc_helper.h>
#include <drm/drm_gem_ttm_helper.h>
#include <drm/drm_ioctl.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_vblank.h>
#include <core/gpuobj.h>
@@ -1322,7 +1322,7 @@ nouveau_drm_init(void)
nouveau_display_options();
if (nouveau_modeset == -1) {
- if (vgacon_text_force())
+ if (!drm_get_modeset())
nouveau_modeset = 0;
}
diff --git drivers/gpu/drm/qxl/qxl_drv.c drivers/gpu/drm/qxl/qxl_drv.c
index fc47b0deb021..2d87a95ae431 100644
--- drivers/gpu/drm/qxl/qxl_drv.c
+++ 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>
@@ -40,6 +39,7 @@
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
#include <drm/drm_gem_ttm_helper.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_modeset_helper.h>
#include <drm/drm_prime.h>
#include <drm/drm_probe_helper.h>
@@ -295,7 +295,7 @@ static struct drm_driver qxl_driver = {
static int __init qxl_init(void)
{
- if (vgacon_text_force() && qxl_modeset == -1)
+ if (!drm_get_modeset() && qxl_modeset == -1)
return -EINVAL;
if (qxl_modeset == 0)
diff --git drivers/gpu/drm/radeon/radeon_drv.c drivers/gpu/drm/radeon/radeon_drv.c
index 380adc61e71c..802a28d55d0a 100644
--- drivers/gpu/drm/radeon/radeon_drv.c
+++ 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>
@@ -45,6 +44,7 @@
#include <drm/drm_file.h>
#include <drm/drm_gem.h>
#include <drm/drm_ioctl.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_pciids.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_vblank.h>
@@ -637,7 +637,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_get_modeset() && radeon_modeset == -1)
radeon_modeset = 0;
if (radeon_modeset == 0)
diff --git drivers/gpu/drm/tiny/bochs.c drivers/gpu/drm/tiny/bochs.c
index 2ce3bd903b70..500101f8a6e9 100644
--- drivers/gpu/drm/tiny/bochs.c
+++ 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>
@@ -11,6 +10,7 @@
#include <drm/drm_gem_framebuffer_helper.h>
#include <drm/drm_gem_vram_helper.h>
#include <drm/drm_managed.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
@@ -719,7 +719,7 @@ static struct pci_driver bochs_pci_driver = {
static int __init bochs_init(void)
{
- if (vgacon_text_force() && bochs_modeset == -1)
+ if (!drm_get_modeset() && bochs_modeset == -1)
return -EINVAL;
if (bochs_modeset == 0)
diff --git drivers/gpu/drm/tiny/cirrus.c drivers/gpu/drm/tiny/cirrus.c
index 4611ec408506..c94a030b6b3b 100644
--- drivers/gpu/drm/tiny/cirrus.c
+++ drivers/gpu/drm/tiny/cirrus.c
@@ -16,7 +16,6 @@
* Copyright 1999-2001 Jeff Garzik <[email protected]>
*/
-#include <linux/console.h>
#include <linux/dma-buf-map.h>
#include <linux/module.h>
#include <linux/pci.h>
@@ -39,6 +38,7 @@
#include <drm/drm_gem_shmem_helper.h>
#include <drm/drm_ioctl.h>
#include <drm/drm_managed.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_probe_helper.h>
#include <drm/drm_simple_kms_helper.h>
@@ -636,8 +636,9 @@ static struct pci_driver cirrus_pci_driver = {
static int __init cirrus_init(void)
{
- if (vgacon_text_force())
+ if (!drm_get_modeset())
return -EINVAL;
+
return pci_register_driver(&cirrus_pci_driver);
}
diff --git drivers/gpu/drm/vboxvideo/vbox_drv.c drivers/gpu/drm/vboxvideo/vbox_drv.c
index fd7abb029c65..74b8c93a664f 100644
--- drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -7,7 +7,6 @@
* Michael Thayer <[email protected],
* Hans de Goede <[email protected]>
*/
-#include <linux/console.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/vt_kern.h>
@@ -19,6 +18,7 @@
#include <drm/drm_file.h>
#include <drm/drm_ioctl.h>
#include <drm/drm_managed.h>
+#include <drm/drm_mode_config.h>
#include "vbox_drv.h"
@@ -193,7 +193,7 @@ static const struct drm_driver driver = {
static int __init vbox_init(void)
{
- if (vgacon_text_force() && vbox_modeset == -1)
+ if (!drm_get_modeset() && vbox_modeset == -1)
return -EINVAL;
if (vbox_modeset == 0)
diff --git drivers/gpu/drm/virtio/virtgpu_drv.c drivers/gpu/drm/virtio/virtgpu_drv.c
index 749db18dcfa2..6ad20c225eed 100644
--- drivers/gpu/drm/virtio/virtgpu_drv.c
+++ 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>
@@ -37,6 +36,7 @@
#include <drm/drm_atomic_helper.h>
#include <drm/drm_drv.h>
#include <drm/drm_file.h>
+#include <drm/drm_mode_config.h>
#include "virtgpu_drv.h"
@@ -104,7 +104,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_get_modeset() && virtio_gpu_modeset == -1)
return -EINVAL;
if (virtio_gpu_modeset == 0)
diff --git drivers/gpu/drm/vmwgfx/vmwgfx_drv.c drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index ab9a1750e1df..049fd017dca5 100644
--- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ 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>
@@ -34,6 +33,7 @@
#include <drm/drm_aperture.h>
#include <drm/drm_drv.h>
#include <drm/drm_ioctl.h>
+#include <drm/drm_mode_config.h>
#include <drm/drm_sysfs.h>
#include <drm/ttm/ttm_bo_driver.h>
#include <drm/ttm/ttm_range_manager.h>
@@ -1651,7 +1651,7 @@ static int __init vmwgfx_init(void)
{
int ret;
- if (vgacon_text_force())
+ if (!drm_get_modeset())
return -EINVAL;
ret = pci_register_driver(&vmw_pci_driver);
diff --git drivers/video/console/vgacon.c drivers/video/console/vgacon.c
index ef9c57ce0906..d4320b147956 100644
--- drivers/video/console/vgacon.c
+++ 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 include/drm/drm_mode_config.h include/drm/drm_mode_config.h
index 48b7de80daf5..f4d407908348 100644
--- include/drm/drm_mode_config.h
+++ 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 drm_get_modeset(void);
+#else
+static inline bool drm_get_modeset(void) { return true; }
+#endif
+
#endif
diff --git include/linux/console.h include/linux/console.h
index 20874db50bc8..d4dd8384898b 100644
--- include/linux/console.h
+++ 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 */
--
2.33.1
This relationship was only for historical reasons and the nomodeset option
should be available even on platforms that don't enable CONFIG_VGA_CONSOLE.
Suggested-by: Thomas Zimmermann <[email protected]>
Signed-off-by: Javier Martinez Canillas <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
---
(no changes since v1)
drivers/gpu/drm/Kconfig | 6 ++++++
drivers/gpu/drm/Makefile | 2 +-
include/drm/drm_mode_config.h | 4 ----
3 files changed, 7 insertions(+), 5 deletions(-)
diff --git drivers/gpu/drm/Kconfig drivers/gpu/drm/Kconfig
index fb144617055b..483d534eb074 100644
--- drivers/gpu/drm/Kconfig
+++ drivers/gpu/drm/Kconfig
@@ -8,6 +8,7 @@
menuconfig DRM
tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
+ select DRM_NOMODESET
select DRM_PANEL_ORIENTATION_QUIRKS
select HDMI
select FB_CMDLINE
@@ -493,6 +494,11 @@ config DRM_EXPORT_FOR_TESTS
config DRM_PANEL_ORIENTATION_QUIRKS
tristate
+# Separate option because nomodeset parameter is global and expected built-in
+config DRM_NOMODESET
+ bool
+ default n
+
config DRM_LIB_RANDOM
bool
default n
diff --git drivers/gpu/drm/Makefile drivers/gpu/drm/Makefile
index c74810c285af..fa16d3e0bbdc 100644
--- drivers/gpu/drm/Makefile
+++ drivers/gpu/drm/Makefile
@@ -33,7 +33,7 @@ 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-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
+obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
drm_cma_helper-y := drm_gem_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git include/drm/drm_mode_config.h include/drm/drm_mode_config.h
index f4d407908348..4bb129040185 100644
--- include/drm/drm_mode_config.h
+++ include/drm/drm_mode_config.h
@@ -969,10 +969,6 @@ 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 drm_get_modeset(void);
-#else
-static inline bool drm_get_modeset(void) { return true; }
-#endif
#endif
--
2.33.1
Hi
Am 08.11.21 um 15:06 schrieb Javier Martinez Canillas:
> There is a lot of historical baggage on this parameter. It is defined in
> the vgacon driver as nomodeset, but its set function is called text_mode()
> and the value 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 should 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.
>
> This is a v4 of the patches, that address issues pointed out by Thomas
> Zimmermann in v3: https://lkml.org/lkml/2021/11/8/384
>
> Patch #1 and #2 are just trivial cleanups.
>
> Patch #3 moves the nomodeset boot option to the DRM subsystem and renames
> the variables and functions names.
>
> Patch #4 removes the relationship between the nomodeset parameter and the
> CONFIG_VGA_CONSOLE Kconfig symbol.
>
> Patch #5 adds nomodeset to the kernel parameters documentation.
>
> Patch #6 improves the message when nomodeset is enabled to make it more
> accurate and less sensational.
Acked-by: Thomas Zimmermann <[email protected]>
Best regards
Thomas
>
> Changes in v4:
> - Don't mention the simpledrm driver and instead explain in high level
> terms what the nomodeset option is about.
> - Don't mention DRM drivers in the kernel message and instead explain
> that only the system framebuffer will be available.
>
> Changes in v3:
> - Drop the drm_drv_enabled() function and just call to drm_get_modeset().
> - Make drm_get_modeset() just a getter function and don't return an error.
> - Move independent cleanups in drivers as separate preparatory patches.
>
> Changes in v2:
> - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
> - Squash patches to move nomodeset logic to DRM and do the renaming.
> - Name the function drm_check_modeset() and make it return -ENODEV.
> - Squash patch to add drm_drv_enabled() and make drivers use it.
> - Make the drivers changes before moving nomodeset logic to DRM.
> - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
> - Remove debug and error messages in drivers.
>
> Javier Martinez Canillas (6):
> drm: Don't print messages if drivers are disabled due nomodeset
> drm/vboxvideo: Drop CONFIG_VGA_CONSOLE guard to call
> vgacon_text_force()
> drm: Move nomodeset kernel parameter to the DRM subsystem
> drm: Decouple nomodeset from CONFIG_VGA_CONSOLE
> Documentation/admin-guide: Document nomodeset kernel parameter
> drm: Make the nomodeset message less sensational
>
> .../admin-guide/kernel-parameters.txt | 7 ++++++
> drivers/gpu/drm/Kconfig | 6 +++++
> drivers/gpu/drm/Makefile | 2 ++
> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++---
> drivers/gpu/drm/ast/ast_drv.c | 4 ++--
> drivers/gpu/drm/drm_nomodeset.c | 24 +++++++++++++++++++
> drivers/gpu/drm/i915/i915_module.c | 4 ++--
> drivers/gpu/drm/mgag200/mgag200_drv.c | 4 ++--
> drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++--
> drivers/gpu/drm/qxl/qxl_drv.c | 4 ++--
> drivers/gpu/drm/radeon/radeon_drv.c | 10 +++-----
> drivers/gpu/drm/tiny/bochs.c | 4 ++--
> drivers/gpu/drm/tiny/cirrus.c | 5 ++--
> drivers/gpu/drm/vboxvideo/vbox_drv.c | 6 ++---
> drivers/gpu/drm/virtio/virtgpu_drv.c | 4 ++--
> drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
> drivers/video/console/vgacon.c | 21 ----------------
> include/drm/drm_mode_config.h | 2 ++
> include/linux/console.h | 6 -----
> 19 files changed, 67 insertions(+), 60 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
On Mon, 8 Nov 2021 15:17:13 +0100
Thomas Zimmermann <[email protected]> wrote:
> Hi
>
> Am 08.11.21 um 15:06 schrieb Javier Martinez Canillas:
> > There is a lot of historical baggage on this parameter. It is defined in
> > the vgacon driver as nomodeset, but its set function is called text_mode()
> > and the value 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 should 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.
> >
> > This is a v4 of the patches, that address issues pointed out by Thomas
> > Zimmermann in v3: https://lkml.org/lkml/2021/11/8/384
> >
> > Patch #1 and #2 are just trivial cleanups.
> >
> > Patch #3 moves the nomodeset boot option to the DRM subsystem and renames
> > the variables and functions names.
> >
> > Patch #4 removes the relationship between the nomodeset parameter and the
> > CONFIG_VGA_CONSOLE Kconfig symbol.
> >
> > Patch #5 adds nomodeset to the kernel parameters documentation.
> >
> > Patch #6 improves the message when nomodeset is enabled to make it more
> > accurate and less sensational.
>
> Acked-by: Thomas Zimmermann <[email protected]>
Hi,
these ideas make sense to me, so FWIW,
Acked-by: Pekka Paalanen <[email protected]>
There is one nitpick I'd like to ask about:
+bool drm_get_modeset(void)
+{
+ return !drm_nomodeset;
+}
+EXPORT_SYMBOL(drm_get_modeset);
Doesn't "get" have a special meaning in the kernel land, like "take a
strong reference on an object and return it"?
As this is just returning bool without changing anything, the usual
word to use is "is". Since this function is also used at most once per
driver, which is rarely, it could have a long and descriptive name.
For example:
bool drm_is_modeset_driver_allowed(void);
- "drm" is the namespace
- "is" implies it is a read-only boolean inspection
- "modeset" is the feature being checked
- "driver" implies it is supposed gate driver loading or
initialization, rather than modesets after drivers have loaded
- "allowed" says it is asking about general policy rather than what a
driver does
Just a bikeshed, I'll leave it to actual kernel devs to say if this
would be more appropriate or worth it.
Thanks,
pq
>
> Best regards
> Thomas
>
> >
> > Changes in v4:
> > - Don't mention the simpledrm driver and instead explain in high level
> > terms what the nomodeset option is about.
> > - Don't mention DRM drivers in the kernel message and instead explain
> > that only the system framebuffer will be available.
> >
> > Changes in v3:
> > - Drop the drm_drv_enabled() function and just call to drm_get_modeset().
> > - Make drm_get_modeset() just a getter function and don't return an error.
> > - Move independent cleanups in drivers as separate preparatory patches.
> >
> > Changes in v2:
> > - Conditionally build drm_nomodeset.o if CONFIG_VGA_CONSOLE is set.
> > - Squash patches to move nomodeset logic to DRM and do the renaming.
> > - Name the function drm_check_modeset() and make it return -ENODEV.
> > - Squash patch to add drm_drv_enabled() and make drivers use it.
> > - Make the drivers changes before moving nomodeset logic to DRM.
> > - Make drm_drv_enabled() return an errno and -ENODEV if nomodeset.
> > - Remove debug and error messages in drivers.
> >
> > Javier Martinez Canillas (6):
> > drm: Don't print messages if drivers are disabled due nomodeset
> > drm/vboxvideo: Drop CONFIG_VGA_CONSOLE guard to call
> > vgacon_text_force()
> > drm: Move nomodeset kernel parameter to the DRM subsystem
> > drm: Decouple nomodeset from CONFIG_VGA_CONSOLE
> > Documentation/admin-guide: Document nomodeset kernel parameter
> > drm: Make the nomodeset message less sensational
> >
> > .../admin-guide/kernel-parameters.txt | 7 ++++++
> > drivers/gpu/drm/Kconfig | 6 +++++
> > drivers/gpu/drm/Makefile | 2 ++
> > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 6 ++---
> > drivers/gpu/drm/ast/ast_drv.c | 4 ++--
> > drivers/gpu/drm/drm_nomodeset.c | 24 +++++++++++++++++++
> > drivers/gpu/drm/i915/i915_module.c | 4 ++--
> > drivers/gpu/drm/mgag200/mgag200_drv.c | 4 ++--
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 4 ++--
> > drivers/gpu/drm/qxl/qxl_drv.c | 4 ++--
> > drivers/gpu/drm/radeon/radeon_drv.c | 10 +++-----
> > drivers/gpu/drm/tiny/bochs.c | 4 ++--
> > drivers/gpu/drm/tiny/cirrus.c | 5 ++--
> > drivers/gpu/drm/vboxvideo/vbox_drv.c | 6 ++---
> > drivers/gpu/drm/virtio/virtgpu_drv.c | 4 ++--
> > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 ++--
> > drivers/video/console/vgacon.c | 21 ----------------
> > include/drm/drm_mode_config.h | 2 ++
> > include/linux/console.h | 6 -----
> > 19 files changed, 67 insertions(+), 60 deletions(-)
> > create mode 100644 drivers/gpu/drm/drm_nomodeset.c
> >
>
Hello Pekka,
On 11/12/21 09:56, Pekka Paalanen wrote:
[snip]
>
> Hi,
>
> these ideas make sense to me, so FWIW,
>
> Acked-by: Pekka Paalanen <[email protected]>
>
Thanks.
> There is one nitpick I'd like to ask about:
>
> +bool drm_get_modeset(void)
> +{
> + return !drm_nomodeset;
> +}
> +EXPORT_SYMBOL(drm_get_modeset);
>
> Doesn't "get" have a special meaning in the kernel land, like "take a
> strong reference on an object and return it"?
That's a very good point.
> As this is just returning bool without changing anything, the usual
> word to use is "is". Since this function is also used at most once per
> driver, which is rarely, it could have a long and descriptive name.
>
> For example:
>
> bool drm_is_modeset_driver_allowed(void);
>
Yeah, naming is hard. Jani also mentioned that he didn't like this
function name, so I don't mind to re-spin the series only for that.
> - "drm" is the namespace
> - "is" implies it is a read-only boolean inspection
> - "modeset" is the feature being checked
> - "driver" implies it is supposed gate driver loading or
> initialization, rather than modesets after drivers have loaded
> - "allowed" says it is asking about general policy rather than what a
> driver does
>
I believe that name is more verbose than needed. But don't have a
strong opinion and could use it if others agree.
> Just a bikeshed, I'll leave it to actual kernel devs to say if this
> would be more appropriate or worth it.
>
I think is worth it and better to do it now before the patches land, but
we could wait for others to chime in.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
Hi
Am 12.11.21 um 10:39 schrieb Javier Martinez Canillas:
> Hello Pekka,
>
> On 11/12/21 09:56, Pekka Paalanen wrote:
>
> [snip]
>
>>
>> Hi,
>>
>> these ideas make sense to me, so FWIW,
>>
>> Acked-by: Pekka Paalanen <[email protected]>
>>
>
> Thanks.
>
>> There is one nitpick I'd like to ask about:
>>
>> +bool drm_get_modeset(void)
>> +{
>> + return !drm_nomodeset;
>> +}
>> +EXPORT_SYMBOL(drm_get_modeset);
>>
>> Doesn't "get" have a special meaning in the kernel land, like "take a
>> strong reference on an object and return it"?
>
> That's a very good point.
>
>> As this is just returning bool without changing anything, the usual
>> word to use is "is". Since this function is also used at most once per
>> driver, which is rarely, it could have a long and descriptive name.
>>
>> For example:
>>
>> bool drm_is_modeset_driver_allowed(void);
I'd nominate
bool drm_native_drivers_enabled()
This is what HW-specific drivers want to query in their init/probing
code. The actual semantics of this decision is hidden from the driver.
It's also easier to read than the other name IMHO
Best regards
Thomas
>>
>
> Yeah, naming is hard. Jani also mentioned that he didn't like this
> function name, so I don't mind to re-spin the series only for that.
>
>> - "drm" is the namespace
>> - "is" implies it is a read-only boolean inspection
>> - "modeset" is the feature being checked
>> - "driver" implies it is supposed gate driver loading or
>> initialization, rather than modesets after drivers have loaded
>> - "allowed" says it is asking about general policy rather than what a
>> driver does
>>
>
> I believe that name is more verbose than needed. But don't have a
> strong opinion and could use it if others agree.
>
>> Just a bikeshed, I'll leave it to actual kernel devs to say if this
>> would be more appropriate or worth it.
>>
>
> I think is worth it and better to do it now before the patches land, but
> we could wait for others to chime in.
>
> Best regards,
> --
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>
--
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
On Fri, 12 Nov 2021 11:09:13 +0100
Thomas Zimmermann <[email protected]> wrote:
> Hi
>
> Am 12.11.21 um 10:39 schrieb Javier Martinez Canillas:
> > Hello Pekka,
> >
> > On 11/12/21 09:56, Pekka Paalanen wrote:
> >
> > [snip]
> >
> >>
> >> Hi,
> >>
> >> these ideas make sense to me, so FWIW,
> >>
> >> Acked-by: Pekka Paalanen <[email protected]>
> >>
> >
> > Thanks.
> >
> >> There is one nitpick I'd like to ask about:
> >>
> >> +bool drm_get_modeset(void)
> >> +{
> >> + return !drm_nomodeset;
> >> +}
> >> +EXPORT_SYMBOL(drm_get_modeset);
> >>
> >> Doesn't "get" have a special meaning in the kernel land, like "take a
> >> strong reference on an object and return it"?
> >
> > That's a very good point.
> >
> >> As this is just returning bool without changing anything, the usual
> >> word to use is "is". Since this function is also used at most once per
> >> driver, which is rarely, it could have a long and descriptive name.
> >>
> >> For example:
> >>
> >> bool drm_is_modeset_driver_allowed(void);
>
> I'd nominate
>
> bool drm_native_drivers_enabled()
>
> This is what HW-specific drivers want to query in their init/probing
> code. The actual semantics of this decision is hidden from the driver.
> It's also easier to read than the other name IMHO
Ok, but what is a "native driver"? Or a "non-native driver"?
Is that established kernel terminology?
I'd think a non-native driver is something that e.g. ndiswrapper is
loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
the driver that would not consult this function, right?
Thanks,
pq
>
> Best regards
> Thomas
>
> >>
> >
> > Yeah, naming is hard. Jani also mentioned that he didn't like this
> > function name, so I don't mind to re-spin the series only for that.
> >
> >> - "drm" is the namespace
> >> - "is" implies it is a read-only boolean inspection
> >> - "modeset" is the feature being checked
> >> - "driver" implies it is supposed gate driver loading or
> >> initialization, rather than modesets after drivers have loaded
> >> - "allowed" says it is asking about general policy rather than what a
> >> driver does
> >>
> >
> > I believe that name is more verbose than needed. But don't have a
> > strong opinion and could use it if others agree.
> >
> >> Just a bikeshed, I'll leave it to actual kernel devs to say if this
> >> would be more appropriate or worth it.
> >>
> >
> > I think is worth it and better to do it now before the patches land, but
> > we could wait for others to chime in.
> >
> > Best regards,
> > --
> > Javier Martinez Canillas
> > Linux Engineering
> > Red Hat
> >
>
On 11/12/21 11:22, Pekka Paalanen wrote:
[snip]
>>>
>>>> As this is just returning bool without changing anything, the usual
>>>> word to use is "is". Since this function is also used at most once per
>>>> driver, which is rarely, it could have a long and descriptive name.
>>>>
>>>> For example:
>>>>
>>>> bool drm_is_modeset_driver_allowed(void);
>>
>> I'd nominate
>>
>> bool drm_native_drivers_enabled()
>>
>> This is what HW-specific drivers want to query in their init/probing
>> code. The actual semantics of this decision is hidden from the driver.
>> It's also easier to read than the other name IMHO
>
> Ok, but what is a "native driver"? Or a "non-native driver"?
> Is that established kernel terminology?
>
For me the term "native" is also vague. I would prefer to call it platform
specific driver or non-generic driver instead. A problem is that "platform
driver" has a very specific meaning in the kernel, which are drivers for
devices in the "platform" bus (which is also a very overloaded term).
> I'd think a non-native driver is something that e.g. ndiswrapper is
Yeah, that's why I think that "generic" and "non-generic" is a better way
to describe the drivers that could be used for any platform as long as the
hardware was already initialized and a struct screen_info filled with data.
> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
> the driver that would not consult this function, right?
>
Correct.
Or maybe just 'bool drm_modeset_enabled()' ? After all, that's really what
the "nomodeset" kernel param disables. The fact that DRM drivers abuse that
boolean semantics to also prevent the drivers to load is a different topic.
>
> Thanks,
> pq
>
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
On Fri, 12 Nov 2021, Pekka Paalanen <[email protected]> wrote:
> On Fri, 12 Nov 2021 11:09:13 +0100
> Thomas Zimmermann <[email protected]> wrote:
>
>> Hi
>>
>> Am 12.11.21 um 10:39 schrieb Javier Martinez Canillas:
>> > Hello Pekka,
>> >
>> > On 11/12/21 09:56, Pekka Paalanen wrote:
>> >
>> > [snip]
>> >
>> >>
>> >> Hi,
>> >>
>> >> these ideas make sense to me, so FWIW,
>> >>
>> >> Acked-by: Pekka Paalanen <[email protected]>
>> >>
>> >
>> > Thanks.
>> >
>> >> There is one nitpick I'd like to ask about:
>> >>
>> >> +bool drm_get_modeset(void)
>> >> +{
>> >> + return !drm_nomodeset;
>> >> +}
>> >> +EXPORT_SYMBOL(drm_get_modeset);
>> >>
>> >> Doesn't "get" have a special meaning in the kernel land, like "take a
>> >> strong reference on an object and return it"?
>> >
>> > That's a very good point.
>> >
>> >> As this is just returning bool without changing anything, the usual
>> >> word to use is "is". Since this function is also used at most once per
>> >> driver, which is rarely, it could have a long and descriptive name.
>> >>
>> >> For example:
>> >>
>> >> bool drm_is_modeset_driver_allowed(void);
>>
>> I'd nominate
>>
>> bool drm_native_drivers_enabled()
>>
>> This is what HW-specific drivers want to query in their init/probing
>> code. The actual semantics of this decision is hidden from the driver.
>> It's also easier to read than the other name IMHO
>
> Ok, but what is a "native driver"? Or a "non-native driver"?
> Is that established kernel terminology?
FWIW, it doesn't mean anything to me.
drm_modeset_enabled()?
*sigh* I worked so hard not to participate in this bikeshed. ;)
BR,
Jani.
>
> I'd think a non-native driver is something that e.g. ndiswrapper is
> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
> the driver that would not consult this function, right?
>
>
> Thanks,
> pq
>
>>
>> Best regards
>> Thomas
>>
>> >>
>> >
>> > Yeah, naming is hard. Jani also mentioned that he didn't like this
>> > function name, so I don't mind to re-spin the series only for that.
>> >
>> >> - "drm" is the namespace
>> >> - "is" implies it is a read-only boolean inspection
>> >> - "modeset" is the feature being checked
>> >> - "driver" implies it is supposed gate driver loading or
>> >> initialization, rather than modesets after drivers have loaded
>> >> - "allowed" says it is asking about general policy rather than what a
>> >> driver does
>> >>
>> >
>> > I believe that name is more verbose than needed. But don't have a
>> > strong opinion and could use it if others agree.
>> >
>> >> Just a bikeshed, I'll leave it to actual kernel devs to say if this
>> >> would be more appropriate or worth it.
>> >>
>> >
>> > I think is worth it and better to do it now before the patches land, but
>> > we could wait for others to chime in.
>> >
>> > Best regards,
>> > --
>> > Javier Martinez Canillas
>> > Linux Engineering
>> > Red Hat
>> >
>>
>
--
Jani Nikula, Intel Open Source Graphics Center
Hi
Am 12.11.21 um 11:22 schrieb Pekka Paalanen:
> On Fri, 12 Nov 2021 11:09:13 +0100
> Thomas Zimmermann <[email protected]> wrote:
>
>> Hi
>>
>> Am 12.11.21 um 10:39 schrieb Javier Martinez Canillas:
>>> Hello Pekka,
>>>
>>> On 11/12/21 09:56, Pekka Paalanen wrote:
>>>
>>> [snip]
>>>
>>>>
>>>> Hi,
>>>>
>>>> these ideas make sense to me, so FWIW,
>>>>
>>>> Acked-by: Pekka Paalanen <[email protected]>
>>>>
>>>
>>> Thanks.
>>>
>>>> There is one nitpick I'd like to ask about:
>>>>
>>>> +bool drm_get_modeset(void)
>>>> +{
>>>> + return !drm_nomodeset;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_get_modeset);
>>>>
>>>> Doesn't "get" have a special meaning in the kernel land, like "take a
>>>> strong reference on an object and return it"?
>>>
>>> That's a very good point.
>>>
>>>> As this is just returning bool without changing anything, the usual
>>>> word to use is "is". Since this function is also used at most once per
>>>> driver, which is rarely, it could have a long and descriptive name.
>>>>
>>>> For example:
>>>>
>>>> bool drm_is_modeset_driver_allowed(void);
>>
>> I'd nominate
>>
>> bool drm_native_drivers_enabled()
>>
>> This is what HW-specific drivers want to query in their init/probing
>> code. The actual semantics of this decision is hidden from the driver.
>> It's also easier to read than the other name IMHO
>
> Ok, but what is a "native driver"? Or a "non-native driver"?
> Is that established kernel terminology?
>
> I'd think a non-native driver is something that e.g. ndiswrapper is
> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
> the driver that would not consult this function, right?
We use that term for hw-specific drivers. A 'non-native' driver would be
called generic or firmware driver.
My concern with the 'modeset' term is that it exposes an implementation
detail, which can mislead a driver to to the wrong thing: a HW-specifc
driver that disables it's modesetting functionality would pass the test
for (!modeset). But that's not what we want, we want to disable all of
the driver and not even load it.
How about we invert the test function and use something like
bool drm_firmware_drivers_only()
? HW-native drivers can do
if (drm_firmware_drivers_only())
return;
as early as possible. fbdev uses the flag FBINFO_MISC_FIRMWARE to mark
rsp drivers. So the terminology is there already.
Best regards
Thomas
>
>
> Thanks,
> pq
>
>>
>> Best regards
>> Thomas
>>
>>>>
>>>
>>> Yeah, naming is hard. Jani also mentioned that he didn't like this
>>> function name, so I don't mind to re-spin the series only for that.
>>>
>>>> - "drm" is the namespace
>>>> - "is" implies it is a read-only boolean inspection
>>>> - "modeset" is the feature being checked
>>>> - "driver" implies it is supposed gate driver loading or
>>>> initialization, rather than modesets after drivers have loaded
>>>> - "allowed" says it is asking about general policy rather than what a
>>>> driver does
>>>>
>>>
>>> I believe that name is more verbose than needed. But don't have a
>>> strong opinion and could use it if others agree.
>>>
>>>> Just a bikeshed, I'll leave it to actual kernel devs to say if this
>>>> would be more appropriate or worth it.
>>>>
>>>
>>> I think is worth it and better to do it now before the patches land, but
>>> we could wait for others to chime in.
>>>
>>> Best regards,
>>> --
>>> Javier Martinez Canillas
>>> Linux Engineering
>>> Red Hat
>>>
>>
>
--
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
On 11/12/21 11:57, Thomas Zimmermann wrote:
[snip]
>>>
>>> This is what HW-specific drivers want to query in their init/probing
>>> code. The actual semantics of this decision is hidden from the driver.
>>> It's also easier to read than the other name IMHO
>>
>> Ok, but what is a "native driver"? Or a "non-native driver"?
>> Is that established kernel terminology?
>>
>> I'd think a non-native driver is something that e.g. ndiswrapper is
>> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
>> the driver that would not consult this function, right?
>
> We use that term for hw-specific drivers. A 'non-native' driver would be
> called generic or firmware driver.
>
> My concern with the 'modeset' term is that it exposes an implementation
> detail, which can mislead a driver to to the wrong thing: a HW-specifc
> driver that disables it's modesetting functionality would pass the test
> for (!modeset). But that's not what we want, we want to disable all of
> the driver and not even load it.
>
> How about we invert the test function and use something like
>
> bool drm_firmware_drivers_only()
>
That name I think is more self explanatory, so it works for me.
There was also another bikeshed about where to put the function declaration,
I added to <drm/drm_mode_config.h> but with that name I believe that should
be in <drm/drm_drv.h> instead.
Best regards, --
Javier Martinez Canillas
Linux Engineering
Red Hat
Hi
Am 12.11.21 um 12:20 schrieb Javier Martinez Canillas:
> On 11/12/21 11:57, Thomas Zimmermann wrote:
>
> [snip]
>
>>>>
>>>> This is what HW-specific drivers want to query in their init/probing
>>>> code. The actual semantics of this decision is hidden from the driver.
>>>> It's also easier to read than the other name IMHO
>>>
>>> Ok, but what is a "native driver"? Or a "non-native driver"?
>>> Is that established kernel terminology?
>>>
>>> I'd think a non-native driver is something that e.g. ndiswrapper is
>>> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
>>> the driver that would not consult this function, right?
>>
>> We use that term for hw-specific drivers. A 'non-native' driver would be
>> called generic or firmware driver.
>>
>> My concern with the 'modeset' term is that it exposes an implementation
>> detail, which can mislead a driver to to the wrong thing: a HW-specifc
>> driver that disables it's modesetting functionality would pass the test
>> for (!modeset). But that's not what we want, we want to disable all of
>> the driver and not even load it.
>>
>> How about we invert the test function and use something like
>>
>> bool drm_firmware_drivers_only()
>>
>
> That name I think is more self explanatory, so it works for me.
>
> There was also another bikeshed about where to put the function declaration,
> I added to <drm/drm_mode_config.h> but with that name I believe that should
> be in <drm/drm_drv.h> instead.
I agree with drm_drv.h. It's a DRM-wide function and it fit's there
best, I'd say.
Best regards
Thomas
>
> Best regards, --
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>
--
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
On Fri, 12 Nov 2021 12:20:14 +0100
Javier Martinez Canillas <[email protected]> wrote:
> On 11/12/21 11:57, Thomas Zimmermann wrote:
>
> [snip]
>
> >>>
> >>> This is what HW-specific drivers want to query in their init/probing
> >>> code. The actual semantics of this decision is hidden from the driver.
> >>> It's also easier to read than the other name IMHO
> >>
> >> Ok, but what is a "native driver"? Or a "non-native driver"?
> >> Is that established kernel terminology?
> >>
> >> I'd think a non-native driver is something that e.g. ndiswrapper is
> >> loading. Is simpledrm like ndiswrapper in a sense? IIRC, simpledrm is
> >> the driver that would not consult this function, right?
> >
> > We use that term for hw-specific drivers. A 'non-native' driver would be
> > called generic or firmware driver.
> >
> > My concern with the 'modeset' term is that it exposes an implementation
> > detail, which can mislead a driver to to the wrong thing: a HW-specifc
> > driver that disables it's modesetting functionality would pass the test
> > for (!modeset). But that's not what we want, we want to disable all of
> > the driver and not even load it.
> >
> > How about we invert the test function and use something like
> >
> > bool drm_firmware_drivers_only()
> >
>
> That name I think is more self explanatory, so it works for me.
I'm not going to argue against that. :-)
Thanks,
pq