Re: [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/18/22 03:44, Jani Nikula wrote:
On Tue, 17 May 2022, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi All,

As mentioned in my RFC titled "drm/kms: control display brightness through
drm_connector properties":
https://lore.kernel.org/dri-devel/0d188965-d809-81b5-74ce-7d30c49fee2d@xxxxxxxxxx/

The first step towards this is to deal with some existing technical debt
in backlight handling on x86/ACPI boards, specifically we need to stop
registering multiple /sys/class/backlight devs for a single display.
I guess my question here is, how do you know it's for a *single*
display?

There are already designs out there with two flat panels, with
independent brightness controls. They're still rare and I don't think we
handle them very well. But we've got code to register multiple native
backlight interfaces, see e.g. commit 20f85ef89d94 ("drm/i915/backlight:
use unique backlight device names").


This is one of my concerns as well. There are a small number of (mostly somewhat old) designs with more than one internal panel. Since the intent here is to tie the new backlight UAPI to DRM connectors, I think it should remain possible to address each panel individually (the goal is to stop having multiple backlight handlers, all of which control the same display, not to stop having multiple backlight handlers in general). Where I think it might get tricky is when the same display might be driven by one GPU or another at different times, for example, with a switchable mux. Notebook designs which use the nvidia-wmi-ec-backlight driver will use the same backlight handler for the display regardless of which GPU is currently driving it, but I believe there are other designs where the same panel might have backlight controlled by either the iGPU or the dGPU, depending on the mux state.


So imagine a design where one of the panels needs backlight control via
ACPI and the other via native backlight control. Granted, I don't know
if one exists, but I think it's very much in the realm of possible
things the OEMs might do. For example, have an EC PWM for primary panel
backlight, and use GPU PWM for secondary. How do you know you actually
do need to register two interfaces?


The "how do you know" question is one I am wondering as well. I need to check with some of our backlight experts here at NVIDIA to figure out how exactly reliably we are able to make this determination.


I'm fine with dealing with such cases as they arise to avoid
over-engineering up front, but I also don't want us to completely paint
ourselves in a corner either.

BR,
Jani.


This series implements my RFC describing my plan for these cleanups:
https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@xxxxxxxxxx/

Specifically patches 1-6 implement the "Fixing kms driver unconditionally
register their "native" backlight dev" part.

And patches 7-14 implement the "Fixing acpi_video0 getting registered for
a brief time" time.

Note this series does not deal yet with the "Other issues" part, I plan
to do a follow up series for that.

The changes in this series are good to have regardless of the further
"drm/kms: control display brightness through drm_connector properties"
plans. So I plan to push these upstream once they are ready (once
reviewed). Since this crosses various subsystems / touches multiple
kms drivers my plan is to provide an immutable branch based on say
5.19-rc1 and then have that get merged into all the relevant trees.

Please review.

Regards,

Hans


Hans de Goede (14):
   ACPI: video: Add a native function parameter to
     acpi_video_get_backlight_type()
   drm/i915: Don't register backlight when another backlight should be
     used
   drm/amdgpu: Don't register backlight when another backlight should be
     used
   drm/radeon: Don't register backlight when another backlight should be
     used
   drm/nouveau: Don't register backlight when another backlight should be
     used
   ACPI: video: Drop backlight_device_get_by_type() call from
     acpi_video_get_backlight_type()
   ACPI: video: Remove acpi_video_bus from list before tearing it down
   ACPI: video: Simplify acpi_video_unregister_backlight()
   ACPI: video: Make backlight class device registration a separate step
   ACPI: video: Remove code to unregister acpi_video backlight when a
     native backlight registers
   drm/i915: Call acpi_video_register_backlight()
   drm/nouveau: Register ACPI video backlight when nv_backlight
     registration fails
   drm/amdgpu: Register ACPI video backlight when skipping amdgpu
     backlight registration
   drm/radeon: Register ACPI video backlight when skipping radeon
     backlight registration

  drivers/acpi/acpi_video.c                     | 69 ++++++++++++++-----
  drivers/acpi/video_detect.c                   | 53 +++-----------
  drivers/gpu/drm/Kconfig                       |  2 +
  .../gpu/drm/amd/amdgpu/atombios_encoders.c    | 14 +++-
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  9 +++
  .../gpu/drm/i915/display/intel_backlight.c    |  7 ++
  drivers/gpu/drm/i915/display/intel_display.c  |  1 +
  drivers/gpu/drm/i915/display/intel_opregion.c |  2 +-
  drivers/gpu/drm/nouveau/nouveau_backlight.c   | 14 ++++
  drivers/gpu/drm/radeon/atombios_encoders.c    |  7 ++
  drivers/gpu/drm/radeon/radeon_encoders.c      | 11 ++-
  .../gpu/drm/radeon/radeon_legacy_encoders.c   |  7 ++
  drivers/platform/x86/acer-wmi.c               |  2 +-
  drivers/platform/x86/asus-laptop.c            |  2 +-
  drivers/platform/x86/asus-wmi.c               |  4 +-
  drivers/platform/x86/compal-laptop.c          |  2 +-
  drivers/platform/x86/dell/dell-laptop.c       |  2 +-
  drivers/platform/x86/eeepc-laptop.c           |  2 +-
  drivers/platform/x86/fujitsu-laptop.c         |  4 +-
  drivers/platform/x86/ideapad-laptop.c         |  2 +-
  drivers/platform/x86/intel/oaktrail.c         |  2 +-
  drivers/platform/x86/msi-laptop.c             |  2 +-
  drivers/platform/x86/msi-wmi.c                |  2 +-
  drivers/platform/x86/samsung-laptop.c         |  2 +-
  drivers/platform/x86/sony-laptop.c            |  2 +-
  drivers/platform/x86/thinkpad_acpi.c          |  4 +-
  drivers/platform/x86/toshiba_acpi.c           |  2 +-
  include/acpi/video.h                          |  8 ++-
  28 files changed, 156 insertions(+), 84 deletions(-)



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux