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

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

[Hans de Goede]

Hi all,

Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:

Changes from version 1:
- Drop bl_brightness_0_is_min_brightness from list of new connector
  properties.
- Clearly define that 0 is always min-brightness when setting the brightness
  through the connector properties.
- Drop bl_brightness_control_method from list of new connector
  properties.
- Phase 1 of the plan has been completed

As discussed already several times in the past:
 https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
 https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/

The current userspace API for brightness control offered by
/sys/class/backlight devices has various issues:

1. There is no way to map the backlight device to a specific
   display-output / panel (1)
2. Controlling the brightness requires root-rights requiring
   desktop-environments to use suid-root helpers for this.
3. The meaning of 0 is not clearly defined, it can be either off,
   or minimum brightness at which the display is still readable
   (in a low light environment)
4. It's not possible to change both the gamma and the brightness in the
   same KMS atomic commit. You'd want to be able to reduce brightness to
   conserve power, and counter the effects of that by changing gamma to
   reach a visually similar image. And you'd want to have the changes take
   effect at the same time instead of reducing brightness at some frame and
   change gamma at some other frame. This is pretty much impossible to do
   via the sysfs interface.

As already discussed on various conference's hallway tracks
and as has been proposed on the dri-devel list once before (2),
it seems that there is consensus that the best way to to solve these
2 issues is to add support for controlling a video-output's brightness
through properties on the drm_connector.

This RFC outlines my plan to try and actually implement this,
which has 3 phases:


Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
=================================================================================

On x86 there can be multiple firmware + direct-hw-access methods
for controlling the backlight and in some cases the kernel registers
multiple backlight-devices for a single internal laptop LCD panel.

A plan to fix this was posted here:
https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
And a pull-req actually implementing this plan has been send out this week:
https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/


Phase 2: Add drm_connector properties mirroring the matching backlight device
=============================================================================

The plan is to add a drm_connector helper function, which optionally takes
a pointer to the backlight device for the GPU's native backlight device,
which will then mirror the backlight settings from the backlight device
in a set of read/write brightness* properties on the connector.

This function can then be called by GPU drivers for the drm_connector for
the internal panel and it will then take care of everything. When there
is no native GPU backlight device, or when it should not be used then
(on x86) the helper will use the acpi_video_get_backlight_type() to
determine which backlight-device should be used instead and it will find
+ mirror that one.


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.


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.


Future extensions
=================

Some hardware do adaptive brightness in hardware, rather then providing
an ALS sensor and letting userspace handle this.

One example of this is the Steam deck, which currently uses some custom
sysfs attributes to allow tweaking (and enable/disable?) the adaptive
brightness. Adding standardized uAPI for this through new
"display brightness *" properties seems like a natural extension of this
proposal.

Regards,

Hans


1) The need to be able to map the backlight device to a specific display
has become clear once more with the recent proposal to add DDCDI support:
https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/

2) https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
Note this proposal included a method for userspace to be able to tell the
kernel if the native/acpi_video/vendor backlight device should be used,
but this has been solved in the kernel for years now:
 https://www.spinics.net/lists/linux-acpi/msg50526.html
An initial implementation of this proposal is available here:
 https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight

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

[Simon Ser]

On Friday, September 9th, 2022 at 12:12, Hans de Goede <hdegoede@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.

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

[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

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2329 bytes --]

On Fri, 09 Sep 2022 13:39:37 +0000
Simon Ser <contact@emersion.fr> wrote:

> On Friday, September 9th, 2022 at 12:12, Hans de Goede <hdegoede@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.

Furthermore, if other compositors are like Weston in their KMS state
handling, and do not track which property has already been programmed
to KMS and which hasn't, and instead just smash all KMS properties
every update anyway (it's also great for debugging, you always have the
full state in flight), anything changed via sysfs will be immediately
reverted.

Therefore I think there is a high probability that the external or
sysfs controls just naturally stop working anyway, even if the kernel
does not remove them first.


Thanks,
pq


> (Also note the utilities used right now are not xbacklight, but
> brightnessctl/light/brillo/etc [2])
> 
> [1]: https://lore.kernel.org/dri-devel/bZJU9OkYWFyaLHVa4XUE4d5iBTPFXBRyPe1wMd_ztKh5VBMu-EDNGoUDpvwtFn_u9-JMvN8QmIZVS3pzMZM_hZTkTCA9gOBnCGXc5HFmsnc=@emersion.fr/
> [2]: https://github.com/swaywm/sway/wiki#xbacklight
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Simon Ser]

On Friday, September 9th, 2022 at 15:53, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Fri, 09 Sep 2022 13:39:37 +0000
> Simon Ser contact@emersion.fr wrote:
> 
> > On Friday, September 9th, 2022 at 12:12, Hans de Goede hdegoede@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.
> 
> Furthermore, if other compositors are like Weston in their KMS state
> handling, and do not track which property has already been programmed
> to KMS and which hasn't, and instead just smash all KMS properties
> every update anyway (it's also great for debugging, you always have the
> full state in flight), anything changed via sysfs will be immediately
> reverted.
> 
> Therefore I think there is a high probability that the external or
> sysfs controls just naturally stop working anyway, even if the kernel
> does not remove them first.

Ah yes, that's a good point. And this wouldn't be a kernel regression,
it would be user-space like Sway or Weston taking the decision to use
the new uAPI in a way which breaks the sysfs interface.

(User-space could also take the decision to _not_ break the sysfs
interface, by implementing a simple "dirty" flag.)

Re: [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@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
> 

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

[Jani Nikula]

On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi all,
>
> Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
>
> Changes from version 1:
> - Drop bl_brightness_0_is_min_brightness from list of new connector
>   properties.
> - Clearly define that 0 is always min-brightness when setting the brightness
>   through the connector properties.
> - Drop bl_brightness_control_method from list of new connector
>   properties.
> - Phase 1 of the plan has been completed
>
> As discussed already several times in the past:
>  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
>  https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
>
> The current userspace API for brightness control offered by
> /sys/class/backlight devices has various issues:
>
> 1. There is no way to map the backlight device to a specific
>    display-output / panel (1)
> 2. Controlling the brightness requires root-rights requiring
>    desktop-environments to use suid-root helpers for this.
> 3. The meaning of 0 is not clearly defined, it can be either off,
>    or minimum brightness at which the display is still readable
>    (in a low light environment)
> 4. It's not possible to change both the gamma and the brightness in the
>    same KMS atomic commit. You'd want to be able to reduce brightness to
>    conserve power, and counter the effects of that by changing gamma to
>    reach a visually similar image. And you'd want to have the changes take
>    effect at the same time instead of reducing brightness at some frame and
>    change gamma at some other frame. This is pretty much impossible to do
>    via the sysfs interface.
>
> As already discussed on various conference's hallway tracks
> and as has been proposed on the dri-devel list once before (2),
> it seems that there is consensus that the best way to to solve these
> 2 issues is to add support for controlling a video-output's brightness
> through properties on the drm_connector.
>
> This RFC outlines my plan to try and actually implement this,
> which has 3 phases:
>
>
> Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
> =================================================================================
>
> On x86 there can be multiple firmware + direct-hw-access methods
> for controlling the backlight and in some cases the kernel registers
> multiple backlight-devices for a single internal laptop LCD panel.
>
> A plan to fix this was posted here:
> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
> And a pull-req actually implementing this plan has been send out this week:
> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
>
>
> Phase 2: Add drm_connector properties mirroring the matching backlight device
> =============================================================================
>
> The plan is to add a drm_connector helper function, which optionally takes
> a pointer to the backlight device for the GPU's native backlight device,
> which will then mirror the backlight settings from the backlight device
> in a set of read/write brightness* properties on the connector.
>
> This function can then be called by GPU drivers for the drm_connector for
> the internal panel and it will then take care of everything. When there
> is no native GPU backlight device, or when it should not be used then
> (on x86) the helper will use the acpi_video_get_backlight_type() to
> determine which backlight-device should be used instead and it will find
> + mirror that one.
>
>
> 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.
>
>
> 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".

This could use a few words explaining the choice of range and property
type. (I assume it's because you can't change a range property's max at
runtime. Which is also why you need a separate max property.)

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

Do you mean every step should be a visible change?

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

I think this could be a bit more restrictive in stating the allowed
runtime changes. Is it only 0 -> actual max for non-hotpluggable
displays, nothing else, and additionally actual max -> 0 when unplugging
a display?

>
>
> Future extensions
> =================
>
> Some hardware do adaptive brightness in hardware, rather then providing
> an ALS sensor and letting userspace handle this.
>
> One example of this is the Steam deck, which currently uses some custom
> sysfs attributes to allow tweaking (and enable/disable?) the adaptive
> brightness. Adding standardized uAPI for this through new
> "display brightness *" properties seems like a natural extension of this
> proposal.

Another example is adjusting for non-linear luminance curve. It would be
nicer for equal steps in brightness value to have similar luminance
changes. I think sometimes we lose precision with the limited number of
steps in UIs. But this is thinking pretty far ahead. :)

Other than the minor clarifications, the plans sounds good to me.

Thanks again for doing all this, much appreciated!


BR,
Jani.

>
> Regards,
>
> Hans
>
>
> 1) The need to be able to map the backlight device to a specific display
> has become clear once more with the recent proposal to add DDCDI support:
> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/
>
> 2) https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> Note this proposal included a method for userspace to be able to tell the
> kernel if the native/acpi_video/vendor backlight device should be used,
> but this has been solved in the kernel for years now:
>  https://www.spinics.net/lists/linux-acpi/msg50526.html
> An initial implementation of this proposal is available here:
>  https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight
>
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Ville Syrjälä]

On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
> On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> > Hi all,
> >
> > Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
> >
> > Changes from version 1:
> > - Drop bl_brightness_0_is_min_brightness from list of new connector
> >   properties.
> > - Clearly define that 0 is always min-brightness when setting the brightness
> >   through the connector properties.
> > - Drop bl_brightness_control_method from list of new connector
> >   properties.
> > - Phase 1 of the plan has been completed
> >
> > As discussed already several times in the past:
> >  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
> >  https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> >
> > The current userspace API for brightness control offered by
> > /sys/class/backlight devices has various issues:
> >
> > 1. There is no way to map the backlight device to a specific
> >    display-output / panel (1)
> > 2. Controlling the brightness requires root-rights requiring
> >    desktop-environments to use suid-root helpers for this.
> > 3. The meaning of 0 is not clearly defined, it can be either off,
> >    or minimum brightness at which the display is still readable
> >    (in a low light environment)
> > 4. It's not possible to change both the gamma and the brightness in the
> >    same KMS atomic commit. You'd want to be able to reduce brightness to
> >    conserve power, and counter the effects of that by changing gamma to
> >    reach a visually similar image. And you'd want to have the changes take
> >    effect at the same time instead of reducing brightness at some frame and
> >    change gamma at some other frame. This is pretty much impossible to do
> >    via the sysfs interface.
> >
> > As already discussed on various conference's hallway tracks
> > and as has been proposed on the dri-devel list once before (2),
> > it seems that there is consensus that the best way to to solve these
> > 2 issues is to add support for controlling a video-output's brightness
> > through properties on the drm_connector.
> >
> > This RFC outlines my plan to try and actually implement this,
> > which has 3 phases:
> >
> >
> > Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
> > =================================================================================
> >
> > On x86 there can be multiple firmware + direct-hw-access methods
> > for controlling the backlight and in some cases the kernel registers
> > multiple backlight-devices for a single internal laptop LCD panel.
> >
> > A plan to fix this was posted here:
> > https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
> > And a pull-req actually implementing this plan has been send out this week:
> > https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
> >
> >
> > Phase 2: Add drm_connector properties mirroring the matching backlight device
> > =============================================================================
> >
> > The plan is to add a drm_connector helper function, which optionally takes
> > a pointer to the backlight device for the GPU's native backlight device,
> > which will then mirror the backlight settings from the backlight device
> > in a set of read/write brightness* properties on the connector.
> >
> > This function can then be called by GPU drivers for the drm_connector for
> > the internal panel and it will then take care of everything. When there
> > is no native GPU backlight device, or when it should not be used then
> > (on x86) the helper will use the acpi_video_get_backlight_type() to
> > determine which backlight-device should be used instead and it will find
> > + mirror that one.
> >
> >
> > 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.
> >
> >
> > 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".
> 
> This could use a few words explaining the choice of range and property
> type. (I assume it's because you can't change a range property's max at
> runtime. Which is also why you need a separate max property.)

Why don't we just normalize the range to something sensible?

> 
> > 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.
> 
> Do you mean every step should be a visible change?

Hmm. I guess due to this. I'd prefer the opposite tbh so I could
just put in my opregion BCLM patch. It's annoying to have to
carry it locally just to have reasonable backlight behaviour.

-- 
Ville Syrjälä
Intel

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

[Ville Syrjälä]

On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
> > On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> > > Hi all,
> > >
> > > Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
> > >
> > > Changes from version 1:
> > > - Drop bl_brightness_0_is_min_brightness from list of new connector
> > >   properties.
> > > - Clearly define that 0 is always min-brightness when setting the brightness
> > >   through the connector properties.
> > > - Drop bl_brightness_control_method from list of new connector
> > >   properties.
> > > - Phase 1 of the plan has been completed
> > >
> > > As discussed already several times in the past:
> > >  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
> > >  https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> > >
> > > The current userspace API for brightness control offered by
> > > /sys/class/backlight devices has various issues:
> > >
> > > 1. There is no way to map the backlight device to a specific
> > >    display-output / panel (1)
> > > 2. Controlling the brightness requires root-rights requiring
> > >    desktop-environments to use suid-root helpers for this.
> > > 3. The meaning of 0 is not clearly defined, it can be either off,
> > >    or minimum brightness at which the display is still readable
> > >    (in a low light environment)
> > > 4. It's not possible to change both the gamma and the brightness in the
> > >    same KMS atomic commit. You'd want to be able to reduce brightness to
> > >    conserve power, and counter the effects of that by changing gamma to
> > >    reach a visually similar image. And you'd want to have the changes take
> > >    effect at the same time instead of reducing brightness at some frame and
> > >    change gamma at some other frame. This is pretty much impossible to do
> > >    via the sysfs interface.
> > >
> > > As already discussed on various conference's hallway tracks
> > > and as has been proposed on the dri-devel list once before (2),
> > > it seems that there is consensus that the best way to to solve these
> > > 2 issues is to add support for controlling a video-output's brightness
> > > through properties on the drm_connector.
> > >
> > > This RFC outlines my plan to try and actually implement this,
> > > which has 3 phases:
> > >
> > >
> > > Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
> > > =================================================================================
> > >
> > > On x86 there can be multiple firmware + direct-hw-access methods
> > > for controlling the backlight and in some cases the kernel registers
> > > multiple backlight-devices for a single internal laptop LCD panel.
> > >
> > > A plan to fix this was posted here:
> > > https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
> > > And a pull-req actually implementing this plan has been send out this week:
> > > https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
> > >
> > >
> > > Phase 2: Add drm_connector properties mirroring the matching backlight device
> > > =============================================================================
> > >
> > > The plan is to add a drm_connector helper function, which optionally takes
> > > a pointer to the backlight device for the GPU's native backlight device,
> > > which will then mirror the backlight settings from the backlight device
> > > in a set of read/write brightness* properties on the connector.
> > >
> > > This function can then be called by GPU drivers for the drm_connector for
> > > the internal panel and it will then take care of everything. When there
> > > is no native GPU backlight device, or when it should not be used then
> > > (on x86) the helper will use the acpi_video_get_backlight_type() to
> > > determine which backlight-device should be used instead and it will find
> > > + mirror that one.
> > >
> > >
> > > 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.
> > >
> > >
> > > 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".
> > 
> > This could use a few words explaining the choice of range and property
> > type. (I assume it's because you can't change a range property's max at
> > runtime. Which is also why you need a separate max property.)
> 
> Why don't we just normalize the range to something sensible?
> 
> > 
> > > 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.
> > 
> > Do you mean every step should be a visible change?
> 
> Hmm. I guess due to this. I'd prefer the opposite tbh so I could
> just put in my opregion BCLM patch. It's annoying to have to
> carry it locally just to have reasonable backlight behaviour

After second though I guess I'm actually agreeing with Hans here.
The current situation is where small change in the value near one
end of the range does basically nothing, while a small change at
the other of the range causes a massive brightness change. That
is no good.

-- 
Ville Syrjälä
Intel

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

[Sebastian Wick]

If it is supposed to be a non-linear luminance curve, which one is it?
It would be much clearer if user space can control linear luminance
and use whatever definition of perceived brightness it wants. The
obvious downside of it is that it requires bits to encode changes that
users can't perceive. What about backlights which only have a few
predefined luminance levels? How would user space differentiate
between the continuous and discrete backlight? What about
self-emitting displays? They usually increase the dynamic range when
they increase in brightness because the black level doesn't rise. They
also probably employ some tonemapping to adjust for that. What about
the range of the backlight? What about the absolute luminance of the
backlight? I want to know about that all in user space.

I understand that most of the time the kernel doesn't have answers to
those questions right now but the API should account for all of that.

On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
> > > On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> > > > Hi all,
> > > >
> > > > Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
> > > >
> > > > Changes from version 1:
> > > > - Drop bl_brightness_0_is_min_brightness from list of new connector
> > > >   properties.
> > > > - Clearly define that 0 is always min-brightness when setting the brightness
> > > >   through the connector properties.
> > > > - Drop bl_brightness_control_method from list of new connector
> > > >   properties.
> > > > - Phase 1 of the plan has been completed
> > > >
> > > > As discussed already several times in the past:
> > > >  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
> > > >  https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
> > > >
> > > > The current userspace API for brightness control offered by
> > > > /sys/class/backlight devices has various issues:
> > > >
> > > > 1. There is no way to map the backlight device to a specific
> > > >    display-output / panel (1)
> > > > 2. Controlling the brightness requires root-rights requiring
> > > >    desktop-environments to use suid-root helpers for this.
> > > > 3. The meaning of 0 is not clearly defined, it can be either off,
> > > >    or minimum brightness at which the display is still readable
> > > >    (in a low light environment)
> > > > 4. It's not possible to change both the gamma and the brightness in the
> > > >    same KMS atomic commit. You'd want to be able to reduce brightness to
> > > >    conserve power, and counter the effects of that by changing gamma to
> > > >    reach a visually similar image. And you'd want to have the changes take
> > > >    effect at the same time instead of reducing brightness at some frame and
> > > >    change gamma at some other frame. This is pretty much impossible to do
> > > >    via the sysfs interface.
> > > >
> > > > As already discussed on various conference's hallway tracks
> > > > and as has been proposed on the dri-devel list once before (2),
> > > > it seems that there is consensus that the best way to to solve these
> > > > 2 issues is to add support for controlling a video-output's brightness
> > > > through properties on the drm_connector.
> > > >
> > > > This RFC outlines my plan to try and actually implement this,
> > > > which has 3 phases:
> > > >
> > > >
> > > > Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
> > > > =================================================================================
> > > >
> > > > On x86 there can be multiple firmware + direct-hw-access methods
> > > > for controlling the backlight and in some cases the kernel registers
> > > > multiple backlight-devices for a single internal laptop LCD panel.
> > > >
> > > > A plan to fix this was posted here:
> > > > https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
> > > > And a pull-req actually implementing this plan has been send out this week:
> > > > https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
> > > >
> > > >
> > > > Phase 2: Add drm_connector properties mirroring the matching backlight device
> > > > =============================================================================
> > > >
> > > > The plan is to add a drm_connector helper function, which optionally takes
> > > > a pointer to the backlight device for the GPU's native backlight device,
> > > > which will then mirror the backlight settings from the backlight device
> > > > in a set of read/write brightness* properties on the connector.
> > > >
> > > > This function can then be called by GPU drivers for the drm_connector for
> > > > the internal panel and it will then take care of everything. When there
> > > > is no native GPU backlight device, or when it should not be used then
> > > > (on x86) the helper will use the acpi_video_get_backlight_type() to
> > > > determine which backlight-device should be used instead and it will find
> > > > + mirror that one.
> > > >
> > > >
> > > > 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.
> > > >
> > > >
> > > > 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".
> > >
> > > This could use a few words explaining the choice of range and property
> > > type. (I assume it's because you can't change a range property's max at
> > > runtime. Which is also why you need a separate max property.)
> >
> > Why don't we just normalize the range to something sensible?
> >
> > >
> > > > 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.
> > >
> > > Do you mean every step should be a visible change?
> >
> > Hmm. I guess due to this. I'd prefer the opposite tbh so I could
> > just put in my opregion BCLM patch. It's annoying to have to
> > carry it locally just to have reasonable backlight behaviour
>
> After second though I guess I'm actually agreeing with Hans here.
> The current situation is where small change in the value near one
> end of the range does basically nothing, while a small change at
> the other of the range causes a massive brightness change. That
> is no good.
>
> --
> Ville Syrjälä
> Intel
>

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4752 bytes --]

On Thu, 29 Sep 2022 20:06:50 +0200
Sebastian Wick <sebastian.wick@redhat.com> wrote:

> If it is supposed to be a non-linear luminance curve, which one is it?
> It would be much clearer if user space can control linear luminance
> and use whatever definition of perceived brightness it wants. The
> obvious downside of it is that it requires bits to encode changes that
> users can't perceive. What about backlights which only have a few
> predefined luminance levels? How would user space differentiate
> between the continuous and discrete backlight? What about
> self-emitting displays? They usually increase the dynamic range when
> they increase in brightness because the black level doesn't rise. They
> also probably employ some tonemapping to adjust for that. What about
> the range of the backlight? What about the absolute luminance of the
> backlight? I want to know about that all in user space.
> 
> I understand that most of the time the kernel doesn't have answers to
> those questions right now but the API should account for all of that.

Hi,

if the API accounts for all that, and the kernel doesn't know, then how
can the API not lie? If the API sometimes lies, how could we ever trust
it at all?

Personally I have the feeling that if we can even get to the level of
"each step in the value is a more or less perceivable change", that
would be good enough. Think of UI, e.g. hotkeys to change brightness.
You'd expect almost every press to change it a bit.

If an end user wants defined and controlled luminance, I'd suggest they
need to profile (physically measure) the response of the display at
hand. This is no different from color profiling displays, but you need
a measurement device that produces absolute measurements if absolute
control is what they want.

If there ever becomes an industry standard and conformance test
definitions for luminance levels and backlight control, then things
could be different. But until that, I believe trying to make one in the
kernel is futile, because I have got the impression that there is
practically no consistency between different displays in general.

Besides, I would expect some backlights to wear over time, grow dimmer
for the same input value. Without a physical active feedback loop
(measurements), it just won't work.

If this is mostly for laptop displays, would end users even care?


Thanks,
pq

> On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> >
> > On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote:  
> > > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:  
> > > > On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:  
> > > > > Hi all,
> > > > >
> > > > > Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:

...

> > > > > 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.  
> > > >
> > > > Do you mean every step should be a visible change?  
> > >
> > > Hmm. I guess due to this. I'd prefer the opposite tbh so I could
> > > just put in my opregion BCLM patch. It's annoying to have to
> > > carry it locally just to have reasonable backlight behaviour  
> >
> > After second though I guess I'm actually agreeing with Hans here.
> > The current situation is where small change in the value near one
> > end of the range does basically nothing, while a small change at
> > the other of the range causes a massive brightness change. That
> > is no good.
> >
> > --
> > Ville Syrjälä
> > Intel
> >  
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sebastian Wick]

On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Thu, 29 Sep 2022 20:06:50 +0200
> Sebastian Wick <sebastian.wick@redhat.com> wrote:
>
> > If it is supposed to be a non-linear luminance curve, which one is it?
> > It would be much clearer if user space can control linear luminance
> > and use whatever definition of perceived brightness it wants. The
> > obvious downside of it is that it requires bits to encode changes that
> > users can't perceive. What about backlights which only have a few
> > predefined luminance levels? How would user space differentiate
> > between the continuous and discrete backlight? What about
> > self-emitting displays? They usually increase the dynamic range when
> > they increase in brightness because the black level doesn't rise. They
> > also probably employ some tonemapping to adjust for that. What about
> > the range of the backlight? What about the absolute luminance of the
> > backlight? I want to know about that all in user space.
> >
> > I understand that most of the time the kernel doesn't have answers to
> > those questions right now but the API should account for all of that.
>
> Hi,
>
> if the API accounts for all that, and the kernel doesn't know, then how
> can the API not lie? If the API sometimes lies, how could we ever trust
> it at all?

Make it possible for the API to say "I don't know". I'd much rather
have an API tell me explicitly what it does and doesn't know instead
of having to guess what data I can actually rely on.

For example if the kernel knows the luminance is linear on one display
and doesn't know anything about the other display and it exposes them
both in the same way I can not possibly write any code which relies on
exact control over the luminance for either display.

>
> Personally I have the feeling that if we can even get to the level of
> "each step in the value is a more or less perceivable change", that
> would be good enough. Think of UI, e.g. hotkeys to change brightness.
> You'd expect almost every press to change it a bit.

The nice thing is that you can have that even if you have no further
information about the brightness control and it might be good enough
for some use cases but it isn't for others.

> If an end user wants defined and controlled luminance, I'd suggest they
> need to profile (physically measure) the response of the display at
> hand. This is no different from color profiling displays, but you need
> a measurement device that produces absolute measurements if absolute
> control is what they want.

If that's the kind of user experience you're after, good for you. I
certainly want things to work out of the box which makes this just a
big no-go.

>
> If there ever becomes an industry standard and conformance test
> definitions for luminance levels and backlight control, then things
> could be different. But until that, I believe trying to make one in the
> kernel is futile, because I have got the impression that there is
> practically no consistency between different displays in general.

I'm aware that this is the current situation but it's one that must
change and we should at least try to create an API which still works
when we get more and better data.

>
> Besides, I would expect some backlights to wear over time, grow dimmer
> for the same input value. Without a physical active feedback loop
> (measurements), it just won't work.
>
> If this is mostly for laptop displays, would end users even care?
>
>
> Thanks,
> pq
>
> > On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote:
> > > > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
> > > > > On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
>
> ...
>
> > > > > > 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.
> > > > >
> > > > > Do you mean every step should be a visible change?
> > > >
> > > > Hmm. I guess due to this. I'd prefer the opposite tbh so I could
> > > > just put in my opregion BCLM patch. It's annoying to have to
> > > > carry it locally just to have reasonable backlight behaviour
> > >
> > > After second though I guess I'm actually agreeing with Hans here.
> > > The current situation is where small change in the value near one
> > > end of the range does basically nothing, while a small change at
> > > the other of the range causes a massive brightness change. That
> > > is no good.
> > >
> > > --
> > > Ville Syrjälä
> > > Intel
> > >
> >
>

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

[Jani Nikula]

On Fri, 30 Sep 2022, Sebastian Wick <sebastian.wick@redhat.com> wrote:
> On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>
>> On Thu, 29 Sep 2022 20:06:50 +0200
>> Sebastian Wick <sebastian.wick@redhat.com> wrote:
>>
>> > If it is supposed to be a non-linear luminance curve, which one is it?
>> > It would be much clearer if user space can control linear luminance
>> > and use whatever definition of perceived brightness it wants. The
>> > obvious downside of it is that it requires bits to encode changes that
>> > users can't perceive. What about backlights which only have a few
>> > predefined luminance levels? How would user space differentiate
>> > between the continuous and discrete backlight? What about
>> > self-emitting displays? They usually increase the dynamic range when
>> > they increase in brightness because the black level doesn't rise. They
>> > also probably employ some tonemapping to adjust for that. What about
>> > the range of the backlight? What about the absolute luminance of the
>> > backlight? I want to know about that all in user space.
>> >
>> > I understand that most of the time the kernel doesn't have answers to
>> > those questions right now but the API should account for all of that.
>>
>> Hi,
>>
>> if the API accounts for all that, and the kernel doesn't know, then how
>> can the API not lie? If the API sometimes lies, how could we ever trust
>> it at all?
>
> Make it possible for the API to say "I don't know". I'd much rather
> have an API tell me explicitly what it does and doesn't know instead
> of having to guess what data I can actually rely on.
>
> For example if the kernel knows the luminance is linear on one display
> and doesn't know anything about the other display and it exposes them
> both in the same way I can not possibly write any code which relies on
> exact control over the luminance for either display.

My idea has been all along that you'd have an API for defining points on
a curve, a kind of mapping, and the kernel would linear intrapolate
between the set points.

If the kernel knows how to pre-fill the information, it would do
so. Otherwise, it would just be linear. The userspace could adjust in
either case.

For i915, in some cases we'd be able to pre-fill the curve, and have a
better brightness adjustment experience. Ville has done something like
this for himself, but IIUC has not polished it into an interface.


BR,
Jani.


>
>>
>> Personally I have the feeling that if we can even get to the level of
>> "each step in the value is a more or less perceivable change", that
>> would be good enough. Think of UI, e.g. hotkeys to change brightness.
>> You'd expect almost every press to change it a bit.
>
> The nice thing is that you can have that even if you have no further
> information about the brightness control and it might be good enough
> for some use cases but it isn't for others.
>
>> If an end user wants defined and controlled luminance, I'd suggest they
>> need to profile (physically measure) the response of the display at
>> hand. This is no different from color profiling displays, but you need
>> a measurement device that produces absolute measurements if absolute
>> control is what they want.
>
> If that's the kind of user experience you're after, good for you. I
> certainly want things to work out of the box which makes this just a
> big no-go.
>
>>
>> If there ever becomes an industry standard and conformance test
>> definitions for luminance levels and backlight control, then things
>> could be different. But until that, I believe trying to make one in the
>> kernel is futile, because I have got the impression that there is
>> practically no consistency between different displays in general.
>
> I'm aware that this is the current situation but it's one that must
> change and we should at least try to create an API which still works
> when we get more and better data.
>
>>
>> Besides, I would expect some backlights to wear over time, grow dimmer
>> for the same input value. Without a physical active feedback loop
>> (measurements), it just won't work.
>>
>> If this is mostly for laptop displays, would end users even care?
>>
>>
>> Thanks,
>> pq
>>
>> > On Wed, Sep 28, 2022 at 1:14 PM Ville Syrjälä
>> > <ville.syrjala@linux.intel.com> wrote:
>> > >
>> > > On Wed, Sep 28, 2022 at 01:57:18PM +0300, Ville Syrjälä wrote:
>> > > > On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
>> > > > > On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>> > > > > > Hi all,
>> > > > > >
>> > > > > > Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
>>
>> ...
>>
>> > > > > > 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.
>> > > > >
>> > > > > Do you mean every step should be a visible change?
>> > > >
>> > > > Hmm. I guess due to this. I'd prefer the opposite tbh so I could
>> > > > just put in my opregion BCLM patch. It's annoying to have to
>> > > > carry it locally just to have reasonable backlight behaviour
>> > >
>> > > After second though I guess I'm actually agreeing with Hans here.
>> > > The current situation is where small change in the value near one
>> > > end of the range does basically nothing, while a small change at
>> > > the other of the range causes a massive brightness change. That
>> > > is no good.
>> > >
>> > > --
>> > > Ville Syrjälä
>> > > Intel
>> > >
>> >
>>
>

-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Ville Syrjälä]

On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:
> On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Thu, 29 Sep 2022 20:06:50 +0200
> > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> >
> > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > It would be much clearer if user space can control linear luminance
> > > and use whatever definition of perceived brightness it wants. The
> > > obvious downside of it is that it requires bits to encode changes that
> > > users can't perceive. What about backlights which only have a few
> > > predefined luminance levels? How would user space differentiate
> > > between the continuous and discrete backlight? What about
> > > self-emitting displays? They usually increase the dynamic range when
> > > they increase in brightness because the black level doesn't rise. They
> > > also probably employ some tonemapping to adjust for that. What about
> > > the range of the backlight? What about the absolute luminance of the
> > > backlight? I want to know about that all in user space.
> > >
> > > I understand that most of the time the kernel doesn't have answers to
> > > those questions right now but the API should account for all of that.
> >
> > Hi,
> >
> > if the API accounts for all that, and the kernel doesn't know, then how
> > can the API not lie? If the API sometimes lies, how could we ever trust
> > it at all?
> 
> Make it possible for the API to say "I don't know". I'd much rather
> have an API tell me explicitly what it does and doesn't know instead
> of having to guess what data I can actually rely on.
> 
> For example if the kernel knows the luminance is linear on one display
> and doesn't know anything about the other display and it exposes them
> both in the same way I can not possibly write any code which relies on
> exact control over the luminance for either display.
> 
> >
> > Personally I have the feeling that if we can even get to the level of
> > "each step in the value is a more or less perceivable change", that
> > would be good enough. Think of UI, e.g. hotkeys to change brightness.
> > You'd expect almost every press to change it a bit.
> 
> The nice thing is that you can have that even if you have no further
> information about the brightness control and it might be good enough
> for some use cases but it isn't for others.
> 
> > If an end user wants defined and controlled luminance, I'd suggest they
> > need to profile (physically measure) the response of the display at
> > hand. This is no different from color profiling displays, but you need
> > a measurement device that produces absolute measurements if absolute
> > control is what they want.
> 
> If that's the kind of user experience you're after, good for you. I
> certainly want things to work out of the box which makes this just a
> big no-go.

I think if we have the information to make the default behaviour
better then we should do that. Ie. if the firmaware gives us a
table to remap the values for a more linear response we should
make use of that by default.

We can of course provide a way for the user to plug in their own
actually measured data later. But IMO that doesn't even have to
happen in the initial implementation. Just need to avoid painting
ourselves totally in the corner in a way that would prevent later
additions like that.

I just hate the current limbo where we're somehow too afraid to
change the current behaviour to do the remapping by default.
I see no upsides in the current behaviour of just blindly
exposing the raw hardware register values more or less. They
mean absolutely nothing to any user.

-- 
Ville Syrjälä
Intel

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

[Simon Ser]

On Friday, September 30th, 2022 at 16:44, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:
> 
> > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen ppaalanen@gmail.com wrote:
> > 
> > > On Thu, 29 Sep 2022 20:06:50 +0200
> > > Sebastian Wick sebastian.wick@redhat.com wrote:
> > > 
> > > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > > It would be much clearer if user space can control linear luminance
> > > > and use whatever definition of perceived brightness it wants. The
> > > > obvious downside of it is that it requires bits to encode changes that
> > > > users can't perceive. What about backlights which only have a few
> > > > predefined luminance levels? How would user space differentiate
> > > > between the continuous and discrete backlight? What about
> > > > self-emitting displays? They usually increase the dynamic range when
> > > > they increase in brightness because the black level doesn't rise. They
> > > > also probably employ some tonemapping to adjust for that. What about
> > > > the range of the backlight? What about the absolute luminance of the
> > > > backlight? I want to know about that all in user space.
> > > > 
> > > > I understand that most of the time the kernel doesn't have answers to
> > > > those questions right now but the API should account for all of that.
> > > 
> > > Hi,
> > > 
> > > if the API accounts for all that, and the kernel doesn't know, then how
> > > can the API not lie? If the API sometimes lies, how could we ever trust
> > > it at all?
> > 
> > Make it possible for the API to say "I don't know". I'd much rather
> > have an API tell me explicitly what it does and doesn't know instead
> > of having to guess what data I can actually rely on.
> > 
> > For example if the kernel knows the luminance is linear on one display
> > and doesn't know anything about the other display and it exposes them
> > both in the same way I can not possibly write any code which relies on
> > exact control over the luminance for either display.
> > 
> > > Personally I have the feeling that if we can even get to the level of
> > > "each step in the value is a more or less perceivable change", that
> > > would be good enough. Think of UI, e.g. hotkeys to change brightness.
> > > You'd expect almost every press to change it a bit.
> > 
> > The nice thing is that you can have that even if you have no further
> > information about the brightness control and it might be good enough
> > for some use cases but it isn't for others.
> > 
> > > If an end user wants defined and controlled luminance, I'd suggest they
> > > need to profile (physically measure) the response of the display at
> > > hand. This is no different from color profiling displays, but you need
> > > a measurement device that produces absolute measurements if absolute
> > > control is what they want.
> > 
> > If that's the kind of user experience you're after, good for you. I
> > certainly want things to work out of the box which makes this just a
> > big no-go.
> 
> 
> I think if we have the information to make the default behaviour
> better then we should do that. Ie. if the firmaware gives us a
> table to remap the values for a more linear response we should
> make use of that by default.
> 
> We can of course provide a way for the user to plug in their own
> actually measured data later. But IMO that doesn't even have to
> happen in the initial implementation. Just need to avoid painting
> ourselves totally in the corner in a way that would prevent later
> additions like that.

Yes. Please don't try to solve all of the problems at once. There have
been many tries to add brightness to KMS, and having *something* would
be a lot better than having nothing. If we try to have the perfect
complete API from the start then we'll never get anything done.

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5147 bytes --]

On Fri, 30 Sep 2022 17:44:17 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:
> > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > >
> > > On Thu, 29 Sep 2022 20:06:50 +0200
> > > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> > >  
> > > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > > It would be much clearer if user space can control linear luminance
> > > > and use whatever definition of perceived brightness it wants. The
> > > > obvious downside of it is that it requires bits to encode changes that
> > > > users can't perceive. What about backlights which only have a few
> > > > predefined luminance levels? How would user space differentiate
> > > > between the continuous and discrete backlight? What about
> > > > self-emitting displays? They usually increase the dynamic range when
> > > > they increase in brightness because the black level doesn't rise. They
> > > > also probably employ some tonemapping to adjust for that. What about
> > > > the range of the backlight? What about the absolute luminance of the
> > > > backlight? I want to know about that all in user space.
> > > >
> > > > I understand that most of the time the kernel doesn't have answers to
> > > > those questions right now but the API should account for all of that.  
> > >
> > > Hi,
> > >
> > > if the API accounts for all that, and the kernel doesn't know, then how
> > > can the API not lie? If the API sometimes lies, how could we ever trust
> > > it at all?  
> > 
> > Make it possible for the API to say "I don't know". I'd much rather
> > have an API tell me explicitly what it does and doesn't know instead
> > of having to guess what data I can actually rely on.
> > 
> > For example if the kernel knows the luminance is linear on one display
> > and doesn't know anything about the other display and it exposes them
> > both in the same way I can not possibly write any code which relies on
> > exact control over the luminance for either display.
> >   
> > >
> > > Personally I have the feeling that if we can even get to the level of
> > > "each step in the value is a more or less perceivable change", that
> > > would be good enough. Think of UI, e.g. hotkeys to change brightness.
> > > You'd expect almost every press to change it a bit.  
> > 
> > The nice thing is that you can have that even if you have no further
> > information about the brightness control and it might be good enough
> > for some use cases but it isn't for others.
> >   
> > > If an end user wants defined and controlled luminance, I'd suggest they
> > > need to profile (physically measure) the response of the display at
> > > hand. This is no different from color profiling displays, but you need
> > > a measurement device that produces absolute measurements if absolute
> > > control is what they want.  
> > 
> > If that's the kind of user experience you're after, good for you. I
> > certainly want things to work out of the box which makes this just a
> > big no-go.  
> 
> I think if we have the information to make the default behaviour
> better then we should do that. Ie. if the firmaware gives us a
> table to remap the values for a more linear response we should
> make use of that by default.

But that's only like 20% of what Sebastian is asking for.

What's "linear"? Radiometric or perceptual?

Radiometric linear control would make a terrible UX, so if the control
is radiometric, userspace needs to remap it. That might be a good
thing, but it's also complicated, because the relationship between
brightness and luminance is somewhere between a power curve and
exponential curve. You need to make sure that the userspace remapping
works for different backlights with different luminance ranges. That's
not obvious to me.

> We can of course provide a way for the user to plug in their own
> actually measured data later. But IMO that doesn't even have to
> happen in the initial implementation. Just need to avoid painting
> ourselves totally in the corner in a way that would prevent later
> additions like that.

For userspace delivering its own curve, you need to define the units.
Absolute or relative? Radiometric or perceptual? Otherwise the
resulting control is not portable between window systems.

> I just hate the current limbo where we're somehow too afraid to
> change the current behaviour to do the remapping by default.
> I see no upsides in the current behaviour of just blindly
> exposing the raw hardware register values more or less. They
> mean absolutely nothing to any user.

I never argued like that.

I'm saying that what looks realistic to me is somewhere *between*
status quo and what Sebastian is asking for. Whatever you mean by "linear
remapping" is probably a realistic goal, because you know you have some
hardware/firmware delivering that information already.

OTOH, designing UAPI for information that exists only in our dreams
is... well.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Sebastian Wick]

On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>
> On Fri, 30 Sep 2022 17:44:17 +0300
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>
> > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:
> > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > > >
> > > > On Thu, 29 Sep 2022 20:06:50 +0200
> > > > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> > > >
> > > > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > > > It would be much clearer if user space can control linear luminance
> > > > > and use whatever definition of perceived brightness it wants. The
> > > > > obvious downside of it is that it requires bits to encode changes that
> > > > > users can't perceive. What about backlights which only have a few
> > > > > predefined luminance levels? How would user space differentiate
> > > > > between the continuous and discrete backlight? What about
> > > > > self-emitting displays? They usually increase the dynamic range when
> > > > > they increase in brightness because the black level doesn't rise. They
> > > > > also probably employ some tonemapping to adjust for that. What about
> > > > > the range of the backlight? What about the absolute luminance of the
> > > > > backlight? I want to know about that all in user space.
> > > > >
> > > > > I understand that most of the time the kernel doesn't have answers to
> > > > > those questions right now but the API should account for all of that.
> > > >
> > > > Hi,
> > > >
> > > > if the API accounts for all that, and the kernel doesn't know, then how
> > > > can the API not lie? If the API sometimes lies, how could we ever trust
> > > > it at all?
> > >
> > > Make it possible for the API to say "I don't know". I'd much rather
> > > have an API tell me explicitly what it does and doesn't know instead
> > > of having to guess what data I can actually rely on.
> > >
> > > For example if the kernel knows the luminance is linear on one display
> > > and doesn't know anything about the other display and it exposes them
> > > both in the same way I can not possibly write any code which relies on
> > > exact control over the luminance for either display.
> > >
> > > >
> > > > Personally I have the feeling that if we can even get to the level of
> > > > "each step in the value is a more or less perceivable change", that
> > > > would be good enough. Think of UI, e.g. hotkeys to change brightness.
> > > > You'd expect almost every press to change it a bit.
> > >
> > > The nice thing is that you can have that even if you have no further
> > > information about the brightness control and it might be good enough
> > > for some use cases but it isn't for others.
> > >
> > > > If an end user wants defined and controlled luminance, I'd suggest they
> > > > need to profile (physically measure) the response of the display at
> > > > hand. This is no different from color profiling displays, but you need
> > > > a measurement device that produces absolute measurements if absolute
> > > > control is what they want.
> > >
> > > If that's the kind of user experience you're after, good for you. I
> > > certainly want things to work out of the box which makes this just a
> > > big no-go.
> >
> > I think if we have the information to make the default behaviour
> > better then we should do that. Ie. if the firmaware gives us a
> > table to remap the values for a more linear response we should
> > make use of that by default.
>
> But that's only like 20% of what Sebastian is asking for.
>
> What's "linear"? Radiometric or perceptual?
>
> Radiometric linear control would make a terrible UX, so if the control
> is radiometric, userspace needs to remap it. That might be a good
> thing, but it's also complicated, because the relationship between
> brightness and luminance is somewhere between a power curve and
> exponential curve. You need to make sure that the userspace remapping
> works for different backlights with different luminance ranges. That's
> not obvious to me.
>
> > We can of course provide a way for the user to plug in their own
> > actually measured data later. But IMO that doesn't even have to
> > happen in the initial implementation. Just need to avoid painting
> > ourselves totally in the corner in a way that would prevent later
> > additions like that.
>
> For userspace delivering its own curve, you need to define the units.
> Absolute or relative? Radiometric or perceptual? Otherwise the
> resulting control is not portable between window systems.
>
> > I just hate the current limbo where we're somehow too afraid to
> > change the current behaviour to do the remapping by default.
> > I see no upsides in the current behaviour of just blindly
> > exposing the raw hardware register values more or less. They
> > mean absolutely nothing to any user.
>
> I never argued like that.
>
> I'm saying that what looks realistic to me is somewhere *between*
> status quo and what Sebastian is asking for. Whatever you mean by "linear
> remapping" is probably a realistic goal, because you know you have some
> hardware/firmware delivering that information already.
>
> OTOH, designing UAPI for information that exists only in our dreams
> is... well.

I also didn't say that we should design an UAPI for what doesn't
exist, just that we should design the UAPI we actually need in a way
that when we get more information we can properly expose that. So if
the UAPI exposes anything other than the luminance (e.g. "each step is
a perceptual step in brightness", "linear brightness", ..) we have to
put some human perception model into the kernel which is ridiculous
and it makes it impossible to expose luminance to user space if the
kernel has that information.

It's really easy to paint yourself into a corner here.

>
>
> Thanks,
> pq

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4286 bytes --]

On Fri, 30 Sep 2022 18:17:39 +0200
Sebastian Wick <sebastian.wick@redhat.com> wrote:

> On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >
> > On Fri, 30 Sep 2022 17:44:17 +0300
> > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >  
> > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:  
> > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > >
> > > > > On Thu, 29 Sep 2022 20:06:50 +0200
> > > > > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> > > > >  
> > > > > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > > > > It would be much clearer if user space can control linear luminance
> > > > > > and use whatever definition of perceived brightness it wants. The
> > > > > > obvious downside of it is that it requires bits to encode changes that
> > > > > > users can't perceive. What about backlights which only have a few
> > > > > > predefined luminance levels? How would user space differentiate
> > > > > > between the continuous and discrete backlight? What about
> > > > > > self-emitting displays? They usually increase the dynamic range when
> > > > > > they increase in brightness because the black level doesn't rise. They
> > > > > > also probably employ some tonemapping to adjust for that. What about
> > > > > > the range of the backlight? What about the absolute luminance of the
> > > > > > backlight? I want to know about that all in user space.
> > > > > >
> > > > > > I understand that most of the time the kernel doesn't have answers to
> > > > > > those questions right now but the API should account for all of that.  

...

> > I'm saying that what looks realistic to me is somewhere *between*
> > status quo and what Sebastian is asking for. Whatever you mean by "linear
> > remapping" is probably a realistic goal, because you know you have some
> > hardware/firmware delivering that information already.
> >
> > OTOH, designing UAPI for information that exists only in our dreams
> > is... well.  
> 
> I also didn't say that we should design an UAPI for what doesn't
> exist, just that we should design the UAPI we actually need in a way
> that when we get more information we can properly expose that. So if
> the UAPI exposes anything other than the luminance (e.g. "each step is
> a perceptual step in brightness", "linear brightness", ..) we have to
> put some human perception model into the kernel which is ridiculous
> and it makes it impossible to expose luminance to user space if the
> kernel has that information.

You don't need a human perception model in the kernel. You also cannot
have one, because I would expect most or all backlight and their
metadata to not define luminance at all. But that is just a guess.

I suppose the firmware may expose some tables that may allow mapping
raw hardware values into something more pleasant to use. Like something
where each step is more or less a visible change. That does not have to
imply anything about linearity in any space, they may just be "good
values" for e.g. keyboard-based changing of backlight levels with no
mathematical or physical basis.

Ville, what kind of tables do you know about? What do they actually
tell?

Let's say we have these first properties defined as "reasonable steps
for manual backlight control": one integer for the value, another
integer for the max. If we later see that we can actually get more
precise or high-level information, we can add new optional properties,
e.g. a blob with table that maps the integers into some better defined
quantity.

Then we know what the values mean, but the steps may be quite
coarse, much coarser than what the raw control value allows. That's the
next problem: if we want as fine control as hardware is capable, how do
you expose that?

Should the answer be, that the exposed integer is actually a raw value
for hardware, merely offsetted so that zero maps to minimum but visible
backlight level, and then add another property from the start with a
table for the "good values" for keyboard control?

And the "good values" would be literally just that, no implication of
linearity of anything.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Hans de Goede]

Hi,

On 9/28/22 12:04, Jani Nikula wrote:
> On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi all,
>>
>> Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
>>
>> Changes from version 1:
>> - Drop bl_brightness_0_is_min_brightness from list of new connector
>>   properties.
>> - Clearly define that 0 is always min-brightness when setting the brightness
>>   through the connector properties.
>> - Drop bl_brightness_control_method from list of new connector
>>   properties.
>> - Phase 1 of the plan has been completed
>>
>> As discussed already several times in the past:
>>  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
>>  https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
>>
>> The current userspace API for brightness control offered by
>> /sys/class/backlight devices has various issues:
>>
>> 1. There is no way to map the backlight device to a specific
>>    display-output / panel (1)
>> 2. Controlling the brightness requires root-rights requiring
>>    desktop-environments to use suid-root helpers for this.
>> 3. The meaning of 0 is not clearly defined, it can be either off,
>>    or minimum brightness at which the display is still readable
>>    (in a low light environment)
>> 4. It's not possible to change both the gamma and the brightness in the
>>    same KMS atomic commit. You'd want to be able to reduce brightness to
>>    conserve power, and counter the effects of that by changing gamma to
>>    reach a visually similar image. And you'd want to have the changes take
>>    effect at the same time instead of reducing brightness at some frame and
>>    change gamma at some other frame. This is pretty much impossible to do
>>    via the sysfs interface.
>>
>> As already discussed on various conference's hallway tracks
>> and as has been proposed on the dri-devel list once before (2),
>> it seems that there is consensus that the best way to to solve these
>> 2 issues is to add support for controlling a video-output's brightness
>> through properties on the drm_connector.
>>
>> This RFC outlines my plan to try and actually implement this,
>> which has 3 phases:
>>
>>
>> Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
>> =================================================================================
>>
>> On x86 there can be multiple firmware + direct-hw-access methods
>> for controlling the backlight and in some cases the kernel registers
>> multiple backlight-devices for a single internal laptop LCD panel.
>>
>> A plan to fix this was posted here:
>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
>> And a pull-req actually implementing this plan has been send out this week:
>> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
>>
>>
>> Phase 2: Add drm_connector properties mirroring the matching backlight device
>> =============================================================================
>>
>> The plan is to add a drm_connector helper function, which optionally takes
>> a pointer to the backlight device for the GPU's native backlight device,
>> which will then mirror the backlight settings from the backlight device
>> in a set of read/write brightness* properties on the connector.
>>
>> This function can then be called by GPU drivers for the drm_connector for
>> the internal panel and it will then take care of everything. When there
>> is no native GPU backlight device, or when it should not be used then
>> (on x86) the helper will use the acpi_video_get_backlight_type() to
>> determine which backlight-device should be used instead and it will find
>> + mirror that one.
>>
>>
>> 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.
>>
>>
>> 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".
> 
> This could use a few words explaining the choice of range and property
> type. (I assume it's because you can't change a range property's max at
> runtime. Which is also why you need a separate max property.)

Right. One of the things some users are asking for is brightness control for
external monitors through e.g. DDC/DI which means that the control only
becomes available when the monitor is plugged in, which with laptops
may be much later then boot.

> 
>> 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.
> 
> Do you mean every step should be a visible change?

No that would mean that more or less raw 16 bit pwm controls would need
to loose a lot of range. This is more about what Ville mentions further
in the thread (and which you mention below) when the raw PWM directly controls
a LED backlight, or when it linearly controls a current-source for a LED backlight
then when going from 10% dutycycle (which would be 0) to 20% would double
the brightness where as going from 90% to 100& only adds 11%.

Some brightness control methods use a curve to make things feel more linear
giving a similar % brightness increase on all parts of the curve.

What I'm trying to say here is that ideally the control should always
follow such a curve (but only if available, the kernel should not make
up its own curves).

> 
>> 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.
> 
> I think this could be a bit more restrictive in stating the allowed
> runtime changes. Is it only 0 -> actual max for non-hotpluggable
> displays, nothing else, and additionally actual max -> 0 when unplugging
> a display?

Right, only 0 -> actual max for non-hotpluggable displays, nothing else
and additionally actual max -> 0 when unplugging a display?

I'll adjust the wording here a bit, but I don't plan to make
the non-hotpluggable vs hotpluggable difference though. I'm not sure
that is helpful. With LCD panel muxes used in some hybrid GPU
designs even a laptop panel can be hotplugged (from the GPU pov
on a switch its unplugged/plugged-in).

> 
>>
>>
>> Future extensions
>> =================
>>
>> Some hardware do adaptive brightness in hardware, rather then providing
>> an ALS sensor and letting userspace handle this.
>>
>> One example of this is the Steam deck, which currently uses some custom
>> sysfs attributes to allow tweaking (and enable/disable?) the adaptive
>> brightness. Adding standardized uAPI for this through new
>> "display brightness *" properties seems like a natural extension of this
>> proposal.
> 
> Another example is adjusting for non-linear luminance curve. It would be
> nicer for equal steps in brightness value to have similar luminance
> changes.

Actually that is sorta what I meant above. So I guess what you are
suggesting is allowing userspace to load some adjustment curve into the
kernel ?  That would definitely fall under the "Future Extensions"
bit, but yeah that would be interesting. In some cases we may even be
able to pre-populate such a curve with firmware provided data.

> I think sometimes we lose precision with the limited number of
> steps in UIs. But this is thinking pretty far ahead. :)
> 
> Other than the minor clarifications, the plans sounds good to me.

Thank you for the review. 
> Thanks again for doing all this, much appreciated.

You're welcome.

Regards,

Hans



> 
> 
> BR,
> Jani.
> 
>>
>> Regards,
>>
>> Hans
>>
>>
>> 1) The need to be able to map the backlight device to a specific display
>> has become clear once more with the recent proposal to add DDCDI support:
>> https://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/
>>
>> 2) https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
>> Note this proposal included a method for userspace to be able to tell the
>> kernel if the native/acpi_video/vendor backlight device should be used,
>> but this has been solved in the kernel for years now:
>>  https://www.spinics.net/lists/linux-acpi/msg50526.html
>> An initial implementation of this proposal is available here:
>>  https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight
>>
>>
> 

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

[Hans de Goede]

Hi,

On 9/28/22 12:57, Ville Syrjälä wrote:
> On Wed, Sep 28, 2022 at 01:04:01PM +0300, Jani Nikula wrote:
>> On Fri, 09 Sep 2022, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi all,
>>>
>>> Here is v2 of my "drm/kms: control display brightness through drm_connector properties" RFC:
>>>
>>> Changes from version 1:
>>> - Drop bl_brightness_0_is_min_brightness from list of new connector
>>>   properties.
>>> - Clearly define that 0 is always min-brightness when setting the brightness
>>>   through the connector properties.
>>> - Drop bl_brightness_control_method from list of new connector
>>>   properties.
>>> - Phase 1 of the plan has been completed
>>>
>>> As discussed already several times in the past:
>>>  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/
>>>  https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.intel.com/
>>>
>>> The current userspace API for brightness control offered by
>>> /sys/class/backlight devices has various issues:
>>>
>>> 1. There is no way to map the backlight device to a specific
>>>    display-output / panel (1)
>>> 2. Controlling the brightness requires root-rights requiring
>>>    desktop-environments to use suid-root helpers for this.
>>> 3. The meaning of 0 is not clearly defined, it can be either off,
>>>    or minimum brightness at which the display is still readable
>>>    (in a low light environment)
>>> 4. It's not possible to change both the gamma and the brightness in the
>>>    same KMS atomic commit. You'd want to be able to reduce brightness to
>>>    conserve power, and counter the effects of that by changing gamma to
>>>    reach a visually similar image. And you'd want to have the changes take
>>>    effect at the same time instead of reducing brightness at some frame and
>>>    change gamma at some other frame. This is pretty much impossible to do
>>>    via the sysfs interface.
>>>
>>> As already discussed on various conference's hallway tracks
>>> and as has been proposed on the dri-devel list once before (2),
>>> it seems that there is consensus that the best way to to solve these
>>> 2 issues is to add support for controlling a video-output's brightness
>>> through properties on the drm_connector.
>>>
>>> This RFC outlines my plan to try and actually implement this,
>>> which has 3 phases:
>>>
>>>
>>> Phase 1: Stop registering multiple /sys/class/backlight devs for a single display
>>> =================================================================================
>>>
>>> On x86 there can be multiple firmware + direct-hw-access methods
>>> for controlling the backlight and in some cases the kernel registers
>>> multiple backlight-devices for a single internal laptop LCD panel.
>>>
>>> A plan to fix this was posted here:
>>> https://lore.kernel.org/dri-devel/98519ba0-7f18-201a-ea34-652f50343158@redhat.com/
>>> And a pull-req actually implementing this plan has been send out this week:
>>> https://lore.kernel.org/dri-devel/261afe3d-7790-e945-adf6-a2c96c9b1eff@redhat.com/
>>>
>>>
>>> Phase 2: Add drm_connector properties mirroring the matching backlight device
>>> =============================================================================
>>>
>>> The plan is to add a drm_connector helper function, which optionally takes
>>> a pointer to the backlight device for the GPU's native backlight device,
>>> which will then mirror the backlight settings from the backlight device
>>> in a set of read/write brightness* properties on the connector.
>>>
>>> This function can then be called by GPU drivers for the drm_connector for
>>> the internal panel and it will then take care of everything. When there
>>> is no native GPU backlight device, or when it should not be used then
>>> (on x86) the helper will use the acpi_video_get_backlight_type() to
>>> determine which backlight-device should be used instead and it will find
>>> + mirror that one.
>>>
>>>
>>> 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.
>>>
>>>
>>> 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".
>>
>> This could use a few words explaining the choice of range and property
>> type. (I assume it's because you can't change a range property's max at
>> runtime. Which is also why you need a separate max property.)
> 
> Why don't we just normalize the range to something sensible?

Because:

1. Userspace needs to know the effective range, some older laptops only have 7 or 8 levels
and GNOME by default does 20 steps when using the hotkeys. If GNOME does not know there are
only 7 or 8 levels then every other step won't do anything on those models.

2. Because we don't want to loose precision so we would need to normalize to something
really large like say 2^24, which means that userspace sees a false precision which
is not really there, see 1.

3. Because we still have a lot of userspace code using the old API and we cannot just
go and change the range there. So we would need to convert when going between the 2,
which eventually leads to rounding errors. E.g. backlight levels get saved/restored
over reboots by systemd using the old API. So we could end up with the value drifting 
over time because of rounding errors in back/forward conversion.

So all in all it is just easier and cleaner to have the new API mirror the old
API and give userspace the raw range of the chosen control method, minus a
possible min value cut off at the bottom of the range to avoid the screen
going black. Since this cutoff is a fixed conversion of +/- the min value
it does not have the round-trip rounding error problem.

> 
>>
>>> 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.
>>
>> Do you mean every step should be a visible change?
> 
> Hmm. I guess due to this. I'd prefer the opposite tbh so I could
> just put in my opregion BCLM patch. It's annoying to have to
> carry it locally just to have reasonable backlight behaviour.
> 

From your next email:

> After second though I guess I'm actually agreeing with Hans here.
> The current situation is where small change in the value near one
> end of the range does basically nothing, while a small change at
> the other of the range causes a massive brightness change. That
> is no good.

Yes exactly, this proposal actually suggests (suggests not mandates)
that your patch gets merged. As Jani mentioned it might even be
interesting too make the "curve" available as a property (on
select hw) which can be read + write by userspace and which then
gets pre-populated with the BCLM data. But that is for once the
basics have all landed. And we could already add the curve
internally before we have hashed out such an extra property.

Regards,

Hans

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

[Ville Syrjälä]

On Mon, Oct 03, 2022 at 11:37:50AM +0300, Pekka Paalanen wrote:
> On Fri, 30 Sep 2022 18:17:39 +0200
> Sebastian Wick <sebastian.wick@redhat.com> wrote:
> 
> > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
> > >
> > > On Fri, 30 Sep 2022 17:44:17 +0300
> > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > >  
> > > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:  
> > > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > > > >
> > > > > > On Thu, 29 Sep 2022 20:06:50 +0200
> > > > > > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> > > > > >  
> > > > > > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > > > > > It would be much clearer if user space can control linear luminance
> > > > > > > and use whatever definition of perceived brightness it wants. The
> > > > > > > obvious downside of it is that it requires bits to encode changes that
> > > > > > > users can't perceive. What about backlights which only have a few
> > > > > > > predefined luminance levels? How would user space differentiate
> > > > > > > between the continuous and discrete backlight? What about
> > > > > > > self-emitting displays? They usually increase the dynamic range when
> > > > > > > they increase in brightness because the black level doesn't rise. They
> > > > > > > also probably employ some tonemapping to adjust for that. What about
> > > > > > > the range of the backlight? What about the absolute luminance of the
> > > > > > > backlight? I want to know about that all in user space.
> > > > > > >
> > > > > > > I understand that most of the time the kernel doesn't have answers to
> > > > > > > those questions right now but the API should account for all of that.  
> 
> ...
> 
> > > I'm saying that what looks realistic to me is somewhere *between*
> > > status quo and what Sebastian is asking for. Whatever you mean by "linear
> > > remapping" is probably a realistic goal, because you know you have some
> > > hardware/firmware delivering that information already.
> > >
> > > OTOH, designing UAPI for information that exists only in our dreams
> > > is... well.  
> > 
> > I also didn't say that we should design an UAPI for what doesn't
> > exist, just that we should design the UAPI we actually need in a way
> > that when we get more information we can properly expose that. So if
> > the UAPI exposes anything other than the luminance (e.g. "each step is
> > a perceptual step in brightness", "linear brightness", ..) we have to
> > put some human perception model into the kernel which is ridiculous
> > and it makes it impossible to expose luminance to user space if the
> > kernel has that information.
> 
> You don't need a human perception model in the kernel. You also cannot
> have one, because I would expect most or all backlight and their
> metadata to not define luminance at all. But that is just a guess.
> 
> I suppose the firmware may expose some tables that may allow mapping
> raw hardware values into something more pleasant to use. Like something
> where each step is more or less a visible change. That does not have to
> imply anything about linearity in any space, they may just be "good
> values" for e.g. keyboard-based changing of backlight levels with no
> mathematical or physical basis.
> 
> Ville, what kind of tables do you know about? What do they actually
> tell?

We just get a set of points (up to 20 originally, and I think up to
32 in later spec revisions) that map input brightness (in %) to
output duty cycle (0-0xff in old spec, 0-0xffff in new spec).
And I *think* we're supposed to just linearly inteprolate between
the entries, though the spec doesn't really state that in super
clear terms.

There is some mention in the spec about this being more or less
designed for Windows Vista, so a look through eg.
https://learn.microsoft.com/en-us/windows-hardware/drivers/display/supporting-brightness-controls-on-integrated-display-panels
might be a good idea here.

-- 
Ville Syrjälä
Intel

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

[Hans de Goede]

Hi,

On 9/30/22 18:17, Sebastian Wick wrote:
> On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>
>> On Fri, 30 Sep 2022 17:44:17 +0300
>> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>>
>>> On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:
>>>> On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:
>>>>>
>>>>> On Thu, 29 Sep 2022 20:06:50 +0200
>>>>> Sebastian Wick <sebastian.wick@redhat.com> wrote:
>>>>>
>>>>>> If it is supposed to be a non-linear luminance curve, which one is it?
>>>>>> It would be much clearer if user space can control linear luminance
>>>>>> and use whatever definition of perceived brightness it wants. The
>>>>>> obvious downside of it is that it requires bits to encode changes that
>>>>>> users can't perceive. What about backlights which only have a few
>>>>>> predefined luminance levels? How would user space differentiate
>>>>>> between the continuous and discrete backlight? What about
>>>>>> self-emitting displays? They usually increase the dynamic range when
>>>>>> they increase in brightness because the black level doesn't rise. They
>>>>>> also probably employ some tonemapping to adjust for that. What about
>>>>>> the range of the backlight? What about the absolute luminance of the
>>>>>> backlight? I want to know about that all in user space.
>>>>>>
>>>>>> I understand that most of the time the kernel doesn't have answers to
>>>>>> those questions right now but the API should account for all of that.
>>>>>
>>>>> Hi,
>>>>>
>>>>> if the API accounts for all that, and the kernel doesn't know, then how
>>>>> can the API not lie? If the API sometimes lies, how could we ever trust
>>>>> it at all?
>>>>
>>>> Make it possible for the API to say "I don't know". I'd much rather
>>>> have an API tell me explicitly what it does and doesn't know instead
>>>> of having to guess what data I can actually rely on.
>>>>
>>>> For example if the kernel knows the luminance is linear on one display
>>>> and doesn't know anything about the other display and it exposes them
>>>> both in the same way I can not possibly write any code which relies on
>>>> exact control over the luminance for either display.
>>>>
>>>>>
>>>>> Personally I have the feeling that if we can even get to the level of
>>>>> "each step in the value is a more or less perceivable change", that
>>>>> would be good enough. Think of UI, e.g. hotkeys to change brightness.
>>>>> You'd expect almost every press to change it a bit.
>>>>
>>>> The nice thing is that you can have that even if you have no further
>>>> information about the brightness control and it might be good enough
>>>> for some use cases but it isn't for others.
>>>>
>>>>> If an end user wants defined and controlled luminance, I'd suggest they
>>>>> need to profile (physically measure) the response of the display at
>>>>> hand. This is no different from color profiling displays, but you need
>>>>> a measurement device that produces absolute measurements if absolute
>>>>> control is what they want.
>>>>
>>>> If that's the kind of user experience you're after, good for you. I
>>>> certainly want things to work out of the box which makes this just a
>>>> big no-go.
>>>
>>> I think if we have the information to make the default behaviour
>>> better then we should do that. Ie. if the firmaware gives us a
>>> table to remap the values for a more linear response we should
>>> make use of that by default.
>>
>> But that's only like 20% of what Sebastian is asking for.
>>
>> What's "linear"? Radiometric or perceptual?
>>
>> Radiometric linear control would make a terrible UX, so if the control
>> is radiometric, userspace needs to remap it. That might be a good
>> thing, but it's also complicated, because the relationship between
>> brightness and luminance is somewhere between a power curve and
>> exponential curve. You need to make sure that the userspace remapping
>> works for different backlights with different luminance ranges. That's
>> not obvious to me.
>>
>>> We can of course provide a way for the user to plug in their own
>>> actually measured data later. But IMO that doesn't even have to
>>> happen in the initial implementation. Just need to avoid painting
>>> ourselves totally in the corner in a way that would prevent later
>>> additions like that.
>>
>> For userspace delivering its own curve, you need to define the units.
>> Absolute or relative? Radiometric or perceptual? Otherwise the
>> resulting control is not portable between window systems.
>>
>>> I just hate the current limbo where we're somehow too afraid to
>>> change the current behaviour to do the remapping by default.
>>> I see no upsides in the current behaviour of just blindly
>>> exposing the raw hardware register values more or less. They
>>> mean absolutely nothing to any user.
>>
>> I never argued like that.
>>
>> I'm saying that what looks realistic to me is somewhere *between*
>> status quo and what Sebastian is asking for. Whatever you mean by "linear
>> remapping" is probably a realistic goal, because you know you have some
>> hardware/firmware delivering that information already.
>>
>> OTOH, designing UAPI for information that exists only in our dreams
>> is... well.
> 
> I also didn't say that we should design an UAPI for what doesn't
> exist, just that we should design the UAPI we actually need in a way
> that when we get more information we can properly expose that. So if
> the UAPI exposes anything other than the luminance (e.g. "each step is
> a perceptual step in brightness", "linear brightness", ..) we have to
> put some human perception model into the kernel which is ridiculous
> and it makes it impossible to expose luminance to user space if the
> kernel has that information.
> 
> It's really easy to paint yourself into a corner here.

The problem is we are already in that corner and this is not something
which we can fix in the kernel.

I agree that we don't want to put a "human perception model" model
into the kernel. But at the same time the scale of the brightness
control methods already is widely unknown.

Simplifying things greatly there are 2 groups of brightness control
methods:

1. Some firmware interface
2. Directly programming a raw PWM output

For 1. we have no clue of the scale it might be linear electrical
power send to the backlight. But it often already has some perceived
brightness curve applied with the exact curve unknown.

For 2. The PWM might directly turn the LEDs on/off rapidly, making
the duty-cycle a 1:1 linear map of the amount of Watts consumed
by the backlight. But on some (a lot?) have the raw PWM output is
used as input to a LED driver IC which is basically a PWM controlled
current-source. There might be a 1:1 mapping here between the PWM
input and the amount of Ampere coming out of the current-source.
OTOH these backlight drive ICs often have the option to load
some PWM -> Amperage curve in them to do something like
perceived brightness again. And the amount of nits coming out
of the LEDs is also not a linear function of the Amperage going
into the LEDs.

Sebastian, I understand that you would really like a control
where you can just say, amount of nits coming form the screen
is 100 / 200 / 400 (when screen is fully white). But we don't
even know how many nits the backlight (or OLEDs) will send
at full brightness settings, we don't know what if any curve
is being applied by the firmware and/or by some backlight driver
IC and even if knew all that the amount of nits will change
over time due to wear and tear.

So I understand that in an ideal world you would want all this
info for HDR support, but we simply do not have it.

So for now it is important to at least move brightness control
into KMS using a simple uAPI where the only thing we can really
say about the scale is higher values result in a higher brightness.

And then once that is in place we can look at trying to export
more information; in so far as we have that information.

One example mentioned here is that laptops with Intel integrated
graphics may have some "perceived brightness" mapping table
in their Video BIOS Tables (VBT) it would be interesting to use
this and to export the curve coming from that to userspace as
extra information (including allow userspace to write it).
Since in this case (unlike many other cases) at least this
curve is done in the kernel I guess we can then also add a boolean
property to disable the curve (making it linear) in case that
is helpful for HDR scenarios.

But I'm afraid that for now we really need to play it by ear
here since so much of what you want is hidden from the kernel
and will differ greatly from one model laptop to the next.

Regards,

Hans

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 4164 bytes --]

On Mon, 3 Oct 2022 12:29:01 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

> On Mon, Oct 03, 2022 at 11:37:50AM +0300, Pekka Paalanen wrote:
> > On Fri, 30 Sep 2022 18:17:39 +0200
> > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> >   
> > > On Fri, Sep 30, 2022 at 5:27 PM Pekka Paalanen <ppaalanen@gmail.com> wrote:  
> > > >
> > > > On Fri, 30 Sep 2022 17:44:17 +0300
> > > > Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > > >    
> > > > > On Fri, Sep 30, 2022 at 04:20:29PM +0200, Sebastian Wick wrote:    
> > > > > > On Fri, Sep 30, 2022 at 9:40 AM Pekka Paalanen <ppaalanen@gmail.com> wrote:    
> > > > > > >
> > > > > > > On Thu, 29 Sep 2022 20:06:50 +0200
> > > > > > > Sebastian Wick <sebastian.wick@redhat.com> wrote:
> > > > > > >    
> > > > > > > > If it is supposed to be a non-linear luminance curve, which one is it?
> > > > > > > > It would be much clearer if user space can control linear luminance
> > > > > > > > and use whatever definition of perceived brightness it wants. The
> > > > > > > > obvious downside of it is that it requires bits to encode changes that
> > > > > > > > users can't perceive. What about backlights which only have a few
> > > > > > > > predefined luminance levels? How would user space differentiate
> > > > > > > > between the continuous and discrete backlight? What about
> > > > > > > > self-emitting displays? They usually increase the dynamic range when
> > > > > > > > they increase in brightness because the black level doesn't rise. They
> > > > > > > > also probably employ some tonemapping to adjust for that. What about
> > > > > > > > the range of the backlight? What about the absolute luminance of the
> > > > > > > > backlight? I want to know about that all in user space.
> > > > > > > >
> > > > > > > > I understand that most of the time the kernel doesn't have answers to
> > > > > > > > those questions right now but the API should account for all of that.    

...

> > I suppose the firmware may expose some tables that may allow mapping
> > raw hardware values into something more pleasant to use. Like something
> > where each step is more or less a visible change. That does not have to
> > imply anything about linearity in any space, they may just be "good
> > values" for e.g. keyboard-based changing of backlight levels with no
> > mathematical or physical basis.
> > 
> > Ville, what kind of tables do you know about? What do they actually
> > tell?  
> 
> We just get a set of points (up to 20 originally, and I think up to
> 32 in later spec revisions) that map input brightness (in %) to
> output duty cycle (0-0xff in old spec, 0-0xffff in new spec).
> And I *think* we're supposed to just linearly inteprolate between
> the entries, though the spec doesn't really state that in super
> clear terms.
> 
> There is some mention in the spec about this being more or less
> designed for Windows Vista, so a look through eg.
> https://learn.microsoft.com/en-us/windows-hardware/drivers/display/supporting-brightness-controls-on-integrated-display-panels
> might be a good idea here.

That's a nice link. Quote:

"Brightness levels are represented as single-byte values in the range
from zero to 100 where zero is off and 100 is the maximum brightness
that a laptop computer supports. Every laptop computer must report a
maximum brightness level of 100; however, a laptop computer is not
required to support a level of zero. The only requirement for values
from zero to 100 is that larger values must represent higher brightness
levels. The increment between levels is not required to be uniform, and
a laptop computer can support any number of distinct values up to the
maximum of 101 levels. You must decide how to map hardware levels to
the range of brightness level values. However, a call to the display
miniport driver's DxgkDdiGetPossibleBrightness function should not
report more brightness level values than the hardware supports."

Sounds like "good values" to me, and that each value must be
distinguishable from any another (at least electrically).


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

On Mon, 3 Oct 2022 11:44:56 +0200
Hans de Goede <hdegoede@redhat.com> wrote:

> One example mentioned here is that laptops with Intel integrated
> graphics may have some "perceived brightness" mapping table
> in their Video BIOS Tables (VBT) it would be interesting to use
> this and to export the curve coming from that to userspace as
> extra information (including allow userspace to write it).
> Since in this case (unlike many other cases) at least this
> curve is done in the kernel I guess we can then also add a boolean
> property to disable the curve (making it linear) in case that
> is helpful for HDR scenarios.

Hi Hans,

what if you would export that curve to userspace and not apply it in
the kernel, aside from the min-luminance=0 offset?

I'm not sure if that was your intention, I didn't see it clearly said.
Of course this can be only about curves that are supposed to be applied
by the OS and not applied in firmware or hardware. Call it "software
curve"?

Let userspace apply that curve if it happens to exist. Then, if we get
some other definition replacing that curve on some hardware, the kernel
could just expose the other thing as a new thing, and the old curve API
would not be interfering. Userspace that does not understand the new
thing (and the old curve property is not populated) would still be able
to control the brightness, just not in as pleasant way.

It would also make using a custom curve a completely userspace thing with
no need for the kernel to support overwriting it.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Hans de Goede]

Hi,

On 10/3/22 12:32, Pekka Paalanen wrote:
> On Mon, 3 Oct 2022 11:44:56 +0200
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> One example mentioned here is that laptops with Intel integrated
>> graphics may have some "perceived brightness" mapping table
>> in their Video BIOS Tables (VBT) it would be interesting to use
>> this and to export the curve coming from that to userspace as
>> extra information (including allow userspace to write it).
>> Since in this case (unlike many other cases) at least this
>> curve is done in the kernel I guess we can then also add a boolean
>> property to disable the curve (making it linear) in case that
>> is helpful for HDR scenarios.
> 
> Hi Hans,
> 
> what if you would export that curve to userspace and not apply it in
> the kernel, aside from the min-luminance=0 offset?
> 
> I'm not sure if that was your intention, I didn't see it clearly said.
> Of course this can be only about curves that are supposed to be applied
> by the OS and not applied in firmware or hardware. Call it "software
> curve"?
> 
> Let userspace apply that curve if it happens to exist. Then, if we get
> some other definition replacing that curve on some hardware, the kernel
> could just expose the other thing as a new thing, and the old curve API
> would not be interfering. Userspace that does not understand the new
> thing (and the old curve property is not populated) would still be able
> to control the brightness, just not in as pleasant way.
> 
> It would also make using a custom curve a completely userspace thing with
> no need for the kernel to support overwriting it.

Userspace comes in many forms, which is why my preference for "software curve"
support would be for it to be applied by the kernel by default (if present
in the VBT) but then with a "software curve enable" flag to allow advanced
userspace to disable it and then apply a curve of userspace's own choosing
itself.

This allows a simple implementation for desktop-environments which are
unlikely to implement all this themselves, giving everyone the benefit of
the nicer behavior of the VBT provided curve while allowing advanced
desktop environments (likely mainly KDE + GNOME/mutter) to do something
more advanced.

Either way, this is all future extensions. First lets get the basic
brightness control with just brightness + brightness-max attributes
in place.

Regards,

Hans