Hi, On 21-Jul-26 19:50, Hans de Goede wrote: > Hi Mario, > > On 24-Jun-26 18:57, Mario Limonciello wrote: >> From: Mario Limonciello (AMD) <[email protected]> >> >> At Display Next Hackfest 2026 we reviewed progress moving brightness >> control into the DRM connector properties. >> >> There is a range LUMINANCE property that will default to 0->0. >> Once a driver attaches a backlight it will be updated to 1->max. >> If the panel supports the minimum backlight turning off the display >> the range can later be updated to 0->max instead of 1->max. >> >> The legacy sysfs interface is synchronized with the DRM connector. >> When a compositor using this feature is loaded, sysfs writes are disabled >> to prevent legacy tools from going out of sync with the compositor. >> >> This has an implementation initially for amdgpu, i915, and Xe with eDP >> connectors. It can be extended to other connectors like DP for displays >> that can be controlled via DDC as well later. > > First of all thank you very much for working on this. > > I'm a bit late to the party, since I only became aware of this > patch-set recently through: > https://blog.sebastianwick.net/posts/display-next-hackfest-2026/ > > Overall this looks pretty good to me. > > My main remark is related to non acpi_backlight=native backlight > control. > > When I first started working on implementing backlight control > as a DRM connector property I never got further than a whole > bunch of prep working making sure that all backlight drivers > (and all drm/kms driver backlight code) were using / honoring > acpi_video_get_backlight_type() resp. its special > acpi_video_backlight_use_native() variant. > > This prep work was meant to allow the drm connector prop code > to deal with the case where e.g. the backlight code from > drivers/acpi/acpi_video.c (acpi_backlight=video) should be used as > is the case on many older laptops. > > The prep work basically made sure that all backlight hw/fw > interface selection handling (on x86) goes through > drivers/acpi/video_detect.c . To have a single source of > truth of which backlight control method should be used. > > Looking at e.g. "[PATCH v6 07/10] drm/amd/display: use drm > backlight" then this is partly honored since > amdgpu_dm_register_backlight_device() starts with: > > if (!acpi_video_backlight_use_native()) { > drm_info(drm, "Skipping amdgpu DM backlight registration\n"); > /* Try registering an ACPI video backlight device instead. */ > acpi_video_register_backlight(); > return; > } > > and the added drm_backlight_link() call happens later and thus > gets skipped if the acpi_video_backlight_use_native() check fails, > but any non native backlight devices never get linked. > > What IMHO needs to happen (perhaps in a follow-up series) is: > > If the acpi_video_backlight_use_native() call returns false then > replace the acpi_video_register_backlight() call with a new: > > drm_backlight_handle_non_native_bl(internal_panel_drm_connector, pci_dev); > > function which does the following: > > 1. Call acpi_video_get_backlight_type() and cache the type it returns > and on acpi_backlight_none bail from the function. > > 2. Add a new acpi_video_register_single_backlight(), which takes > a pci_dev pointer as argument and then only registers the acpi_video > backlight matching that PCI device, see the parent handling in > acpi_video_dev_register_backlight(). > > This new function then returns the registered backlight on success > or NULL (or maybe an ERR_PTR()?). > > 3. If the type is acpi_backlight_video call > acpi_video_register_single_backlight() and link the returned > backlight instead of the native one. > > 4. For other types register a backlight notifier which listens for > new backlight drivers to show up and then checks those against > the cached type. And on a successful match, link the new > backlight device to the connector. > > This is for drivers/platform/x86 backlight drivers, like > dell-laptop on old Dell Latitudes which will typically not > be in the initramfs, while the GPU driver will be in > the initramfs. > > 5. After registering the notifier, check if a backlight device > of the expected type has not already been registered beforehand > and if yes, link it and remove the notifier. > > Stating the obvious: 5. needs to be done last to avoid a race > between checking for the backlight device already being there > and a new backlight getting registered. > > I specifically have retained a bunch of old laptops which use > the acpi_backlight_vendor and acpi_backlight_native methods to > test this. I even have one with 2 GPUs which IIRC both > work with acpi_backlight=video and I can switch which one > drivers the panel in the BIOS. > > Regards, > > Hans
p.s. i. For non native backlights set-brightness calls should probably be deferred to a workqueue because these often fw calls can be quite slow. ii. For the next version of this series please Cc me. > > > > > > >> >> The following compositors have implemented matching support: >> * Kwin: https://invent.kde.org/plasma/kwin/-/merge_requests/9298 >> * Mutter: >> https://gitlab.gnome.org/swick/mutter/-/commits/wip/kms-luminance-prop >> * Wlroots: >> https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/5384 >> >> --- >> v5->v6: >> * Rebase on drm-next >> * Drop patch 7/11 (Move backlight tracing out of the dc lock) >> >> Mario Limonciello (AMD) (10): >> Revert "backlight: Remove notifier" >> backlight: add kernel-internal backlight API >> drm: link connectors to backlight devices >> DRM: Add support for client indicating support for luminance >> drm/amd/display: Pass up errors reading actual brightness >> drm/amd/display: Allow backlight registration to fail >> drm/amd/display: use drm backlight >> drm/amd/display: Drop brightness caching in amdgpu_dm >> drm/bridge: auto-link panel backlight in bridge connector >> drm/i915/display: use drm backlight >> >> drivers/gpu/drm/Kconfig | 1 + >> drivers/gpu/drm/Makefile | 1 + >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 111 ++--- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 13 - >> drivers/gpu/drm/bridge/panel.c | 15 + >> .../gpu/drm/display/drm_bridge_connector.c | 15 +- >> drivers/gpu/drm/drm_atomic_helper.c | 7 + >> drivers/gpu/drm/drm_atomic_uapi.c | 59 ++- >> drivers/gpu/drm/drm_backlight.c | 445 ++++++++++++++++++ >> drivers/gpu/drm/drm_connector.c | 63 +++ >> drivers/gpu/drm/drm_drv.c | 8 + >> drivers/gpu/drm/drm_file.c | 5 + >> drivers/gpu/drm/drm_ioctl.c | 15 + >> drivers/gpu/drm/drm_mode_config.c | 7 + >> drivers/gpu/drm/drm_mode_object.c | 66 ++- >> drivers/gpu/drm/drm_property.c | 6 + >> drivers/gpu/drm/drm_sysfs.c | 28 +- >> .../gpu/drm/i915/display/intel_backlight.c | 4 + >> drivers/gpu/drm/i915/display/intel_dp.c | 8 + >> drivers/video/backlight/backlight.c | 97 ++++ >> include/drm/drm_backlight.h | 51 ++ >> include/drm/drm_bridge.h | 1 + >> include/drm/drm_connector.h | 8 + >> include/drm/drm_file.h | 8 + >> include/drm/drm_mode_config.h | 5 + >> include/linux/backlight.h | 63 +++ >> include/uapi/drm/drm.h | 22 + >> 27 files changed, 1046 insertions(+), 86 deletions(-) >> create mode 100644 drivers/gpu/drm/drm_backlight.c >> create mode 100644 include/drm/drm_backlight.h >> >> >> base-commit: 0e8233409d4f6def051dd42a432c6815bb780d78 >
