Re: [PATCH v3 0/5] apple-gmux: support MMIO gmux type on T2 Macs

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

[-- Attachment #1: Type: text/plain, Size: 5343 bytes --]

Hi,

On 2/21/23 10:24, Aditya Garg wrote:
> 
>> On 18-Feb-2023, at 6:50 PM, Orlando Chamberlain <orlandoch.dev@gmail.com> wrote:
>> Hi All,
>> This patch series adds support for the MMIO based gmux present on these
>> Dual GPU Apple T2 Macs: MacBookPro15,1, MacBookPro15,3, MacBookPro16,1,
>> MacBookPro16,4 (although amdgpu isn't working on MacBookPro16,4 [1]).
>>
> 
> Could be an upstream bug, but I’ve noticed that after using these patches, if I add `acpi_backlight=video` in the command line, it still uses `gmux_backlight`
> 
> Hans, any ideas why?

Currently the backlight registration in apple-gmux.c is unconditional
(if a GMUX is detected).

I have attached a patch to make it honor the acpi_backlight=xxx
kernel commandline option like most other x86/ACPI backlight drivers
do, please give this a test.

Regards,

Hans


> 
>> Changes from v2[2]:
>>
>> - Add "," to last item in apple_gmux_type enum
>> - Don't not clear interrupts when status is 0
>> - Don't check if we failed to make debugfs folder
>> - Check for fake mmio gmux
>>
>> # 1:
>>
>> has a slight change in how the switch state is read: instead of checking
>> for x == 2, check !(x & 1)
>>
>> # 2:
>>
>> implements a system to support more than 2 gmux types
>>
>> # 3:
>>
>> start using the gmux's GMSP acpi method when handling interrupts on MMIO
>> gmux's. This is needed for the MMIO gmux's to clear interrupts.
>>
>> # 4:
>>
>> Adds support for the MMIO based gmux on T2 macs.
>>
>> # 5:
>>
>> Add a debugfs interface to apple-gmux so data from ports can be read
>> and written to from userspace.
>>
>> This can be used for more easily researching what unknown ports do,
>> and switching gpus when vga_switcheroo isn't ready (e.g. when one gpu
>> is bound to vfio-pci and in use by a Windows VM, I can use this to
>> switch my internal display between Linux and Windows easily).
>>
>> # Issues:
>>
>> 1. Switching gpus at runtime has the same issue as indexed gmux's: the
>> inactive gpu can't probe the DDC lines for eDP [3]
>>
>> 2. iMacPro1,1, iMac20,1 and iMac20,2 all seem to have a gmux in their
>> acpi tables, but they shouldn't. A check that hopefully will detect this
>> is used, but it's untested as I don't have any of those computers.
>>
>> 3. Powering on the amdgpu with vga_switcheroo doesn't work well. I'm
>> told on the MacBookPro15,1 it works sometimes, and adding delays helps,
>> but on my MacBookPro16,1 I haven't been able to get it to work at all:
>>
>> amdgpu: switched off
>> amdgpu: switched on
>> amdgpu 0000:03:00.0:
>>    Unable to change power state from D3hot to D0, device inaccessible
>> amdgpu 0000:03:00.0:
>>    Unable to change power state from D3cold to D0, device inaccessible
>> [drm] PCIE GART of 512M enabled (table at 0x00000080FEE00000).
>> [drm] PSP is resuming...
>> [drm:psp_hw_start [amdgpu]] *ERROR* PSP create ring failed!
>> [drm:psp_resume [amdgpu]] *ERROR* PSP resume failed
>> [drm:amdgpu_device_fw_loading [amdgpu]]
>>    *ERROR* resume of IP block <psp> failed -62
>> amdgpu 0000:03:00.0: amdgpu: amdgpu_device_ip_resume failed (-62).
>> snd_hda_intel 0000:03:00.1: Enabling via vga_switcheroo
>> snd_hda_intel 0000:03:00.1:
>>    Unable to change power state from D3cold to D0, device inaccessible
>> snd_hda_intel 0000:03:00.1: CORB reset timeout#2, CORBRP = 65535
>> snd_hda_codec_hdmi hdaudioC0D0: Unable to sync register 0x2f0d00. -5
>>
>> There are some acpi methods (PWRD, PWG1 [4, 5]) that macOS calls when
>> changing the amdgpu's power state, but we don't use them and that could be
>> a cause. Additionally unlike previous generation Macbooks which work
>> better, on MacBookPro16,1 the gpu is located behind 2 pci bridges:
>>
>> 01:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI]
>>    Navi 10 XL Upstream Port of PCI Express Switch (rev 43)
>> 02:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI]
>>    Navi 10 XL Downstream Port of PCI Express Switch
>> 03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI]
>>    Navi 14 [Radeon RX 5500/5500M / Pro 5500M] (rev 43)
>> 03:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI]
>>    Navi 10 HDMI Audio
>>
>> Upon attempting to power on the gpu with vga_switcheroo, all these
>> devices except 01:00.0 have their config space filled with 1s.
>> Rescanning pci makes the config space of all the devices go back to
>> normal, however amdgpu still fails to resume with the same logs as
>> above.
>>
>> [1]: https://lore.kernel.org/all/3AFB9142-2BD0-46F9-AEA9-C9C5D13E68E6@live.com/
>> [2]: https://lore.kernel.org/platform-driver-x86/20230216122342.5918-1-orlandoch.dev@gmail.com/
>> [3]: https://lore.kernel.org/all/9eed8ede6f15a254ad578e783b050e1c585d5a15.1439288957.git.lukas@wunner.de/
>> [4]: https://gist.github.com/Redecorating/6c7136b7a4ac7ce3b77d8e41740dd87b
>> [5]: https://lore.kernel.org/all/20120710160555.GA31562@srcf.ucam.org/
>>
>> Orlando Chamberlain (5):
>>  apple-gmux: use first bit to check switch state
>>  apple-gmux: refactor gmux types
>>  apple-gmux: Use GMSP acpi method for interrupt clear
>>  apple-gmux: support MMIO gmux on T2 Macs
>>  apple-gmux: add debugfs interface
>>
>> drivers/platform/x86/apple-gmux.c | 349 ++++++++++++++++++++++++++----
>> include/linux/apple-gmux.h        |  70 ++++--
>> 2 files changed, 357 insertions(+), 62 deletions(-)
>>
>> -- 
>> 2.39.1

[-- Attachment #2: 0001-platform-x86-apple-gmux-Add-acpi_video_get_backlight.patch --]
[-- Type: text/x-patch, Size: 3580 bytes --]

From 97228b107fe075b94c59a5b68183e00fe7367d15 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 21 Feb 2023 13:26:19 +0100
Subject: [PATCH] platform/x86: apple-gmux: Add acpi_video_get_backlight_type()
 check

Make apple-gmux backlight registration honor the acpi_backlight=...
kernel commandline option which is used to select the backlight
control method on x86/ACPI devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/apple-gmux.c | 59 +++++++++++++++++--------------
 1 file changed, 33 insertions(+), 26 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 9333f82cfa8a..246caaab27aa 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -21,6 +21,7 @@
 #include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/vga_switcheroo.h>
+#include <acpi/video.h>
 #include <asm/io.h>
 
 /**
@@ -562,6 +563,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	struct backlight_properties props;
 	struct backlight_device *bdev;
 	u8 ver_major, ver_minor, ver_release;
+	bool register_bdev = true;
 	int ret = -ENXIO;
 	acpi_status status;
 	unsigned long long gpe;
@@ -610,33 +612,38 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	props.type = BACKLIGHT_PLATFORM;
 	props.max_brightness = gmux_read32(gmux_data, GMUX_PORT_MAX_BRIGHTNESS);
 
-	/*
-	 * Currently it's assumed that the maximum brightness is less than
-	 * 2^24 for compatibility with old gmux versions. Cap the max
-	 * brightness at this value, but print a warning if the hardware
-	 * reports something higher so that it can be fixed.
-	 */
-	if (WARN_ON(props.max_brightness > GMUX_MAX_BRIGHTNESS))
-		props.max_brightness = GMUX_MAX_BRIGHTNESS;
-
-	bdev = backlight_device_register("gmux_backlight", &pnp->dev,
-					 gmux_data, &gmux_bl_ops, &props);
-	if (IS_ERR(bdev)) {
-		ret = PTR_ERR(bdev);
-		goto err_release;
-	}
-
-	gmux_data->bdev = bdev;
-	bdev->props.brightness = gmux_get_brightness(bdev);
-	backlight_update_status(bdev);
+#if IS_REACHABLE(CONFIG_ACPI_VIDEO)
+	register_bdev = acpi_video_get_backlight_type() == acpi_backlight_apple_gmux;
+#endif
+	if (register_bdev) {
+		/*
+		 * Currently it's assumed that the maximum brightness is less than
+		 * 2^24 for compatibility with old gmux versions. Cap the max
+		 * brightness at this value, but print a warning if the hardware
+		 * reports something higher so that it can be fixed.
+		 */
+		if (WARN_ON(props.max_brightness > GMUX_MAX_BRIGHTNESS))
+			props.max_brightness = GMUX_MAX_BRIGHTNESS;
+
+		bdev = backlight_device_register("gmux_backlight", &pnp->dev,
+						 gmux_data, &gmux_bl_ops, &props);
+		if (IS_ERR(bdev)) {
+			ret = PTR_ERR(bdev);
+			goto err_release;
+		}
 
-	/*
-	 * The backlight situation on Macs is complicated. If the gmux is
-	 * present it's the best choice, because it always works for
-	 * backlight control and supports more levels than other options.
-	 * Disable the other backlight choices.
-	 */
-	apple_bl_unregister();
+		gmux_data->bdev = bdev;
+		bdev->props.brightness = gmux_get_brightness(bdev);
+		backlight_update_status(bdev);
+
+		/*
+		 * The backlight situation on Macs is complicated. If the gmux is
+		 * present it's the best choice, because it always works for
+		 * backlight control and supports more levels than other options.
+		 * Disable the other backlight choices.
+		 */
+		apple_bl_unregister();
+	}
 
 	gmux_data->power_state = VGA_SWITCHEROO_ON;
 
-- 
2.39.1