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 > > 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
