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: <02f28fe4-bca4-f9d7-a9be-0f1999662d62@redhat.com>
Date:   Mon, 15 May 2023 14:39: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
From:   Hans de Goede <hdegoede@redhat.com>
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>
 <4be2cc57-59b9-24e2-fd10-f2af175ff518@redhat.com>
In-Reply-To: <4be2cc57-59b9-24e2-fd10-f2af175ff518@redhat.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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