Jani Nikula
2022-May-18 08:44 UTC
[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
On Tue, 17 May 2022, Hans de Goede <hdegoede at redhat.com> 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 at redhat.com/ > > 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? 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 at redhat.com/ > > 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
Hans de Goede
2022-May-18 10:04 UTC
[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
Hi, On 5/18/22 10:44, Jani Nikula wrote:> On Tue, 17 May 2022, Hans de Goede <hdegoede at redhat.com> 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 at redhat.com/ >> >> 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. 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 at redhat.com/ >> >> 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(-) >
Daniel Dadap
2022-May-25 16:24 UTC
[Nouveau] [PATCH 00/14] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
On 5/18/22 03:44, Jani Nikula wrote:> On Tue, 17 May 2022, Hans de Goede <hdegoede at redhat.com> 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 at redhat.com/ >> >> 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 at redhat.com/ >> >> 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(-)