[Nouveau] [PATCH v2 0/2] Cleanups for the nomodeset kernel command line parameter logic

[Nouveau] [PATCH v2 0/2] Cleanups for the nomodeset kernel command line parameter logic

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

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.

This is a v2 of the patches, that address the issues pointed out by Thomas
Zimmermann and Jani Nikula in v1:

https://lore.kernel.org/lkml/5b4e4534-4786-d231-e331-78fdb5d8496a@redhat.com/T/

Patch #1 adds a drm_drv_enabled() function that could be used by drivers to
check if these could be enabled, instead of just using vgacon_text_force().

Patch #2 moves the nomodeset logic to the DRM subsystem and renames the
functions and variables to better explain what these actually do.

Changes in v2:
- 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.
- 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.

Javier Martinez Canillas (2):
  drm: Add a drm_drv_enabled() to check if drivers should be enabled
  drm: Move nomodeset kernel parameter to the DRM subsystem

 drivers/gpu/drm/Makefile                |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  8 +++-----
 drivers/gpu/drm/ast/ast_drv.c           |  8 +++++---
 drivers/gpu/drm/drm_drv.c               | 21 ++++++++++++++++++++
 drivers/gpu/drm/drm_nomodeset.c         | 26 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_module.c      |  8 +++++---
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  8 +++++---
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  6 ++++--
 drivers/gpu/drm/qxl/qxl_drv.c           |  8 +++++---
 drivers/gpu/drm/radeon/radeon_drv.c     |  7 ++++---
 drivers/gpu/drm/tiny/bochs.c            |  8 +++++---
 drivers/gpu/drm/tiny/cirrus.c           |  9 ++++++---
 drivers/gpu/drm/vboxvideo/vbox_drv.c    | 10 +++++-----
 drivers/gpu/drm/virtio/virtgpu_drv.c    |  6 +++---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  6 +++---
 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, 109 insertions(+), 66 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_nomodeset.c

-- 
2.33.1

[Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Javier Martinez Canillas]

Some DRM drivers check the vgacon_text_force() function return value as an
indication on whether they should be allowed to be enabled or not.

This function returns true if the nomodeset kernel command line parameter
was set. But there may be other conditions besides this to determine if a
driver should be enabled.

Let's add a drm_drv_enabled() helper function to encapsulate that logic so
can be later extended if needed, without having to modify all the drivers.

Also, while being there do some cleanup. The vgacon_text_force() function
is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.

Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- 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/amd/amdgpu/amdgpu_drv.c |  7 +++----
 drivers/gpu/drm/ast/ast_drv.c           |  7 +++++--
 drivers/gpu/drm/drm_drv.c               | 20 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_module.c      |  6 +++++-
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  7 +++++--
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  5 ++++-
 drivers/gpu/drm/qxl/qxl_drv.c           |  7 +++++--
 drivers/gpu/drm/radeon/radeon_drv.c     |  6 ++++--
 drivers/gpu/drm/tiny/bochs.c            |  7 +++++--
 drivers/gpu/drm/tiny/cirrus.c           |  8 ++++++--
 drivers/gpu/drm/vboxvideo/vbox_drv.c    |  9 +++++----
 drivers/gpu/drm/virtio/virtgpu_drv.c    |  5 +++--
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  5 +++--
 include/drm/drm_drv.h                   |  1 +
 14 files changed, 74 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index c718fb5f3f8a..7fde40d06181 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2514,10 +2514,9 @@ static int __init amdgpu_init(void)
 {
 	int r;
 
-	if (vgacon_text_force()) {
-		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
-		return -EINVAL;
-	}
+	r = drm_drv_enabled(&amdgpu_kms_driver)
+	if (r)
+		return r;
 
 	r = amdgpu_sync_init();
 	if (r)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..802063279b86 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -233,8 +233,11 @@ static struct pci_driver ast_pci_driver = {
 
 static int __init ast_init(void)
 {
-	if (vgacon_text_force() && ast_modeset == -1)
-		return -EINVAL;
+	int ret;
+
+	ret = drm_drv_enabled(&ast_driver);
+	if (ret && ast_modeset == -1)
+		return ret;
 
 	if (ast_modeset == 0)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..3fb567d62881 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
 }
 EXPORT_SYMBOL(drm_dev_set_unique);
 
+/**
+ * drm_drv_enabled - Checks if a DRM driver can be enabled
+ * @driver: DRM driver to check
+ *
+ * Checks whether a DRM driver can be enabled or not. This may be the case
+ * if the "nomodeset" kernel command line parameter is used.
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int drm_drv_enabled(const struct drm_driver *driver)
+{
+	if (vgacon_text_force()) {
+		DRM_INFO("%s driver is disabled\n", driver->name);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_drv_enabled);
+
 /*
  * DRM Core
  * The DRM core module initializes all global DRM objects and makes them
diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
index ab2295dd4500..45cb3e540eff 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -18,9 +18,12 @@
 #include "i915_selftest.h"
 #include "i915_vma.h"
 
+static const struct drm_driver driver;
+
 static int i915_check_nomodeset(void)
 {
 	bool use_kms = true;
+	int ret;
 
 	/*
 	 * Enable KMS by default, unless explicitly overriden by
@@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
 	if (i915_modparams.modeset == 0)
 		use_kms = false;
 
-	if (vgacon_text_force() && i915_modparams.modeset == -1)
+	ret = drm_drv_enabled(&driver);
+	if (ret && 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 6b9243713b3c..2a581094ba2b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -378,8 +378,11 @@ static struct pci_driver mgag200_pci_driver = {
 
 static int __init mgag200_init(void)
 {
-	if (vgacon_text_force() && mgag200_modeset == -1)
-		return -EINVAL;
+	int ret;
+
+	ret = drm_drv_enabled(&mgag200_driver);
+	if (ret && mgag200_modeset == -1)
+		return ret;
 
 	if (mgag200_modeset == 0)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 1f828c9f691c..8844d3602d87 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1316,13 +1316,16 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
 static int __init
 nouveau_drm_init(void)
 {
+	int ret;
+
 	driver_pci = driver_stub;
 	driver_platform = driver_stub;
 
 	nouveau_display_options();
 
 	if (nouveau_modeset == -1) {
-		if (vgacon_text_force())
+		ret = drm_drv_enabled(&driver_stub);
+		if (ret)
 			nouveau_modeset = 0;
 	}
 
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index fc47b0deb021..3ac2ef2bf545 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -295,8 +295,11 @@ static struct drm_driver qxl_driver = {
 
 static int __init qxl_init(void)
 {
-	if (vgacon_text_force() && qxl_modeset == -1)
-		return -EINVAL;
+	int ret;
+
+	ret = drm_drv_enabled(&qxl_driver);
+	if (ret && qxl_modeset == -1)
+		return ret;
 
 	if (qxl_modeset == 0)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index b74cebca1f89..56d688c04346 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -637,8 +637,10 @@ 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");
+	int ret;
+
+	ret = drm_drv_enabled(&kms_driver)
+	if (ret && radeon_modeset == -1) {
 		radeon_modeset = 0;
 	}
 
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 2ce3bd903b70..ee6b1ff9128b 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -719,8 +719,11 @@ static struct pci_driver bochs_pci_driver = {
 
 static int __init bochs_init(void)
 {
-	if (vgacon_text_force() && bochs_modeset == -1)
-		return -EINVAL;
+	int ret;
+
+	ret = drm_drv_enabled(&bochs_driver);
+	if (ret && bochs_modeset == -1)
+		return ret;
 
 	if (bochs_modeset == 0)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index 4611ec408506..4706c5bc3067 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -636,8 +636,12 @@ static struct pci_driver cirrus_pci_driver = {
 
 static int __init cirrus_init(void)
 {
-	if (vgacon_text_force())
-		return -EINVAL;
+	int ret;
+
+	ret = drm_drv_enabled(&cirrus_driver);
+	if (ret)
+		return ret;
+
 	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 a6c81af37345..e4377c37cf33 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -193,10 +193,11 @@ static const struct drm_driver driver = {
 
 static int __init vbox_init(void)
 {
-#ifdef CONFIG_VGA_CONSOLE
-	if (vgacon_text_force() && vbox_modeset == -1)
-		return -EINVAL;
-#endif
+	int ret;
+
+	ret = drm_drv_enabled(&driver);
+	if (ret && vbox_modeset == -1)
+		return ret;
 
 	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 749db18dcfa2..28200dfba2d1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -104,8 +104,9 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
 	struct drm_device *dev;
 	int ret;
 
-	if (vgacon_text_force() && virtio_gpu_modeset == -1)
-		return -EINVAL;
+	ret = drm_drv_enabled(&driver);
+	if (ret && virtio_gpu_modeset == -1)
+		return ret;
 
 	if (virtio_gpu_modeset == 0)
 		return -EINVAL;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index ab9a1750e1df..05e9949293d5 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -1651,8 +1651,9 @@ static int __init vmwgfx_init(void)
 {
 	int ret;
 
-	if (vgacon_text_force())
-		return -EINVAL;
+	ret = drm_drv_enabled(&driver);
+	if (ret)
+		return ret;
 
 	ret = pci_register_driver(&vmw_pci_driver);
 	if (ret)
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 0cd95953cdf5..77abfc7e078b 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -598,5 +598,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
 
 int drm_dev_set_unique(struct drm_device *dev, const char *name);
 
+int drm_drv_enabled(const struct drm_driver *driver);
 
 #endif
-- 
2.33.1

[Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[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 the vgacon_text_force() function and related logic to the DRM
subsystem. While doing that, rename the function to drm_check_modeset()
which better reflects what the function is really used to test for.

Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

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.

 drivers/gpu/drm/Makefile                |  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  1 -
 drivers/gpu/drm/ast/ast_drv.c           |  1 -
 drivers/gpu/drm/drm_drv.c               |  9 +++++----
 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 ------
 18 files changed, 39 insertions(+), 44 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_nomodeset.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..c74810c285af 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-$(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 a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 7fde40d06181..b4b6993861e6 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>
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 802063279b86..6222082c3082 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_drv.c b/drivers/gpu/drm/drm_drv.c
index 3fb567d62881..80b85b8ea776 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
  */
 int drm_drv_enabled(const struct drm_driver *driver)
 {
-	if (vgacon_text_force()) {
+	int ret;
+
+	ret = drm_check_modeset();
+	if (ret)
 		DRM_INFO("%s driver is disabled\n", driver->name);
-		return -ENODEV;
-	}
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(drm_drv_enabled);
 
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index 000000000000..6683e396d2c5
--- /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 drm_nomodeset;
+
+int drm_check_modeset(void)
+{
+	return drm_nomodeset ? -ENODEV : 0;
+}
+EXPORT_SYMBOL(drm_check_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 a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
index 45cb3e540eff..c890c1ca20c4 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 2a581094ba2b..8e000cac11ba 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 8844d3602d87..bd1456521b7c 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 3ac2ef2bf545..ff070ac76111 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 56d688c04346..f59cc971ec95 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 ee6b1ff9128b..6e9a31f1a0f3 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 4706c5bc3067..659208d5aef9 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 e4377c37cf33..b1e63fd543bb 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 28200dfba2d1..ba9c0c2f8ae6 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 05e9949293d5..115ec9518277 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..18982d3507e4 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 int drm_check_modeset(void);
+#else
+static inline int drm_check_modeset(void) { return 0; }
+#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 */
-- 
2.33.1

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Jani Nikula]

On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
> Some DRM drivers check the vgacon_text_force() function return value as an
> indication on whether they should be allowed to be enabled or not.
>
> This function returns true if the nomodeset kernel command line parameter
> was set. But there may be other conditions besides this to determine if a
> driver should be enabled.
>
> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
> can be later extended if needed, without having to modify all the drivers.
>
> Also, while being there do some cleanup. The vgacon_text_force() function
> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
>
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
> Changes in v2:
> - 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/amd/amdgpu/amdgpu_drv.c |  7 +++----
>  drivers/gpu/drm/ast/ast_drv.c           |  7 +++++--
>  drivers/gpu/drm/drm_drv.c               | 20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_module.c      |  6 +++++-
>  drivers/gpu/drm/mgag200/mgag200_drv.c   |  7 +++++--
>  drivers/gpu/drm/nouveau/nouveau_drm.c   |  5 ++++-
>  drivers/gpu/drm/qxl/qxl_drv.c           |  7 +++++--
>  drivers/gpu/drm/radeon/radeon_drv.c     |  6 ++++--
>  drivers/gpu/drm/tiny/bochs.c            |  7 +++++--
>  drivers/gpu/drm/tiny/cirrus.c           |  8 ++++++--
>  drivers/gpu/drm/vboxvideo/vbox_drv.c    |  9 +++++----
>  drivers/gpu/drm/virtio/virtgpu_drv.c    |  5 +++--
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c     |  5 +++--
>  include/drm/drm_drv.h                   |  1 +
>  14 files changed, 74 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index c718fb5f3f8a..7fde40d06181 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2514,10 +2514,9 @@ static int __init amdgpu_init(void)
>  {
>  	int r;
>  
> -	if (vgacon_text_force()) {
> -		DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
> -		return -EINVAL;
> -	}
> +	r = drm_drv_enabled(&amdgpu_kms_driver)
> +	if (r)
> +		return r;
>  
>  	r = amdgpu_sync_init();
>  	if (r)
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 86d5cd7b6318..802063279b86 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -233,8 +233,11 @@ static struct pci_driver ast_pci_driver = {
>  
>  static int __init ast_init(void)
>  {
> -	if (vgacon_text_force() && ast_modeset == -1)
> -		return -EINVAL;
> +	int ret;
> +
> +	ret = drm_drv_enabled(&ast_driver);
> +	if (ret && ast_modeset == -1)
> +		return ret;
>  
>  	if (ast_modeset == 0)
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..3fb567d62881 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
>  }
>  EXPORT_SYMBOL(drm_dev_set_unique);
>  
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
> +{
> +	if (vgacon_text_force()) {
> +		DRM_INFO("%s driver is disabled\n", driver->name);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
> +
>  /*
>   * DRM Core
>   * The DRM core module initializes all global DRM objects and makes them
> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
> index ab2295dd4500..45cb3e540eff 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -18,9 +18,12 @@
>  #include "i915_selftest.h"
>  #include "i915_vma.h"
>  
> +static const struct drm_driver driver;
> +

No, this makes absolutely no sense, and will also oops on nomodeset.

BR,
Jani.


>  static int i915_check_nomodeset(void)
>  {
>  	bool use_kms = true;
> +	int ret;
>  
>  	/*
>  	 * Enable KMS by default, unless explicitly overriden by
> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
>  	if (i915_modparams.modeset == 0)
>  		use_kms = false;
>  
> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
> +	ret = drm_drv_enabled(&driver);
> +	if (ret && 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 6b9243713b3c..2a581094ba2b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -378,8 +378,11 @@ static struct pci_driver mgag200_pci_driver = {
>  
>  static int __init mgag200_init(void)
>  {
> -	if (vgacon_text_force() && mgag200_modeset == -1)
> -		return -EINVAL;
> +	int ret;
> +
> +	ret = drm_drv_enabled(&mgag200_driver);
> +	if (ret && mgag200_modeset == -1)
> +		return ret;
>  
>  	if (mgag200_modeset == 0)
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 1f828c9f691c..8844d3602d87 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1316,13 +1316,16 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
>  static int __init
>  nouveau_drm_init(void)
>  {
> +	int ret;
> +
>  	driver_pci = driver_stub;
>  	driver_platform = driver_stub;
>  
>  	nouveau_display_options();
>  
>  	if (nouveau_modeset == -1) {
> -		if (vgacon_text_force())
> +		ret = drm_drv_enabled(&driver_stub);
> +		if (ret)
>  			nouveau_modeset = 0;
>  	}
>  
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
> index fc47b0deb021..3ac2ef2bf545 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.c
> +++ b/drivers/gpu/drm/qxl/qxl_drv.c
> @@ -295,8 +295,11 @@ static struct drm_driver qxl_driver = {
>  
>  static int __init qxl_init(void)
>  {
> -	if (vgacon_text_force() && qxl_modeset == -1)
> -		return -EINVAL;
> +	int ret;
> +
> +	ret = drm_drv_enabled(&qxl_driver);
> +	if (ret && qxl_modeset == -1)
> +		return ret;
>  
>  	if (qxl_modeset == 0)
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index b74cebca1f89..56d688c04346 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -637,8 +637,10 @@ 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");
> +	int ret;
> +
> +	ret = drm_drv_enabled(&kms_driver)
> +	if (ret && radeon_modeset == -1) {
>  		radeon_modeset = 0;
>  	}
>  
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 2ce3bd903b70..ee6b1ff9128b 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -719,8 +719,11 @@ static struct pci_driver bochs_pci_driver = {
>  
>  static int __init bochs_init(void)
>  {
> -	if (vgacon_text_force() && bochs_modeset == -1)
> -		return -EINVAL;
> +	int ret;
> +
> +	ret = drm_drv_enabled(&bochs_driver);
> +	if (ret && bochs_modeset == -1)
> +		return ret;
>  
>  	if (bochs_modeset == 0)
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
> index 4611ec408506..4706c5bc3067 100644
> --- a/drivers/gpu/drm/tiny/cirrus.c
> +++ b/drivers/gpu/drm/tiny/cirrus.c
> @@ -636,8 +636,12 @@ static struct pci_driver cirrus_pci_driver = {
>  
>  static int __init cirrus_init(void)
>  {
> -	if (vgacon_text_force())
> -		return -EINVAL;
> +	int ret;
> +
> +	ret = drm_drv_enabled(&cirrus_driver);
> +	if (ret)
> +		return ret;
> +
>  	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 a6c81af37345..e4377c37cf33 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -193,10 +193,11 @@ static const struct drm_driver driver = {
>  
>  static int __init vbox_init(void)
>  {
> -#ifdef CONFIG_VGA_CONSOLE
> -	if (vgacon_text_force() && vbox_modeset == -1)
> -		return -EINVAL;
> -#endif
> +	int ret;
> +
> +	ret = drm_drv_enabled(&driver);
> +	if (ret && vbox_modeset == -1)
> +		return ret;
>  
>  	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 749db18dcfa2..28200dfba2d1 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -104,8 +104,9 @@ static int virtio_gpu_probe(struct virtio_device *vdev)
>  	struct drm_device *dev;
>  	int ret;
>  
> -	if (vgacon_text_force() && virtio_gpu_modeset == -1)
> -		return -EINVAL;
> +	ret = drm_drv_enabled(&driver);
> +	if (ret && virtio_gpu_modeset == -1)
> +		return ret;
>  
>  	if (virtio_gpu_modeset == 0)
>  		return -EINVAL;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> index ab9a1750e1df..05e9949293d5 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> @@ -1651,8 +1651,9 @@ static int __init vmwgfx_init(void)
>  {
>  	int ret;
>  
> -	if (vgacon_text_force())
> -		return -EINVAL;
> +	ret = drm_drv_enabled(&driver);
> +	if (ret)
> +		return ret;
>  
>  	ret = pci_register_driver(&vmw_pci_driver);
>  	if (ret)
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 0cd95953cdf5..77abfc7e078b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -598,5 +598,6 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
>  
>  int drm_dev_set_unique(struct drm_device *dev, const char *name);
>  
> +int drm_drv_enabled(const struct drm_driver *driver);
>  
>  #endif

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Javier Martinez Canillas]

On 11/4/21 17:24, Jani Nikula wrote:

[snip]

>> index ab2295dd4500..45cb3e540eff 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -18,9 +18,12 @@
>>  #include "i915_selftest.h"
>>  #include "i915_vma.h"
>>  
>> +static const struct drm_driver driver;
>> +
> 
> No, this makes absolutely no sense, and will also oops on nomodeset.
>

Ups, sorry about that. For some reason I thought that it was defined in
the same compilation unit, but I noticed now that it is in i915_drv.c.
 
> BR,
> Jani.
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Sam Ravnborg]

Hi Javier,

On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
> Some DRM drivers check the vgacon_text_force() function return value as an
> indication on whether they should be allowed to be enabled or not.
> 
> This function returns true if the nomodeset kernel command line parameter
> was set. But there may be other conditions besides this to determine if a
> driver should be enabled.
> 
> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
> can be later extended if needed, without having to modify all the drivers.
> 
> Also, while being there do some cleanup. The vgacon_text_force() function
> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
> 
> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 8214a0b1ab7f..3fb567d62881 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
>  }
>  EXPORT_SYMBOL(drm_dev_set_unique);
>  
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
> +{
> +	if (vgacon_text_force()) {
> +		DRM_INFO("%s driver is disabled\n", driver->name);

DRM_INFO is deprecated, please do not use it in new code.
Also other users had an error message and not just info - is info
enough?


> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);
> +
>  /*
>   * DRM Core
>   * The DRM core module initializes all global DRM objects and makes them
> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
> index ab2295dd4500..45cb3e540eff 100644
> --- a/drivers/gpu/drm/i915/i915_module.c
> +++ b/drivers/gpu/drm/i915/i915_module.c
> @@ -18,9 +18,12 @@
>  #include "i915_selftest.h"
>  #include "i915_vma.h"
>  
> +static const struct drm_driver driver;
Hmmm...

> +
>  static int i915_check_nomodeset(void)
>  {
>  	bool use_kms = true;
> +	int ret;
>  
>  	/*
>  	 * Enable KMS by default, unless explicitly overriden by
> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
>  	if (i915_modparams.modeset == 0)
>  		use_kms = false;
>  
> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
> +	ret = drm_drv_enabled(&driver);

You pass the local driver variable here - which looks wrong as this is
not the same as the driver variable declared in another file.

Maybe move the check to new function you can add to init_funcs,
and locate the new function in i915_drv - so it has access to driver?


	Sam

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Jani Nikula]

On Thu, 04 Nov 2021, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Javier,
>
> On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
>> Some DRM drivers check the vgacon_text_force() function return value as an
>> indication on whether they should be allowed to be enabled or not.
>> 
>> This function returns true if the nomodeset kernel command line parameter
>> was set. But there may be other conditions besides this to determine if a
>> driver should be enabled.
>> 
>> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
>> can be later extended if needed, without having to modify all the drivers.
>> 
>> Also, while being there do some cleanup. The vgacon_text_force() function
>> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
>> 
>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>> 
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 8214a0b1ab7f..3fb567d62881 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
>>  }
>>  EXPORT_SYMBOL(drm_dev_set_unique);
>>  
>> +/**
>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>> + * @driver: DRM driver to check
>> + *
>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>> + * if the "nomodeset" kernel command line parameter is used.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int drm_drv_enabled(const struct drm_driver *driver)
>> +{
>> +	if (vgacon_text_force()) {
>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>
> DRM_INFO is deprecated, please do not use it in new code.
> Also other users had an error message and not just info - is info
> enough?
>
>
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_drv_enabled);
>> +
>>  /*
>>   * DRM Core
>>   * The DRM core module initializes all global DRM objects and makes them
>> diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
>> index ab2295dd4500..45cb3e540eff 100644
>> --- a/drivers/gpu/drm/i915/i915_module.c
>> +++ b/drivers/gpu/drm/i915/i915_module.c
>> @@ -18,9 +18,12 @@
>>  #include "i915_selftest.h"
>>  #include "i915_vma.h"
>>  
>> +static const struct drm_driver driver;
> Hmmm...
>
>> +
>>  static int i915_check_nomodeset(void)
>>  {
>>  	bool use_kms = true;
>> +	int ret;
>>  
>>  	/*
>>  	 * Enable KMS by default, unless explicitly overriden by
>> @@ -31,7 +34,8 @@ static int i915_check_nomodeset(void)
>>  	if (i915_modparams.modeset == 0)
>>  		use_kms = false;
>>  
>> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
>> +	ret = drm_drv_enabled(&driver);
>
> You pass the local driver variable here - which looks wrong as this is
> not the same as the driver variable declared in another file.

Indeed.

> Maybe move the check to new function you can add to init_funcs,
> and locate the new function in i915_drv - so it has access to driver?

We don't really want that, though. This check is pretty much as early as
it can be, and there's a ton of useless initialization that would happen
if we waited until drm_driver is available.

From my POV, drm_drv_enabled() is a solution that creates a worse
problem for us than it solves.


BR,
Jani.


>
>
> 	Sam

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Javier Martinez Canillas]

Hello Sam,

On 11/4/21 18:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Sam Ravnborg <sam@ravnborg.org> wrote:
>> Hi Javier,
>>
>> On Thu, Nov 04, 2021 at 05:07:06PM +0100, Javier Martinez Canillas wrote:
>>> Some DRM drivers check the vgacon_text_force() function return value as an
>>> indication on whether they should be allowed to be enabled or not.
>>>
>>> This function returns true if the nomodeset kernel command line parameter
>>> was set. But there may be other conditions besides this to determine if a
>>> driver should be enabled.
>>>
>>> Let's add a drm_drv_enabled() helper function to encapsulate that logic so
>>> can be later extended if needed, without having to modify all the drivers.
>>>
>>> Also, while being there do some cleanup. The vgacon_text_force() function
>>> is guarded by CONFIG_VGA_CONSOLE and there's no need for callers to do it.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> ---
>>>
>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 8214a0b1ab7f..3fb567d62881 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -975,6 +975,26 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name)
>>>  }
>>>  EXPORT_SYMBOL(drm_dev_set_unique);
>>>  
>>> +/**
>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>> + * @driver: DRM driver to check
>>> + *
>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>> + * if the "nomodeset" kernel command line parameter is used.
>>> + *
>>> + * Return: 0 on success or a negative error code on failure.
>>> + */
>>> +int drm_drv_enabled(const struct drm_driver *driver)
>>> +{
>>> +	if (vgacon_text_force()) {
>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>
>> DRM_INFO is deprecated, please do not use it in new code.
>> Also other users had an error message and not just info - is info
>> enough?
>>

Thanks, I didn't know that. Right, they had an error but I do wonder
if that was correct though. After all isn't an error but an explicit
disable due "nomodeset" being set in the kernel command line.

[snip]

>>>  
>>> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
>>> +	ret = drm_drv_enabled(&driver);
>>
>> You pass the local driver variable here - which looks wrong as this is
>> not the same as the driver variable declared in another file.
>

Yes, Jani mentioned it already. I got confused and thought that the driver
variable was also defined in the same compilation unit...

Maybe I could squash the following change?

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b18a250e5d2e..b8f399b76363 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -89,7 +89,7 @@
 #include "intel_region_ttm.h"
 #include "vlv_suspend.h"
 
-static const struct drm_driver driver;
+const struct drm_driver driver;
 
 static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
 {
@@ -1777,7 +1777,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
        DRM_IOCTL_DEF_DRV(I915_GEM_VM_DESTROY, i915_gem_vm_destroy_ioctl, DRM_RENDER_ALLOW),
 };
 
-static const struct drm_driver driver = {
+const struct drm_driver driver = {
        /* Don't use MTRRs here; the Xserver or userspace app should
         * deal with them for Intel hardware.
         */
diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c
index c890c1ca20c4..88f770920324 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -16,7 +16,7 @@
 #include "i915_selftest.h"
 #include "i915_vma.h"
 
-static const struct drm_driver driver;
+extern const struct drm_driver driver;
 
 static int i915_check_nomodeset(void)
 {

That should work and I actually got a laptop with an i915 and tested the change
both when CONFIG_DRM_I915=m and CONFIG_DRM_I915=y is set.

Another option is to declare it in i915_drv.h and not make the definition static.
 
> Indeed.
> 
>> Maybe move the check to new function you can add to init_funcs,
>> and locate the new function in i915_drv - so it has access to driver?
> 
> We don't really want that, though. This check is pretty much as early as
> it can be, and there's a ton of useless initialization that would happen
> if we waited until drm_driver is available.
>

Agreed.
 
> From my POV, drm_drv_enabled() is a solution that creates a worse
> problem for us than it solves.
>

I don't have a strong opinion on this. I could just do patch #2 without adding
a level of indirection through drm_drv_enabled(). But Thomas and Daniel Vetter
suggested that we should do this change before.

That is, the drivers could just check if should be enabled by calling to the
drm_check_modeset() function directly if people agree that encapsulating that
logic in a drm_drv_enabled() is not needed.

> 
> BR,
> Jani.
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Sam Ravnborg]

Hi Javier,

> 
> >>>  
> >>> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
> >>> +	ret = drm_drv_enabled(&driver);
> >>
> >> You pass the local driver variable here - which looks wrong as this is
> >> not the same as the driver variable declared in another file.
> >
> 
> Yes, Jani mentioned it already. I got confused and thought that the driver
> variable was also defined in the same compilation unit...
> 
> Maybe I could squash the following change?
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b18a250e5d2e..b8f399b76363 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -89,7 +89,7 @@
>  #include "intel_region_ttm.h"
>  #include "vlv_suspend.h"
>  
> -static const struct drm_driver driver;
> +const struct drm_driver driver;
No, variables with such a generic name will clash.

Just add a
const drm_driver * i915_drm_driver(void)
{
	return &driver;
}

And then use this function to access the drm_driver variable.

	Sam

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Jani Nikula]

On Thu, 04 Nov 2021, Sam Ravnborg <sam@ravnborg.org> wrote:
> Hi Javier,
>
>> 
>> >>>  
>> >>> -	if (vgacon_text_force() && i915_modparams.modeset == -1)
>> >>> +	ret = drm_drv_enabled(&driver);
>> >>
>> >> You pass the local driver variable here - which looks wrong as this is
>> >> not the same as the driver variable declared in another file.
>> >
>> 
>> Yes, Jani mentioned it already. I got confused and thought that the driver
>> variable was also defined in the same compilation unit...
>> 
>> Maybe I could squash the following change?
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index b18a250e5d2e..b8f399b76363 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -89,7 +89,7 @@
>>  #include "intel_region_ttm.h"
>>  #include "vlv_suspend.h"
>>  
>> -static const struct drm_driver driver;
>> +const struct drm_driver driver;
> No, variables with such a generic name will clash.
>
> Just add a
> const drm_driver * i915_drm_driver(void)
> {
> 	return &driver;
> }
>
> And then use this function to access the drm_driver variable.

Agreed on the general principle of exposing interfaces over data.

But... why? I'm still at a loss what problem this solves. We pass
&driver to exactly one place, devm_drm_dev_alloc(), and it's neatly
hidden away.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Jani Nikula]

On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
> +/**
> + * drm_drv_enabled - Checks if a DRM driver can be enabled
> + * @driver: DRM driver to check
> + *
> + * Checks whether a DRM driver can be enabled or not. This may be the case
> + * if the "nomodeset" kernel command line parameter is used.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int drm_drv_enabled(const struct drm_driver *driver)
> +{
> +	if (vgacon_text_force()) {
> +		DRM_INFO("%s driver is disabled\n", driver->name);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_drv_enabled);

The name implies a bool return, but it's not.

	if (drm_drv_enabled(...)) {
		/* surprise, it's disabled! */
	}


BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Javier Martinez Canillas]

Hello Jani,

On 11/4/21 20:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
>> +/**
>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>> + * @driver: DRM driver to check
>> + *
>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>> + * if the "nomodeset" kernel command line parameter is used.
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int drm_drv_enabled(const struct drm_driver *driver)
>> +{
>> +	if (vgacon_text_force()) {
>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>> +		return -ENODEV;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_drv_enabled);
> 
> The name implies a bool return, but it's not.
> 
> 	if (drm_drv_enabled(...)) {
> 		/* surprise, it's disabled! */
> 	}
> 

It used to return a bool in v2 but Thomas suggested an int instead to
have consistency on the errno code that was returned by the callers.

I should probably name that function differently to avoid confusion.

But I think you are correct and this change is caused too much churn
for not that much benefit, specially since is unclear that there might
be another condition to prevent a DRM driver to load besides nomodeset.

I'll just drop this patch and post only #2 but making drivers to test
using the drm_check_modeset() function (which doesn't have a name that
implies a bool return).

> 
> BR,
> Jani.
> 
> 
> 

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2232 bytes --]

Hi

Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
> Hello Jani,
> 
> On 11/4/21 20:57, Jani Nikula wrote:
>> On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
>>> +/**
>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>> + * @driver: DRM driver to check
>>> + *
>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>> + * if the "nomodeset" kernel command line parameter is used.
>>> + *
>>> + * Return: 0 on success or a negative error code on failure.
>>> + */
>>> +int drm_drv_enabled(const struct drm_driver *driver)

Jani mentioned that i915 absolutely wants this to run from the 
module_init function. Best is to drop the parameter.

>>> +{
>>> +	if (vgacon_text_force()) {
>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>> +		return -ENODEV;
>>> +	}

If we run this from within a module_init function, we'd get plenty of 
these warnings if drivers are compiled into the kernel. Maybe simply 
remove the message. There's already a warning printed by the nomodeset 
handler.

>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>
>> The name implies a bool return, but it's not.
>>
>> 	if (drm_drv_enabled(...)) {
>> 		/* surprise, it's disabled! */
>> 	}
>>
> 
> It used to return a bool in v2 but Thomas suggested an int instead to
> have consistency on the errno code that was returned by the callers.
> 
> I should probably name that function differently to avoid confusion.

Yes, please.

Best regards
Thomas

> 
> But I think you are correct and this change is caused too much churn
> for not that much benefit, specially since is unclear that there might
> be another condition to prevent a DRM driver to load besides nomodeset.
> 
> I'll just drop this patch and post only #2 but making drivers to test
> using the drm_check_modeset() function (which doesn't have a name that
> implies a bool return).
> 
>>
>> BR,
>> Jani.
>>
>>
>>
> 
> Best regards,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 12282 bytes --]

Hi

Am 04.11.21 um 17:07 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 the vgacon_text_force() function and related logic to the DRM
> subsystem. While doing that, rename the function to drm_check_modeset()
> which better reflects what the function is really used to test for.
> 
> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> 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.
> 
>   drivers/gpu/drm/Makefile                |  2 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  1 -
>   drivers/gpu/drm/ast/ast_drv.c           |  1 -
>   drivers/gpu/drm/drm_drv.c               |  9 +++++----
>   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 ------
>   18 files changed, 39 insertions(+), 44 deletions(-)
>   create mode 100644 drivers/gpu/drm/drm_nomodeset.c
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c41156deb5f..c74810c285af 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-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
> +

This now depends on the VGA textmode console. Even if you have no VGA 
console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can 
provide graphics. Non-PC systems don't even have a VGA device.

I think we really want a separate boolean config option that gets 
selected by CONFIG_DRM.


>   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 7fde40d06181..b4b6993861e6 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>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 802063279b86..6222082c3082 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_drv.c b/drivers/gpu/drm/drm_drv.c
> index 3fb567d62881..80b85b8ea776 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
>    */
>   int drm_drv_enabled(const struct drm_driver *driver)
>   {
> -	if (vgacon_text_force()) {
> +	int ret;
> +
> +	ret = drm_check_modeset();
> +	if (ret)
>   		DRM_INFO("%s driver is disabled\n", driver->name);
> -		return -ENODEV;
> -	}
>   
> -	return 0;
> +	return ret;
>   }
>   EXPORT_SYMBOL(drm_drv_enabled);
>   
> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
> new file mode 100644
> index 000000000000..6683e396d2c5
> --- /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 drm_nomodeset;
> +
> +int drm_check_modeset(void)
> +{
> +	return drm_nomodeset ? -ENODEV : 0;
> +}
> +EXPORT_SYMBOL(drm_check_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");

I'd update this text to be less sensational.

> +
> +	return 1;
> +}
> +
> +/* 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 45cb3e540eff..c890c1ca20c4 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>
> -

These changes should be in patch 1?

>   #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 2a581094ba2b..8e000cac11ba 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 8844d3602d87..bd1456521b7c 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 3ac2ef2bf545..ff070ac76111 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 56d688c04346..f59cc971ec95 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 ee6b1ff9128b..6e9a31f1a0f3 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 4706c5bc3067..659208d5aef9 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 e4377c37cf33..b1e63fd543bb 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 28200dfba2d1..ba9c0c2f8ae6 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 05e9949293d5..115ec9518277 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..18982d3507e4 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 int drm_check_modeset(void);
> +#else
> +static inline int drm_check_modeset(void) { return 0; }
> +#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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[Jani Nikula]

On Fri, 05 Nov 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 04.11.21 um 17:07 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 the vgacon_text_force() function and related logic to the DRM
>> subsystem. While doing that, rename the function to drm_check_modeset()
>> which better reflects what the function is really used to test for.
>> 
>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
>> 
>> 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.
>> 
>>   drivers/gpu/drm/Makefile                |  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  1 -
>>   drivers/gpu/drm/ast/ast_drv.c           |  1 -
>>   drivers/gpu/drm/drm_drv.c               |  9 +++++----
>>   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 ------
>>   18 files changed, 39 insertions(+), 44 deletions(-)
>>   create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>> 
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 1c41156deb5f..c74810c285af 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-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>> +
>
> This now depends on the VGA textmode console. Even if you have no VGA 
> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can 
> provide graphics. Non-PC systems don't even have a VGA device.

This was discussed in an earlier version, which had this builtin but the
header still had a stub for CONFIG_VGA_CONSOLE=n.

> I think we really want a separate boolean config option that gets 
> selected by CONFIG_DRM.

Perhaps that should be a separate change on top.

BR,
Jani.

>
>
>>   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 7fde40d06181..b4b6993861e6 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>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
>> index 802063279b86..6222082c3082 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_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 3fb567d62881..80b85b8ea776 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
>>    */
>>   int drm_drv_enabled(const struct drm_driver *driver)
>>   {
>> -	if (vgacon_text_force()) {
>> +	int ret;
>> +
>> +	ret = drm_check_modeset();
>> +	if (ret)
>>   		DRM_INFO("%s driver is disabled\n", driver->name);
>> -		return -ENODEV;
>> -	}
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL(drm_drv_enabled);
>>   
>> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
>> new file mode 100644
>> index 000000000000..6683e396d2c5
>> --- /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 drm_nomodeset;
>> +
>> +int drm_check_modeset(void)
>> +{
>> +	return drm_nomodeset ? -ENODEV : 0;
>> +}
>> +EXPORT_SYMBOL(drm_check_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");
>
> I'd update this text to be less sensational.
>
>> +
>> +	return 1;
>> +}
>> +
>> +/* 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 45cb3e540eff..c890c1ca20c4 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>
>> -
>
> These changes should be in patch 1?
>
>>   #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 2a581094ba2b..8e000cac11ba 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 8844d3602d87..bd1456521b7c 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 3ac2ef2bf545..ff070ac76111 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 56d688c04346..f59cc971ec95 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 ee6b1ff9128b..6e9a31f1a0f3 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 4706c5bc3067..659208d5aef9 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 e4377c37cf33..b1e63fd543bb 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 28200dfba2d1..ba9c0c2f8ae6 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 05e9949293d5..115ec9518277 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..18982d3507e4 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 int drm_check_modeset(void);
>> +#else
>> +static inline int drm_check_modeset(void) { return 0; }
>> +#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 */
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 13647 bytes --]

Hi

Am 05.11.21 um 10:22 schrieb Jani Nikula:
> On Fri, 05 Nov 2021, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 04.11.21 um 17:07 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 the vgacon_text_force() function and related logic to the DRM
>>> subsystem. While doing that, rename the function to drm_check_modeset()
>>> which better reflects what the function is really used to test for.
>>>
>>> Suggested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> ---
>>>
>>> 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.
>>>
>>>    drivers/gpu/drm/Makefile                |  2 ++
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  1 -
>>>    drivers/gpu/drm/ast/ast_drv.c           |  1 -
>>>    drivers/gpu/drm/drm_drv.c               |  9 +++++----
>>>    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 ------
>>>    18 files changed, 39 insertions(+), 44 deletions(-)
>>>    create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>>>
>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>> index 1c41156deb5f..c74810c285af 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-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>>> +
>>
>> This now depends on the VGA textmode console. Even if you have no VGA
>> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can
>> provide graphics. Non-PC systems don't even have a VGA device.
> 
> This was discussed in an earlier version, which had this builtin but the
> header still had a stub for CONFIG_VGA_CONSOLE=n.
> 
>> I think we really want a separate boolean config option that gets
>> selected by CONFIG_DRM.
> 
> Perhaps that should be a separate change on top.

Sure, make it a separate patch.

We want to make this work on ARM systems. I even have a request to 
replace offb on Power architecture by simpledrm. So the final config has 
to be system agnostic.

Best regards
Thomas

> 
> BR,
> Jani.
> 
>>
>>
>>>    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 7fde40d06181..b4b6993861e6 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>
>>> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
>>> index 802063279b86..6222082c3082 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_drv.c b/drivers/gpu/drm/drm_drv.c
>>> index 3fb567d62881..80b85b8ea776 100644
>>> --- a/drivers/gpu/drm/drm_drv.c
>>> +++ b/drivers/gpu/drm/drm_drv.c
>>> @@ -986,12 +986,13 @@ EXPORT_SYMBOL(drm_dev_set_unique);
>>>     */
>>>    int drm_drv_enabled(const struct drm_driver *driver)
>>>    {
>>> -	if (vgacon_text_force()) {
>>> +	int ret;
>>> +
>>> +	ret = drm_check_modeset();
>>> +	if (ret)
>>>    		DRM_INFO("%s driver is disabled\n", driver->name);
>>> -		return -ENODEV;
>>> -	}
>>>    
>>> -	return 0;
>>> +	return ret;
>>>    }
>>>    EXPORT_SYMBOL(drm_drv_enabled);
>>>    
>>> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
>>> new file mode 100644
>>> index 000000000000..6683e396d2c5
>>> --- /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 drm_nomodeset;
>>> +
>>> +int drm_check_modeset(void)
>>> +{
>>> +	return drm_nomodeset ? -ENODEV : 0;
>>> +}
>>> +EXPORT_SYMBOL(drm_check_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");
>>
>> I'd update this text to be less sensational.
>>
>>> +
>>> +	return 1;
>>> +}
>>> +
>>> +/* 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 45cb3e540eff..c890c1ca20c4 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>
>>> -
>>
>> These changes should be in patch 1?
>>
>>>    #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 2a581094ba2b..8e000cac11ba 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 8844d3602d87..bd1456521b7c 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 3ac2ef2bf545..ff070ac76111 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 56d688c04346..f59cc971ec95 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 ee6b1ff9128b..6e9a31f1a0f3 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 4706c5bc3067..659208d5aef9 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 e4377c37cf33..b1e63fd543bb 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 28200dfba2d1..ba9c0c2f8ae6 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 05e9949293d5..115ec9518277 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..18982d3507e4 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 int drm_check_modeset(void);
>>> +#else
>>> +static inline int drm_check_modeset(void) { return 0; }
>>> +#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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Javier Martinez Canillas]

Hello Thomas,

On 11/5/21 09:43, Thomas Zimmermann wrote:
> Hi
> 
> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>> Hello Jani,
>>
>> On 11/4/21 20:57, Jani Nikula wrote:
>>> On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
>>>> +/**
>>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>>> + * @driver: DRM driver to check
>>>> + *
>>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>>> + * if the "nomodeset" kernel command line parameter is used.
>>>> + *
>>>> + * Return: 0 on success or a negative error code on failure.
>>>> + */
>>>> +int drm_drv_enabled(const struct drm_driver *driver)
> 
> Jani mentioned that i915 absolutely wants this to run from the 
> module_init function. Best is to drop the parameter.
>

Ok. I now wonder though how much value would add this function since
it will just be a wrapper around the nomodeset check.

We talked about adding a new DRIVER_GENERIC feature flag and check for
this, but as danvet mentioned that is not really needed. We just need
to avoid testing for nomodeset in the simpledrm driver.

Do you envision other condition that could be added later to disable a
DRM driver ? Or do you think that just from a code readability point of
view makes worth it ?

>>>> +{
>>>> +	if (vgacon_text_force()) {
>>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>>> +		return -ENODEV;
>>>> +	}
> 
> If we run this from within a module_init function, we'd get plenty of 
> these warnings if drivers are compiled into the kernel. Maybe simply 
> remove the message. There's already a warning printed by the nomodeset 
> handler.
>

Indeed. I'll just drop it.

>>>> +
>>>> +	return 0;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>>
>>> The name implies a bool return, but it's not.
>>>
>>> 	if (drm_drv_enabled(...)) {
>>> 		/* surprise, it's disabled! */
>>> 	}
>>>
>>
>> It used to return a bool in v2 but Thomas suggested an int instead to
>> have consistency on the errno code that was returned by the callers.
>>
>> I should probably name that function differently to avoid confusion.
> 
> Yes, please.
>

drm_driver_check() maybe ?
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[Javier Martinez Canillas]

On 11/5/21 10:00, Thomas Zimmermann wrote:

[snip]

>> +
>> +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");
> 
> I'd update this text to be less sensational.
>

This is indeed quite sensational. But think we can do it as a follow-up patch.

Since we will have to stick with nomodeset for backward compatibility, I was
planning to add documentation to explain what this parameter is about and what
is the actual effect of setting it.

So I think we can change this as a part of that patch-set.
 
>> +
>> +	return 1;
>> +}
>> +
>> +/* 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 45cb3e540eff..c890c1ca20c4 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>
>> -
>
> These changes should be in patch 1?
>

Yes, I forgot to move these when changed the order of the patches.

Best regards, -- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[Javier Martinez Canillas]

On 11/5/21 10:39, Thomas Zimmermann wrote:

[snip]

>>>>    
>>>> +obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>>>> +
>>>
>>> This now depends on the VGA textmode console. Even if you have no VGA
>>> console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can
>>> provide graphics. Non-PC systems don't even have a VGA device.
>>
>> This was discussed in an earlier version, which had this builtin but the
>> header still had a stub for CONFIG_VGA_CONSOLE=n.
>>
>>> I think we really want a separate boolean config option that gets
>>> selected by CONFIG_DRM.
>>
>> Perhaps that should be a separate change on top.
> 
> Sure, make it a separate patch.
>

Agreed. I was planning to do it as a follow-up as well and drop the
#ifdef CONFIG_VGA_CONSOLE guard in the header.
 
> We want to make this work on ARM systems. I even have a request to 
> replace offb on Power architecture by simpledrm. So the final config has 
> to be system agnostic.
>

Same, since we want to drop the fbdev drivers in Fedora, for all arches.
 
> Best regards
> Thomas
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Jani Nikula]

On Fri, 05 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
> Hello Thomas,
>
> On 11/5/21 09:43, Thomas Zimmermann wrote:
>> Hi
>> 
>> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>>> Hello Jani,
>>>
>>> On 11/4/21 20:57, Jani Nikula wrote:
>>>> On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
>>>>> +/**
>>>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>>>> + * @driver: DRM driver to check
>>>>> + *
>>>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>>>> + * if the "nomodeset" kernel command line parameter is used.
>>>>> + *
>>>>> + * Return: 0 on success or a negative error code on failure.
>>>>> + */
>>>>> +int drm_drv_enabled(const struct drm_driver *driver)
>> 
>> Jani mentioned that i915 absolutely wants this to run from the 
>> module_init function. Best is to drop the parameter.
>>
>
> Ok. I now wonder though how much value would add this function since
> it will just be a wrapper around the nomodeset check.
>
> We talked about adding a new DRIVER_GENERIC feature flag and check for
> this, but as danvet mentioned that is not really needed. We just need
> to avoid testing for nomodeset in the simpledrm driver.
>
> Do you envision other condition that could be added later to disable a
> DRM driver ? Or do you think that just from a code readability point of
> view makes worth it ?

Taking a step back for perspective.

I think there's broad consensus in moving the parameter to drm, naming
the check function to drm_something_something(), and breaking the ties
to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that
effect.

I think everything beyond that is still a bit vague and/or
contentious. So how about making the first 2-3 patches just that?
Something we can all agree on, makes good progress, improves the kernel,
and gives us something to build on?

BR,
Jani.


>
>>>>> +{
>>>>> +	if (vgacon_text_force()) {
>>>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>>>> +		return -ENODEV;
>>>>> +	}
>> 
>> If we run this from within a module_init function, we'd get plenty of 
>> these warnings if drivers are compiled into the kernel. Maybe simply 
>> remove the message. There's already a warning printed by the nomodeset 
>> handler.
>>
>
> Indeed. I'll just drop it.
>
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>>>
>>>> The name implies a bool return, but it's not.
>>>>
>>>> 	if (drm_drv_enabled(...)) {
>>>> 		/* surprise, it's disabled! */
>>>> 	}
>>>>
>>>
>>> It used to return a bool in v2 but Thomas suggested an int instead to
>>> have consistency on the errno code that was returned by the callers.
>>>
>>> I should probably name that function differently to avoid confusion.
>> 
>> Yes, please.
>>
>
> drm_driver_check() maybe ?
>  
> Best regards,

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2913 bytes --]

Hi

Am 05.11.21 um 10:48 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 11/5/21 09:43, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 04.11.21 um 21:09 schrieb Javier Martinez Canillas:
>>> Hello Jani,
>>>
>>> On 11/4/21 20:57, Jani Nikula wrote:
>>>> On Thu, 04 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
>>>>> +/**
>>>>> + * drm_drv_enabled - Checks if a DRM driver can be enabled
>>>>> + * @driver: DRM driver to check
>>>>> + *
>>>>> + * Checks whether a DRM driver can be enabled or not. This may be the case
>>>>> + * if the "nomodeset" kernel command line parameter is used.
>>>>> + *
>>>>> + * Return: 0 on success or a negative error code on failure.
>>>>> + */
>>>>> +int drm_drv_enabled(const struct drm_driver *driver)
>>
>> Jani mentioned that i915 absolutely wants this to run from the
>> module_init function. Best is to drop the parameter.
>>
> 
> Ok. I now wonder though how much value would add this function since
> it will just be a wrapper around the nomodeset check.
> 
> We talked about adding a new DRIVER_GENERIC feature flag and check for
> this, but as danvet mentioned that is not really needed. We just need
> to avoid testing for nomodeset in the simpledrm driver.
> 
> Do you envision other condition that could be added later to disable a
> DRM driver ? Or do you think that just from a code readability point of
> view makes worth it ?

DRIVER_GENERIC would have been nice, but it's not happening now.

I suggest to move over the nomodeset parameter (i.e., this patchset), 
then make the config option system agnostic, and finally add the test to 
all drivers expect simpledrm. That should solve the imminent problem.

Best regards
Thomas

> 
>>>>> +{
>>>>> +	if (vgacon_text_force()) {
>>>>> +		DRM_INFO("%s driver is disabled\n", driver->name);
>>>>> +		return -ENODEV;
>>>>> +	}
>>
>> If we run this from within a module_init function, we'd get plenty of
>> these warnings if drivers are compiled into the kernel. Maybe simply
>> remove the message. There's already a warning printed by the nomodeset
>> handler.
>>
> 
> Indeed. I'll just drop it.
> 
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL(drm_drv_enabled);
>>>>
>>>> The name implies a bool return, but it's not.
>>>>
>>>> 	if (drm_drv_enabled(...)) {
>>>> 		/* surprise, it's disabled! */
>>>> 	}
>>>>
>>>
>>> It used to return a bool in v2 but Thomas suggested an int instead to
>>> have consistency on the errno code that was returned by the callers.
>>>
>>> I should probably name that function differently to avoid confusion.
>>
>> Yes, please.
>>
> 
> drm_driver_check() maybe ?
>   
> Best regards,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Javier Martinez Canillas]

On 11/5/21 11:04, Jani Nikula wrote:
> On Fri, 05 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:

[snip]

>>
>> Do you envision other condition that could be added later to disable a
>> DRM driver ? Or do you think that just from a code readability point of
>> view makes worth it ?
> 
> Taking a step back for perspective.
> 
> I think there's broad consensus in moving the parameter to drm, naming
> the check function to drm_something_something(), and breaking the ties
> to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that
> effect.
>

Thanks, I appreciate your feedback and comments.
 
> I think everything beyond that is still a bit vague and/or
> contentious. So how about making the first 2-3 patches just that?
> Something we can all agree on, makes good progress, improves the kernel,
> and gives us something to build on?
>

That works for me. Thomas, do you agree with that approach ?
 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [Nouveau] [PATCH v2 1/2] drm: Add a drm_drv_enabled() to check if drivers should be enabled

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1372 bytes --]

Hi

Am 05.11.21 um 13:00 schrieb Javier Martinez Canillas:
> On 11/5/21 11:04, Jani Nikula wrote:
>> On Fri, 05 Nov 2021, Javier Martinez Canillas <javierm@redhat.com> wrote:
> 
> [snip]
> 
>>>
>>> Do you envision other condition that could be added later to disable a
>>> DRM driver ? Or do you think that just from a code readability point of
>>> view makes worth it ?
>>
>> Taking a step back for perspective.
>>
>> I think there's broad consensus in moving the parameter to drm, naming
>> the check function to drm_something_something(), and breaking the ties
>> to CONFIG_VGA_CONSOLE. I appreciate the work you're doing to that
>> effect.
>>
> 
> Thanks, I appreciate your feedback and comments.
>   
>> I think everything beyond that is still a bit vague and/or
>> contentious. So how about making the first 2-3 patches just that?
>> Something we can all agree on, makes good progress, improves the kernel,
>> and gives us something to build on?
>>
> 
> That works for me. Thomas, do you agree with that approach ?

Sure. I think that's more or less what I proposed in my reply to that mail.

Best regards
Thomas

>   
> Best regards,
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]