From: Hans de Goede <hdegoede@redhat.com>
To: "dri-devel@lists.freedesktop.org"
<dri-devel@lists.freedesktop.org>,
wayland <wayland-devel@lists.freedesktop.org>
Cc: Sebastian Wick <sebastian.wick@redhat.com>,
Martin Roukala <martin.roukala@mupuf.org>,
Christoph Grenz <christophg+lkml@grenz-bonn.de>,
Yusuf Khan <yusisamerican@gmail.com>
Subject: [RFC] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
Date: Wed, 13 Apr 2022 18:24:09 +0200 [thread overview]
Message-ID: <98519ba0-7f18-201a-ea34-652f50343158@redhat.com> (raw)
Hi All,
As discussed in the "[RFC] drm/kms: control display brightness through
drm_connector properties" thread, step 1 of the plan is to stop
registering multiple /sys/class/backlight devs for a single display.
On x86 there can be multiple firmware + direct-hw-access methods
for controlling the backlight and in some cases the kernel registers
multiple backlight-devices for a single internal laptop LCD panel,
2 common scenarios where this happens are:
1) i915 and nouveau unconditionally register their "native" backlight dev
even on devices where /sys/class/backlight/acpi_video0 must be used
to control the backlight, relying on userspace to prefer the "firmware"
acpi_video0 device over "native" devices.
2) amdgpu and nouveau rely on the acpi_video driver initializing before
them, which currently causes /sys/class/backlight/acpi_video0 to usually
show up and then they register their own native backlight driver after
which the drivers/acpi/video_detect.c code unregisters the acpi_video0
device. This means that userspace briefly sees 2 devices and the
disappearing of acpi_video0 after a brief time confuses the systemd
backlight level save/restore code, see e.g.:
https://bbs.archlinux.org/viewtopic.php?id=269920
Fixing kms driver unconditionally register their "native" backlight dev
=======================================================================
The plan for fixing 1) is to add a "bool native_backlight_available"
parameter to acpi_video_get_backlight_type(), which will be set to
true when called by e.g. the i915 code to check if it should register
its native backlight-device. This way acpi_video_get_backlight_type()
will know that a native backlight-device is (will be) available even
though it has not been registered yet and then it can return
acpi_backlight_native if that is the best option.
And then the i915 code will only actually register its native
backlight when acpi_backlight_native gets returned, thus hiding it
in scenario 1.
Fixing acpi_video0 getting registered for a brief time
======================================================
ATM the acpi_video code will delay parsing the ACPI video extensions
when an i915 opregion is present and will immediately parse these
+ register an acpi_video backlight device on laptops without Intel
integrated graphics. On laptops with i915 gfx the i915 driver calls
acpi_video_register() near the end of its probe() function when things
are ready for the acpi_video code to run, avoiding scenario 2.
Where as on systems without i915 gfx acpi_video's module_init()
immediately calls acpi_video_register() and then later the ACPI
video_detect code calls acpi_video_unregister_backlight() to hide
the acpi_video# backlight-device on systems where the native
backlight-device should be used. The plan to fix this is to add
an acpi_video_register_backlight() and to make acpi_video_register()
do all the usual ACPI video extension probing, but have it skip
the actual registering of the backlight devices and have drivers
explicitly call acpi_video_register() after they have setup their
own native backlight-device. This way acpi_video_get_backlight_type()
already will know that a native backlight-device is available
(and preferred) when acpi_video_register_backlight() runs and
the registering of the acpi_video# device will then be skipped,
removing it briefly showing up and disappearing again.
One problem with this approach is that this relies on the GPU
driver to call acpi_video_register_backlight() when it is done.
One could argue that this is actually a feature, we have had
issues with some desktops where acpi_video falsely registers
its backlight (even though there is no internal LCD panel), but
this will likely cause issues on some systems (1). So the plan is
to queue a delayed work with an 8 second (1) delay from
acpi_video_register() and have that register the backlight-device
if not done already.
Other issues
============
The above only takes native vs acpi_video backlight issues into
account, there are also a couple of other scenarios which may
lead to multiple backlight-devices getting registered:
1) Apple laptops using the apple_gmux driver
2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver
3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type()
to override the normal acpi_video_get_backlight_type() heuristics after
the native/acpi_video drivers have already loaded
The plan for 1) + 2) is to extend the acpi_backlight_type enum with
acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add
detection of these 2 to acpi_video_get_backlight_type().
The plan for 3) is to move the DMI quirks from drivers/platform/x86
drivers which call acpi_video_set_dmi_backlight_type() in to the
existing DMI quirk table in drivers/acpi/video_detect.c, so that they
will be available during the first/every call of
acpi_video_get_backlight_type() and then remove
acpi_video_set_dmi_backlight_type()
Regards,
Hans
Footnotes:
1) E.g. systems using the nvidia binary driver, esp. systems using
the legacy nvidia binary driver versions. I at least expect the current
nvidia binary driver versions to get updated to actually call
acpi_video_register_backlight() when necessary.
2) The 8 second delay is the timeout plymouth uses waiting for a kms
device to show up when showing the bootsplash. This is based on the
amdgpu driver taking 5 seconds to initialize on some systems +
margin.
reply other threads:[~2022-04-13 16:24 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98519ba0-7f18-201a-ea34-652f50343158@redhat.com \
--to=hdegoede@redhat.com \
--cc=christophg+lkml@grenz-bonn.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=martin.roukala@mupuf.org \
--cc=sebastian.wick@redhat.com \
--cc=wayland-devel@lists.freedesktop.org \
--cc=yusisamerican@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).