Hi, On 5/6/23 13:52, Hans de Goede wrote: > Hi Luke, > > On 5/5/23 06:30, Luke D. Jones wrote: >> Adds support for the screenpad(-plus) found on a few ASUS laptops that have a main 16:9 or 16:10 screen and a shorter screen below the main but above the keyboard. >> The support consists of: >> - On off control >> - Setting brightness from 0-255 >> >> There are some small quirks with this device when considering only the raw WMI methods: >> 1. The Off method can only switch the device off >> 2. Changing the brightness turns the device back on >> 3. To turn the device back on the brightness must be > 1 >> 4. When the device is off the brightness can't be changed (so it is stored by the driver if device is off). >> 5. Booting with a value of 0 brightness (retained by bios) means the bios will set a value of > 0, < 15 which is far too dim and was unexpected by testers. The compromise was to set the brightness to 60 which is a usable brightness if the module init brightness was under 15. >> 6. When the device is off it is "unplugged" >> >> All of the above points are addressed within the patch to create a good user experience and keep within user expectations. >> >> Changelog: >> - V2 >> - Complete refactor to use as a backlight device > > Thank you on your work for this. > > Unfortunately I did not get a chance to react to the v1 posting and > the remarks to switch to using /sys/class/backlight there before you > posted this v2. > > Technically the remark to use /sys/class/backlight for this is > completely correct. But due to the way how userspace uses > /sys/class/backlight this is a problematic. > > Userspace basically always assumes there is only 1 LCD panel > and it then looks at /sys/class/backlight and picks 1 > /sys/class/backlight entry and uses that for the brightness > slider in the desktop-environment UI / system-menu as well > as to handle brightness up/down keyboard hotkey presses. > > In the (recent) past the kernel used to register e.g. > both /sys/class/backlight/acpi_video0 and > /sys/class/backlight/intel_backlight > > For ACPI resp. direct hw control of the LCD panel backlight > (so both control the same backlight, sometimes both work > sometimes only 1 works). > > Userspace uses the backlight-type to determine which backlight > class to use, using (for GNOME, but I believe everywhere) the > following preference order: > > 1. First look for "firmware" type backlight devices (like acpi_video0) > 2. Then try "platform" type backlight devices > 3. Last try "raw" type backlight devices > > And to make things work the kernel has been hiding the "acpi_video0" > entry in cases where it is known that we need the "raw" aka native > type backlight. > > Luke you seem to already be partly aware of this, because the patch > now has this: > > props.type = BACKLIGHT_RAW; /* ensure this bd is last to be picked */ > > but almost all modern laptops exclusively use the raw/native type > for backlight control of the main LCD panel. > > So now we end up with 2 "raw" type backlight devices and if > e.g. gnome-settings-daemon picks the right one now sort of > is left to luck. > > Well that is not entirely true, at least gnome-settings-daemon > prefers raw backlight devices where the parent has an "enabled" > sysfs attribute (it expects the parent to be a drm_connector > object) and where that enabled attribute reads as "enabled". > > This is done for hybrid-gfx laptops where there already may > be 2 raw backlight-class devices, 1 for each GPU but only > 1 of the 2 drm_connectors going to the main LCD panel should > actually show as enabled. > > So typing all this out I guess we could go ahead with using > the backlight class for this after all, but this relies > on userspace preferring raw backlight-class devices > with a drm_connector-object parent which show as being > enabled. > > Any userspace code which does not do the parent has > an enabled attr reading "enabled" or a similar check > will end up picking a random backlight class device > as control for the main panel brightness which will not > always end well. So this all is a bit fragile ... > > And I'm not sure what is the best thing to do here. > > Barnabás, Ilpo, Guenter, any comments on this ? Hmm, no comments from anyone on the potential problems of using /sys/class/backlight for this causing potential userspace confusion since normally /sys/class/backlight devices control the main LCD brightness ? Luke do you have any thoughts on this yourself ? And can you answer this question please ? : > Luke, question how does the second/exta panel look > from an outputting video to it pov ? Does it show > up as an extra screen connected to a drm_connector > on one of the GPUs. IOW can it be used with standard > kernel-modesetting APIs ? Regards, Hans
