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

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

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