Re: [PATCH] WMI: asus: Reduce G14 and G15 match to min product name

[Luke Jones <luke@ljones.dev>]

First report:

G14 / 2020 - integrated graphics only (NVIDIA disabled)

# cat /proc/cmdline
BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.10.19-200.fc33.x86_64 
root=UUID=c3fe67fb-a740-440b-bc2d-e7cfc9f19a42 ro rootflags=subvol=@ 
rd.luks.uuid=luks-cf5c7a8a-d5da-45ae-b556-db9c78b359e5 rhgb quiet 
rd.driver.blacklist=nouveau modprobe.blacklist=nouveau 
nvidia-drm.modeset=1 acpi_backlight=video

1. # ls /sys/class/backlight
acpi_video0 amdgpu_bl0

2. acpi_video0 max_brightness 49, brightness is changing
amdgpu_bl0 max_brightness 255, brightness is changing
Both equal bright at max value

# cat /proc/cmdline
BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.10.19-200.fc33.x86_64 
root=UUID=c3fe67fb-a740-440b-bc2d-e7cfc9f19a42 ro rootflags=subvol=@ 
rd.luks.uuid=luks-cf5c7a8a-d5da-45ae-b556-db9c78b359e5 rhgb quiet 
rd.driver.blacklist=nouveau modprobe.blacklist=nouveau 
nvidia-drm.modeset=1 acpi_backlight=native

3. # ls /sys/class/backlight
amdgpu_bl0

4. amdgpu_bl0 max_brightness 255, brightness is changing

# cat /proc/cmdline
BOOT_IMAGE=(hd0,gpt2)/vmlinuz-5.10.19-200.fc33.x86_64 
root=UUID=c3fe67fb-a740-440b-bc2d-e7cfc9f19a42 ro rootflags=subvol=@ 
rd.luks.uuid=luks-cf5c7a8a-d5da-45ae-b556-db9c78b359e5 rhgb quiet 
rd.driver.blacklist=nouveau modprobe.blacklist=nouveau 
nvidia-drm.modeset=1 acpi_backlight=vendor

5. # ls /sys/class/backlight
amdgpu_bl0

6. amdgpu_bl0 max_brightness 255, brightness is changing

5.10 kernel includes the quirk patches. Were you wanting data
with that patch removed completely? (not just the submitted one)

 From what you've said, I think I expected acpi_backlight=video to
work regardless of the patch being applied.

Regards,
Luke.

On Fri, Mar 5, 2021 at 10:36, Hans de Goede <hdegoede@redhat.com> wrote:
> <note adding the pdx86 list back to the Cc>
> 
> Hi Luke,
> 
> On 3/5/21 10:21 AM, Luke Jones wrote:
>>  Hello Hans,
>> 
>>  Many thanks for taking the time to review this and provide some
>>  knowledge.
>> 
>>  Most of what has come about in this patch is through direct user
>>  with a fairly large community I maintain on Discord centered
>>  around a utility I wrote to make ROG laptop users lives a bit
>>  better - https://gitlab.com/asus-linux. The community has been
>>  beneficial to getting some realtime testing done, shortening
>>  time.
>> 
>>  In the case of the 2021 Ryzen laptops I'm pleased you asked
>>  questions. I asked the single G15 laptop owner we have to
>>  check functionality and yes, we don't need that code.
>>  It appears there were some crossed wires in conversations.
>> 
>>  I will amend the patch keeping the glob style got 2020 G14
>>  and G15 as we do have concrete anecdotal evidence that all
>>  *those* models, and some un-added ones need the quirk.
> 
> If possible I would like to see some more research done on
> those 2020 models too.
> 
> Specifically it would be good to get the following info:
> 
> 1. Output of "ls /sys/class/backlight" with "acpi_backlight=video"
> on the kernel commandline.
> 
> 2. Testing of *all* the present backlight devices with 
> "acpi_backlight=video"
> on the kernel commandline. The user can test this by doing:
> 
> cd /sys/class/backlight/$backlight-name
> cat max_brightness
> echo $value-between-0-and-max_brightness > brightness
> echo $another-value-between-0-and-max_brightness > brightness
> etc.
> 
> And then see if the brightness of the screen actually changes.
> 
> 3. Output of "ls /sys/class/backlight" with "acpi_backlight=native"
> on the kernel commandline.
> 
> 4. Testing of *all* the present backlight devices with 
> "acpi_backlight=native"
> on the kernel commandline (see 2.)
> 
> 5. Output of "ls /sys/class/backlight" with "acpi_backlight=vendor"
> on the kernel commandline.
> 
> 6. Testing of *all* the present backlight devices with 
> "acpi_backlight=vendor"
> on the kernel commandline (see 2.)
> 
> 
> My hope/expectation is that acpi_backlight=video will also work,
> because as said using acpi_backlight=vendor is somewhat weird for
> 2020 / 2021 laptop models.  Also typically the acpi-video backlight
> interface will have a larger max_brightness giving finer grained
> (more steps) brightness control.
> 
> Regards,
> 
> Hans
> 
> 
> 
>>  On Thu, Mar 4, 2021 at 15:35, Hans de Goede <hdegoede@redhat.com> 
>> wrote:
>>>  Hi,
>>> 
>>>  On 2/27/21 11:20 AM, Luke D Jones wrote:
>>>>   This patch reduces the product match for GA401 series laptops to
>>>>   the minimum string required.
>>>> 
>>>>   The GA401 series of laptops has a lengthy list of product
>>>>   variations in the 2020 series and the 2021 series refresh
>>>>   is using the same base product ID of GA401.
>>>> 
>>>>   The same is also true for the GA502 series, and the new GA503
>>>>   series which is added in this patch as a variant of the G15.
>>> 
>>>  Thank you for your patch.
>>> 
>>>  I msut say that I find it very strange that 2021 series laptops 
>>> need
>>>  to use the Asus vendor specific WMI interface for backlight 
>>> control.
>>> 
>>>  I see that the GA401 GA502 and GA503 models are all models with
>>>  AMD 4000/5000 series CPUs + Nvidia 2060 series GPUs.
>>>  So I guess it may be possible that this is the right thing
>>>  to do, and I do realize that we are already doing this for the
>>>  listed models. But it seems weird.
>>> 
>>>  Modern laptops almost always use the native backlight control
>>>  build into the drm/kms driver. And in some special cases
>>>  (hybrid GPU setups) they might use the good old ACPI-video
>>>  interface. But using vendor specific interfaces sounds very
>>>  wrong to me. That is something which was typically done on
>>>  pre Windows XP era hardware.
>>> 
>>>  Have you tried passing acpi_backlight=video on the kernel 
>>> commandline?
>>> 
>>>  What is the output of ls /sys/class/backlight before and after this
>>>  patch?
>>> 
>>>  What is the output of ls /sys/class/backlight when using
>>>  acpi_backlight=video on the kernel commandline?
>>> 
>>>  If the ls output shows multiple interfaces have you tried using all
>>>  listed interfaces directly from sysfs / the commandline ?
>>> 
>>>  (perhaps userspace is picking the wrong interface in the case there
>>>  are multiple interfaces?)
>>> 
>>>  Note what you are doing now is the equivalent of passing
>>>  acpi_backlight=vendor, which again is a weird thing to do on
>>>  recent / new hardware.
>>> 
>>>  Also your commit message seems to lack a lot of details like:
>>> 
>>>  1. Do you own an effected or multiple affected models yourself on
>>>  which you tested this?
>>> 
>>>  2. Was this tested by others on other models of these series?
>>> 
>>>  3. I assume this was discussed with others in some mailinglist /
>>>  forum discussion please provide links to this discussion.
>>> 
>>>  4. Has this been tested with with both the nouveau and the
>>>  nvidia binary driver or only with the nvidia binary driver ?
>>> 
>>>  5. What were the symptoms / problems noticed before making this 
>>> change
>>>  and how do things work after making the change?
>>> 
>>>  Regards,
>>> 
>>>  Hans
>>> 
>>> 
>>> 
>>> 
>>> 
>>>> 
>>>>   Signed-off-by: Luke D Jones <luke@ljones.dev>
>>>>   ---
>>>>    drivers/platform/x86/asus-nb-wmi.c | 57 
>>>> ++++--------------------------
>>>>    1 file changed, 6 insertions(+), 51 deletions(-)
>>>> 
>>>>   diff --git a/drivers/platform/x86/asus-nb-wmi.c 
>>>> b/drivers/platform/x86/asus-nb-wmi.c
>>>>   index d41d7ad14be0..f4db67c3eba2 100644
>>>>   --- a/drivers/platform/x86/asus-nb-wmi.c
>>>>   +++ b/drivers/platform/x86/asus-nb-wmi.c
>>>>   @@ -427,73 +427,28 @@ static const struct dmi_system_id 
>>>> asus_quirks[] = {
>>>>        },
>>>>        {
>>>>            .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA401IH",
>>>>   +        .ident = "ASUSTeK COMPUTER INC. GA401",
>>>>            .matches = {
>>>>                DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA401IH"),
>>>>   +            DMI_MATCH(DMI_PRODUCT_NAME, "GA401"),
>>>>            },
>>>>            .driver_data = &quirk_asus_vendor_backlight,
>>>>        },
>>>>        {
>>>>            .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA401II",
>>>>   +        .ident = "ASUSTeK COMPUTER INC. GA502",
>>>>            .matches = {
>>>>                DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA401II"),
>>>>   +            DMI_MATCH(DMI_PRODUCT_NAME, "GA502"),
>>>>            },
>>>>            .driver_data = &quirk_asus_vendor_backlight,
>>>>        },
>>>>        {
>>>>            .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA401IU",
>>>>   +        .ident = "ASUSTeK COMPUTER INC. GA503",
>>>>            .matches = {
>>>>                DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA401IU"),
>>>>   -        },
>>>>   -        .driver_data = &quirk_asus_vendor_backlight,
>>>>   -    },
>>>>   -    {
>>>>   -        .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA401IV",
>>>>   -        .matches = {
>>>>   -            DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA401IV"),
>>>>   -        },
>>>>   -        .driver_data = &quirk_asus_vendor_backlight,
>>>>   -    },
>>>>   -    {
>>>>   -        .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA401IVC",
>>>>   -        .matches = {
>>>>   -            DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA401IVC"),
>>>>   -        },
>>>>   -        .driver_data = &quirk_asus_vendor_backlight,
>>>>   -    },
>>>>   -        {
>>>>   -        .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA502II",
>>>>   -        .matches = {
>>>>   -            DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA502II"),
>>>>   -        },
>>>>   -        .driver_data = &quirk_asus_vendor_backlight,
>>>>   -    },
>>>>   -    {
>>>>   -        .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA502IU",
>>>>   -        .matches = {
>>>>   -            DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA502IU"),
>>>>   -        },
>>>>   -        .driver_data = &quirk_asus_vendor_backlight,
>>>>   -    },
>>>>   -    {
>>>>   -        .callback = dmi_matched,
>>>>   -        .ident = "ASUSTeK COMPUTER INC. GA502IV",
>>>>   -        .matches = {
>>>>   -            DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."),
>>>>   -            DMI_MATCH(DMI_PRODUCT_NAME, "GA502IV"),
>>>>   +            DMI_MATCH(DMI_PRODUCT_NAME, "GA503"),
>>>>            },
>>>>            .driver_data = &quirk_asus_vendor_backlight,
>>>>        },
>>>> 
>>> 
>> 
>> 
>