Re: [PATCH v8 00/14] Add support for a DRM backlight capability

[Mario Limonciello <mario.limonciello@amd.com>]

On 9/22/26 07:28, Javier Martinez Canillas wrote:
> Mario Limonciello <mario.limonciello@amd.com> writes:
> 
> Hello Mario,
> 
>> On 9/22/26 03:33, Javier Martinez Canillas wrote:
>>> Hello Mario,
>>>
>>> On Tue, Sep 8, 2026 at 6:41 AM Mario Limonciello
>>> <mario.limonciello@amd.com> wrote:
>>>>
>>>> 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.
>>>>
>>>
>>> I don't think I agree with the direction of this series. The main
>>> issue for me is that if the sysfs interface is disabled, then I don't
>>> understand the value of doing all the hops between the DRM and
>>> backlight subsystems...
>>
>> The reason for all the hops is that users can switch between compositors
>> that support this and don't.  If you're in a compositor that supports it
>> that compositor will want to affirm it's in control.  If you're in a
>> compositor without support then you should still have a way to change
>> things, and that's what the sysfs interface exists for.
>>
> 
> That's Ok but still doesn't explain why it must go through the backlight
> subsystem. Both the struct drm_backlight .{s,g}et_luminance() callbacks
> and the struct backlight_ops .update_status() can call to the same code
> to manage the brightness.
> 
> For example, in amdgput this could be amdgpu_dm_backlight_get_level()
> and amdgpu_dm_backlight_set_level().
> 

I guess the other point would be not storing two sets of data.

>>>
>>> IMO when a driver sets the DRIVER_CONNECTOR_LUMINANCE feature and the
>>> client advertise the DRM_CLIENT_CAP_LUMINANCE capability, then the DRM
>>> driver should be in full control of the brightness control and not go
>>> through the backlight subsystem at all.
>>
>> OK but so let's say I start at 100% brightness.  I open up Kwin, I
>> change the luminance property to 0%.  Let's pretend that backlight
>> subsystem doesn't get updated.
>>
>> Then I log into Xorg + Xfce.  The luminance property should be left at
>> 0%, the brightness subsystem is 100%.
>>
>> The hardware would be left at 0%.  I press the brightness up key (or
>> call brightnessctl) and I can't change it because backlight subsystem is
>> 100% already.
>>
> 
> Not really because struct backlight_ops .get_brightness() will be called
> and this will query the HW state (in the case of amdgput this will be a
> call to amdgpu_dm_backlight_get_level() as mentioned above).
> 
> So the HW state will be changed and both subsystems are going to query
> the same information.
> 

Thanks for pointing that out.