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