[RFC v2] drm/kms: control display brightness through drm_connector properties

[Hans de Goede]

Hi,

On 9/9/22 15:39, Simon Ser wrote:
> On Friday, September 9th, 2022 at 12:12, Hans de Goede <hdegoede at redhat.com> wrote:
> 
>> Phase 3: Deprecate /sys/class/backlight uAPI
>> ============================================
>>
>> Once most userspace has moved over to using the new drm_connector
>> brightness props, a Kconfig option can be added to stop exporting
>> the backlight-devices under /sys/class/backlight. The plan is to
>> just disable the sysfs interface and keep the existing backlight-device
>> internal kernel abstraction as is, since some abstraction for (non GPU
>> native) backlight devices will be necessary regardless.
>>
>> It is unsure if we will ever be able to do this. For example people using
>> non fully integrated desktop environments like e.g. sway often use custom
>> scripts binded to hotkeys to get functionality like the brightness
>> up/down keyboard hotkeys changing the brightness. This typically involves
>> e.g. the xbacklight utility.
>>
>> Even if the xbacklight utility is ported to use kms with the new connector
>> object brightness properties then this still will not work because
>> changing the properties will require drm-master rights and e.g. sway will
>> already hold those.
> 
> I replied to this here in another thread [1].
> 
> tl;dr I think it would be fine even for Sway-like compositors.

Ok, that is good to know.

> (Also note the utilities used right now are not xbacklight, but
> brightnessctl/light/brillo/etc [2])

Ah I thought that xbacklight got patched at one point to support
the sysfs API, but I see now that instead alternative utilities
have popped up.

Regards,

Hans


> 
> [1]: https://lore.kernel.org/dri-devel/bZJU9OkYWFyaLHVa4XUE4d5iBTPFXBRyPe1wMd_ztKh5VBMu-EDNGoUDpvwtFn_u9-JMvN8QmIZVS3pzMZM_hZTkTCA9gOBnCGXc5HFmsnc=@emersion.fr/
> [2]: https://github.com/swaywm/sway/wiki#xbacklight
> 
>> The drm_connector brightness properties
>> =======================================
>>
>> The new uAPI for this consists of 2 properties:
>>
>> 1. "display brightness": rw 0-int32_max property controlling the brightness setting
>> of the connected display. The actual maximum of this will be less then
>> int32_max and is given in "display brightness max".
>>
>> Unlike the /sys/class/backlight/foo/brightness this brightness property
>> has a clear definition for the value 0. The kernel must ensure that 0
>> means minimum brightness (so 0 should never turn the backlight off).
>> If necessary the kernel must enforce a minimum value by adding
>> an offset to the value seen in the property to ensure this behavior.
>>
>> For example if necessary the driver must clamp 0-255 to 10-255, which then
>> becomes 0-245 on the brightness property, adding 10 internally to writes
>> done to the brightness property. This adding of an extra offset when
>> necessary must only be done on the brightness property,
>> the /sys/class/backlight interface should be left unchanged to not break
>> userspace which may rely on 0 = off on some systems.
>>
>> Note amdgpu already does something like this even for /sys/class/backlight,
>> see the use of AMDGPU_DM_DEFAULT_MIN_BACKLIGHT in amdgpu.
>>
>> Also whenever possible the kernel must ensure that the brightness range
>> is in perceived brightness, but this cannot always be guaranteed.
>>
>>
>> 2. "display brightness max": ro 0-int32_max property giving the actual maximum
>> of the display's brightness setting. This will report 0 when brightness
>> control is not available.
>>
>> The value of "display brightness max" may change at runtime, either by
>> a legacy drivers/platform/x86 backlight driver loading after the drm
>> driver has loaded; or when plugging in a monitor which allows brightness
>> control over DDC/CI. In both these cases the max value will change from 0
>> to the actual max value, indicating backlight control has become available
>> on this connector.
> 
> The kernel will need to ensure that a hotplug uevent is sent to
> user-space at this point. Otherwise user-space has no way to figure out
> that the prop has changed.
> 
> Overall this all looks very solid to me!
> 
> Simon
>