| Message ID | 20221215094138.7120-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | ACPI: video: Fix Apple GMUX backlight detection | expand |
On Thu, Dec 15, 2022 at 10:41:38AM +0100, Hans de Goede wrote: > The apple-gmux driver only binds to old GMUX devices which have an > IORESOURCE_IO resource (using inb()/outb()) rather then memory-mapped > IO (IORESOURCE_MEM). > > T2 MacBooks use the new style GMUX devices (with IORESOURCE_MEM access), > so these are not supported by the apple-gmux driver. This is not a problem > since they have working ACPI video backlight support. Interesting. > +static bool apple_gmux_backlight_present(void) > +{ > + struct acpi_device *adev; > + struct device *dev; > + > + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); > + if (!adev) > + return false; > + > + dev = acpi_get_first_physical_node(adev); > + if (!dev) > + return false; > + > + /* > + * drivers/platform/x86/apple-gmux.c only supports old style > + * Apple GMUX with an IO-resource. > + */ > + return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL; > +} The T2 is represented by a PCI device with ID 106B:1802. Instead of the above, how about amending apple_gmux_present() with a simple check like this: /* T2 Macs drive GMUX via MMIO, which is unsupported for now */ if (pci_dev_present({{PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x1802)}, {}})) return false; Thanks, Lukas
Hi Lukas, On 12/15/22 11:22, Lukas Wunner wrote: > On Thu, Dec 15, 2022 at 10:41:38AM +0100, Hans de Goede wrote: >> The apple-gmux driver only binds to old GMUX devices which have an >> IORESOURCE_IO resource (using inb()/outb()) rather then memory-mapped >> IO (IORESOURCE_MEM). >> >> T2 MacBooks use the new style GMUX devices (with IORESOURCE_MEM access), >> so these are not supported by the apple-gmux driver. This is not a problem >> since they have working ACPI video backlight support. > > Interesting. > > >> +static bool apple_gmux_backlight_present(void) >> +{ >> + struct acpi_device *adev; >> + struct device *dev; >> + >> + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); >> + if (!adev) >> + return false; >> + >> + dev = acpi_get_first_physical_node(adev); >> + if (!dev) >> + return false; >> + >> + /* >> + * drivers/platform/x86/apple-gmux.c only supports old style >> + * Apple GMUX with an IO-resource. >> + */ >> + return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL; >> +} > > The T2 is represented by a PCI device with ID 106B:1802. > > Instead of the above, how about amending apple_gmux_present() > with a simple check like this: > > /* T2 Macs drive GMUX via MMIO, which is unsupported for now */ > if (pci_dev_present({{PCI_DEVICE(PCI_VENDOR_ID_APPLE, 0x1802)}, {}})) > return false; Where ever possible I prefer to have the same checks in video_detect.c as the actual backlight driver uses and: drivers/platform/x86/apple-gmux.c refuses to bind because the pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) call fails. So IMHO it is better to do the same thing in video_detect.c . Specifically likely more then 1 model of MacBook is affected and doing the same check as apple-gmux.c does will catch them all, while an (PCI) id based approach often turns into a game of whack-a-mole. Also changing apple_gmux_present() itself impacts more callers then just the drivers/acpi_video_detect.c code. Specifically drivers/gpu/drm/nouveau/nouveau_backlight.c and drivers/gpu/vga/vga_switcheroo.c also call this function. And at least in the nouveau case where the check looks like this: if (apple_gmux_present()) { NV_INFO_ONCE(drm, "Apple GMUX detected: not registering Nouveau backlight interface\n"); return 0; } Not registering the GPU/native backlight device is the right thing to do on the models with the new GMUX too, since native backlight control won't work there either. And I don't know about the check in vga_switcheroo.c. The goal of this patch is to fix the 6.1 regression introduced by adding a apple_gmux_present() to video_acpi.c and in its current form it fixes that regression without introducing any behavioral changes elsewhere, which seems best for a regressions fix which is intended to be backported to 6.1 . Regards, Hans p.s. The nouveau driver not registering its native backlight is actually something which the amdgpu driver gets wrong, it registers a non working amdgpu_bl0 on the laptop Aditya ran their tests on. Starting with 6.2 however amdgpu will honor the return value of acpi_video_get_backlight_type() (on x86/ACPI platforms). So the non working amdgpu_bl0 should disappear with 6.2, as long as we get acpi_video_get_backlight_type() to return the right type, which it does after this patch (it returns acpi_backlight_video after this patch).
On Thu, Dec 15, 2022 at 10:41 AM Hans de Goede <hdegoede@redhat.com> wrote: > > The apple-gmux driver only binds to old GMUX devices which have an > IORESOURCE_IO resource (using inb()/outb()) rather then memory-mapped > IO (IORESOURCE_MEM). > > T2 MacBooks use the new style GMUX devices (with IORESOURCE_MEM access), > so these are not supported by the apple-gmux driver. This is not a problem > since they have working ACPI video backlight support. > > But the apple_gmux_present() helper only checks if an ACPI device with > the "APP000B" HID is present, causing acpi_video_get_backlight_type() > to return acpi_backlight_apple_gmux disabling the acpi_video backlight > device. > > Add a new apple_gmux_backlight_present() helper which checks that > the "APP000B" device actually is an old GMUX device with an IORESOURCE_IO > resource. > > This fixes the acpi_video0 backlight no longer registering on T2 MacBooks. > > Note people are working to add support for the new style GMUX to Linux: > https://github.com/kekrby/linux-t2/commits/wip/hybrid-graphics > > Once this lands this patch should be reverted so that > acpi_video_get_backlight_type() also prefers the gmux on new style GMUX > MacBooks, but for now this is necessary to avoid regressing backlight > control on T2 Macs. > > Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection") > Reported-and-tested-by: Aditya Garg <gargaditya08@live.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/acpi/video_detect.c | 23 ++++++++++++++++++++++- > 1 file changed, 22 insertions(+), 1 deletion(-) > > diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c > index a934bbc9dd37..1b78c7434492 100644 > --- a/drivers/acpi/video_detect.c > +++ b/drivers/acpi/video_detect.c > @@ -34,6 +34,7 @@ > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/platform_data/x86/nvidia-wmi-ec-backlight.h> > +#include <linux/pnp.h> > #include <linux/types.h> > #include <linux/workqueue.h> > #include <acpi/video.h> > @@ -105,6 +106,26 @@ static bool nvidia_wmi_ec_supported(void) > } > #endif > > +static bool apple_gmux_backlight_present(void) > +{ > + struct acpi_device *adev; > + struct device *dev; > + > + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); > + if (!adev) > + return false; > + > + dev = acpi_get_first_physical_node(adev); > + if (!dev) > + return false; > + > + /* > + * drivers/platform/x86/apple-gmux.c only supports old style > + * Apple GMUX with an IO-resource. > + */ > + return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL; > +} > + > /* Force to use vendor driver when the ACPI device is known to be > * buggy */ > static int video_detect_force_vendor(const struct dmi_system_id *d) > @@ -767,7 +788,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native) > if (nvidia_wmi_ec_present) > return acpi_backlight_nvidia_wmi_ec; > > - if (apple_gmux_present()) > + if (apple_gmux_backlight_present()) > return acpi_backlight_apple_gmux; > > /* Use ACPI video if available, except when native should be preferred. */ > -- Applied as 6.2-rc material, thanks!
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c index a934bbc9dd37..1b78c7434492 100644 --- a/drivers/acpi/video_detect.c +++ b/drivers/acpi/video_detect.c @@ -34,6 +34,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/platform_data/x86/nvidia-wmi-ec-backlight.h> +#include <linux/pnp.h> #include <linux/types.h> #include <linux/workqueue.h> #include <acpi/video.h> @@ -105,6 +106,26 @@ static bool nvidia_wmi_ec_supported(void) } #endif +static bool apple_gmux_backlight_present(void) +{ + struct acpi_device *adev; + struct device *dev; + + adev = acpi_dev_get_first_match_dev(GMUX_ACPI_HID, NULL, -1); + if (!adev) + return false; + + dev = acpi_get_first_physical_node(adev); + if (!dev) + return false; + + /* + * drivers/platform/x86/apple-gmux.c only supports old style + * Apple GMUX with an IO-resource. + */ + return pnp_get_resource(to_pnp_dev(dev), IORESOURCE_IO, 0) != NULL; +} + /* Force to use vendor driver when the ACPI device is known to be * buggy */ static int video_detect_force_vendor(const struct dmi_system_id *d) @@ -767,7 +788,7 @@ static enum acpi_backlight_type __acpi_video_get_backlight_type(bool native) if (nvidia_wmi_ec_present) return acpi_backlight_nvidia_wmi_ec; - if (apple_gmux_present()) + if (apple_gmux_backlight_present()) return acpi_backlight_apple_gmux; /* Use ACPI video if available, except when native should be preferred. */
The apple-gmux driver only binds to old GMUX devices which have an IORESOURCE_IO resource (using inb()/outb()) rather then memory-mapped IO (IORESOURCE_MEM). T2 MacBooks use the new style GMUX devices (with IORESOURCE_MEM access), so these are not supported by the apple-gmux driver. This is not a problem since they have working ACPI video backlight support. But the apple_gmux_present() helper only checks if an ACPI device with the "APP000B" HID is present, causing acpi_video_get_backlight_type() to return acpi_backlight_apple_gmux disabling the acpi_video backlight device. Add a new apple_gmux_backlight_present() helper which checks that the "APP000B" device actually is an old GMUX device with an IORESOURCE_IO resource. This fixes the acpi_video0 backlight no longer registering on T2 MacBooks. Note people are working to add support for the new style GMUX to Linux: https://github.com/kekrby/linux-t2/commits/wip/hybrid-graphics Once this lands this patch should be reverted so that acpi_video_get_backlight_type() also prefers the gmux on new style GMUX MacBooks, but for now this is necessary to avoid regressing backlight control on T2 Macs. Fixes: 21245df307cb ("ACPI: video: Add Apple GMUX brightness control detection") Reported-and-tested-by: Aditya Garg <gargaditya08@live.com> Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/video_detect.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)