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