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

[Hans de Goede <hdegoede@redhat.com>]

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,
>  	},
>