Re: [PATCH v6 00/10] Add support for a DRM backlight capability

[Hans de Goede <johannes.goede@oss.qualcomm.com>]

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) <superm1@kernel.org>
>>
>> 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
>