From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 68E7AECAAA3 for ; Fri, 26 Aug 2022 07:05:29 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 99F3110E634; Fri, 26 Aug 2022 07:05:27 +0000 (UTC) Received: from mail-yw1-x1130.google.com (mail-yw1-x1130.google.com [IPv6:2607:f8b0:4864:20::1130]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6383510E3DD; Thu, 25 Aug 2022 21:41:02 +0000 (UTC) Received: by mail-yw1-x1130.google.com with SMTP id 00721157ae682-333a4a5d495so574411067b3.10; Thu, 25 Aug 2022 14:41:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc; bh=O6Ua9HoxOpSTIIWrDVj4QfwdKSRV26nNyBSDT2nPg2A=; b=ib8aMbWttBNnuMf3woweau4e/iPPuBYOLPamvwUGG+cHIsZWkxEX+x5l0eqh+zFKAu 27yNMJACwAsT1QVacvR/GihLTyFMNfbK4vvgxn5lMQCiA0kYoTRCGRh1sHk86ahjx/9Q XulRH8cz4+3hyiapZhLd4iin8fDyBean8rddktNq7e1bOjhg1a5x1pxajwiNM4x4KmBp eyGzA6COQexW0AG6zqG83mMVXvvIsSFeyAIxGdBwSz4PhjMw0uxp2jJ/TEyg8xwWQla+ NlM5HIHSM4AAoEXheAWxIKxWCqr/KcjKNt6cVzYbnBxczGtpu5ODMkBa7dTKZXR/vdIW qY/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc; bh=O6Ua9HoxOpSTIIWrDVj4QfwdKSRV26nNyBSDT2nPg2A=; b=5GUadObq8CkT/G/5QKPa+quvF3FrJ+YROIJwLmYgK0KPUWoYvvIfyZNtHsUyUi5SDj fBzAwfPoZ4vuqYL0MnF/JOdI0qRcJRAhEMU0U430I3hovfWK1z7YbQkpj3qdCletR1IO z8pWxAh9p+J5YlAJgiKAU/OioB6i+ksgDa6ZPaeEO3xEcJ7kreKCJAPyCGvBxEhjGnCl m6uhm/YWma29XoptnB0G+c6GweKM5msPGVB45FxvG2ENpVDipEpCE7V22gt+lnK1rWaX 2P/LnL7R3AfWPwkoxhZcXwFpZg2IJVhYP03A0CIolE9vcYQPyeMMeFigq+ewUhbz2hjc M4uw== X-Gm-Message-State: ACgBeo3u9S6H7q1lByGRULEVd2Hv+MQPRrNV+rf8kdOHiBIjcmIo1Vdh nWJgeISqHLg1sN1d6TLPmqObrg+ivzVyRbyL7UVYWz+o X-Google-Smtp-Source: AA6agR7AdW2BCux5Adwd7Cf9boN1HH94XTUbxhV7/t3ksRD1O4wZTOqieskyvhuaV1F0QhCPld9g/a7wm47AgL5bZP8= X-Received: by 2002:a25:391:0:b0:695:b61c:c87 with SMTP id 139-20020a250391000000b00695b61c0c87mr5143269ybd.330.1661463661218; Thu, 25 Aug 2022 14:41:01 -0700 (PDT) MIME-Version: 1.0 References: <0d188965-d809-81b5-74ce-7d30c49fee2d@redhat.com> In-Reply-To: From: Yusuf Khan Date: Thu, 25 Aug 2022 16:40:50 -0500 Message-ID: Subject: Re: [RFC] drm/kms: control display brightness through drm_connector properties To: Hans de Goede Content-Type: multipart/alternative; boundary="00000000000055972f05e717a774" X-Mailman-Approved-At: Fri, 26 Aug 2022 07:05:22 +0000 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Sebastian Wick , Martin Roukala , Christoph Grenz , "dri-devel@lists.freedesktop.org" , wayland Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" --00000000000055972f05e717a774 Content-Type: text/plain; charset="UTF-8" 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-d37d844b02c6@linux.intel.com/ > < > 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, 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 < > 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 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. > > > > An alternative to disabling the sysfs class entirely, would be > > to allow setting it to read-only through Kconfig. > > > > > > What scale to use for the drm_connector bl_brightness property? > > =============================================================== > > > > The tricky part of this plan is phase 2 and then esp. defining what > the > > new brightness properties will look like and how they will work. > > > > The biggest challenge here is to decide on a fixed scale for the main > > brightness property, say 0-65535, using scaling where the actual hw > scale > > is different, or if this should simply be a 1:1 mirror of the current > > backlight interface, with the actual hw scale / brightness_max value > > exposed as a drm_connector property. > > > > 1:1 advantages / 0-65535 disadvantages > > - Userspace will likely move over to the connector-props quite > slowly and > > we can expect various userspace bits, esp. also custom user > scripts, to > > keep using the old uAPI for a long time. Using the 2 APIs are > intermixed > > is fine when using a 1:1 brightness scale mapping. But if we end > up doing > > a scaling round-trip all the time then eventually the brightness > is going > > do drift. This can even happen if the user never changes the > brightness > > when userspace saves it over suspend/resume or reboots. > > - Almost all laptops have brightness up/down hotkeys. E.g GNOME > decides > > on a step size for the hotkeys by doing min(brightness_max/20, 1). > > Some of the vendor specific backlight fw APIs (e.g. dell-laptop) > have > > only 8 steps. When giving userspace the actual max_brightness > value, then > > this will all work just fine. When hardcode brightness_max to > 65535 OTOH > > then in this case GNOME will still give the user 20 steps where > only 1 > > in every 2-3 steps actually changes the brightness which IMHO is > > an unacceptably bad user experience. > > > > 0-65535 advantages / 1:1 disadvantages > > - Without a fixed scale for the brightness property the > brightness_max > > value may change after an userspace application's initial > enumeration > > of the drm_connector. This can happen when neither the native GPU > nor > > the acpi_video backlight devices are present/usable in this case > > acpi_video_get_backlight_type() will _assume_ a vendor specific fw > API > > will be used for backlight control and the driver proving the > "vendor" > > backlight device will show up much later and may even never > show-up, > > so waiting for it is not an option. With a fixed 0-65535 scale > userspace > > can just always assume this and the drm_connector backlight props > helper > > code can even cache writes and send it to the actual backlight > device > > when it shows up. With a 1:1 mapping userspace needs to listen for > > a uevent and then update the brightness range on such an event. > > > > I believe that the 1:1 mapping advantages out way the disadvantages > > here. Also note that current userspace already blindly assumes that > > all relevant drivers are loaded before the graphical-environment > > starts and all the desktop environments as such already only do > > a single scan of /sys/class/backlight when they start. So when > > userspace forgets to add code to listen for the uevent when switching > > to the new API nothing changes; and with the uevent userspace > actually > > gets a good mechanism to detect backlight drivers loading after > > the graphical-environment has already started. > > > > So based on this here is my proposal for a set of new brightness > > properties on the drm_connector object. Note these are all prefixed > with > > bl which stands for backlight, which is technically not correct for > OLED. > > But we need a prefix to avoid a name collision with the "brightness" > > attribute which is part of the existing TV specific properties and > IMHO > > it is good to have a common prefix to make it clear that these all > > belong together. > > > > > > The drm_connector brightness properties > > ======================================= > > > > bl_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 bl_brightness_max. > > > > bl_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 (yet). > > > > bl_brightness_0_is_min_brightness: ro, boolean > > When this is set to true then it is safe to set brightness to 0 > > without worrying that this completely turns the backlight off causing > > the screen to become unreadable. When this is false setting > brightness > > to 0 may turn the backlight off, but this is _not_ guaranteed. > > This will e.g. be true when directly driving a PWM and the video-BIOS > > has provided a minimum (non 0) duty-cycle below which the driver will > > never go. > > > > bl_brightness_control_method: ro, enum, possible values: > > none: The GPU driver expects brightness control to be provided > by another > > driver and that driver has not loaded yet. > > unknown: The underlying control mechanism is unknown. > > pwm: The brightness property directly controls the duty-cycle > of a PWM > > output. > > firmware: The brightness is controlled through firmware calls. > > DDC/CI: The brightness is controlled through the DDC/CI protocol. > > gmux: The brightness is controlled by the GMUX. > > Note this enum may be extended in the future, so other values may > > be read, these should be treated as "unknown". > > > > When brightness control becomes available after being reported > > as not available before (bl_brightness_control_method=="none") > > a uevent with CONNECTOR= and > > PROPERTY= will be generated > > at this point all the properties must be re-read. > > > > When/once brightness control is available then all the read-only > > properties are fixed and will never change. > > > > Besides the "none" value for no driver having loaded yet, > > the different bl_brightness_control_method values are intended for > > (userspace) heuristics for such things as the brightness setting > > linearly controlling electrical power or setting perceived > brightness. > > > > 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/ > < > 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/ > < > 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 < > 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 < > https://cgit.freedesktop.org/~mperes/linux/log/?h=backlight> > > > > --00000000000055972f05e717a774 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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 t= o recompile
their kernels and such. Also stage 2 should probably = take ~2 decades
until it comes into being, for reference fbdev SP= ECIFIC drivers
were removed from fedora just recently and because= of that there
were some issues with some user's systems. I u= nderstand it's much
easier to change from the /sys/class/back= light/ 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 Goed= e <hdegoede@redhat.com> wr= ote:
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/ba= cklight/.
> Currently on the user(user, not userland) level its easier for me to j= ust modify
> the file and be done with it. xbacklight doesnt tell me when its faile= d,
> brightnessctl doesnt make assumptions about what device is what, and > other brightness setting applications ive seen are much worse than the= m.
> 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 "setbrig= htness"
> 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 pre= ssure
> app makers into changing the interface? As a big draw for DDC/CI is th= at
> many displays support it as a way to change brightness(even if you are= nt
> 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 mor= e
focused on getting the new API in place then on deprecating the old one :)<= br>
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 <hdegoede@redhat.com <mailto:hdegoede@redhat.com>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0As discussed already several times in the past:
>=C2=A0 =C2=A0 =C2=A0=C2=A0https://ww= w.x.org/wiki/Events/XDC2014/XDC2014GoedeBacklight/ <https://www.x.org/wiki/Events/XDC2014/XDC2014GoedeBackli= ght/>
>=C2=A0 =C2=A0 =C2=A0=C2=A0https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c= 6@linux.intel.com/ <https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@li= nux.intel.com/>
>
>=C2=A0 =C2=A0 =C2=A0The current userspace API for brightness control of= fered by
>=C2=A0 =C2=A0 =C2=A0/sys/class/backlight devices has various issues, th= e biggest 2 being:
>
>=C2=A0 =C2=A0 =C2=A01. There is no way to map the backlight device to a= specific
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0display-output / panel (1)
>=C2=A0 =C2=A0 =C2=A02. Controlling the brightness requires root-rights = requiring
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0desktop-environments to use suid-root = helpers for this.
>
>=C2=A0 =C2=A0 =C2=A0As already discussed on various conference's ha= llway tracks
>=C2=A0 =C2=A0 =C2=A0and as has been proposed on the dri-devel list once= before (2),
>=C2=A0 =C2=A0 =C2=A0it seems that there is consensus that the best way = to to solve these
>=C2=A0 =C2=A0 =C2=A02 issues is to add support for controlling a video-= output's brightness
>=C2=A0 =C2=A0 =C2=A0through properties on the drm_connector.
>
>=C2=A0 =C2=A0 =C2=A0This RFC outlines my plan to try and actually imple= ment this,
>=C2=A0 =C2=A0 =C2=A0which has 3 phases:
>
>
>=C2=A0 =C2=A0 =C2=A0Phase 1: Stop registering multiple /sys/class/backl= ight devs for a single display
>=C2=A0 =C2=A0 =C2=A0=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
>=C2=A0 =C2=A0 =C2=A0On x86 there can be multiple firmware + direct-hw-a= ccess methods
>=C2=A0 =C2=A0 =C2=A0for controlling the backlight and in some cases the= kernel registers
>=C2=A0 =C2=A0 =C2=A0multiple backlight-devices for a single internal la= ptop LCD panel:
>
>=C2=A0 =C2=A0 =C2=A0a) i915 and nouveau unconditionally register their = "native" backlight dev
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0even on devices where /sys/class/backl= ight/acpi_video0 must be used
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0to control the backlight, relying on u= serspace to prefer the "firmware"
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0acpi_video0 device over "native&q= uot; devices.
>=C2=A0 =C2=A0 =C2=A0b) amdgpu and nouveau rely on the acpi_video driver= initializing before
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0them, which currently causes /sys/clas= s/backlight/acpi_video0 to usually
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0show up and then they register their o= wn native backlight driver after
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0which the drivers/acpi/video_detect.c = code unregisters the acpi_video0
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0device. This means that userspace brie= fly sees 2 devices and the
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0disappearing of acpi_video0 after a br= ief time confuses the systemd
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0backlight level save/restore code, see= e.g.:
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0https://bbs.= archlinux.org/viewtopic.php?id=3D269920 <h= ttps://bbs.archlinux.org/viewtopic.php?id=3D269920>
>
>=C2=A0 =C2=A0 =C2=A0I already have a pretty detailed plan to tackle thi= s, which I will
>=C2=A0 =C2=A0 =C2=A0post in a separate RFC email. I plan to start worki= ng on this right
>=C2=A0 =C2=A0 =C2=A0away, as it will be good to have this fixed regardl= ess.
>
>
>=C2=A0 =C2=A0 =C2=A0Phase 2: Add drm_connector properties mirroring the= matching backlight device
>=C2=A0 =C2=A0 =C2=A0=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
>=C2=A0 =C2=A0 =C2=A0The plan is to add a drm_connector helper function,= which optionally takes
>=C2=A0 =C2=A0 =C2=A0a pointer to the backlight device for the GPU's= native backlight device,
>=C2=A0 =C2=A0 =C2=A0which will then mirror the backlight settings from = the backlight device
>=C2=A0 =C2=A0 =C2=A0in a set of read/write brightness* properties on th= e connector.
>
>=C2=A0 =C2=A0 =C2=A0This function can then be called by GPU drivers for= the drm_connector for
>=C2=A0 =C2=A0 =C2=A0the internal panel and it will then take care of ev= erything. When there
>=C2=A0 =C2=A0 =C2=A0is no native GPU backlight device, or when it shoul= d not be used then
>=C2=A0 =C2=A0 =C2=A0(on x86) the helper will use the acpi_video_get_bac= klight_type() to
>=C2=A0 =C2=A0 =C2=A0determine which backlight-device should be used ins= tead and it will find
>=C2=A0 =C2=A0 =C2=A0+ mirror that one.
>
>
>=C2=A0 =C2=A0 =C2=A0Phase 3: Deprecate /sys/class/backlight uAPI
>=C2=A0 =C2=A0 =C2=A0=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D
>
>=C2=A0 =C2=A0 =C2=A0Once most userspace has moved over to using the new= drm_connector
>=C2=A0 =C2=A0 =C2=A0brightness props, a Kconfig option can be added to = stop exporting
>=C2=A0 =C2=A0 =C2=A0the backlight-devices under /sys/class/backlight. T= he plan is to
>=C2=A0 =C2=A0 =C2=A0just disable the sysfs interface and keep the exist= ing backlight-device
>=C2=A0 =C2=A0 =C2=A0internal kernel abstraction as is, since some abstr= action for (non GPU
>=C2=A0 =C2=A0 =C2=A0native) backlight devices will be necessary regardl= ess.
>
>=C2=A0 =C2=A0 =C2=A0An alternative to disabling the sysfs class entirel= y, would be
>=C2=A0 =C2=A0 =C2=A0to allow setting it to read-only through Kconfig. >
>
>=C2=A0 =C2=A0 =C2=A0What scale to use for the drm_connector bl_brightne= ss property?
>=C2=A0 =C2=A0 =C2=A0=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
>=C2=A0 =C2=A0 =C2=A0The tricky part of this plan is phase 2 and then es= p. defining what the
>=C2=A0 =C2=A0 =C2=A0new brightness properties will look like and how th= ey will work.
>
>=C2=A0 =C2=A0 =C2=A0The biggest challenge here is to decide on a fixed = scale for the main
>=C2=A0 =C2=A0 =C2=A0brightness property, say 0-65535, using scaling whe= re the actual hw scale
>=C2=A0 =C2=A0 =C2=A0is different, or if this should simply be a 1:1 mir= ror of the current
>=C2=A0 =C2=A0 =C2=A0backlight interface, with the actual hw scale / bri= ghtness_max value
>=C2=A0 =C2=A0 =C2=A0exposed as a drm_connector property.
>
>=C2=A0 =C2=A0 =C2=A01:1 advantages / 0-65535 disadvantages
>=C2=A0 =C2=A0 =C2=A0- Userspace will likely move over to the connector-= props quite slowly and
>=C2=A0 =C2=A0 =C2=A0=C2=A0 we can expect various userspace bits, esp. a= lso custom user scripts, to
>=C2=A0 =C2=A0 =C2=A0=C2=A0 keep using the old uAPI for a long time. Usi= ng the 2 APIs are intermixed
>=C2=A0 =C2=A0 =C2=A0=C2=A0 is fine when using a 1:1 brightness scale ma= pping. But if we end up doing
>=C2=A0 =C2=A0 =C2=A0=C2=A0 a scaling round-trip all the time then event= ually the brightness is going
>=C2=A0 =C2=A0 =C2=A0=C2=A0 do drift. This can even happen if the user n= ever changes the brightness
>=C2=A0 =C2=A0 =C2=A0=C2=A0 when userspace saves it over suspend/resume = or reboots.
>=C2=A0 =C2=A0 =C2=A0- Almost all laptops have brightness up/down hotkey= s. E.g GNOME decides
>=C2=A0 =C2=A0 =C2=A0=C2=A0 on a step size for the hotkeys by doing min(= brightness_max/20, 1).
>=C2=A0 =C2=A0 =C2=A0=C2=A0 Some of the vendor specific backlight fw API= s (e.g. dell-laptop) have
>=C2=A0 =C2=A0 =C2=A0=C2=A0 only 8 steps. When giving userspace the actu= al max_brightness value, then
>=C2=A0 =C2=A0 =C2=A0=C2=A0 this will all work just fine. When hardcode = brightness_max to 65535 OTOH
>=C2=A0 =C2=A0 =C2=A0=C2=A0 then in this case GNOME will still give the = user 20 steps where only 1
>=C2=A0 =C2=A0 =C2=A0=C2=A0 in every 2-3 steps actually changes the brig= htness which IMHO is
>=C2=A0 =C2=A0 =C2=A0=C2=A0 an unacceptably bad user experience.
>
>=C2=A0 =C2=A0 =C2=A00-65535 advantages / 1:1 disadvantages
>=C2=A0 =C2=A0 =C2=A0- Without a fixed scale for the brightness property= the brightness_max
>=C2=A0 =C2=A0 =C2=A0=C2=A0 value may change after an userspace applicat= ion's initial enumeration
>=C2=A0 =C2=A0 =C2=A0=C2=A0 of the drm_connector. This can happen when n= either the native GPU nor
>=C2=A0 =C2=A0 =C2=A0=C2=A0 the acpi_video backlight devices are present= /usable in this case
>=C2=A0 =C2=A0 =C2=A0=C2=A0 acpi_video_get_backlight_type() will _assume= _ a vendor specific fw API
>=C2=A0 =C2=A0 =C2=A0=C2=A0 will be used for backlight control and the d= river proving the "vendor"
>=C2=A0 =C2=A0 =C2=A0=C2=A0 backlight device will show up much later and= may even never show-up,
>=C2=A0 =C2=A0 =C2=A0=C2=A0 so waiting for it is not an option. With a f= ixed 0-65535 scale userspace
>=C2=A0 =C2=A0 =C2=A0=C2=A0 can just always assume this and the drm_conn= ector backlight props helper
>=C2=A0 =C2=A0 =C2=A0=C2=A0 code can even cache writes and send it to th= e actual backlight device
>=C2=A0 =C2=A0 =C2=A0=C2=A0 when it shows up. With a 1:1 mapping userspa= ce needs to listen for
>=C2=A0 =C2=A0 =C2=A0=C2=A0 a uevent and then update the brightness rang= e on such an event.
>
>=C2=A0 =C2=A0 =C2=A0I believe that the 1:1 mapping advantages out way t= he disadvantages
>=C2=A0 =C2=A0 =C2=A0here. Also note that current userspace already blin= dly assumes that
>=C2=A0 =C2=A0 =C2=A0all relevant drivers are loaded before the graphica= l-environment
>=C2=A0 =C2=A0 =C2=A0starts and all the desktop environments as such alr= eady only do
>=C2=A0 =C2=A0 =C2=A0a single scan of /sys/class/backlight when they sta= rt. So when
>=C2=A0 =C2=A0 =C2=A0userspace forgets to add code to listen for the uev= ent when switching
>=C2=A0 =C2=A0 =C2=A0to the new API nothing changes; and with the uevent= userspace actually
>=C2=A0 =C2=A0 =C2=A0gets a good mechanism to detect backlight drivers l= oading after
>=C2=A0 =C2=A0 =C2=A0the graphical-environment has already started.
>
>=C2=A0 =C2=A0 =C2=A0So based on this here is my proposal for a set of n= ew brightness
>=C2=A0 =C2=A0 =C2=A0properties on the drm_connector object. Note these = are all prefixed with
>=C2=A0 =C2=A0 =C2=A0bl which stands for backlight, which is technically= not correct for OLED.
>=C2=A0 =C2=A0 =C2=A0But we need a prefix to avoid a name collision with= the "brightness"
>=C2=A0 =C2=A0 =C2=A0attribute which is part of the existing TV specific= properties and IMHO
>=C2=A0 =C2=A0 =C2=A0it is good to have a common prefix to make it clear= that these all
>=C2=A0 =C2=A0 =C2=A0belong together.
>
>
>=C2=A0 =C2=A0 =C2=A0The drm_connector brightness properties
>=C2=A0 =C2=A0 =C2=A0=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
>=C2=A0 =C2=A0 =C2=A0bl_brightness: rw 0-int32_max property controlling = the brightness setting
>=C2=A0 =C2=A0 =C2=A0of the connected display. The actual maximum of thi= s will be less then
>=C2=A0 =C2=A0 =C2=A0int32_max and is given in bl_brightness_max.
>
>=C2=A0 =C2=A0 =C2=A0bl_brightness_max: ro 0-int32_max property giving t= he actual maximum
>=C2=A0 =C2=A0 =C2=A0of the display's brightness setting. This will = report 0 when brightness
>=C2=A0 =C2=A0 =C2=A0control is not available (yet).
>
>=C2=A0 =C2=A0 =C2=A0bl_brightness_0_is_min_brightness: ro, boolean
>=C2=A0 =C2=A0 =C2=A0When this is set to true then it is safe to set bri= ghtness to 0
>=C2=A0 =C2=A0 =C2=A0without worrying that this completely turns the bac= klight off causing
>=C2=A0 =C2=A0 =C2=A0the screen to become unreadable. When this is false= setting brightness
>=C2=A0 =C2=A0 =C2=A0to 0 may turn the backlight off, but this is _not_ = guaranteed.
>=C2=A0 =C2=A0 =C2=A0This will e.g. be true when directly driving a PWM = and the video-BIOS
>=C2=A0 =C2=A0 =C2=A0has provided a minimum (non 0) duty-cycle below whi= ch the driver will
>=C2=A0 =C2=A0 =C2=A0never go.
>
>=C2=A0 =C2=A0 =C2=A0bl_brightness_control_method: ro, enum, possible va= lues:
>=C2=A0 =C2=A0 =C2=A0none:=C2=A0 =C2=A0 =C2=A0The GPU driver expects bri= ghtness control to be provided by another
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 driver and that = driver has not loaded yet.
>=C2=A0 =C2=A0 =C2=A0unknown:=C2=A0 The underlying control mechanism is = unknown.
>=C2=A0 =C2=A0 =C2=A0pwm:=C2=A0 =C2=A0 =C2=A0 The brightness property di= rectly controls the duty-cycle of a PWM
>=C2=A0 =C2=A0 =C2=A0=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 output.
>=C2=A0 =C2=A0 =C2=A0firmware: The brightness is controlled through firm= ware calls.
>=C2=A0 =C2=A0 =C2=A0DDC/CI:=C2=A0 =C2=A0The brightness is controlled th= rough the DDC/CI protocol.
>=C2=A0 =C2=A0 =C2=A0gmux:=C2=A0 =C2=A0 =C2=A0The brightness is controll= ed by the GMUX.
>=C2=A0 =C2=A0 =C2=A0Note this enum may be extended in the future, so ot= her values may
>=C2=A0 =C2=A0 =C2=A0be read, these should be treated as "unknown&q= uot;.
>
>=C2=A0 =C2=A0 =C2=A0When brightness control becomes available after bei= ng reported
>=C2=A0 =C2=A0 =C2=A0as not available before (bl_brightness_control_meth= od=3D=3D"none")
>=C2=A0 =C2=A0 =C2=A0a uevent with CONNECTOR=3D<connector-id> and<= br> >=C2=A0 =C2=A0 =C2=A0PROPERTY=3D<bl_brightness_control_method-id> = will be generated
>=C2=A0 =C2=A0 =C2=A0at this point all the properties must be re-read. >
>=C2=A0 =C2=A0 =C2=A0When/once brightness control is available then all = the read-only
>=C2=A0 =C2=A0 =C2=A0properties are fixed and will never change.
>
>=C2=A0 =C2=A0 =C2=A0Besides the "none" value for no driver ha= ving loaded yet,
>=C2=A0 =C2=A0 =C2=A0the different bl_brightness_control_method values a= re intended for
>=C2=A0 =C2=A0 =C2=A0(userspace) heuristics for such things as the brigh= tness setting
>=C2=A0 =C2=A0 =C2=A0linearly controlling electrical power or setting pe= rceived brightness.
>
>=C2=A0 =C2=A0 =C2=A0Regards,
>
>=C2=A0 =C2=A0 =C2=A0Hans
>
>
>=C2=A0 =C2=A0 =C2=A01) The need to be able to map the backlight device = to a specific display
>=C2=A0 =C2=A0 =C2=A0has become clear once more with the recent proposal= to add DDCDI support:
>=C2=A0 =C2=A0 =C2=A0h= ttps://lore.kernel.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/<= /a> <https://lore.kern= el.org/lkml/20220403230850.2986-1-yusisamerican@gmail.com/>
>
>=C2=A0 =C2=A0 =C2=A02) https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@li= nux.intel.com/ <https://lore.kernel.org/all/4b17ba08-39f3-57dd-5aad-d37d844b02c6@linux.= intel.com/>
>=C2=A0 =C2=A0 =C2=A0Note this proposal included a method for userspace = to be able to tell the
>=C2=A0 =C2=A0 =C2=A0kernel if the native/acpi_video/vendor backlight de= vice should be used,
>=C2=A0 =C2=A0 =C2=A0but this has been solved in the kernel for years no= w:
>=C2=A0 =C2=A0 =C2=A0=C2=A0https://www.spin= ics.net/lists/linux-acpi/msg50526.html <https://www.spinics.net/lists/linux-acpi/msg50526.html>
>=C2=A0 =C2=A0 =C2=A0An initial implementation of this proposal is avail= able here:
>=C2=A0 =C2=A0 =C2=A0=C2=A0https://c= git.freedesktop.org/~mperes/linux/log/?h=3Dbacklight <https://cgit.freedesktop.org/~mperes/linux/log/?h=3Db= acklight>
>

--00000000000055972f05e717a774--