dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
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).