Received: by 2002:a05:6a10:5bc5:0:0:0:0 with SMTP id os5csp2657865pxb; Fri, 5 Nov 2021 02:24:06 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw2Nsyc8ZTSmlge+JoXvSQQxIWXz1NXx47lrxj3/P5brkp/Ji7cIwPfYEauf7BRmBej/oWQ X-Received: by 2002:a6b:7802:: with SMTP id j2mr36072697iom.95.1636104246516; Fri, 05 Nov 2021 02:24:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1636104246; cv=none; d=google.com; s=arc-20160816; b=cKqZ2e4b35BvtqE5sjAln9LQx48uZp4qjrq/VzwlZefs2xH9fenYaXOCW/hd1rBCaE SXLWtGlR13heWymdv0650q8zfjqnCh7BsmypeCV7xP2M/R16MgGkqHPd+0fQyy0O2v2N ELmpsEIVriNamS+QiCy5Xoe+Q1H45qle0cm7HrG9FeSPhkUjp+h3UHSmWhKtMZmYvmkL aQIKUMVPm6byp1MS3HUK6u/RKT/VBEbw/xDJtGOEFdAcGz9iFmcUtO3me+7UOhCemeQ+ 2WzXu6UGGF8JhJVuHue3WqeqoKo4YH8MRbCxo98a+McC+3CnB4KQO/d6lPuwBiAkTwdh ZxHA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:references:organization:in-reply-to:subject:cc:to :from; bh=iMcLBNdMJ/kyaB9sSHiLKvwl0bDwpygtZ5Lz874boEI=; b=vipGG1YkKqHXW2rnwDzOvYqj4VCtI6dhOhtLkiTIxcVoto8kOJJrJmzfqcS4zeBhW7 YWU71LCWWA8s01GxYyJM8wrkfA79d5NrDxc0L+5Hq5D0CypPkbwBfD6Z06b1RXGeR9SI HGHogJmyEDTrNrNPMS3PTjAoviRX+G83W/jlSCm6Z72LT50wbj3en0Q6+CU0S2G659Np jzQV/s/Z0Ofnti/y0YJ63lbKJ94IpkLlRuSSIVKRRKSZyLhInSv00F/Y1B2oLqSzhmb5 fdMIqhOtrcM13MKgRhJvBG2j151oMRsU6Pz8LKxFhhJmxbOnfLTD55kk0gtkwOod0gaM PYeA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id q17si10455789jah.106.2021.11.05.02.23.53; Fri, 05 Nov 2021 02:24:06 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232674AbhKEJZK convert rfc822-to-8bit (ORCPT + 99 others); Fri, 5 Nov 2021 05:25:10 -0400 Received: from mga17.intel.com ([192.55.52.151]:34006 "EHLO mga17.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232620AbhKEJZI (ORCPT ); Fri, 5 Nov 2021 05:25:08 -0400 X-IronPort-AV: E=McAfee;i="6200,9189,10158"; a="212615363" X-IronPort-AV: E=Sophos;i="5.87,211,1631602800"; d="scan'208";a="212615363" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Nov 2021 02:22:28 -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 Cc: Michel =?utf-8?Q?D=C3=A4nzer?= , Daniel Vetter , Peter Robinson , Pekka Paalanen , Alex Deucher , Ben Skeggs , Chia-I Wu , Christian =?utf-8?Q?K=C3=B6nig?= , Daniel Vetter , Dave Airlie , David Airlie , Gerd Hoffmann , Greg Kroah-Hartman , Gurchetan Singh , Hans de Goede , Joonas Lahtinen , Maarten Lankhorst , Maxime Ripard , "Pan\, Xinhui" , Rodrigo Vivi , VMware Graphics , Zack Rusin , amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, linux-fbdev@vger.kernel.org, nouveau@lists.freedesktop.org, spice-devel@lists.freedesktop.org, virtualization@lists.linux-foundation.org Subject: Re: [PATCH v2 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem 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: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. >> >> 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 >> Signed-off-by: Javier Martinez Canillas >> --- >> >> 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 >> -#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 >> */ >> >> -#include >> #include >> #include >> >> 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 >> +#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 = 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 >> - > > 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 >> #include >> #include >> #include >> 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 >> #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 @@ >> >> #include "qxl_drv.h" >> >> -#include >> #include >> #include >> #include >> 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 >> -#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 >> >> -#include >> #include >> >> #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 >> */ >> >> -#include >> #include >> #include >> #include >> 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 > * Hans de Goede >> */ >> -#include >> #include >> #include >> #include >> 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 >> -#include >> #include >> #include >> #include >> 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 >> #include >> #include >> #include >> 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