| Message ID | 20220712193910.439171-21-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/kms: Stop registering multiple /sys/class/backlight devs for a single display | expand |
Hi, On 7/12/22 22:24, Daniel Dadap wrote: > I'll ask around to see if there's some DMI property we can match in order to detect whether a system is expected to use the EC backlight driver: if so, maybe we can avoid the WMI interactions in patch 16/29 of this series. Although I suppose even if there were a DMI property, we'd still need to call the WMI-wrapped ACPI method to check whether the system is currently configured to drive the backlight through the EC, unless the system somehow exports a different DMI table depending on the current backlight control configuration, which I imagine to be unlikely. IMHO the duplication is fine, it is also important that the video_detect.c code and the actual backlight driver use the same detection mechanism where possible. Otherwise acpi_video_get_backlight_type() may return acpi_backlight_nvidia_wmi_ec while the EC backlight driver refuses to load... Regards, Hans > > This change looks fine to me, although I suppose somebody who maintains the acer-wmi driver should comment. The bugzilla links are a nice touch. > > On 7/12/22 14:39, Hans de Goede wrote: >> Move the backlight DMI quirks to acpi/video_detect.c, so that >> the driver no longer needs to call acpi_video_set_dmi_backlight_type(). >> >> acpi_video_set_dmi_backlight_type() is troublesome because it may end up >> getting called after other backlight drivers have already called >> acpi_video_get_backlight_type() resulting in the other drivers >> already being registered even though they should not. >> >> Note that even though the DMI quirk table name was video_vendor_dmi_table, >> 5/6 quirks were actually quirks to use the GPU native backlight. >> >> These 5 quirks also had a callback in their dmi_system_id entry which >> disabled the acer-wmi vendor driver; and any DMI match resulted in: >> >> acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); >> >> which disabled the acpi_video driver, so only the native driver was left. >> The new entries for these 5/6 devices correctly marks these as needing >> the native backlight driver. >> >> Also note that other changes in this series change the native backlight >> drivers to no longer unconditionally register their backlight. Instead >> these drivers now do this check: >> >> if (acpi_video_get_backlight_type(false) != acpi_backlight_native) >> return 0; /* bail */ >> >> which without this patch would have broken these 5/6 "special" quirks. >> >> Since I had to look at all the commits adding the quirks anyways, to make >> sure that I understood the code correctly, I've also added links to >> the various original bugzillas for these quirks to the new entries. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/acpi/video_detect.c | 53 ++++++++++++++++++++++++++ >> drivers/platform/x86/acer-wmi.c | 66 --------------------------------- >> 2 files changed, 53 insertions(+), 66 deletions(-) >> >> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c >> index a514adaec14d..cd51cb0d7821 100644 >> --- a/drivers/acpi/video_detect.c >> +++ b/drivers/acpi/video_detect.c >> @@ -147,6 +147,15 @@ static const struct dmi_system_id video_detect_dmi_table[] = { >> DMI_MATCH(DMI_BOARD_NAME, "X360"), >> }, >> }, >> + { >> + /* https://bugzilla.redhat.com/show_bug.cgi?id=1128309 */ >> + .callback = video_detect_force_vendor, >> + /* Acer KAV80 */ >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "KAV80"), >> + }, >> + }, >> { >> .callback = video_detect_force_vendor, >> /* Asus UL30VT */ >> @@ -427,6 +436,41 @@ static const struct dmi_system_id video_detect_dmi_table[] = { >> DMI_MATCH(DMI_BOARD_NAME, "JV50"), >> }, >> }, >> + { >> + /* https://bugzilla.redhat.com/show_bug.cgi?id=1012674 */ >> + .callback = video_detect_force_native, >> + /* Acer Aspire 5741 */ >> + .matches = { >> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"), >> + }, >> + }, >> + { >> + /* https://bugzilla.kernel.org/show_bug.cgi?id=42993 */ >> + .callback = video_detect_force_native, >> + /* Acer Aspire 5750 */ >> + .matches = { >> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"), >> + }, >> + }, >> + { >> + /* https://bugzilla.kernel.org/show_bug.cgi?id=42833 */ >> + .callback = video_detect_force_native, >> + /* Acer Extensa 5235 */ >> + .matches = { >> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "Extensa 5235"), >> + }, >> + }, >> + { >> + .callback = video_detect_force_native, >> + /* Acer TravelMate 4750 */ >> + .matches = { >> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"), >> + }, >> + }, >> { >> /* https://bugzilla.kernel.org/show_bug.cgi?id=207835 */ >> .callback = video_detect_force_native, >> @@ -437,6 +481,15 @@ static const struct dmi_system_id video_detect_dmi_table[] = { >> DMI_MATCH(DMI_BOARD_NAME, "BA51_MV"), >> }, >> }, >> + { >> + /* https://bugzilla.kernel.org/show_bug.cgi?id=36322 */ >> + .callback = video_detect_force_native, >> + /* Acer TravelMate 5760 */ >> + .matches = { >> + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 5760"), >> + }, >> + }, >> { >> .callback = video_detect_force_native, >> /* ASUSTeK COMPUTER INC. GA401 */ >> diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c >> index 9c6943e401a6..c08eeb1e0e05 100644 >> --- a/drivers/platform/x86/acer-wmi.c >> +++ b/drivers/platform/x86/acer-wmi.c >> @@ -643,69 +643,6 @@ static const struct dmi_system_id non_acer_quirks[] __initconst = { >> {} >> }; >> -static int __init >> -video_set_backlight_video_vendor(const struct dmi_system_id *d) >> -{ >> - interface->capability &= ~ACER_CAP_BRIGHTNESS; >> - pr_info("Brightness must be controlled by generic video driver\n"); >> - return 0; >> -} >> - >> -static const struct dmi_system_id video_vendor_dmi_table[] __initconst = { >> - { >> - .callback = video_set_backlight_video_vendor, >> - .ident = "Acer TravelMate 4750", >> - .matches = { >> - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"), >> - }, >> - }, >> - { >> - .callback = video_set_backlight_video_vendor, >> - .ident = "Acer Extensa 5235", >> - .matches = { >> - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Extensa 5235"), >> - }, >> - }, >> - { >> - .callback = video_set_backlight_video_vendor, >> - .ident = "Acer TravelMate 5760", >> - .matches = { >> - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 5760"), >> - }, >> - }, >> - { >> - .callback = video_set_backlight_video_vendor, >> - .ident = "Acer Aspire 5750", >> - .matches = { >> - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"), >> - }, >> - }, >> - { >> - .callback = video_set_backlight_video_vendor, >> - .ident = "Acer Aspire 5741", >> - .matches = { >> - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"), >> - }, >> - }, >> - { >> - /* >> - * Note no video_set_backlight_video_vendor, we must use the >> - * acer interface, as there is no native backlight interface. >> - */ >> - .ident = "Acer KAV80", >> - .matches = { >> - DMI_MATCH(DMI_SYS_VENDOR, "Acer"), >> - DMI_MATCH(DMI_PRODUCT_NAME, "KAV80"), >> - }, >> - }, >> - {} >> -}; >> - >> /* Find which quirks are needed for a particular vendor/ model pair */ >> static void __init find_quirks(void) >> { >> @@ -2482,9 +2419,6 @@ static int __init acer_wmi_init(void) >> set_quirks(); >> - if (dmi_check_system(video_vendor_dmi_table)) >> - acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); >> - >> if (acpi_video_get_backlight_type() != acpi_backlight_vendor) >> interface->capability &= ~ACER_CAP_BRIGHTNESS; >> >
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index a514adaec14d..cd51cb0d7821 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -147,6 +147,15 @@ static const struct dmi_system_id video_detect_dmi_table[] = { DMI_MATCH(DMI_BOARD_NAME, "X360"), }, }, + { + /* https://bugzilla.redhat.com/show_bug.cgi?id=1128309 */ + .callback = video_detect_force_vendor, + /* Acer KAV80 */ + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "KAV80"), + }, + }, { .callback = video_detect_force_vendor, /* Asus UL30VT */ @@ -427,6 +436,41 @@ static const struct dmi_system_id video_detect_dmi_table[] = { DMI_MATCH(DMI_BOARD_NAME, "JV50"), }, }, + { + /* https://bugzilla.redhat.com/show_bug.cgi?id=1012674 */ + .callback = video_detect_force_native, + /* Acer Aspire 5741 */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"), + }, + }, + { + /* https://bugzilla.kernel.org/show_bug.cgi?id=42993 */ + .callback = video_detect_force_native, + /* Acer Aspire 5750 */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"), + }, + }, + { + /* https://bugzilla.kernel.org/show_bug.cgi?id=42833 */ + .callback = video_detect_force_native, + /* Acer Extensa 5235 */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "Extensa 5235"), + }, + }, + { + .callback = video_detect_force_native, + /* Acer TravelMate 4750 */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"), + }, + }, { /* https://bugzilla.kernel.org/show_bug.cgi?id=207835 */ .callback = video_detect_force_native, @@ -437,6 +481,15 @@ static const struct dmi_system_id video_detect_dmi_table[] = { DMI_MATCH(DMI_BOARD_NAME, "BA51_MV"), }, }, + { + /* https://bugzilla.kernel.org/show_bug.cgi?id=36322 */ + .callback = video_detect_force_native, + /* Acer TravelMate 5760 */ + .matches = { + DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), + DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 5760"), + }, + }, { .callback = video_detect_force_native, /* ASUSTeK COMPUTER INC. GA401 */ diff --git a/drivers/platform/x86/acer-wmi.c b/drivers/platform/x86/acer-wmi.c index 9c6943e401a6..c08eeb1e0e05 100644 --- a/drivers/platform/x86/acer-wmi.c +++ b/drivers/platform/x86/acer-wmi.c @@ -643,69 +643,6 @@ static const struct dmi_system_id non_acer_quirks[] __initconst = { {} }; -static int __init -video_set_backlight_video_vendor(const struct dmi_system_id *d) -{ - interface->capability &= ~ACER_CAP_BRIGHTNESS; - pr_info("Brightness must be controlled by generic video driver\n"); - return 0; -} - -static const struct dmi_system_id video_vendor_dmi_table[] __initconst = { - { - .callback = video_set_backlight_video_vendor, - .ident = "Acer TravelMate 4750", - .matches = { - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), - DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 4750"), - }, - }, - { - .callback = video_set_backlight_video_vendor, - .ident = "Acer Extensa 5235", - .matches = { - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), - DMI_MATCH(DMI_PRODUCT_NAME, "Extensa 5235"), - }, - }, - { - .callback = video_set_backlight_video_vendor, - .ident = "Acer TravelMate 5760", - .matches = { - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), - DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 5760"), - }, - }, - { - .callback = video_set_backlight_video_vendor, - .ident = "Acer Aspire 5750", - .matches = { - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), - DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5750"), - }, - }, - { - .callback = video_set_backlight_video_vendor, - .ident = "Acer Aspire 5741", - .matches = { - DMI_MATCH(DMI_BOARD_VENDOR, "Acer"), - DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5741"), - }, - }, - { - /* - * Note no video_set_backlight_video_vendor, we must use the - * acer interface, as there is no native backlight interface. - */ - .ident = "Acer KAV80", - .matches = { - DMI_MATCH(DMI_SYS_VENDOR, "Acer"), - DMI_MATCH(DMI_PRODUCT_NAME, "KAV80"), - }, - }, - {} -}; - /* Find which quirks are needed for a particular vendor/ model pair */ static void __init find_quirks(void) { @@ -2482,9 +2419,6 @@ static int __init acer_wmi_init(void) set_quirks(); - if (dmi_check_system(video_vendor_dmi_table)) - acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); - if (acpi_video_get_backlight_type() != acpi_backlight_vendor) interface->capability &= ~ACER_CAP_BRIGHTNESS;
Move the backlight DMI quirks to acpi/video_detect.c, so that the driver no longer needs to call acpi_video_set_dmi_backlight_type(). acpi_video_set_dmi_backlight_type() is troublesome because it may end up getting called after other backlight drivers have already called acpi_video_get_backlight_type() resulting in the other drivers already being registered even though they should not. Note that even though the DMI quirk table name was video_vendor_dmi_table, 5/6 quirks were actually quirks to use the GPU native backlight. These 5 quirks also had a callback in their dmi_system_id entry which disabled the acer-wmi vendor driver; and any DMI match resulted in: acpi_video_set_dmi_backlight_type(acpi_backlight_vendor); which disabled the acpi_video driver, so only the native driver was left. The new entries for these 5/6 devices correctly marks these as needing the native backlight driver. Also note that other changes in this series change the native backlight drivers to no longer unconditionally register their backlight. Instead these drivers now do this check: if (acpi_video_get_backlight_type(false) != acpi_backlight_native) return 0; /* bail */ which without this patch would have broken these 5/6 "special" quirks. Since I had to look at all the commits adding the quirks anyways, to make sure that I understood the code correctly, I've also added links to the various original bugzillas for these quirks to the new entries. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/video_detect.c | 53 ++++++++++++++++++++++++++ drivers/platform/x86/acer-wmi.c | 66 --------------------------------- 2 files changed, 53 insertions(+), 66 deletions(-)