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

[Hans de Goede]

Hi Yusuf,

On 8/24/22 04:18, Yusuf Khan wrote:
> Sorry for the necro-bump, I hadnt seen this go by

No problem.

> My main concern with this proposal is the phasing out of 
> /sys/class/backlight/.
> Currently on the user(user, not userland) level its easier for me to just 
> modify
> the file and be done with it. xbacklight doesnt tell me when its failed,
> brightnessctl doesnt make assumptions about what device is what, and
> other brightness setting applications ive seen are much worse than them.
> Someone needs to create a userland application thats less inconvenient
> than `echo`ing into /sys/class/backlight with a name that human beings can
> actually remember before I stop using the sysfs, perhaps "setbrightness"
> could be the binary's name? Also I dont think its wise to disable or make it
> read only though Kconfig as older apps may depend on it, maybe add a
> kernel param that disables the old interface so bigger distros can pressure
> app makers into changing the interface? As a big draw for DDC/CI is that
> many displays support it as a way to change brightness(even if you arent
> doing anything special that would break the old interface) perhaps it could
> be an early adopter to that kernel parameter?

Right, so deprecating the /sys/class/backlight API definitely is the last
step and probably is years away. As you say hiding / making it read-only
should probably be a kernel-parameter at first, with maybe a Kconfig
option to set the default. So the depcration would go like this:

1. Add:
A kernel-parameter to allow hiding or read-only-ing the sysfs interface +
Kconfig to select the default +
dev_warn_once() when the old API is used

2. (much later) Drop the Kconfig option and default to hiding/read-only

3. (even later) Maybe completely remove the sysfs interface?

Note the hiding vs read-only thing is to be decided. ATM I'm rather more
focused on getting the new API in place then on deprecating the old one :)

Anyways I fully agree that we need to do the deprecation carefully and
slowly. This is likely going to take multiple years and then some ...

Regards,

Hans



> 
> On Thu, Apr 7, 2022 at 10:39 AM Hans de Goede  > wrote:
> 
> 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-d37d844b0...@linux.intel.com/
>  
> 
> 
> The current userspace API for brightness control offered by
> /sys/class/backlight devices has various issues, the biggest 2 being:
> 
> 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.
> 
> 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) i915 and nouveau unconditionally register their "native" backlight dev
>    even on devices where /sys/class/backlight/acpi_video0 must be used
>    to control the backlight, relying on userspace to prefer the "firmware"
>    acpi_video0 device over "native" devices.
> b) amdgpu and nouveau rely on the acpi_video driver initializing before
>    them, which currently causes /sys/class/backlight/acpi_video0 to 
> usually
>    show up and then they register their own native backlight driver after
>    which the drivers/acpi/video_detect.c code unregisters the acpi_video0
>    device. This means that userspace briefly sees 2 devices and the
>    disappearing of acpi_video0 after a brief time confuses the systemd
>    backlight level save/restore code, see e.g.:
>    https://bbs.archlinux.org/viewtopic.php?id=269920 
> 
> 
> I already have a pretty detailed plan to tackle this, which I will
> post in a separate RFC email. I plan to start working on this right
> away, as it will be good to have this fixed regardless.
> 
> 
> Phase 2: Add drm_connector propert

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

[Yusuf Khan]

Perhaps the Kconfig modifications could be postponed to stage 2
since for people running distros that suddenly decide to disable
/sys/class/backlight/ it may be impractical for them to recompile
their kernels and such. Also stage 2 should probably take ~2 decades
until it comes into being, for reference fbdev SPECIFIC drivers
were removed from fedora just recently and because of that there
were some issues with some user's systems. I understand it's much
easier to change from the /sys/class/backlight/ interface to the one
you have proposed than to change from fbdev to KMS though.

On Thu, Aug 25, 2022 at 3:27 AM Hans de Goede  wrote:

> Hi Yusuf,
>
> On 8/24/22 04:18, Yusuf Khan wrote:
> > Sorry for the necro-bump, I hadnt seen this go by
>
> No problem.
>
> > My main concern with this proposal is the phasing out of
> /sys/class/backlight/.
> > Currently on the user(user, not userland) level its easier for me to
> just modify
> > the file and be done with it. xbacklight doesnt tell me when its failed,
> > brightnessctl doesnt make assumptions about what device is what, and
> > other brightness setting applications ive seen are much worse than them.
> > Someone needs to create a userland application thats less inconvenient
> > than `echo`ing into /sys/class/backlight with a name that human beings
> can
> > actually remember before I stop using the sysfs, perhaps "setbrightness"
> > could be the binary's name? Also I dont think its wise to disable or
> make it
> > read only though Kconfig as older apps may depend on it, maybe add a
> > kernel param that disables the old interface so bigger distros can
> pressure
> > app makers into changing the interface? As a big draw for DDC/CI is that
> > many displays support it as a way to change brightness(even if you arent
> > doing anything special that would break the old interface) perhaps it
> could
> > be an early adopter to that kernel parameter?
>
> Right, so deprecating the /sys/class/backlight API definitely is the last
> step and probably is years away. As you say hiding / making it read-only
> should probably be a kernel-parameter at first, with maybe a Kconfig
> option to set the default. So the depcration would go like this:
>
> 1. Add:
> A kernel-parameter to allow hiding or read-only-ing the sysfs interface +
> Kconfig to select the default +
> dev_warn_once() when the old API is used
>
> 2. (much later) Drop the Kconfig option and default to hiding/read-only
>
> 3. (even later) Maybe completely remove the sysfs interface?
>
> Note the hiding vs read-only thing is to be decided. ATM I'm rather more
> focused on getting the new API in place then on deprecating the old one :)
>
> Anyways I fully agree that we need to do the deprecation carefully and
> slowly. This is likely going to take multiple years and then some ...
>
> Regards,
>
> Hans
>
>
>
> >
> > On Thu, Apr 7, 2022 at 10:39 AM Hans de Goede  > wrote:
> >
> > As discussed already several times in the past:
> >  https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ <
> https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/>
> >
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/
> <
> https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b0...@linux.intel.com/
> >
> >
> > The current userspace API for brightness control offered by
> > /sys/class/backlight devices has various issues, the biggest 2 being:
> >
> > 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.
> >
> > 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) i915 and nouveau unconditionally register their "native"
> backlight dev
> >even on devices where /sys/class/backlight/acpi_video0 must be
> used
> >to control the backlight, relying on userspace to prefer the
> "firmware"
> >acpi_video0 device over "native" devices.
> > b) amdgpu and nouveau rely on the acpi_video driver initializing
> before
> >them, whic