From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 68205C4332F for ; Fri, 5 Nov 2021 09:22:37 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 08EC560ED4 for ; Fri, 5 Nov 2021 09:22:37 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 08EC560ED4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 07E1C6E139; Fri, 5 Nov 2021 09:22:29 +0000 (UTC) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 299856E111; Fri, 5 Nov 2021 09:22:28 +0000 (UTC) X-IronPort-AV: E=McAfee;i="6200,9189,10158"; a="231714554" X-IronPort-AV: E=Sophos;i="5.87,211,1631602800"; d="scan'208";a="231714554" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Nov 2021 02:22:27 -0700 X-IronPort-AV: E=Sophos;i="5.87,211,1631602800"; d="scan'208";a="501878634" Received: from jprisaca-mobl.ger.corp.intel.com (HELO localhost) ([10.251.214.70]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Nov 2021 02:22:19 -0700 From: Jani Nikula To: Thomas Zimmermann , Javier Martinez Canillas , linux-kernel@vger.kernel.org In-Reply-To: Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <20211104160707.1407052-1-javierm@redhat.com> <20211104160707.1407052-3-javierm@redhat.com> Date: Fri, 05 Nov 2021 11:22:17 +0200 Message-ID: <87cznf9cty.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Intel-gfx] [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-fbdev@vger.kernel.org, David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org, Gurchetan Singh , Gerd Hoffmann , amd-gfx@lists.freedesktop.org, VMware Graphics , Peter Robinson , nouveau@lists.freedesktop.org, Dave Airlie , Chia-I Wu , Ben Skeggs , Michel =?utf-8?Q?D=C3=A4nzer?= , Maxime Ripard , virtualization@lists.linux-foundation.org, Pekka Paalanen , Greg Kroah-Hartman , "Pan, Xinhui" , spice-devel@lists.freedesktop.org, Alex Deucher , intel-gfx@lists.freedesktop.org, Christian =?utf-8?Q?K=C3=B6nig?= , Zack Rusin Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Fri, 05 Nov 2021, Thomas Zimmermann 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. >>=20 >> It makes much more sense for the parameter logic to be in the subsystem >> of the drivers that are making use of it. >>=20 >> 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. >>=20 >> Suggested-by: Daniel Vetter >> Signed-off-by: Javier Martinez Canillas >> --- >>=20 >> 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. >>=20 >> 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 >>=20 >> 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) +=3D drm_privacy_scre= en.o drm_privacy_screen_x86. >>=20=20=20 >> obj-$(CONFIG_DRM_DP_AUX_BUS) +=3D drm_dp_aux_bus.o >>=20=20=20 >> +obj-$(CONFIG_VGA_CONSOLE) +=3D drm_nomodeset.o >> + > > This now depends on the VGA textmode console. Even if you have no VGA=20 > console, you'd want drm_nomodeset.o. Simpledrm might be built-in and can= =20 > 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=3Dn. > I think we really want a separate boolean config option that gets=20 > selected by CONFIG_DRM. Perhaps that should be a separate change on top. BR, Jani. > > >> drm_cma_helper-y :=3D drm_gem_cma_helper.o >> obj-$(CONFIG_DRM_GEM_CMA_HELPER) +=3D drm_cma_helper.o >>=20=20=20 >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/a= md/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" >>=20=20=20 >> #include >> -#include >> #include >> #include >> #include >> 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 >> */ >>=20=20=20 >> -#include >> #include >> #include >>=20=20=20 >> 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 =3D drm_check_modeset(); >> + if (ret) >> DRM_INFO("%s driver is disabled\n", driver->name); >> - return -ENODEV; >> - } >>=20=20=20 >> - return 0; >> + return ret; >> } >> EXPORT_SYMBOL(drm_drv_enabled); >>=20=20=20 >> diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomod= eset.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 >> +#include >> + >> +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 =3D true; >> + >> + pr_warn("You have booted with nomodeset. This means your GPU drivers a= re DISABLED\n"); >> + pr_warn("Any video related functionality will be severely degraded, an= d you may not even be able to suspend the system properly\n"); >> + pr_warn("Unless you actually understand what nomodeset does, you shoul= d 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/i= 915_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 =C2=A9 2021 Intel Corporation >> */ >>=20=20=20 >> -#include >> - > > 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/mga= g200/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 >> */ >>=20=20=20 >> -#include >> #include >> #include >> #include >> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nou= veau/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 >> */ >>=20=20=20 >> -#include >> #include >> #include >> #include >> 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 @@ >>=20=20=20 >> #include "qxl_drv.h" >>=20=20=20 >> -#include >> #include >> #include >> #include >> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeo= n/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 @@ >>=20=20=20 >>=20=20=20 >> #include >> -#include >> #include >> #include >> #include >> 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 >>=20=20=20 >> -#include >> #include >>=20=20=20 >> #include >> 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 >> */ >>=20=20=20 >> -#include >> #include >> #include >> #include >> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vbox= video/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 > * Hans de Goede >> */ >> -#include >> #include >> #include >> #include >> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virt= io/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 @@ >> */ >>=20=20=20 >> #include >> -#include >> #include >> #include >> #include >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgf= x/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 @@ >> * >> *********************************************************************= *****/ >>=20=20=20 >> -#include >> #include >> #include >> #include >> diff --git a/drivers/video/console/vgacon.c b/drivers/video/console/vgac= on.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 */ >>=20=20=20 >> -static bool vgacon_text_mode_force; >> static bool vga_hardscroll_enabled; >> static bool vga_hardscroll_user_enable =3D true; >>=20=20=20 >> -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 =3D true; >> - >> - pr_warn("You have booted with nomodeset. This means your GPU drivers a= re DISABLED\n"); >> - pr_warn("Any video related functionality will be severely degraded, an= d you may not even be able to suspend the system properly\n"); >> - pr_warn("Unless you actually understand what nomodeset does, you shoul= d 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_d= evice *dev) >> void drm_mode_config_reset(struct drm_device *dev); >> void drm_mode_config_cleanup(struct drm_device *dev); >>=20=20=20 >> +#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 >>=20=20=20 >> -#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); >>=20=20=20 >> /* For deferred console takeover */ >>=20 --=20 Jani Nikula, Intel Open Source Graphics Center