Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4be2cc57-59b9-24e2-fd10-f2af175ff518@redhat.com>
Date:   Sat, 6 May 2023 13:52:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS
 screenpad
Content-Language: en-US, nl
To:     "Luke D. Jones" <luke@ljones.dev>,
        platform-driver-x86@vger.kernel.org,
        =?UTF-8?Q?Barnab=c3=a1s_P=c5=91cze?= <pobrn@protonmail.com>,
        =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, acpi4asus-user@lists.sourceforge.net,
        corentin.chary@gmail.com, markgross@kernel.org, jdelvare@suse.com,
        linux@roeck-us.net
References: <20230505043013.2622603-1-luke@ljones.dev>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230505043013.2622603-1-luke@ljones.dev>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@redhat.com/

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 ?