[PATCH] ACPI: video: Fix Apple GMUX backlight detection

[PATCH] ACPI: video: Fix Apple GMUX backlight detection

[Hans de Goede]

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. */
-- 
2.38.1

Re: [PATCH] ACPI: video: Fix Apple GMUX backlight detection

[Lukas Wunner]

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

Re: [PATCH] ACPI: video: Fix Apple GMUX backlight detection

[Hans de Goede]

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).

Re: [PATCH] ACPI: video: Fix Apple GMUX backlight detection

[Rafael J. Wysocki]

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!