Re: [Nouveau] [PATCH v5 3/5] drm/dp: Don't read back backlight mode in drm_edp_backlight_enable()

[Doug Anderson]

Hi,

On Fri, Nov 5, 2021 at 11:34 AM Lyude Paul  wrote:
>
> As it turns out, apparently some machines will actually leave additional
> backlight functionality like dynamic backlight control on before the OS
> loads. Currently we don't take care to disable unsupported features when
> writing back the backlight mode, which can lead to some rather strange
> looking behavior when adjusting the backlight.
>
> So, let's fix this by just not reading back the current backlight mode on
> initial enable. I don't think there should really be any downsides to this,
> and this will ensure we don't leave any unsupported functionality enabled.
>
> This should fix at least one (but not all) of the issues seen with DPCD
> backlight support on fi-bdw-samus
>
> v5:
> * Just avoid reading back DPCD register - Doug Anderson
>
> Signed-off-by: Lyude Paul 
> Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM 
> helpers")
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 40 ++---
>  1 file changed, 12 insertions(+), 28 deletions(-)

You forgot to CC me on this one! ;-)

This looks good to me now, so FWIW:

Reviewed-by: Douglas Anderson 

[Nouveau] [PATCH v5 5/5] drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()

[Lyude Paul]

Hooray! We've managed to hit enough bugs upstream that I've been able to
come up with a pretty solid explanation for how backlight controls are
actually supposed to be detected and used these days. As well, having the
rest of the PWM bits in VESA's backlight interface implemented seems to
have fixed all of the problematic brightness controls laptop panels that
we've hit so far.

So, let's actually document this instead of just calling the laptop panels
liars. As well, I would like to formally apologize to all of the laptop
panels I called liars. I'm sorry laptop panels, hopefully you can all
forgive me and we can move past this~

Signed-off-by: Lyude Paul 
Acked-by: Ville Syrjälä 
---
 .../drm/i915/display/intel_dp_aux_backlight.c| 16 +++-
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c 
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 96fe3eaba44a..8b9c925c4c16 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -456,11 +456,17 @@ int intel_dp_aux_init_backlight_funcs(struct 
intel_connector *connector)
}
 
/*
-* A lot of eDP panels in the wild will report supporting both the
-* Intel proprietary backlight control interface, and the VESA
-* backlight control interface. Many of these panels are liars though,
-* and will only work with the Intel interface. So, always probe for
-* that first.
+* Since Intel has their own backlight control interface, the majority 
of machines out there
+* using DPCD backlight controls with Intel GPUs will be using this 
interface as opposed to
+* the VESA interface. However, other GPUs (such as Nvidia's) will 
always use the VESA
+* interface. This means that there's quite a number of panels out 
there that will advertise
+* support for both interfaces, primarily systems with Intel/Nvidia 
hybrid GPU setups.
+*
+* There's a catch to this though: on many panels that advertise 
support for both
+* interfaces, the VESA backlight interface will stop working once 
we've programmed the
+* panel with Intel's OUI - which is also required for us to be able to 
detect Intel's
+* backlight interface at all. This means that the only sensible way 
for us to detect both
+* interfaces is to probe for Intel's first, and VESA's second.
 */
if (try_intel_interface && 
intel_dp_aux_supports_hdr_backlight(connector)) {
drm_dbg_kms(dev, "Using Intel proprietary eDP backlight 
controls\n");
-- 
2.31.1

[Nouveau] [PATCH v5 4/5] drm/dp, drm/i915: Add support for VESA backlights using PWM for brightness control

[Lyude Paul]

Now that we've added support to i915 for controlling panel backlights that
need PWM to be enabled/disabled, let's finalize this and add support for
controlling brightness levels via PWM as well. This should hopefully put us
towards the path of supporting _ALL_ backlights via VESA's DPCD interface
which would allow us to finally start trusting the DPCD again.

Note however that we still don't enable using this by default on i915 when
it's not needed, primarily because I haven't yet had a chance to confirm if
it's safe to do this on the one machine in Intel's CI that had an issue
with this: samus-fi-bdw. I have done basic testing of this on other
machines though, by manually patching i915 to force it into PWM-only mode
on some of my laptops.

v2:
* Correct documentation (thanks Doug!)
* Get rid of backlight caps

Signed-off-by: Lyude Paul 
Reviewed-by: Doug Anderson 
Cc: Rajeev Nandan 
Cc: Satadru Pramanik 
---
 drivers/gpu/drm/drm_dp_helper.c   | 72 +--
 .../drm/i915/display/intel_dp_aux_backlight.c | 44 +---
 include/drm/drm_dp_helper.h   |  7 +-
 3 files changed, 89 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index af2aad2f4725..23f9073bc473 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3290,6 +3290,10 @@ int drm_edp_backlight_set_level(struct drm_dp_aux *aux, 
const struct drm_edp_bac
int ret;
u8 buf[2] = { 0 };
 
+   /* The panel uses the PWM for controlling brightness levels */
+   if (!bl->aux_set)
+   return 0;
+
if (bl->lsb_reg_used) {
buf[0] = (level & 0xff00) >> 8;
buf[1] = (level & 0x00ff);
@@ -3316,7 +3320,7 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux, 
const struct drm_edp_backli
int ret;
u8 buf;
 
-   /* The panel uses something other then DPCD for enabling its backlight 
*/
+   /* This panel uses the EDP_BL_PWR GPIO for enablement */
if (!bl->aux_enable)
return 0;
 
@@ -3351,11 +3355,11 @@ drm_edp_backlight_set_enable(struct drm_dp_aux *aux, 
const struct drm_edp_backli
  * restoring any important backlight state such as the given backlight level, 
the brightness byte
  * count, backlight frequency, etc.
  *
- * Note that certain panels, while supporting brightness level controls over 
DPCD, may not support
- * having their backlights enabled via the standard 
%DP_EDP_DISPLAY_CONTROL_REGISTER. On such panels
- * &drm_edp_backlight_info.aux_enable will be set to %false, this function 
will skip the step of
- * programming the %DP_EDP_DISPLAY_CONTROL_REGISTER, and the driver must 
perform the required
- * implementation specific step for enabling the backlight after calling this 
function.
+ * Note that certain panels do not support being enabled or disabled via DPCD, 
but instead require
+ * that the driver handle enabling/disabling the panel through 
implementation-specific means using
+ * the EDP_BL_PWR GPIO. For such panels, &drm_edp_backlight_info.aux_enable 
will be set to %false,
+ * this function becomes a no-op, and the driver is expected to handle 
powering the panel on using
+ * the EDP_BL_PWR GPIO.
  *
  * Returns: %0 on success, negative error code on failure.
  */
@@ -3363,7 +3367,12 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux, 
const struct drm_edp_backli
 const u16 level)
 {
int ret;
-   u8 dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+   u8 dpcd_buf;
+
+   if (bl->aux_set)
+   dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
+   else
+   dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_PWM;
 
if (bl->pwmgen_bit_count) {
ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, 
bl->pwmgen_bit_count);
@@ -3405,12 +3414,13 @@ EXPORT_SYMBOL(drm_edp_backlight_enable);
  * @aux: The DP AUX channel to use
  * @bl: Backlight capability info from drm_edp_backlight_init()
  *
- * This function handles disabling DPCD backlight controls on a panel over 
AUX. Note that some
- * panels have backlights that are enabled/disabled by other means, despite 
having their brightness
- * values controlled through DPCD. On such panels 
&drm_edp_backlight_info.aux_enable will be set to
- * %false, this function will become a no-op (and we will skip updating
- * %DP_EDP_DISPLAY_CONTROL_REGISTER), and the driver must take care to perform 
it's own
- * implementation specific step for disabling the backlight.
+ * This function handles disabling DPCD backlight controls on a panel over AUX.
+ *
+ * Note that certain panels do not support being enabled or disabled via DPCD, 
but instead require
+ * that the driver handle enabling/disabling the panel through 
implementation-specific means using
+ * the EDP_BL_PWR GPIO. For such panels, &drm_edp_backlight_info.aux_enable 
will be set to %false,
+ * this function becomes a

[Nouveau] [PATCH v5 2/5] drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux enable/brightness

[Lyude Paul]

Since we don't support hybrid AUX/PWM backlights in nouveau right now,
let's add some explicit checks so that we don't break nouveau once we
enable support for these backlights in other drivers.

Reviewed-by: Karol Herbst 
Signed-off-by: Lyude Paul 
---
 drivers/gpu/drm/nouveau/nouveau_backlight.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_backlight.c 
b/drivers/gpu/drm/nouveau/nouveau_backlight.c
index 1cbd71abc80a..ae2f2abc8f5a 100644
--- a/drivers/gpu/drm/nouveau/nouveau_backlight.c
+++ b/drivers/gpu/drm/nouveau/nouveau_backlight.c
@@ -308,7 +308,10 @@ nv50_backlight_init(struct nouveau_backlight *bl,
if (ret < 0)
return ret;
 
-   if (drm_edp_backlight_supported(edp_dpcd)) {
+   /* TODO: Add support for hybrid PWM/DPCD panels */
+   if (drm_edp_backlight_supported(edp_dpcd) &&
+   (edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
+   (edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_AUX_SET_CAP)) {
NV_DEBUG(drm, "DPCD backlight controls supported on 
%s\n",
 nv_conn->base.name);
 
-- 
2.31.1

[Nouveau] [PATCH v5 3/5] drm/dp: Don't read back backlight mode in drm_edp_backlight_enable()

[Lyude Paul]

As it turns out, apparently some machines will actually leave additional
backlight functionality like dynamic backlight control on before the OS
loads. Currently we don't take care to disable unsupported features when
writing back the backlight mode, which can lead to some rather strange
looking behavior when adjusting the backlight.

So, let's fix this by just not reading back the current backlight mode on
initial enable. I don't think there should really be any downsides to this,
and this will ensure we don't leave any unsupported functionality enabled.

This should fix at least one (but not all) of the issues seen with DPCD
backlight support on fi-bdw-samus

v5:
* Just avoid reading back DPCD register - Doug Anderson

Signed-off-by: Lyude Paul 
Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM 
helpers")
---
 drivers/gpu/drm/drm_dp_helper.c | 40 ++---
 1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ada0a1ff262d..af2aad2f4725 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -3363,27 +3363,13 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux, 
const struct drm_edp_backli
 const u16 level)
 {
int ret;
-   u8 dpcd_buf, new_dpcd_buf;
+   u8 dpcd_buf = DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
 
-   ret = drm_dp_dpcd_readb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, 
&dpcd_buf);
-   if (ret != 1) {
-   drm_dbg_kms(aux->drm_dev,
-   "%s: Failed to read backlight mode: %d\n", 
aux->name, ret);
-   return ret < 0 ? ret : -EIO;
-   }
-
-   new_dpcd_buf = dpcd_buf;
-
-   if ((dpcd_buf & DP_EDP_BACKLIGHT_CONTROL_MODE_MASK) != 
DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
-   new_dpcd_buf &= ~DP_EDP_BACKLIGHT_CONTROL_MODE_MASK;
-   new_dpcd_buf |= DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD;
-
-   if (bl->pwmgen_bit_count) {
-   ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, 
bl->pwmgen_bit_count);
-   if (ret != 1)
-   drm_dbg_kms(aux->drm_dev, "%s: Failed to write 
aux pwmgen bit count: %d\n",
-   aux->name, ret);
-   }
+   if (bl->pwmgen_bit_count) {
+   ret = drm_dp_dpcd_writeb(aux, DP_EDP_PWMGEN_BIT_COUNT, 
bl->pwmgen_bit_count);
+   if (ret != 1)
+   drm_dbg_kms(aux->drm_dev, "%s: Failed to write aux 
pwmgen bit count: %d\n",
+   aux->name, ret);
}
 
if (bl->pwm_freq_pre_divider) {
@@ -3393,16 +3379,14 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux, 
const struct drm_edp_backli
"%s: Failed to write aux backlight 
frequency: %d\n",
aux->name, ret);
else
-   new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
+   dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE;
}
 
-   if (new_dpcd_buf != dpcd_buf) {
-   ret = drm_dp_dpcd_writeb(aux, 
DP_EDP_BACKLIGHT_MODE_SET_REGISTER, new_dpcd_buf);
-   if (ret != 1) {
-   drm_dbg_kms(aux->drm_dev, "%s: Failed to write aux 
backlight mode: %d\n",
-   aux->name, ret);
-   return ret < 0 ? ret : -EIO;
-   }
+   ret = drm_dp_dpcd_writeb(aux, DP_EDP_BACKLIGHT_MODE_SET_REGISTER, 
dpcd_buf);
+   if (ret != 1) {
+   drm_dbg_kms(aux->drm_dev, "%s: Failed to write aux backlight 
mode: %d\n",
+   aux->name, ret);
+   return ret < 0 ? ret : -EIO;
}
 
ret = drm_edp_backlight_set_level(aux, bl, level);
-- 
2.31.1

[Nouveau] [PATCH v5 1/5] drm/i915: Add support for panels with VESA backlights with PWM enable/disable

[Lyude Paul]

This simply adds proper support for panel backlights that can be controlled
via VESA's backlight control protocol, but which also require that we
enable and disable the backlight via PWM instead of via the DPCD interface.
We also enable this by default, in order to fix some people's backlights
that were broken by not having this enabled.

For reference, backlights that require this and use VESA's backlight
interface tend to be laptops with hybrid GPUs, but this very well may
change in the future.

v4:
* Make sure that we call intel_backlight_level_to_pwm() in
  intel_dp_aux_vesa_enable_backlight() - vsyrjala

Signed-off-by: Lyude Paul 
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/3680
Fixes: fe7d52bccab6 ("drm/i915/dp: Don't use DPCD backlights that need PWM 
enable/disable")
Reviewed-by: Ville Syrjälä 
Cc:  # v5.12+
---
 .../drm/i915/display/intel_dp_aux_backlight.c | 27 ++-
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c 
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index 569d17b4d00f..f05b71c01b8e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -293,6 +293,13 @@ intel_dp_aux_vesa_enable_backlight(const struct 
intel_crtc_state *crtc_state,
struct intel_panel *panel = &connector->panel;
struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
+   if (!panel->backlight.edp.vesa.info.aux_enable) {
+   u32 pwm_level = intel_backlight_invert_pwm_level(connector,
+
panel->backlight.pwm_level_max);
+
+   panel->backlight.pwm_funcs->enable(crtc_state, conn_state, 
pwm_level);
+   }
+
drm_edp_backlight_enable(&intel_dp->aux, 
&panel->backlight.edp.vesa.info, level);
 }
 
@@ -304,6 +311,10 @@ static void intel_dp_aux_vesa_disable_backlight(const 
struct drm_connector_state
struct intel_dp *intel_dp = enc_to_intel_dp(connector->encoder);
 
drm_edp_backlight_disable(&intel_dp->aux, 
&panel->backlight.edp.vesa.info);
+
+   if (!panel->backlight.edp.vesa.info.aux_enable)
+   panel->backlight.pwm_funcs->disable(old_conn_state,
+   
intel_backlight_invert_pwm_level(connector, 0));
 }
 
 static int intel_dp_aux_vesa_setup_backlight(struct intel_connector 
*connector, enum pipe pipe)
@@ -321,6 +332,15 @@ static int intel_dp_aux_vesa_setup_backlight(struct 
intel_connector *connector,
if (ret < 0)
return ret;
 
+   if (!panel->backlight.edp.vesa.info.aux_enable) {
+   ret = panel->backlight.pwm_funcs->setup(connector, pipe);
+   if (ret < 0) {
+   drm_err(&i915->drm,
+   "Failed to setup PWM backlight controls for eDP 
backlight: %d\n",
+   ret);
+   return ret;
+   }
+   }
panel->backlight.max = panel->backlight.edp.vesa.info.max;
panel->backlight.min = 0;
if (current_mode == DP_EDP_BACKLIGHT_CONTROL_MODE_DPCD) {
@@ -340,12 +360,7 @@ intel_dp_aux_supports_vesa_backlight(struct 
intel_connector *connector)
struct intel_dp *intel_dp = intel_attached_dp(connector);
struct drm_i915_private *i915 = dp_to_i915(intel_dp);
 
-   /* TODO: We currently only support AUX only backlight configurations, 
not backlights which
-* require a mix of PWM and AUX controls to work. In the mean time, 
these machines typically
-* work just fine using normal PWM controls anyway.
-*/
-   if ((intel_dp->edp_dpcd[1] & DP_EDP_BACKLIGHT_AUX_ENABLE_CAP) &&
-   drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
+   if (drm_edp_backlight_supported(intel_dp->edp_dpcd)) {
drm_dbg_kms(&i915->drm, "AUX Backlight Control Supported!\n");
return true;
}
-- 
2.31.1

[Nouveau] [PATCH v5 0/5] drm/dp, drm/i915: Finish basic PWM support for VESA backlight helpers

[Lyude Paul]

When I originally moved all of the VESA backlight code in i915 into DRM
helpers, one of the things I didn't have the hardware or time for
testing was machines that used a combination of PWM and DPCD in order to
control their backlights. This has since then caused some breakages and
resulted in us disabling DPCD backlight support on such machines. This
works fine, unless you have a machine that actually needs this
functionality for backlight controls to work at all. Additionally, we
will need to support PWM for when we start adding support for VESA's
product (as in the product of multiplication) control mode for better
brightness ranges.

So - let's finally finish up implementing basic support for these types
of backlights to solve these problems in our DP helpers, along with
implementing support for this in i915. And since digging into this issue
solved the last questions we really had about probing backlights in i915
for the most part, let's update some of the comments around that as
well!

Lyude Paul (5):
  drm/i915: Add support for panels with VESA backlights with PWM
enable/disable
  drm/nouveau/kms/nv50-: Explicitly check DPCD backlights for aux
enable/brightness
  drm/dp: Don't read back backlight mode in drm_edp_backlight_enable()
  drm/dp, drm/i915: Add support for VESA backlights using PWM for
brightness control
  drm/i915: Clarify probing order in intel_dp_aux_init_backlight_funcs()

 drivers/gpu/drm/drm_dp_helper.c   | 108 ++
 .../drm/i915/display/intel_dp_aux_backlight.c |  81 ++---
 drivers/gpu/drm/nouveau/nouveau_backlight.c   |   5 +-
 include/drm/drm_dp_helper.h   |   7 +-
 4 files changed, 132 insertions(+), 69 deletions(-)

-- 
2.31.1

Re: [Nouveau] Ubuntu 20.04 and vlc

[Jerry Geis]

On Fri, Nov 5, 2021 at 11:02 AM Ilia Mirkin  wrote:

> Hey Jerry,
>
> I'd look in the kernel log to see what's up. Perhaps the GPU hangs?
>
> Cheers,
>
>   -ilia
>
> On Fri, Nov 5, 2021 at 10:42 AM Jerry Geis  wrote:
> >
> > Hi All,
> >
> > I am using Ubuntu 20.04 with VLC... normally the box boots up runs an
> plays videos just fien with about 20% usage for VLC (celeron and or intel
> Atom hardware).
> > After some time - day or two of playing videos. Something happens.
> > The CPU usage jumps way up like 120% kind of usage.  Like the vdpau
> stuff is no longer working.
> >
> > Is this a know issue ?
> > What should I look for  ?
> > What can I provide to help ?
> >
> >  Intel(R) Atom(TM) CPU D525   @ 1.80GHz
> > 03:00.0 VGA compatible controller: NVIDIA Corporation GT218 [ION] (rev
> a2)
> >
> > Thanks
> >
> > Jerry
>

dmesg log
   37.212407] kernel: bpfilter: Loaded bpfilter_umh pid 794
[   37.214069] unknown: Started bpfilter
[   38.778000] kernel: RTL8211B Gigabit Ethernet r8169-0-458:00: attached
PHY driver (mii_bus:phy_addr=r8169-0-458:00, irq=MAC)
[   38.856101] kernel: r8169 :04:0b.0 enp4s11: Link is Down
[   41.275140] kernel: r8169 :04:0b.0 enp4s11: Link is Up - 1Gbps/Full
- flow control off
[   41.275192] kernel: IPv6: ADDRCONF(NETDEV_CHANGE): enp4s11: link becomes
ready


/var/log/syslog
Nov  5 11:11:50 mediaport44 smboot.sh[16157]: libva info: VA-API version
1.7.0
Nov  5 11:11:50 mediaport44 smboot.sh[16157]: libva info: Trying to open
/usr/lib/x86_64-linux-gnu/dri/nouveau_drv_video.so
Nov  5 11:11:50 mediaport44 smboot.sh[16157]: libva info: Found init
function __vaDriverInit_1_7
Nov  5 11:11:50 mediaport44 smboot.sh[16157]: libva info: va_openDriver()
returns 0

if I reboot the box - and look again at syslog it shows the same thing
above.

Not seeing any  errors or anything.

Thoughts?

 Jerry

Re: [Nouveau] Ubuntu 20.04 and vlc

[Ilia Mirkin]

Hey Jerry,

I'd look in the kernel log to see what's up. Perhaps the GPU hangs?

Cheers,

  -ilia

On Fri, Nov 5, 2021 at 10:42 AM Jerry Geis  wrote:
>
> Hi All,
>
> I am using Ubuntu 20.04 with VLC... normally the box boots up runs an plays 
> videos just fien with about 20% usage for VLC (celeron and or intel Atom 
> hardware).
> After some time - day or two of playing videos. Something happens.
> The CPU usage jumps way up like 120% kind of usage.  Like the vdpau stuff is 
> no longer working.
>
> Is this a know issue ?
> What should I look for  ?
> What can I provide to help ?
>
>  Intel(R) Atom(TM) CPU D525   @ 1.80GHz
> 03:00.0 VGA compatible controller: NVIDIA Corporation GT218 [ION] (rev a2)
>
> Thanks
>
> Jerry

[Nouveau] Ubuntu 20.04 and vlc

[Jerry Geis]

Hi All,

I am using Ubuntu 20.04 with VLC... normally the box boots up runs an
plays videos just fien with about 20% usage for VLC (celeron and or intel
Atom hardware).
After some time - day or two of playing videos. Something happens.
The CPU usage jumps way up like 120% kind of usage.  Like the vdpau stuff
is no longer working.

Is this a know issue ?
What should I look for  ?
What can I provide to help ?

 Intel(R) Atom(TM) CPU D525   @ 1.80GHz
03:00.0 VGA compatible controller: NVIDIA Corporation GT218 [ION] (rev a2)

Thanks

Jerry

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

[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 
Signed-off-by: Javier Martinez Canillas 
---

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 mgag

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  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 
>>> Signed-off-by: Javier Martinez Canillas 
>>> ---
>>>
>>> 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

[Nouveau] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()

[Javier Martinez Canillas]

This function is used by some DRM drivers to determine if the "nomodeset"
kernel command line parameter was set and prevent these drivers to probe.

But the function name is quite confusing and does not reflect what really
the drivers are testing when calling it. Use a better naming now that it
is part of the DRM subsystem.

Also, vgacon_text_force() is guarded by #ifdef CONFIG_VGA_CONSOLE already
so there is no need to do the same when calling the function.

Suggested-by: Suggested-by: Daniel Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  2 +-
 drivers/gpu/drm/ast/ast_drv.c   |  2 +-
 drivers/gpu/drm/drm_nomodeset.c | 16 
 drivers/gpu/drm/i915/i915_module.c  |  2 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  2 +-
 drivers/gpu/drm/nouveau/nouveau_drm.c   |  2 +-
 drivers/gpu/drm/qxl/qxl_drv.c   |  2 +-
 drivers/gpu/drm/radeon/radeon_drv.c |  2 +-
 drivers/gpu/drm/tiny/bochs.c|  2 +-
 drivers/gpu/drm/tiny/cirrus.c   |  2 +-
 drivers/gpu/drm/vboxvideo/vbox_drv.c|  4 +---
 drivers/gpu/drm/virtio/virtgpu_drv.c|  2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c |  2 +-
 include/drm/drm_mode_config.h   |  4 ++--
 14 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 2680a2aaa877..f7bd2616cf23 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2513,7 +2513,7 @@ static int __init amdgpu_init(void)
 {
int r;
 
-   if (vgacon_text_force()) {
+   if (drm_modeset_disabled()) {
DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 048be607b182..6706050414c3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -232,7 +232,7 @@ static struct pci_driver ast_pci_driver = {
 
 static int __init ast_init(void)
 {
-   if (vgacon_text_force() && ast_modeset == -1)
+   if (drm_modeset_disabled() && ast_modeset == -1)
return -EINVAL;
 
if (ast_modeset == 0)
diff --git a/drivers/gpu/drm/drm_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
index 1ac9a8d5a8fe..dfc8b30f0625 100644
--- a/drivers/gpu/drm/drm_nomodeset.c
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -3,17 +3,17 @@
 #include 
 #include 
 
-static bool vgacon_text_mode_force;
+static bool drm_nomodeset;
 
-bool vgacon_text_force(void)
+bool drm_modeset_disabled(void)
 {
-   return vgacon_text_mode_force;
+   return drm_nomodeset;
 }
-EXPORT_SYMBOL(vgacon_text_force);
+EXPORT_SYMBOL(drm_modeset_disabled);
 
-static int __init text_mode(char *str)
+static int __init disable_modeset(char *str)
 {
-   vgacon_text_mode_force = true;
+   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");
@@ -22,5 +22,5 @@ static int __init text_mode(char *str)
return 1;
 }
 
-/* force text mode - used by kernel modesetting */
-__setup("nomodeset", text_mode);
+/* 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 14a59226519d..3e5531040e4d 100644
--- a/drivers/gpu/drm/i915/i915_module.c
+++ b/drivers/gpu/drm/i915/i915_module.c
@@ -29,7 +29,7 @@ static int i915_check_nomodeset(void)
if (i915_modparams.modeset == 0)
use_kms = false;
 
-   if (vgacon_text_force() && i915_modparams.modeset == -1)
+   if (drm_modeset_disabled() && 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 685e766db6a4..7ee87564bade 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -377,7 +377,7 @@ static struct pci_driver mgag200_pci_driver = {
 
 static int __init mgag200_init(void)
 {
-   if (vgacon_text_force() && mgag200_modeset == -1)
+   if (drm_modeset_disabled() && mgag200_modeset == -1)
return -EINVAL;
 
if (mgag200_modeset == 0)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 029997f50d1a..903d0e626954 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -1321,7 +1321,7 @@ nouveau_drm_init(void)
nouveau_display_options();
 
if (nouveau_modeset == -1) {
-   if (vgacon_text_force())
+   if (drm_modeset_disabled())
nouveau_modeset = 0;
}
 
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/dri

[Nouveau] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic

[Javier Martinez Canillas]

[ resend with all relevant people as Cc now, sorry to others for the spam ]

There is a lot of historical baggage on this parameter. It's defined in
the vgacon driver as a "nomodeset" parameter, but it's handler function is
called text_mode() that sets a variable named vgacon_text_mode_force whose
value is 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 could 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.

Patch #1 is just a trivial fix for a comment that isn't referring to the
correct kernel parameter.

Patch #2 moves the nomodeset logic to the DRM subsystem.

Patch #3 renames the vgacon_text_force() function and accompaning logic as
drm_modeset_disabled(), which is what this function is really about.

Patch #4 adds a drm_drv_enabled() function that could be used by drivers
to check if could be enabled.

Patch #5 uses the drm_drv_enabled() function to check this instead of just
checking if nomodeset has been set.


Javier Martinez Canillas (5):
  drm/i915: Fix comment about modeset parameters
  drm: Move nomodeset kernel parameter handler to the DRM subsystem
  drm: Rename vgacon_text_force() function to drm_modeset_disabled()
  drm: Add a drm_drv_enabled() helper function
  drm: Use drm_drv_enabled() instead of drm_modeset_disabled()

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

-- 
2.33.1

Re: [Nouveau] [RESEND PATCH 0/5] Cleanups for the nomodeset kernel command line parameter logic

[Javier Martinez Canillas]

Hello Thomas,

On 11/3/21 14:01, Thomas Zimmermann wrote:

[snip]

>>
>>
>> Javier Martinez Canillas (5):
>>drm/i915: Fix comment about modeset parameters
>>drm: Move nomodeset kernel parameter handler to the DRM subsystem
>>drm: Rename vgacon_text_force() function to drm_modeset_disabled()
>>drm: Add a drm_drv_enabled() helper function
>>drm: Use drm_drv_enabled() instead of drm_modeset_disabled()
> 
> There's too much churn here IMHO. Please merge patches 2+3 and 4+5. And 
> I'd put patch (4+5) first, so you have the drivers out of the way. After 
> that patch (2+3) should only modify drm_drv_enabled().
>

Sure, I'm happy with less patches.

Thanks for your feedback.
 
> Best regards
> Thomas
> 
Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

[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-78fdb5d84...@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

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

[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 
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
+
 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 ..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");
+
+   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 
-
 #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
@@ 

Re: [Nouveau] [RESEND PATCH 3/5] drm: Rename vgacon_text_force() function to drm_modeset_disabled()

[Javier Martinez Canillas]

On 11/3/21 13:57, Thomas Zimmermann wrote:

[snip]

>>   
>> -if (vgacon_text_force()) {
>> +if (drm_modeset_disabled()) {
>>  DRM_ERROR("amdgpu kernel modesetting disabled.\n");
> 
> Please remove all such error messages from drivers. 
> drm_modeset_disabled() should print a unified message instead.
>

Agreed.

>> -static bool vgacon_text_mode_force;
>> +static bool drm_nomodeset;
>>   
>> -bool vgacon_text_force(void)
>> +bool drm_modeset_disabled(void)
> 
> I suggest to rename this function to drm_check_modeset() and have it 
> return a negative errno code on failure. This gives maximum flexibility 
> and reduces errors in drivers. Right now the drivers return something 
> like -EINVAL, which seems wrong. Returning -ENODEV seems more appropriate.
>

Good idea. I'll do it in v2 as well.
 
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

[Javier Martinez Canillas]

Hello Jani,

On 11/4/21 20:57, Jani Nikula wrote:
> On Thu, 04 Nov 2021, Javier Martinez Canillas  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

[Javier Martinez Canillas]

On 11/5/21 11:04, Jani Nikula wrote:
> On Fri, 05 Nov 2021, Javier Martinez Canillas  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

[Nouveau] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler 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 that to DRM.

Suggested-by: Daniel Vetter 
Signed-off-by: Javier Martinez Canillas 
---

 drivers/gpu/drm/Makefile|  2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
 drivers/gpu/drm/ast/ast_drv.c   |  1 -
 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 --
 17 files changed, 35 insertions(+), 41 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_nomodeset.c

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1c41156deb5f..0e2d60ea93ca 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-y += 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 c718fb5f3f8a..2680a2aaa877 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 
@@ -2515,7 +2514,7 @@ static int __init amdgpu_init(void)
int r;
 
if (vgacon_text_force()) {
-   DRM_ERROR("VGACON disables amdgpu kernel modesetting.\n");
+   DRM_ERROR("amdgpu kernel modesetting disabled.\n");
return -EINVAL;
}
 
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 86d5cd7b6318..048be607b182 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_nomodeset.c b/drivers/gpu/drm/drm_nomodeset.c
new file mode 100644
index ..1ac9a8d5a8fe
--- /dev/null
+++ b/drivers/gpu/drm/drm_nomodeset.c
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include 
+#include 
+
+static bool vgacon_text_mode_force;
+
+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);
diff --git a/drivers/gpu/drm/i915/i915_module.c 
b/drivers/gpu/drm/i915/i915_module.c
index c7507266aa83..14a59226519d 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 
-
 #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 6b9243713b3c..685e766db6a4 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 1f828c9f691c..029997f50d1a 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 fc47b0deb021..3cd6bd9f059d 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

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

[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] [RESEND PATCH 2/5] drm: Move nomodeset kernel parameter handler to the DRM subsystem

[Javier Martinez Canillas]

Hello Thomas,

On 11/3/21 13:41, Thomas Zimmermann wrote:
> Hi
> 
> Am 03.11.21 um 13:28 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 that to DRM.
>>
>> Suggested-by: Daniel Vetter 
>> Signed-off-by: Javier Martinez Canillas 
>> ---
>>
>>   drivers/gpu/drm/Makefile|  2 ++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c |  3 +--
>>   drivers/gpu/drm/ast/ast_drv.c   |  1 -
>>   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 --
>>   17 files changed, 35 insertions(+), 41 deletions(-)
>>   create mode 100644 drivers/gpu/drm/drm_nomodeset.c
>>
>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>> index 1c41156deb5f..0e2d60ea93ca 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-y += drm_nomodeset.o
> 
> Repeating my other comment, should this rather be protected by a 
> separate config symbol that is selected by CONFIG_DRM?
>

I actually thought about that and my opinion is that obj-y reflects
what we really want here or do you envision this getting disabled
in some cases ?

Probably the problem is Kbuild descending into the drivers/gpu dir
even when CONFIG_DRM is not set. Maybe what we want is something
like the following instead?

diff --git a/drivers/gpu/Makefile b/drivers/gpu/Makefile
index 835c88318cec..ef12ee05ba6e 100644
--- a/drivers/gpu/Makefile
+++ b/drivers/gpu/Makefile
@@ -3,6 +3,7 @@
 # taken to initialize them in the correct order. Link order is the only way
 # to ensure this currently.
 obj-$(CONFIG_TEGRA_HOST1X) += host1x/
-obj-y  += drm/ vga/
+obj-$(CONFIG_DRM)  += drm/
+obj-y  += vga/
 obj-$(CONFIG_IMX_IPUV3_CORE)   += ipu-v3/
 obj-$(CONFIG_TRACE_GPU_MEM)+= trace/

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

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

[Javier Martinez Canillas]

Hello Jani,

On 11/3/21 13:56, Jani Nikula wrote:

[snip]

>>  
>> +obj-y += drm_nomodeset.o
> 
> This is a subtle functional change. With this, you'll always have
> __setup("nomodeset", text_mode) builtin and the parameter available. And
> using nomodeset will print out the pr_warn() splat from text_mode(). But
> removing nomodeset will have no impact if CONFIG_VGA_CONSOLE=n as that
> leads to vgacon_text_force() always returning false.
>

Yes, that's what I decided at the end to make it unconditional. That
way the same behaviour is preserved (even when only DRM drivers are
using the exported symbol).
 
> To not make functional changes, this should be:
> 
> obj-$(CONFIG_VGA_CONSOLE) += drm_nomodeset.o
>

Right, that should work.

> Now, going with the cleanup in this series, maybe we should make the
> functional change, and break the connection to CONFIG_VGA_CONSOLE
> altogether, also in the header?
> 
> (Maybe we'll also need a proxy drm kconfig option to only have
> drm_modeset.o builtin when CONFIG_DRM != n.)
>

See my other email. I believe the issue is drivers/gpu/drm always
being included even when CONFIG_DRM is not set.

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]

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


OpenPGP_signature
Description: OpenPGP digital signature

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

[Thomas Zimmermann]

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


OpenPGP_signature
Description: OpenPGP digital signature

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  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  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 2/2] drm: Move nomodeset kernel parameter to the DRM subsystem

[Thomas Zimmermann]

Hi

Am 05.11.21 um 10:22 schrieb Jani Nikula:

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.


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 

-#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 ..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 
DISABLE

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

[Jani Nikula]

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

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

[Thomas Zimmermann]

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.


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 

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

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

[Thomas Zimmermann]

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


OpenPGP_signature
Description: OpenPGP digital signature