diff mbox series

[1/2] drm: Move nomodeset kernel parameter to drivers/video

Message ID 20221107104916.18733-2-tzimmermann@suse.de
State Superseded
Headers show
Series video/fbdev: Support 'nomodeset' in PCI drivers | expand

Commit Message

Thomas Zimmermann Nov. 7, 2022, 10:49 a.m. UTC 
Move the nomodeset kernel parameter to drivers/video to make it
available to non-DRM drivers. Adapt the interface, but keep the DRM
interface drm_firmware_drivers_only() to avoid churn within DRM. The
function should later be inlined into callers.

The parameter disables any DRM graphics driver that would replace a
driver for firmware-provided scanout buffers. It is an option to easily
fallback to basic graphics output if the hardware's native driver is
broken. Moving it to a more prominent location wil make it available
to fbdev as well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/admin-guide/kernel-parameters.txt      |  2 +-
 MAINTAINERS                                          |  2 ++
 drivers/gpu/drm/Kconfig                              |  7 +------
 drivers/gpu/drm/Makefile                             |  1 -
 drivers/video/Kconfig                                |  4 ++++
 drivers/video/Makefile                               |  1 +
 .../{gpu/drm/drm_nomodeset.c => video/nomodeset.c}   | 12 +++++++-----
 include/drm/drm_drv.h                                |  8 +++++++-
 include/video/nomodeset.h                            |  8 ++++++++
 9 files changed, 31 insertions(+), 14 deletions(-)
 rename drivers/{gpu/drm/drm_nomodeset.c => video/nomodeset.c} (63%)
 create mode 100644 include/video/nomodeset.h

Comments

Javier Martinez Canillas Nov. 11, 2022, 9:28 a.m. UTC | #1 
Hello Thomas,

On 11/7/22 11:49, Thomas Zimmermann wrote:

[...]

> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index a465d5242774a..70178c5f53956 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3777,7 +3777,7 @@
>  			shutdown the other cpus.  Instead use the REBOOT_VECTOR
>  			irq.
>  
> -	nomodeset	Disable kernel modesetting. DRM drivers will not perform
> +	nomodeset	Disable kernel modesetting. Graphics drivers will not perform
>  			display-mode changes or accelerated rendering. Only the
>  			system framebuffer will be available for use if this was
>  			set-up by the firmware or boot loader.

Not really part of your patch but probably we should reword this a little bit.

Because as this is written, it implies that not only DRM drivers with feature
DRIVER_MODESET will not be available but also drivers with DRIVER_RENDER. But
that's not the case, render-only drivers usually just ignore this parameter
(but not all IIRC), so I wonder how we could make this comment more accurate.

Also maybe we can mention in the comment fbdev and DRM? Just to make it clear
that this will affect to both subsystems? When I first worked on this, there
were a lot of assumptions in the stack (gdm, mutter, plymouth) that nomodeset
basically meant "no DRM but fbdev".

[...]

>  
>  int drm_dev_set_unique(struct drm_device *dev, const char *name);
>  
> -extern bool drm_firmware_drivers_only(void);
> +/* TODO: Inline drm_firmware_drivers_only() in all its callers. */

I guess you plan to do that as follow-up patches once this series land? Just
to avoid the churn to require acks for all the drivers to merge this series?

The changes looks good to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Thomas Zimmermann Nov. 11, 2022, 12:37 p.m. UTC | #2 
Hi

Am 11.11.22 um 10:28 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 11/7/22 11:49, Thomas Zimmermann wrote:
> 
> [...]
> 
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index a465d5242774a..70178c5f53956 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3777,7 +3777,7 @@
>>   			shutdown the other cpus.  Instead use the REBOOT_VECTOR
>>   			irq.
>>   
>> -	nomodeset	Disable kernel modesetting. DRM drivers will not perform
>> +	nomodeset	Disable kernel modesetting. Graphics drivers will not perform
>>   			display-mode changes or accelerated rendering. Only the
>>   			system framebuffer will be available for use if this was
>>   			set-up by the firmware or boot loader.
> 
> Not really part of your patch but probably we should reword this a little bit.
> 
> Because as this is written, it implies that not only DRM drivers with feature
> DRIVER_MODESET will not be available but also drivers with DRIVER_RENDER. But
> that's not the case, render-only drivers usually just ignore this parameter
> (but not all IIRC), so I wonder how we could make this comment more accurate.
> 
> Also maybe we can mention in the comment fbdev and DRM? Just to make it clear
> that this will affect to both subsystems? When I first worked on this, there
> were a lot of assumptions in the stack (gdm, mutter, plymouth) that nomodeset
> basically meant "no DRM but fbdev".
> 
> [...]
> 
>>   
>>   int drm_dev_set_unique(struct drm_device *dev, const char *name);
>>   
>> -extern bool drm_firmware_drivers_only(void);
>> +/* TODO: Inline drm_firmware_drivers_only() in all its callers. */
> 
> I guess you plan to do that as follow-up patches once this series land? Just
> to avoid the churn to require acks for all the drivers to merge this series?

Yes. It has no hurry, but we can do that.

Best regards
Thomas

> 
> The changes looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
Thomas Zimmermann Nov. 11, 2022, 1:06 p.m. UTC | #3 
Hi

Am 11.11.22 um 10:28 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 11/7/22 11:49, Thomas Zimmermann wrote:
> 
> [...]
> 
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index a465d5242774a..70178c5f53956 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3777,7 +3777,7 @@
>>   			shutdown the other cpus.  Instead use the REBOOT_VECTOR
>>   			irq.
>>   
>> -	nomodeset	Disable kernel modesetting. DRM drivers will not perform
>> +	nomodeset	Disable kernel modesetting. Graphics drivers will not perform
>>   			display-mode changes or accelerated rendering. Only the
>>   			system framebuffer will be available for use if this was
>>   			set-up by the firmware or boot loader.
> 
> Not really part of your patch but probably we should reword this a little bit.
> 
> Because as this is written, it implies that not only DRM drivers with feature
> DRIVER_MODESET will not be available but also drivers with DRIVER_RENDER. But
> that's not the case, render-only drivers usually just ignore this parameter
> (but not all IIRC), so I wonder how we could make this comment more accurate.

I see what you mean, but it's hard to describe in simple words. The 
option is a bit fuzzy. It means that a driver will not replace a 
firmware buffer; even if that means it won't initialize at all. I guess 
we should spell that out.

> 
> Also maybe we can mention in the comment fbdev and DRM? Just to make it clear
> that this will affect to both subsystems? When I first worked on this, there
> were a lot of assumptions in the stack (gdm, mutter, plymouth) that nomodeset
> basically meant "no DRM but fbdev".

I can change the text to say 'DRM and fbdev drivers'.

Best regards
Thomas

> 
> [...]
> 
>>   
>>   int drm_dev_set_unique(struct drm_device *dev, const char *name);
>>   
>> -extern bool drm_firmware_drivers_only(void);
>> +/* TODO: Inline drm_firmware_drivers_only() in all its callers. */
> 
> I guess you plan to do that as follow-up patches once this series land? Just
> to avoid the churn to require acks for all the drivers to merge this series?
> 
> The changes looks good to me.
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
diff mbox series

Patch

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a465d5242774a..70178c5f53956 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3777,7 +3777,7 @@ 
 			shutdown the other cpus.  Instead use the REBOOT_VECTOR
 			irq.
 
-	nomodeset	Disable kernel modesetting. DRM drivers will not perform
+	nomodeset	Disable kernel modesetting. Graphics drivers will not perform
 			display-mode changes or accelerated rendering. Only the
 			system framebuffer will be available for use if this was
 			set-up by the firmware or boot loader.
diff --git a/MAINTAINERS b/MAINTAINERS
index 0f624e3ef6235..84b008f5aacbc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6698,8 +6698,10 @@  F:	drivers/gpu/drm/drm_aperture.c
 F:	drivers/gpu/drm/tiny/ofdrm.c
 F:	drivers/gpu/drm/tiny/simpledrm.c
 F:	drivers/video/aperture.c
+F:	drivers/video/nomodeset.c
 F:	include/drm/drm_aperture.h
 F:	include/linux/aperture.h
+F:	include/video/nomodeset.h
 
 DRM DRIVER FOR SIS VIDEO CARDS
 S:	Orphan / Obsolete
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f30f99166531f..6b11614aecc5b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -8,7 +8,6 @@ 
 menuconfig DRM
 	tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI support)"
 	depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && HAS_DMA
-	select DRM_NOMODESET
 	select DRM_PANEL_ORIENTATION_QUIRKS
 	select HDMI
 	select FB_CMDLINE
@@ -19,6 +18,7 @@  menuconfig DRM
 # gallium uses SYS_kcmp for os_same_file_description() to de-duplicate
 # device and dmabuf fd. Let's make sure that is available for our userspace.
 	select KCMP
+	select VIDEO_NOMODESET
 	help
 	  Kernel-level support for the Direct Rendering Infrastructure (DRI)
 	  introduced in XFree86 4.0. If you say Y here, you need to select
@@ -515,11 +515,6 @@  config DRM_EXPORT_FOR_TESTS
 config DRM_PANEL_ORIENTATION_QUIRKS
 	tristate
 
-# Separate option because nomodeset parameter is global and expected built-in
-config DRM_NOMODESET
-	bool
-	default n
-
 config DRM_LIB_RANDOM
 	bool
 	default n
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index c44a54cadb618..f92cd78927110 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -72,7 +72,6 @@  drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \
 	drm_privacy_screen_x86.o
 obj-$(CONFIG_DRM)	+= drm.o
 
-obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
 obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o
 
 #
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0587e21abad94..6d2fde6c5d11a 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -11,6 +11,10 @@  config APERTURE_HELPERS
 	  Support tracking and hand-over of aperture ownership. Required
 	  by graphics drivers for firmware-provided framebuffers.
 
+config VIDEO_NOMODESET
+	bool
+	default n
+
 if HAS_IOMEM
 
 config HAVE_FB_ATMEL
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 5bb6b452cc83a..a50eb528ed3c3 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -2,6 +2,7 @@ 
 
 obj-$(CONFIG_APERTURE_HELPERS)    += aperture.o
 obj-$(CONFIG_VGASTATE)            += vgastate.o
+obj-$(CONFIG_VIDEO_NOMODESET)     += nomodeset.o
 obj-$(CONFIG_HDMI)                += hdmi.o
 
 obj-$(CONFIG_VT)		  += console/
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/video/nomodeset.c
similarity index 63%
rename from drivers/gpu/drm/drm_nomodeset.c
rename to drivers/video/nomodeset.c
index f3978d5bd3a1d..13cc8b7196978 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/video/nomodeset.c
@@ -3,17 +3,19 @@ 
 #include <linux/module.h>
 #include <linux/types.h>
 
-static bool drm_nomodeset;
+#include <video/nomodeset.h>
 
-bool drm_firmware_drivers_only(void)
+static bool video_nomodeset;
+
+bool video_firmware_drivers_only(void)
 {
-	return drm_nomodeset;
+	return video_nomodeset;
 }
-EXPORT_SYMBOL(drm_firmware_drivers_only);
+EXPORT_SYMBOL(video_firmware_drivers_only);
 
 static int __init disable_modeset(char *str)
 {
-	drm_nomodeset = true;
+	video_nomodeset = true;
 
 	pr_warn("Booted with the nomodeset parameter. Only the system framebuffer will be available\n");
 
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index f6159acb88569..700d3857e0881 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -30,6 +30,8 @@ 
 #include <linux/list.h>
 #include <linux/irqreturn.h>
 
+#include <video/nomodeset.h>
+
 #include <drm/drm_device.h>
 
 struct drm_file;
@@ -602,6 +604,10 @@  static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev)
 
 int drm_dev_set_unique(struct drm_device *dev, const char *name);
 
-extern bool drm_firmware_drivers_only(void);
+/* TODO: Inline drm_firmware_drivers_only() in all its callers. */
+static inline bool drm_firmware_drivers_only(void)
+{
+	return video_firmware_drivers_only();
+}
 
 #endif
diff --git a/include/video/nomodeset.h b/include/video/nomodeset.h
new file mode 100644
index 0000000000000..8f8688b8beabe
--- /dev/null
+++ b/include/video/nomodeset.h
@@ -0,0 +1,8 @@ 
+/* SPDX-License-Identifier: MIT */
+
+#ifndef VIDEO_NOMODESET_H
+#define VIDEO_NOMODESET_H
+
+bool video_firmware_drivers_only(void);
+
+#endif