Message-Id: <FD1QUR.SO09CWU6HM4Q1@ljones.dev>
Date:   Tue, 16 May 2023 10:34:27 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     platform-driver-x86@...r.kernel.org,
        Barnabás Pőcze <pobrn@...tonmail.com>,
        Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, acpi4asus-user@...ts.sourceforge.net,
        corentin.chary@...il.com, markgross@...nel.org, jdelvare@...e.com,
        linux@...ck-us.net
Subject: Re: [PATCH v2 0/1] platform/x86: asus-wmi: add support for ASUS
 screenpad



On Mon, May 15 2023 at 14:39:10 +0200, Hans de Goede 
<hdegoede@...hat.com> wrote:
> 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.
>> 

IMO, desktops need to adjust this expectation and start offering 
controls for all possible screens. This would open up the possibility 
of setting brightness of modern external screens also. And then they 
should use the "Primary display" brightness controls, or at least offer 
an option to set which is controlled.

>> 
>>  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.
>> 

In a test KDE at least picked the right one.

>> 
>>  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".
>> 

Ah I see. Parent for screenpad is "platform", while the main is on igpu 
with parent "pci". I will paste some udev info at the end of this reply.

>> 
>>  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 ?
> 

Hi Hans, sorry about delay in response, just been tied up with work.

As I don't actually have this kind of laptop I can't easily get info, 
but I can ask a few people in my discord for information. Is there 
anything in particular you would need to know? From my basic 
understanding some of the points are:

1. It does show as an actual additional screen
2. Internal wiring is unclear, when dispaly MUX is switched to dgpu the 
screen is still detected but not drawn to
3. Point 2 is actually more uncertain as it seems only wayland had this 
issue? I will get more info.

So I think now is probably a good time to raise a particular issue I've 
encountered with the last two years: the display MUX.

As I understand it now, there are two types of new MUX - the manual 
switch, and the newer "Advanced Optimus" automatic switch. The issues I 
have are with the manual switch since I've not encountered the advanced 
optimus yet.

When the switch is. uh. switched. the dgpu drives the internal display, 
and I expect that since the display is now detected through the dgpu, 
this is why the dgpu is kept awake to drive it. But, the igpu is also 
still active, and because of this the initial boot from grub to 
display-manager is a black screen including tty. This means anyone with 
an encrypted drive will never see the prompt and they believe they have 
a failed boot. I don't know what to do about this?

What I would love is somehow to either disable the igpu in kernel if 
the MUX is toggled, or to change which device is the primary. Do you 
have any thoughts on where I should start on this?

An additional problem: `boot_vga` property of display adaptors. I've 
been using this as a first-stage check in supergfxctl to determine if 
there was a switch, but it is never ever reliable - sometimes it 
changes, sometimes it is entirely blank (using udev to fetch 
properties). And then I need to use a combination of checks to 
determine state. So this `boot_vga` seems to always be available but is 
practically unusable.



UDEV info dumps:

Device {
    initialized: true,
    device_major_minor_number: None,
    system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0/backlight/amdgpu_bl1",
    device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/backlight/amdgpu_bl1",
    device_node: None,
    subsystem_name: Some(
        "backlight",
    ),
    system_name: "amdgpu_bl1",
    instance_number: Some(
        1,
    ),
    device_type: None,
    driver: None,
    action: None,
    parent: Some(
        Device {
            initialized: true,
            device_major_minor_number: None,
            system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
            device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
            device_node: None,
            subsystem_name: Some(
                "pci",
            ),
            system_name: "0000:09:00.0",
            instance_number: Some(
                0,
            ),
            device_type: None,
            driver: Some(
                "amdgpu",
            ),
            action: None,
            parent: Some(
                Device {
                    initialized: true,
                    device_major_minor_number: None,
                    system_path: "/sys/devices/pci0000:00/0000:00:08.1",
                    device_path: "/devices/pci0000:00/0000:00:08.1",
                    device_node: None,
                    subsystem_name: Some(
                        "pci",
                    ),
                    system_name: "0000:00:08.1",
                    instance_number: Some(
                        1,
                    ),
                    device_type: None,
                    driver: Some(
                        "pcieport",
                    ),
                    action: None,
                    parent: Some(
                        Device {
                            initialized: false,
                            device_major_minor_number: None,
                            system_path: "/sys/devices/pci0000:00",
                            device_path: "/devices/pci0000:00",
                            device_node: None,
                            subsystem_name: None,
                            system_name: "pci0000:00",
                            instance_number: Some(
                                0,
                            ),
                            device_type: None,
                            driver: None,
                            action: None,
                            parent: None,
                        },
                    ),
                },
            ),
        },
    ),
}
  [properties]
    - "CURRENT_TAGS" ":systemd:seat:"
    - "DEVPATH" 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/backlight/amdgpu_bl1"
    - "DMI_FAMILY" "ROG Zephyrus Duo 16"
    - "DMI_VENDOR" "ASUSTeK COMPUTER INC."
    - "ID_FOR_SEAT" "backlight-pci-0000_09_00_0"
    - "ID_PATH" "pci-0000:09:00.0"
    - "ID_PATH_TAG" "pci-0000_09_00_0"
    - "NVME_HOST_IFACE" "none"
    - "SUBSYSTEM" "backlight"
    - "SYSTEMD_WANTS" "systemd-backlight@...klight:amdgpu_bl1.service"
    - "TAGS" ":systemd:seat:"
    - "USEC_INITIALIZED" "6189554"
  [attributes]
    - "actual_brightness" ""
    - "bl_power" ""
    - "brightness" ""
    - "device" ""
    - "max_brightness" ""
    - "power/autosuspend_delay_ms" ""
    - "power/control" ""
    - "power/runtime_active_time" ""
    - "power/runtime_status" ""
    - "power/runtime_suspended_time" ""
    - "scale" ""
    - "subsystem" ""
    - "type" ""
    - "uevent" ""

Device {
    initialized: true,
    device_major_minor_number: None,
    system_path: 
"/sys/devices/platform/asus-nb-wmi/backlight/asus_screenpad",
    device_path: 
"/devices/platform/asus-nb-wmi/backlight/asus_screenpad",
    device_node: None,
    subsystem_name: Some(
        "backlight",
    ),
    system_name: "asus_screenpad",
    instance_number: None,
    device_type: None,
    driver: None,
    action: None,
    parent: Some(
        Device {
            initialized: true,
            device_major_minor_number: None,
            system_path: "/sys/devices/platform/asus-nb-wmi",
            device_path: "/devices/platform/asus-nb-wmi",
            device_node: None,
            subsystem_name: Some(
                "platform",
            ),
            system_name: "asus-nb-wmi",
            instance_number: None,
            device_type: None,
            driver: Some(
                "asus-nb-wmi",
            ),
            action: None,
            parent: Some(
                Device {
                    initialized: false,
                    device_major_minor_number: None,
                    system_path: "/sys/devices/platform",
                    device_path: "/devices/platform",
                    device_node: None,
                    subsystem_name: None,
                    system_name: "platform",
                    instance_number: None,
                    device_type: None,
                    driver: None,
                    action: None,
                    parent: None,
                },
            ),
        },
    ),
}
  [properties]
    - "CURRENT_TAGS" ":systemd:seat:"
    - "DEVPATH" "/devices/platform/asus-nb-wmi/backlight/asus_screenpad"
    - "DMI_FAMILY" "ROG Zephyrus Duo 16"
    - "DMI_VENDOR" "ASUSTeK COMPUTER INC."
    - "ID_FOR_SEAT" "backlight-platform-asus-nb-wmi"
    - "ID_PATH" "platform-asus-nb-wmi"
    - "ID_PATH_TAG" "platform-asus-nb-wmi"
    - "NVME_HOST_IFACE" "none"
    - "SUBSYSTEM" "backlight"
    - "SYSTEMD_WANTS" 
"systemd-backlight@...klight:asus_screenpad.service"
    - "TAGS" ":systemd:seat:"
    - "USEC_INITIALIZED" "6323833"
  [attributes]
    - "actual_brightness" ""
    - "bl_power" ""
    - "brightness" ""
    - "device" ""
    - "max_brightness" ""
    - "power/autosuspend_delay_ms" ""
    - "power/control" ""
    - "power/runtime_active_time" ""
    - "power/runtime_status" ""
    - "power/runtime_suspended_time" ""
    - "scale" ""
    - "subsystem" ""
    - "type" ""
    - "uevent" ""


Device {
    initialized: true,
    device_major_minor_number: None,
    system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1/card1-DP-1",
    device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1/card1-DP-1",
    device_node: None,
    subsystem_name: Some(
        "drm",
    ),
    system_name: "card1-DP-1",
    instance_number: Some(
        1,
    ),
    device_type: Some(
        "drm_connector",
    ),
    driver: None,
    action: None,
    parent: Some(
        Device {
            initialized: true,
            device_major_minor_number: Some(
                57857,
            ),
            system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1",
            device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1",
            device_node: Some(
                "/dev/dri/card1",
            ),
            subsystem_name: Some(
                "drm",
            ),
            system_name: "card1",
            instance_number: Some(
                1,
            ),
            device_type: Some(
                "drm_minor",
            ),
            driver: None,
            action: None,
            parent: Some(
                Device {
                    initialized: true,
                    device_major_minor_number: None,
                    system_path: 
"/sys/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
                    device_path: 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0",
                    device_node: None,
                    subsystem_name: Some(
                        "pci",
                    ),
                    system_name: "0000:09:00.0",
                    instance_number: Some(
                        0,
                    ),
                    device_type: None,
                    driver: Some(
                        "amdgpu",
                    ),
                    action: None,
                    parent: Some(
                        Device {
                            initialized: true,
                            device_major_minor_number: None,
                            system_path: 
"/sys/devices/pci0000:00/0000:00:08.1",
                            device_path: 
"/devices/pci0000:00/0000:00:08.1",
                            device_node: None,
                            subsystem_name: Some(
                                "pci",
                            ),
                            system_name: "0000:00:08.1",
                            instance_number: Some(
                                1,
                            ),
                            device_type: None,
                            driver: Some(
                                "pcieport",
                            ),
                            action: None,
                            parent: Some(
                                Device {
                                    initialized: false,
                                    device_major_minor_number: None,
                                    system_path: 
"/sys/devices/pci0000:00",
                                    device_path: "/devices/pci0000:00",
                                    device_node: None,
                                    subsystem_name: None,
                                    system_name: "pci0000:00",
                                    instance_number: Some(
                                        0,
                                    ),
                                    device_type: None,
                                    driver: None,
                                    action: None,
                                    parent: None,
                                },
                            ),
                        },
                    ),
                },
            ),
        },
    ),
}
  [properties]
    - "CURRENT_TAGS" ":master-of-seat:seat:"
    - "DEVPATH" 
"/devices/pci0000:00/0000:00:08.1/0000:09:00.0/drm/card1/card1-DP-1"
    - "DEVTYPE" "drm_connector"
    - "DMI_FAMILY" "ROG Zephyrus Duo 16"
    - "DMI_VENDOR" "ASUSTeK COMPUTER INC."
    - "ID_FOR_SEAT" "drm-pci-0000_09_00_0"
    - "ID_PATH" "pci-0000:09:00.0"
    - "ID_PATH_TAG" "pci-0000_09_00_0"
    - "SUBSYSTEM" "drm"
    - "TAGS" ":master-of-seat:seat:"
    - "USEC_INITIALIZED" "6196005"
  [attributes]
    - "ddc" ""
    - "device" ""
    - "dpms" ""
    - "edid" ""
    - "enabled" ""
    - "modes" ""
    - "power/autosuspend_delay_ms" ""
    - "power/control" ""
    - "power/runtime_active_time" ""
    - "power/runtime_status" ""
    - "power/runtime_suspended_time" ""
    - "status" ""
    - "subsystem" ""
    - "uevent" ""
> 

Powered by blists - more mailing lists