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 ? Regards, Hans p.s. Note I'm working on allowing brightness control for multiple screens in a sane way, see: https://lore.kernel.org/dri-devel/b61d3eeb-6213-afac-2e70-7b9791c86d2e@xxxxxxxxxx/ The last few kernel-cycles I have landed a refactor/cleanup of the existing backlight code so that we only ever register 1 /sys/class/backlight entry for the main LCD panel, instead of having e.g. both acpi_video0 + intel_backlight and relying on userspace preferring acpi_video0 in that case. And when I can find time for it I plan to implement the API in the linked RFC, which allows properly dealing with all this. 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 ?
