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 Wed, May 18, 2022 at 01:12:33PM +0300, Jani Nikula wrote:
> On Wed, 18 May 2022, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > Hi,
> >
> > On 5/18/22 10: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").
> >> 
> >> 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?
> >
> > On x86/ACPI devices this is all driven by acpi_video_get_backlight_type() /
> > by the drivers/acpi/video_detect.c code. That code already will break on
> > systems where there are 2 backlight controls, with one being ACPI based
> > and the other a native GPU PWM backlight device.
> >
> > In this scenario as soon as the native GPU PWM backlight device shows up
> > then, if acpi_video_get_backlight_type()==native, video_detect.c will
> > currently unregister the acpi_video# backlight device(s) since userspace
> > prefers the firmware type over the native type, so for userspace to
> > actually honor the acpi_video_get_backlight_type()==native we need to get
> > the acpi_video# backlight device "out of the way" which is currently
> > handled by unregistering it.
> >
> > Note in a way we already have a case where userspace sees 2 panels,
> > in hybrid laptop setups with a mux and on those systems we may see
> > either 2 native backlight devices; or 2 native backlight devices +
> > 2 acpi_video backlight devices with userspace preferring the ACPI
> > ones.
> >
> > Also note that userspace already has code to detect if the related
> > panel is active (iow which way the mux between the GPU and the panels
> > points) and then uses that backlight device. Userspace here very
> > much assumes a single panel though.
> >
> >> 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.
> >
> > Right. Note that the current code (both with and without this patchset)
> > already will work fine from a kernel pov as long as both panels
> > are either using native GPU PWM or are both using ACPI. But if we
> > ever get a mix then this will need special handling.
> >
> > Note that all userspace code I know is currently hardcoded
> > to assume a single panel. Userspace already picks one preferred
> > device under /sys/class/backlight and ignores the rest. Actually
> > atm userspace must behave this way, because on x86/ACPI boards we
> > do often register multiple backlight devices for a single panel.
> >
> > So in a way moving to registering only a single backlight device
> > prepares for actually having multiple panels work.
> >
> > Also keep in mind that this is preparation work for making the
> > panel brightness a drm_connector property. When the panel uses
> > a backlight device other then the native GPU PWM to control the
> > brightness then the helper code for this needs to have a way to
> > select which backlight_device to use then and the non native
> > types are not "linked" to a specific connector so in this case
> > we really need there to be only 1 backlight device registered
> > so that the code looking up the (non native) backlight device
> > for the connector gets the right one and not merely the one
> > which happened to get registered first.
> >
> > And I believe that having the panel brightness be a drm_connector
> > property is the way to make it possible for userspace to deal
> > with the multiple panels which each have a separate brightness
> > control case.
> 
> Agreed.
> 
> Thanks for the explanations and recording them here.

Can we stuff a summary of all the things we've discussed here into
Documentation/gpu/todo.rst so that we have this recorded somewhere more
permanently? Also good place to make sure everyone who was part of these
discussions has a chance to ack the overall plan as part of merging such a
patch.

Just feels like this is big&complex enough to justify a todo entry with
what's going on and what's still to be done.
-Daniel


> 
> BR,
> Jani.
> 
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >> 
> >> 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(-)
> >> 
> >
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch



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

  Powered by Linux