[RFC PATCH 0/9] apple-gmux: support MMIO gmux type on T2 Macs

[RFC PATCH 0/9] apple-gmux: support MMIO gmux type on T2 Macs

[Orlando Chamberlain]

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

It's only been tested by people on T2 Macs with MMIO based gmux's using
t2linux [2] kernels, but some changes may impact older port io and indexed
gmux's so testing, especially on those older Macbooks, would be
appreciated.

# 1-2:

refactor code to make it easier to add the 3rd gmux type.

# 3:

has a slight change in how the switch state is read, I don't
expect this to cause issues for older models (but still, please test if
you have one!)

# 4:

implements a system to support more than 2 gmux types

# 5:

start using the gmux's GMSP acpi method when handling interrupts. This
is needed for the MMIO gmux's, and its present in the acpi tables of some
indexed gmux's I could find so hopefully enabling this for all models
will be fine, but if not it can be only used on MMIO gmux's.

# 6:

Adds support for the MMIO based gmux on T2 macs.

# 7:

Add a sysfs interface to apple-gmux so data from ports can be read
from userspace, and written to if the user enables an unsafe kernel
parameter.

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

# 8-9:

These patches make amdgpu and snd_hda_intel register with vga_switcheroo
on Macbooks. I would like advice from the AMD folks on how they want
this to work, so that both PX and apple-gmux laptops work properly.

For radeon and nouveau we just register for every non-thunderbolt
device, but this was changed for AMD cards in commit 3840c5bcc245
("drm/amdgpu: disentangle runtime pm and vga_switcheroo") and commit
586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD").

This meant that only gpu's with PX register. Commit #8 makes amdgpu
register for all non-thinderbolt cards, and commit #9 makes snd_hda_intel
register for all amd cards with the PWRD (mentioned below) acpi method.
An alternative would be using apple-gmux-detect(), but that won't work
after apple-gmux has probed and claimed its memory resources.

# 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. 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:

snd_hda_intel 0000:03:00.1: Disabling via vga_switcheroo
snd_hda_intel 0000:03:00.1: Cannot lock devices!
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 in `lspci -x` filled with
0xff. `echo 1 > /sys/bus/pci/rescan` fixes that and the dmesg errors about
changing power state, but "PSP create ring failed" still happens, and
the gpu doesn't resume properly.

[1]: https://lore.kernel.org/all/3AFB9142-2BD0-46F9-AEA9-C9C5D13E68E6@live.com/
[2]: https://t2linux.org
[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/

Kerem Karabay (1):
  drm/amdgpu: register a vga_switcheroo client for all GPUs that are not
    thunderbolt attached

Orlando Chamberlain (8):
  apple-gmux: use cpu_to_be32 instead of manual reorder
  apple-gmux: consolidate version reading
  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 sysfs interface
  hda/hdmi: Register with vga_switcheroo on Dual GPU Macbooks

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  18 +-
 drivers/platform/x86/apple-gmux.c          | 416 +++++++++++++++++----
 include/linux/apple-gmux.h                 |  50 ++-
 sound/pci/hda/hda_intel.c                  |  19 +-
 4 files changed, 409 insertions(+), 94 deletions(-)

-- 
2.39.1

[RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[Orlando Chamberlain]

Currently it manually flips the byte order, but we can instead use
cpu_to_be32(val) for this.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 9333f82cfa8a..e8cb084cb81f 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -94,13 +94,7 @@ static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
 static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
 			     u32 val)
 {
-	int i;
-	u8 tmpval;
-
-	for (i = 0; i < 4; i++) {
-		tmpval = (val >> (i * 8)) & 0xff;
-		outb(tmpval, gmux_data->iostart + port + i);
-	}
+	outl(cpu_to_be32(val), gmux_data->iostart + port);
 }
 
 static int gmux_index_wait_ready(struct apple_gmux_data *gmux_data)
@@ -177,16 +171,8 @@ static u32 gmux_index_read32(struct apple_gmux_data *gmux_data, int port)
 static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
 			       u32 val)
 {
-	int i;
-	u8 tmpval;
-
 	mutex_lock(&gmux_data->index_lock);
-
-	for (i = 0; i < 4; i++) {
-		tmpval = (val >> (i * 8)) & 0xff;
-		outb(tmpval, gmux_data->iostart + GMUX_PORT_VALUE + i);
-	}
-
+	outl(cpu_to_be32(val), gmux_data->iostart + GMUX_PORT_VALUE);
 	gmux_index_wait_ready(gmux_data);
 	outb(port & 0xff, gmux_data->iostart + GMUX_PORT_WRITE);
 	gmux_index_wait_complete(gmux_data);
-- 
2.39.1

[RFC PATCH 2/9] apple-gmux: consolidate version reading

[Orlando Chamberlain]

Read gmux version in one go as 32 bits on both indexed and classic
gmux's.

Classic gmux's used to read the version as

major = inb(base + 0x4);
minor = inb(base + 0x5);
release = inb(base + 0x6);

but this can instead be done the same way as indexed gmux's with
gmux_read32(), so the same version reading code is used for classic
and indexed gmux's (as well as mmio gmux's that will be added to this
driver).

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 14 ++++++--------
 include/linux/apple-gmux.h        |  6 +-----
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index e8cb084cb81f..67628104f31a 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -580,15 +580,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	if (indexed) {
 		mutex_init(&gmux_data->index_lock);
 		gmux_data->indexed = true;
-		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
-		ver_major = (version >> 24) & 0xff;
-		ver_minor = (version >> 16) & 0xff;
-		ver_release = (version >> 8) & 0xff;
-	} else {
-		ver_major = gmux_read8(gmux_data, GMUX_PORT_VERSION_MAJOR);
-		ver_minor = gmux_read8(gmux_data, GMUX_PORT_VERSION_MINOR);
-		ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE);
 	}
+
+	version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
+	ver_major = (version >> 24) & 0xff;
+	ver_minor = (version >> 16) & 0xff;
+	ver_release = (version >> 8) & 0xff;
+
 	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
 		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
 
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index 1f68b49bcd68..eb2caee04abd 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -67,7 +67,6 @@ static inline bool apple_gmux_is_indexed(unsigned long iostart)
  */
 static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
 {
-	u8 ver_major, ver_minor, ver_release;
 	struct device *dev = NULL;
 	struct acpi_device *adev;
 	struct resource *res;
@@ -95,10 +94,7 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
 	 * Invalid version information may indicate either that the gmux
 	 * device isn't present or that it's a new one that uses indexed io.
 	 */
-	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
-	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
-	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
-	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
+	if (!(~inl(res->start + GMUX_PORT_VERSION_MAJOR))) {
 		indexed = apple_gmux_is_indexed(res->start);
 		if (!indexed)
 			goto out;
-- 
2.39.1

[RFC PATCH 3/9] apple-gmux: use first bit to check switch state

[Orlando Chamberlain]

On T2 Macs with MMIO gmux, when GMUX_PORT_SWITCH_DISPLAY is read, it can
have values of 2, 3, 4, and 5. Odd values correspond to the discrete gpu,
and even values correspond to the integrated gpu. The current logic is
that only 2 corresponds to IGD, but this doesn't work for T2 Macs.
Instead, check the first bit to determine the connected gpu.

As T2 Macs with gmux only can switch the internal display, it is
untested if this change (or a similar change) would be applicable
to GMUX_PORT_SWITCH_DDC and GMUX_PORT_SWITCH_EXTERNAL.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 67628104f31a..6109f4c2867c 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -332,10 +332,10 @@ static void gmux_read_switch_state(struct apple_gmux_data *gmux_data)
 	else
 		gmux_data->switch_state_ddc = VGA_SWITCHEROO_DIS;
 
-	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) == 2)
-		gmux_data->switch_state_display = VGA_SWITCHEROO_IGD;
-	else
+	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_DISPLAY) & 1)
 		gmux_data->switch_state_display = VGA_SWITCHEROO_DIS;
+	else
+		gmux_data->switch_state_display = VGA_SWITCHEROO_IGD;
 
 	if (gmux_read8(gmux_data, GMUX_PORT_SWITCH_EXTERNAL) == 2)
 		gmux_data->switch_state_external = VGA_SWITCHEROO_IGD;
-- 
2.39.1

[RFC PATCH 4/9] apple-gmux: refactor gmux types

[Orlando Chamberlain]

Add apple_gmux_config struct containing operations and data specific to
each mux type.

This is in preparation for adding a third, MMIO based, gmux type.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 91 ++++++++++++++++++++-----------
 include/linux/apple-gmux.h        | 18 ++++--
 2 files changed, 70 insertions(+), 39 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 6109f4c2867c..760434a527c1 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -5,6 +5,7 @@
  *  Copyright (C) Canonical Ltd. <seth.forshee@canonical.com>
  *  Copyright (C) 2010-2012 Andreas Heider <andreas@meetr.de>
  *  Copyright (C) 2015 Lukas Wunner <lukas@wunner.de>
+ *  Copyright (C) 2023 Orlando Chamberlain <orlandoch.dev@gmail.com>
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -43,10 +44,12 @@
  *     http://www.renesas.com/products/mpumcu/h8s/h8s2100/h8s2113/index.jsp
  */
 
+struct apple_gmux_config;
+
 struct apple_gmux_data {
 	unsigned long iostart;
 	unsigned long iolen;
-	bool indexed;
+	const struct apple_gmux_config *config;
 	struct mutex index_lock;
 
 	struct backlight_device *bdev;
@@ -64,6 +67,17 @@ struct apple_gmux_data {
 
 static struct apple_gmux_data *apple_gmux_data;
 
+struct apple_gmux_config {
+	u8 (*read8)(struct apple_gmux_data *gmux_data, int port);
+	void (*write8)(struct apple_gmux_data *gmux_data, int port, u8 val);
+	u32 (*read32)(struct apple_gmux_data *gmux_data, int port);
+	void (*write32)(struct apple_gmux_data *gmux_data, int port, u32 val);
+	const struct vga_switcheroo_handler *gmux_handler;
+	enum vga_switcheroo_handler_flags_t handler_flags;
+	unsigned long resource_type;
+	char *name;
+};
+
 #define GMUX_INTERRUPT_ENABLE		0xff
 #define GMUX_INTERRUPT_DISABLE		0x00
 
@@ -181,35 +195,23 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
 
 static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
 {
-	if (gmux_data->indexed)
-		return gmux_index_read8(gmux_data, port);
-	else
-		return gmux_pio_read8(gmux_data, port);
+	return gmux_data->config->read8(gmux_data, port);
 }
 
 static void gmux_write8(struct apple_gmux_data *gmux_data, int port, u8 val)
 {
-	if (gmux_data->indexed)
-		gmux_index_write8(gmux_data, port, val);
-	else
-		gmux_pio_write8(gmux_data, port, val);
+	return gmux_data->config->write8(gmux_data, port, val);
 }
 
 static u32 gmux_read32(struct apple_gmux_data *gmux_data, int port)
 {
-	if (gmux_data->indexed)
-		return gmux_index_read32(gmux_data, port);
-	else
-		return gmux_pio_read32(gmux_data, port);
+	return gmux_data->config->read32(gmux_data, port);
 }
 
 static void gmux_write32(struct apple_gmux_data *gmux_data, int port,
 			     u32 val)
 {
-	if (gmux_data->indexed)
-		gmux_index_write32(gmux_data, port, val);
-	else
-		gmux_pio_write32(gmux_data, port, val);
+	return gmux_data->config->write32(gmux_data, port, val);
 }
 
 /**
@@ -449,19 +451,41 @@ static enum vga_switcheroo_client_id gmux_get_client_id(struct pci_dev *pdev)
 		return VGA_SWITCHEROO_DIS;
 }
 
-static const struct vga_switcheroo_handler gmux_handler_indexed = {
+static const struct vga_switcheroo_handler gmux_handler_no_ddc = {
 	.switchto = gmux_switchto,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
 };
 
-static const struct vga_switcheroo_handler gmux_handler_classic = {
+static const struct vga_switcheroo_handler gmux_handler_ddc = {
 	.switchto = gmux_switchto,
 	.switch_ddc = gmux_switch_ddc,
 	.power_state = gmux_set_power_state,
 	.get_client_id = gmux_get_client_id,
 };
 
+static const struct apple_gmux_config apple_gmux_pio = {
+	.read8 = &gmux_pio_read8,
+	.write8 = &gmux_pio_write8,
+	.read32 = &gmux_pio_read32,
+	.write32 = &gmux_pio_write32,
+	.gmux_handler = &gmux_handler_ddc,
+	.handler_flags = VGA_SWITCHEROO_CAN_SWITCH_DDC,
+	.resource_type = IORESOURCE_IO,
+	.name = "classic"
+};
+
+static const struct apple_gmux_config apple_gmux_index = {
+	.read8 = &gmux_index_read8,
+	.write8 = &gmux_index_write8,
+	.read32 = &gmux_index_read32,
+	.write32 = &gmux_index_write32,
+	.gmux_handler = &gmux_handler_no_ddc,
+	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
+	.resource_type = IORESOURCE_IO,
+	.name = "indexed"
+};
+
 /**
  * DOC: Interrupt
  *
@@ -551,13 +575,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	int ret = -ENXIO;
 	acpi_status status;
 	unsigned long long gpe;
-	bool indexed = false;
+	enum apple_gmux_type type;
 	u32 version;
 
 	if (apple_gmux_data)
 		return -EBUSY;
 
-	if (!apple_gmux_detect(pnp, &indexed)) {
+	if (!apple_gmux_detect(pnp, &type)) {
 		pr_info("gmux device not present\n");
 		return -ENODEV;
 	}
@@ -567,6 +591,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		return -ENOMEM;
 	pnp_set_drvdata(pnp, gmux_data);
 
+	switch (type) {
+	case APPLE_GMUX_TYPE_INDEXED:
+		gmux_data->config = &apple_gmux_index;
+		mutex_init(&gmux_data->index_lock);
+		break;
+	case APPLE_GMUX_TYPE_PIO:
+		gmux_data->config = &apple_gmux_pio;
+		break;
+	}
+
 	res = pnp_get_resource(pnp, IORESOURCE_IO, 0);
 	gmux_data->iostart = res->start;
 	gmux_data->iolen = resource_size(res);
@@ -577,18 +611,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		goto err_free;
 	}
 
-	if (indexed) {
-		mutex_init(&gmux_data->index_lock);
-		gmux_data->indexed = true;
-	}
-
 	version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
 	ver_major = (version >> 24) & 0xff;
 	ver_minor = (version >> 16) & 0xff;
 	ver_release = (version >> 8) & 0xff;
 
 	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
-		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
+			ver_release, gmux_data->config->name);
 
 	memset(&props, 0, sizeof(props));
 	props.type = BACKLIGHT_PLATFORM;
@@ -678,12 +707,8 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	 *
 	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
 	 */
-	if (gmux_data->indexed)
-		ret = vga_switcheroo_register_handler(&gmux_handler_indexed,
-					      VGA_SWITCHEROO_NEEDS_EDP_CONFIG);
-	else
-		ret = vga_switcheroo_register_handler(&gmux_handler_classic,
-					      VGA_SWITCHEROO_CAN_SWITCH_DDC);
+	ret = vga_switcheroo_register_handler(gmux_data->config->gmux_handler,
+			gmux_data->config->handler_flags);
 	if (ret) {
 		pr_err("Failed to register vga_switcheroo handler\n");
 		goto err_register_handler;
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index eb2caee04abd..25c1de4a716e 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -36,6 +36,11 @@
 
 #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
 
+enum apple_gmux_type {
+	APPLE_GMUX_TYPE_PIO,
+	APPLE_GMUX_TYPE_INDEXED
+};
+
 #if IS_ENABLED(CONFIG_APPLE_GMUX)
 static inline bool apple_gmux_is_indexed(unsigned long iostart)
 {
@@ -65,12 +70,12 @@ static inline bool apple_gmux_is_indexed(unsigned long iostart)
  * Return: %true if a supported gmux ACPI device is detected and the kernel
  * was configured with CONFIG_APPLE_GMUX, %false otherwise.
  */
-static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
+static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_type *type_ret)
 {
 	struct device *dev = NULL;
 	struct acpi_device *adev;
 	struct resource *res;
-	bool indexed = false;
+	enum apple_gmux_type type = APPLE_GMUX_TYPE_PIO;
 	bool ret = false;
 
 	if (!pnp_dev) {
@@ -95,13 +100,14 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
 	 * device isn't present or that it's a new one that uses indexed io.
 	 */
 	if (!(~inl(res->start + GMUX_PORT_VERSION_MAJOR))) {
-		indexed = apple_gmux_is_indexed(res->start);
-		if (!indexed)
+		if (apple_gmux_is_indexed(res->start))
+			type = APPLE_GMUX_TYPE_INDEXED;
+		else
 			goto out;
 	}
 
-	if (indexed_ret)
-		*indexed_ret = indexed;
+	if (type_ret)
+		*type_ret = type;
 
 	ret = true;
 out:
-- 
2.39.1

[RFC PATCH 5/9] apple-gmux: Use GMSP acpi method for interrupt clear

[Orlando Chamberlain]

This is needed for interrupts to be cleared correctly on MMIO based
gmux's. It is untested if this helps/hinders other gmux types, but I
have seen the GMSP method in the acpi tables of a MacBook with an
indexed gmux.

If this turns out to break support for older gmux's, this can instead
be only done on MMIO gmux's.

There is also a "GMLV" acpi method, and the "GMSP" method can be called
with 1 as its argument, but the purposes of these aren't known and they
don't seem to be needed.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index 760434a527c1..c605f036ea0b 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -494,8 +494,29 @@ static const struct apple_gmux_config apple_gmux_index = {
  * MCP79, on all following generations it's GPIO pin 6 of the Intel PCH.
  * The GPE merely signals that an interrupt occurred, the actual type of event
  * is identified by reading a gmux register.
+ *
+ * On MMIO gmux's, we also need to call the acpi method GMSP to properly clear
+ * interrupts. TODO: Do other types need this? Does this break other types?
  */
 
+static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data, int arg)
+{
+	acpi_status status = AE_OK;
+	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
+	struct acpi_object_list arg_list = { 1, &arg0 };
+
+	arg0.integer.value = arg;
+
+	status = acpi_evaluate_object(gmux_data->dhandle, "GMSP", &arg_list, NULL);
+	if (ACPI_FAILURE(status)) {
+		pr_err("GMSP call failed: %s\n",
+		       acpi_format_exception(status));
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
 {
 	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
@@ -519,7 +540,10 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
 
 	/* to clear interrupts write back current status */
 	status = gmux_interrupt_get_status(gmux_data);
-	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
+	if (status) {
+		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
+		gmux_call_acpi_gmsp(gmux_data, 0);
+	}
 }
 
 static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
-- 
2.39.1

[RFC PATCH 6/9] apple-gmux: support MMIO gmux on T2 Macs

[Orlando Chamberlain]

In some newer dual gpu MacBooks, gmux is controlled by the T2 security
chip, and acessed with MMIO. Add support for these gmux controllers

Interestingly, the ACPI table only allocates 8 bytes for GMUX, but we
actually need 16, and as such we request 16 with request_mem_region.

Reading and writing from ports:
    16 bytes from 0xfe0b0200 are used. 0x0 to 0x4 are where data
    to read appears, and where data to write goes. Writing to 0xe
    sets the gmux port being accessed, and writing to 0xf sends commands.

    These commands are 0x40 & data_length for write, and data_length for
    read, where data_length is 1, 2 or 4. Once byte base+0xf is 0, the
    command is done.

Interrupts:
    Clearing interrupts on T2 macs seems different to older models,
    0 has to be written for them to stop, but this isn't what macOS
    does and further investigation may be needed.

Issues:
    As with other retina models, we can't switch DDC lines so
    switching at runtime doesn't work if the inactive gpu driver
    already disabled eDP due to it not being connected when that
    driver loaded.

    Additionally, turning on the dgpu back on T2 macs doesn't work,
    and it fails in amdgpu's code for bringing the gpu back online.

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 134 ++++++++++++++++++++++++++++--
 include/linux/apple-gmux.h        |  34 +++++---
 2 files changed, 149 insertions(+), 19 deletions(-)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index c605f036ea0b..c38d6ef0c15a 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -28,15 +28,17 @@
  * DOC: Overview
  *
  * gmux is a microcontroller built into the MacBook Pro to support dual GPUs:
- * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on retinas.
+ * A `Lattice XP2`_ on pre-retinas, a `Renesas R4F2113`_ on pre-T2 retinas.
+ * The chip used on T2 Macs is not known.
  *
  * (The MacPro6,1 2013 also has a gmux, however it is unclear why since it has
  * dual GPUs but no built-in display.)
  *
  * gmux is connected to the LPC bus of the southbridge. Its I/O ports are
  * accessed differently depending on the microcontroller: Driver functions
- * to access a pre-retina gmux are infixed ``_pio_``, those for a retina gmux
- * are infixed ``_index_``.
+ * to access a pre-retina gmux are infixed ``_pio_``, those for a pre-T2
+ * retina gmux are infixed ``_index_``, and those on T2 Macs are infixed
+ * with ``_mmio_``.
  *
  * .. _Lattice XP2:
  *     http://www.latticesemi.com/en/Products/FPGAandCPLD/LatticeXP2.aspx
@@ -47,6 +49,7 @@
 struct apple_gmux_config;
 
 struct apple_gmux_data {
+	u8 *__iomem iomem_base;
 	unsigned long iostart;
 	unsigned long iolen;
 	const struct apple_gmux_config *config;
@@ -193,6 +196,79 @@ static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
 	mutex_unlock(&gmux_data->index_lock);
 }
 
+static int gmux_mmio_wait(struct apple_gmux_data *gmux_data)
+{
+	int i = 200;
+	u8 gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+
+	while (i && gwr) {
+		gwr = ioread8(gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+		udelay(100);
+		i--;
+	}
+
+	return !!i;
+}
+
+static u8 gmux_mmio_read8(struct apple_gmux_data *gmux_data, int port)
+{
+	u8 val;
+
+	mutex_lock(&gmux_data->index_lock);
+	gmux_mmio_wait(gmux_data);
+	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_READ | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	gmux_mmio_wait(gmux_data);
+	val = ioread8(gmux_data->iomem_base);
+	mutex_unlock(&gmux_data->index_lock);
+
+	return val;
+}
+
+static void gmux_mmio_write8(struct apple_gmux_data *gmux_data, int port,
+			      u8 val)
+{
+	mutex_lock(&gmux_data->index_lock);
+	gmux_mmio_wait(gmux_data);
+	iowrite8(val, gmux_data->iomem_base);
+
+	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+
+	gmux_mmio_wait(gmux_data);
+	mutex_unlock(&gmux_data->index_lock);
+}
+
+static u32 gmux_mmio_read32(struct apple_gmux_data *gmux_data, int port)
+{
+	u32 val;
+
+	mutex_lock(&gmux_data->index_lock);
+	gmux_mmio_wait(gmux_data);
+	iowrite8((port & 0xff), gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_READ | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	gmux_mmio_wait(gmux_data);
+	val = be32_to_cpu(ioread32(gmux_data->iomem_base));
+	mutex_unlock(&gmux_data->index_lock);
+
+	return val;
+}
+
+static void gmux_mmio_write32(struct apple_gmux_data *gmux_data, int port,
+			       u32 val)
+{
+	mutex_lock(&gmux_data->index_lock);
+	iowrite32(cpu_to_be32(val), gmux_data->iomem_base);
+	iowrite8(port & 0xff, gmux_data->iomem_base + GMUX_MMIO_PORT_SELECT);
+	iowrite8(GMUX_MMIO_WRITE | sizeof(val),
+		gmux_data->iomem_base + GMUX_MMIO_COMMAND_SEND);
+	gmux_mmio_wait(gmux_data);
+	mutex_unlock(&gmux_data->index_lock);
+}
+
 static u8 gmux_read8(struct apple_gmux_data *gmux_data, int port)
 {
 	return gmux_data->config->read8(gmux_data, port);
@@ -486,6 +562,18 @@ static const struct apple_gmux_config apple_gmux_index = {
 	.name = "indexed"
 };
 
+static const struct apple_gmux_config apple_gmux_mmio = {
+	.read8 = &gmux_mmio_read8,
+	.write8 = &gmux_mmio_write8,
+	.read32 = &gmux_mmio_read32,
+	.write32 = &gmux_mmio_write32,
+	.gmux_handler = &gmux_handler_no_ddc,
+	.handler_flags = VGA_SWITCHEROO_NEEDS_EDP_CONFIG,
+	.resource_type = IORESOURCE_MEM,
+	.name = "T2"
+};
+
+
 /**
  * DOC: Interrupt
  *
@@ -538,7 +626,7 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
 {
 	u8 status;
 
-	/* to clear interrupts write back current status */
+	/* to clear interrupts write back current status. */
 	status = gmux_interrupt_get_status(gmux_data);
 	if (status) {
 		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
@@ -616,6 +704,25 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	pnp_set_drvdata(pnp, gmux_data);
 
 	switch (type) {
+	case APPLE_GMUX_TYPE_MMIO:
+		gmux_data->config = &apple_gmux_mmio;
+		mutex_init(&gmux_data->index_lock);
+
+		res = pnp_get_resource(pnp, IORESOURCE_MEM, 0);
+		gmux_data->iostart = res->start;
+		/* Although the ACPI table only allocates 8 bytes, we need 16. */
+		gmux_data->iolen = 16;
+		if (!request_mem_region(gmux_data->iostart, gmux_data->iolen,
+					"Apple gmux")) {
+			pr_err("gmux I/O already in use\n");
+			goto err_free;
+		}
+		gmux_data->iomem_base = ioremap(gmux_data->iostart, gmux_data->iolen);
+		if (!gmux_data->iomem_base) {
+			pr_err("couldn't remap gmux mmio region");
+			goto err_release;
+		}
+		goto get_version;
 	case APPLE_GMUX_TYPE_INDEXED:
 		gmux_data->config = &apple_gmux_index;
 		mutex_init(&gmux_data->index_lock);
@@ -635,6 +742,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		goto err_free;
 	}
 
+get_version:
 	version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
 	ver_major = (version >> 24) & 0xff;
 	ver_minor = (version >> 16) & 0xff;
@@ -660,7 +768,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 					 gmux_data, &gmux_bl_ops, &props);
 	if (IS_ERR(bdev)) {
 		ret = PTR_ERR(bdev);
-		goto err_release;
+		goto err_unmap;
 	}
 
 	gmux_data->bdev = bdev;
@@ -727,7 +835,7 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 	/*
 	 * Retina MacBook Pros cannot switch the panel's AUX separately
 	 * and need eDP pre-calibration. They are distinguishable from
-	 * pre-retinas by having an "indexed" gmux.
+	 * pre-retinas by having an "indexed" or "T2" gmux.
 	 *
 	 * Pre-retina MacBook Pros can switch the panel's DDC separately.
 	 */
@@ -752,8 +860,14 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 					   &gmux_notify_handler);
 err_notify:
 	backlight_device_unregister(bdev);
+err_unmap:
+	if (gmux_data->iomem_base)
+		iounmap(gmux_data->iomem_base);
 err_release:
-	release_region(gmux_data->iostart, gmux_data->iolen);
+	if (gmux_data->config->resource_type == IORESOURCE_MEM)
+		release_mem_region(gmux_data->iostart, gmux_data->iolen);
+	else
+		release_region(gmux_data->iostart, gmux_data->iolen);
 err_free:
 	kfree(gmux_data);
 	return ret;
@@ -774,7 +888,11 @@ static void gmux_remove(struct pnp_dev *pnp)
 
 	backlight_device_unregister(gmux_data->bdev);
 
-	release_region(gmux_data->iostart, gmux_data->iolen);
+	if (gmux_data->iomem_base) {
+		iounmap(gmux_data->iomem_base);
+		release_mem_region(gmux_data->iostart, gmux_data->iolen);
+	} else
+		release_region(gmux_data->iostart, gmux_data->iolen);
 	apple_gmux_data = NULL;
 	kfree(gmux_data);
 
diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
index 25c1de4a716e..55b18f0f320d 100644
--- a/include/linux/apple-gmux.h
+++ b/include/linux/apple-gmux.h
@@ -34,11 +34,18 @@
 #define GMUX_PORT_READ			0xd0
 #define GMUX_PORT_WRITE			0xd4
 
+#define GMUX_MMIO_PORT_SELECT		0x0e
+#define GMUX_MMIO_COMMAND_SEND		0x0f
+
+#define GMUX_MMIO_READ			0x00
+#define GMUX_MMIO_WRITE			0x40
+
 #define GMUX_MIN_IO_LEN			(GMUX_PORT_BRIGHTNESS + 4)
 
 enum apple_gmux_type {
 	APPLE_GMUX_TYPE_PIO,
-	APPLE_GMUX_TYPE_INDEXED
+	APPLE_GMUX_TYPE_INDEXED,
+	APPLE_GMUX_TYPE_MMIO
 };
 
 #if IS_ENABLED(CONFIG_APPLE_GMUX)
@@ -92,16 +99,21 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, enum apple_gmux_ty
 	}
 
 	res = pnp_get_resource(pnp_dev, IORESOURCE_IO, 0);
-	if (!res || resource_size(res) < GMUX_MIN_IO_LEN)
-		goto out;
-
-	/*
-	 * Invalid version information may indicate either that the gmux
-	 * device isn't present or that it's a new one that uses indexed io.
-	 */
-	if (!(~inl(res->start + GMUX_PORT_VERSION_MAJOR))) {
-		if (apple_gmux_is_indexed(res->start))
-			type = APPLE_GMUX_TYPE_INDEXED;
+	if (res && resource_size(res) >= GMUX_MIN_IO_LEN) {
+		/*
+		 * Invalid version information may indicate either that the gmux
+		 * device isn't present or that it's a new one that uses indexed io.
+		 */
+		if (!(~inl(res->start + GMUX_PORT_VERSION_MAJOR))) {
+			if (apple_gmux_is_indexed(res->start))
+				type = APPLE_GMUX_TYPE_INDEXED;
+			else
+				goto out;
+		}
+	} else {
+		res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
+		if (res)
+			type = APPLE_GMUX_TYPE_MMIO;
 		else
 			goto out;
 	}
-- 
2.39.1

[RFC PATCH 7/9] apple-gmux: add sysfs interface

[Orlando Chamberlain]

Allow reading gmux ports from userspace. When the unsafe module
parameter allow_user_writes is true, writing 1 byte
values is also allowed.

For example:

cd /sys/bus/acpi/devices/APP000B:00/physical_node/
echo 4 > gmux_selected_port
cat gmux_selected_port_data | xxd -p

Will show the gmux version information (00000005 in this case)

Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/platform/x86/apple-gmux.c | 129 ++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
index c38d6ef0c15a..756059d48393 100644
--- a/drivers/platform/x86/apple-gmux.c
+++ b/drivers/platform/x86/apple-gmux.c
@@ -66,6 +66,11 @@ struct apple_gmux_data {
 	enum vga_switcheroo_client_id switch_state_external;
 	enum vga_switcheroo_state power_state;
 	struct completion powerchange_done;
+
+#ifdef CONFIG_SYSFS
+	/* sysfs data */
+	int selected_port;
+#endif /* CONFIG_SYSFS */
 };
 
 static struct apple_gmux_data *apple_gmux_data;
@@ -651,6 +656,121 @@ static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
 		complete(&gmux_data->powerchange_done);
 }
 
+/**
+ * DOC: Sysfs Interface
+ *
+ * gmux ports can be read from userspace as a sysfs interface. For example:
+ *
+ * # echo 4 > /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port
+ * # cat /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port_data | xxd -p
+ * 00000005
+ *
+ * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
+ *
+ * Single byte writes are also supported, however this must be enabled with the
+ * unsafe allow_user_writes module parameter.
+ *
+ */
+
+#ifdef CONFIG_SYSFS
+
+static bool allow_user_writes;
+module_param_unsafe(allow_user_writes, bool, 0);
+MODULE_PARM_DESC(allow_user_writes, "Allow userspace to write to gmux ports (default: false) (bool)");
+
+static ssize_t gmux_selected_port_store(struct device *dev,
+		struct device_attribute *attr, const char *sysfsbuf, size_t count)
+{
+	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
+	u8 port;
+
+	if (kstrtou8(sysfsbuf, 10, &port) < 0)
+		return -EINVAL;
+
+	/* On pio gmux's, make sure the user doesn't access too high of a port. */
+	if ((gmux_data->config == &apple_gmux_pio) &&
+		port > (gmux_data->iolen - 4))
+		return -EINVAL;
+
+	gmux_data->selected_port = port;
+	return count;
+}
+
+static ssize_t gmux_selected_port_show(struct device *dev,
+		struct device_attribute *attr, char *sysfsbuf)
+{
+	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
+
+	return sysfs_emit(sysfsbuf, "%d\n", gmux_data->selected_port);
+}
+
+DEVICE_ATTR_RW(gmux_selected_port);
+
+static ssize_t gmux_selected_port_data_store(struct device *dev,
+		struct device_attribute *attr, const char *sysfsbuf, size_t count)
+{
+	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
+
+	if (count == 1)
+		gmux_write8(gmux_data, gmux_data->selected_port, *sysfsbuf);
+	else
+		return -EINVAL;
+
+	return count;
+}
+
+static ssize_t gmux_selected_port_data_show(struct device *dev,
+		struct device_attribute *attr, char *sysfsbuf)
+{
+	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
+	u32 data;
+
+	data = gmux_read32(gmux_data, gmux_data->selected_port);
+	memcpy(sysfsbuf, &data, sizeof(data));
+
+	return sizeof(data);
+}
+
+struct device_attribute dev_attr_gmux_selected_port_data_rw = __ATTR_RW(gmux_selected_port_data);
+struct device_attribute dev_attr_gmux_selected_port_data_ro = __ATTR_RO(gmux_selected_port_data);
+
+static int gmux_init_sysfs(struct pnp_dev *pnp)
+{
+	int ret;
+
+	ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port);
+	if (ret)
+		return ret;
+	if (allow_user_writes)
+		ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port_data_rw);
+	else
+		ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port_data_ro);
+	if (ret)
+		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port);
+	return ret;
+}
+
+static void gmux_fini_sysfs(struct pnp_dev *pnp)
+{
+	device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port);
+	if (allow_user_writes)
+		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port_data_rw);
+	else
+		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port_data_ro);
+}
+
+#else
+
+static int gmux_init_sysfs(struct pnp_dev *pnp)
+{
+	return 0;
+}
+static void gmux_fini_sysfs(struct pnp_dev *pnp)
+{
+}
+
+#endif /* CONFIG_SYSFS */
+
 static int gmux_suspend(struct device *dev)
 {
 	struct pnp_dev *pnp = to_pnp_dev(dev);
@@ -846,8 +966,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
 		goto err_register_handler;
 	}
 
+	ret = gmux_init_sysfs(pnp);
+	if (ret) {
+		pr_err("Failed to register gmux sysfs entries\n");
+		goto err_sysfs;
+	}
+
 	return 0;
 
+err_sysfs:
+	vga_switcheroo_unregister_handler();
 err_register_handler:
 	gmux_disable_interrupts(gmux_data);
 	apple_gmux_data = NULL;
@@ -877,6 +1005,7 @@ static void gmux_remove(struct pnp_dev *pnp)
 {
 	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
 
+	gmux_fini_sysfs(pnp);
 	vga_switcheroo_unregister_handler();
 	gmux_disable_interrupts(gmux_data);
 	if (gmux_data->gpe >= 0) {
-- 
2.39.1

[RFC PATCH 8/9] hda/hdmi: Register with vga_switcheroo on Dual GPU Macbooks

[Orlando Chamberlain]

Commit 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for
AMD") caused only AMD gpu's with PX to have their audio component register
with vga_switcheroo. This meant that Apple Macbooks with apple-gmux as the
gpu switcher no longer had the audio client registering, so when the gpu is
powered off by vga_switcheroo snd_hda_intel is unaware that it should have
suspended the device:

amdgpu: switched off
snd_hda_intel 0000:03:00.1:
    Unable to change power state from D3hot to D0, device inaccessible
snd_hda_intel 0000:03:00.1: CORB reset timeout#2, CORBRP = 65535

Simialar to ATPX, we use the presence of an acpi method (PWRD in this
case) to ensure we only register with the correct devices.

Fixes: 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD")
Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 sound/pci/hda/hda_intel.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 87002670c0c9..c97bbe60e603 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1435,11 +1435,25 @@ static bool atpx_present(void)
 	}
 	return false;
 }
+
+static bool pwrd_present(struct pci_dev *pci)
+{
+	acpi_handle pwrd_handle;
+	acpi_status status;
+
+	status = acpi_get_handle(ACPI_HANDLE(&pci->dev), "PWRD", &pwrd_handle);
+	return ACPI_FAILURE(status) ? false : true;
+}
 #else
 static bool atpx_present(void)
 {
 	return false;
 }
+
+static bool pwrd_present(struct pci_dev *pci)
+{
+	return false;
+}
 #endif
 
 /*
@@ -1461,9 +1475,12 @@ static struct pci_dev *get_bound_vga(struct pci_dev *pci)
 				 * rather than the dGPU's namespace. However,
 				 * the dGPU is the one who is involved in
 				 * vgaswitcheroo.
+				 *
+				 * PWRD is in the dGPU's ACPI namespace on Apple
+				 * Macbooks with dual gpu's.
 				 */
 				if (((p->class >> 16) == PCI_BASE_CLASS_DISPLAY) &&
-				    atpx_present())
+						(atpx_present() || pwrd_present(p)))
 					return p;
 				pci_dev_put(p);
 			}
-- 
2.39.1

[RFC PATCH 9/9] drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached

[Orlando Chamberlain]

From: Kerem Karabay <kekrby@gmail.com>

Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
vga_switcheroo") made amdgpu only register a vga_switcheroo client for
GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
register, but don't have PX. Instead of AMD's PX, they use apple-gmux.

Revert to the old logic of registering for all non-thunderbolt gpus,
like radeon and nouveau.

Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
Signed-off-by: Kerem Karabay <kekrby@gmail.com>
[Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 2f28a8c02f64..0bb553a61552 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
 
-	if (amdgpu_device_supports_px(ddev)) {
-		px = true;
-		vga_switcheroo_register_client(adev->pdev,
-					       &amdgpu_switcheroo_ops, px);
+	px = amdgpu_device_supports_px(ddev);
+
+	if (!pci_is_thunderbolt_attached(adev->pdev))
+		vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
+
+	if (px)
 		vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
-	}
 
 	if (adev->gmc.xgmi.pending_reset)
 		queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
@@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 
 	kfree(adev->bios);
 	adev->bios = NULL;
-	if (amdgpu_device_supports_px(adev_to_drm(adev))) {
+
+	if (!pci_is_thunderbolt_attached(adev->pdev))
 		vga_switcheroo_unregister_client(adev->pdev);
+
+	if (amdgpu_device_supports_px(adev_to_drm(adev)))
 		vga_switcheroo_fini_domain_pm_ops(adev->dev);
-	}
+
 	if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
 		vga_client_unregister(adev->pdev);
 
-- 
2.39.1

Re: [RFC PATCH 9/9] drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached

[Alex Deucher]

On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
<orlandoch.dev@gmail.com> wrote:
>
> From: Kerem Karabay <kekrby@gmail.com>
>
> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> vga_switcheroo") made amdgpu only register a vga_switcheroo client for
> GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
> register, but don't have PX. Instead of AMD's PX, they use apple-gmux.

Is there a way to detect apple-gmux instead?  Otherwise, we register
vga_switcheroo on any system with multiple GPUs which is not what we
want.

Alex

>
> Revert to the old logic of registering for all non-thunderbolt gpus,
> like radeon and nouveau.
>
> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 2f28a8c02f64..0bb553a61552 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>                 vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
>
> -       if (amdgpu_device_supports_px(ddev)) {
> -               px = true;
> -               vga_switcheroo_register_client(adev->pdev,
> -                                              &amdgpu_switcheroo_ops, px);
> +       px = amdgpu_device_supports_px(ddev);
> +
> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> +               vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
> +
> +       if (px)
>                 vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
> -       }
>
>         if (adev->gmc.xgmi.pending_reset)
>                 queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
> @@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>
>         kfree(adev->bios);
>         adev->bios = NULL;
> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
> +
> +       if (!pci_is_thunderbolt_attached(adev->pdev))
>                 vga_switcheroo_unregister_client(adev->pdev);
> +
> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
> -       }
> +
>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>                 vga_client_unregister(adev->pdev);
>
> --
> 2.39.1
>

Re: [RFC PATCH 9/9] drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached

[Hans de Goede]

Hi,

On 2/10/23 16:53, Alex Deucher wrote:
> On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
> <orlandoch.dev@gmail.com> wrote:
>>
>> From: Kerem Karabay <kekrby@gmail.com>
>>
>> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
>> vga_switcheroo") made amdgpu only register a vga_switcheroo client for
>> GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
>> register, but don't have PX. Instead of AMD's PX, they use apple-gmux.
> 
> Is there a way to detect apple-gmux instead?  Otherwise, we register
> vga_switcheroo on any system with multiple GPUs which is not what we
> want.

Yes since 6.1.y (either stable series or just take 6.2.0) the apple-gmux
detect code has been factored out into a stand-alone
apple_gmux_detect() helper inside:

include/linux/apple-gmux.h

For usage outside of the actual apple-gmux driver you can simply
pass NULL for both arguments.

This was necessary to reliably check if the apple-gmux should be
used for backlight control.

Note there also is the older apple_gmux_present() helper, which is
already used in some drm code. That function is not reliable though
it detects if the ACPI tables contain an ACPI device describing
the presence of a gmux, but it turns out even Apple has buggy ACPI
tables and the mere presence of that ACPI device is not a reliable
indicator the gmux is actually there.

I have not changed over any of the existing apple_gmux_present()
users for fear of unwanted side effects...

Regards,

Hans




>> Revert to the old logic of registering for all non-thunderbolt gpus,
>> like radeon and nouveau.
>>
>> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
>> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
>> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
>> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 2f28a8c02f64..0bb553a61552 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>>                 vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
>>
>> -       if (amdgpu_device_supports_px(ddev)) {
>> -               px = true;
>> -               vga_switcheroo_register_client(adev->pdev,
>> -                                              &amdgpu_switcheroo_ops, px);
>> +       px = amdgpu_device_supports_px(ddev);
>> +
>> +       if (!pci_is_thunderbolt_attached(adev->pdev))
>> +               vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
>> +
>> +       if (px)
>>                 vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
>> -       }
>>
>>         if (adev->gmc.xgmi.pending_reset)
>>                 queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
>> @@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>>
>>         kfree(adev->bios);
>>         adev->bios = NULL;
>> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
>> +
>> +       if (!pci_is_thunderbolt_attached(adev->pdev))
>>                 vga_switcheroo_unregister_client(adev->pdev);
>> +
>> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
>>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
>> -       }
>> +
>>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
>>                 vga_client_unregister(adev->pdev);
>>
>> --
>> 2.39.1
>>
> 

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

[Alex Deucher]

On Fri, Feb 10, 2023 at 3:04 AM 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]).
>
> It's only been tested by people on T2 Macs with MMIO based gmux's using
> t2linux [2] kernels, but some changes may impact older port io and indexed
> gmux's so testing, especially on those older Macbooks, would be
> appreciated.
>
> # 1-2:
>
> refactor code to make it easier to add the 3rd gmux type.
>
> # 3:
>
> has a slight change in how the switch state is read, I don't
> expect this to cause issues for older models (but still, please test if
> you have one!)
>
> # 4:
>
> implements a system to support more than 2 gmux types
>
> # 5:
>
> start using the gmux's GMSP acpi method when handling interrupts. This
> is needed for the MMIO gmux's, and its present in the acpi tables of some
> indexed gmux's I could find so hopefully enabling this for all models
> will be fine, but if not it can be only used on MMIO gmux's.
>
> # 6:
>
> Adds support for the MMIO based gmux on T2 macs.
>
> # 7:
>
> Add a sysfs interface to apple-gmux so data from ports can be read
> from userspace, and written to if the user enables an unsafe kernel
> parameter.
>
> 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).
>
> # 8-9:
>
> These patches make amdgpu and snd_hda_intel register with vga_switcheroo
> on Macbooks. I would like advice from the AMD folks on how they want
> this to work, so that both PX and apple-gmux laptops work properly.
>
> For radeon and nouveau we just register for every non-thunderbolt
> device, but this was changed for AMD cards in commit 3840c5bcc245
> ("drm/amdgpu: disentangle runtime pm and vga_switcheroo") and commit
> 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD").
>
> This meant that only gpu's with PX register. Commit #8 makes amdgpu
> register for all non-thinderbolt cards, and commit #9 makes snd_hda_intel
> register for all amd cards with the PWRD (mentioned below) acpi method.
> An alternative would be using apple-gmux-detect(), but that won't work
> after apple-gmux has probed and claimed its memory resources.
>
> # 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. 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:
>
> snd_hda_intel 0000:03:00.1: Disabling via vga_switcheroo
> snd_hda_intel 0000:03:00.1: Cannot lock devices!
> 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

That is likely the cause.  On non-Mac platforms, the power is
controlled via the PX ACPI interface (for old platforms) or standard
ACPI power resources on more recent platforms.  This is handled by the
ACPI core on these platforms (i.e., D3cold).

> 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 in `lspci -x` filled with
> 0xff. `echo 1 > /sys/bus/pci/rescan` fixes that and the dmesg errors about
> changing power state, but "PSP create ring failed" still happens, and
> the gpu doesn't resume properly.

All of those devices are part of the dGPU itself.  When the power is
cut to the dGPU, all of those devices will lose power.  If you are
reading all 1's from the PCI config space for any of those devices,
that is a good sign that the power is off to the GPU.

Alex

>
> [1]: https://lore.kernel.org/all/3AFB9142-2BD0-46F9-AEA9-C9C5D13E68E6@live.com/
> [2]: https://t2linux.org
> [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/
>
> Kerem Karabay (1):
>   drm/amdgpu: register a vga_switcheroo client for all GPUs that are not
>     thunderbolt attached
>
> Orlando Chamberlain (8):
>   apple-gmux: use cpu_to_be32 instead of manual reorder
>   apple-gmux: consolidate version reading
>   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 sysfs interface
>   hda/hdmi: Register with vga_switcheroo on Dual GPU Macbooks
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  18 +-
>  drivers/platform/x86/apple-gmux.c          | 416 +++++++++++++++++----
>  include/linux/apple-gmux.h                 |  50 ++-
>  sound/pci/hda/hda_intel.c                  |  19 +-
>  4 files changed, 409 insertions(+), 94 deletions(-)
>
> --
> 2.39.1
>

Re: [RFC PATCH 9/9] drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached

[Alex Deucher]

On Fri, Feb 10, 2023 at 11:07 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 2/10/23 16:53, Alex Deucher wrote:
> > On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
> > <orlandoch.dev@gmail.com> wrote:
> >>
> >> From: Kerem Karabay <kekrby@gmail.com>
> >>
> >> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> >> vga_switcheroo") made amdgpu only register a vga_switcheroo client for
> >> GPU's with PX, however AMD GPUs in dual gpu Apple Macbooks do need to
> >> register, but don't have PX. Instead of AMD's PX, they use apple-gmux.
> >
> > Is there a way to detect apple-gmux instead?  Otherwise, we register
> > vga_switcheroo on any system with multiple GPUs which is not what we
> > want.
>
> Yes since 6.1.y (either stable series or just take 6.2.0) the apple-gmux
> detect code has been factored out into a stand-alone
> apple_gmux_detect() helper inside:
>
> include/linux/apple-gmux.h
>
> For usage outside of the actual apple-gmux driver you can simply
> pass NULL for both arguments.
>
> This was necessary to reliably check if the apple-gmux should be
> used for backlight control.
>
> Note there also is the older apple_gmux_present() helper, which is
> already used in some drm code. That function is not reliable though
> it detects if the ACPI tables contain an ACPI device describing
> the presence of a gmux, but it turns out even Apple has buggy ACPI
> tables and the mere presence of that ACPI device is not a reliable
> indicator the gmux is actually there.
>
> I have not changed over any of the existing apple_gmux_present()
> users for fear of unwanted side effects...

Looks like we could maybe use the PWRD ACPI check like patch 8 does as well.

Alex

>
> Regards,
>
> Hans
>
>
>
>
> >> Revert to the old logic of registering for all non-thunderbolt gpus,
> >> like radeon and nouveau.
> >>
> >> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo")
> >> Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> >> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit description]
> >> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +++++++++++-------
> >>  1 file changed, 11 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 2f28a8c02f64..0bb553a61552 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -3919,12 +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> >>                 vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> >>
> >> -       if (amdgpu_device_supports_px(ddev)) {
> >> -               px = true;
> >> -               vga_switcheroo_register_client(adev->pdev,
> >> -                                              &amdgpu_switcheroo_ops, px);
> >> +       px = amdgpu_device_supports_px(ddev);
> >> +
> >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> >> +               vga_switcheroo_register_client(adev->pdev, &amdgpu_switcheroo_ops, px);
> >> +
> >> +       if (px)
> >>                 vga_switcheroo_init_domain_pm_ops(adev->dev, &adev->vga_pm_domain);
> >> -       }
> >>
> >>         if (adev->gmc.xgmi.pending_reset)
> >>                 queue_delayed_work(system_wq, &mgpu_info.delayed_reset_work,
> >> @@ -4048,10 +4049,13 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
> >>
> >>         kfree(adev->bios);
> >>         adev->bios = NULL;
> >> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
> >> +
> >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> >>                 vga_switcheroo_unregister_client(adev->pdev);
> >> +
> >> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
> >>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
> >> -       }
> >> +
> >>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> >>                 vga_client_unregister(adev->pdev);
> >>
> >> --
> >> 2.39.1
> >>
> >
>

Re: [RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[Hans de Goede]

Hi,

On 2/10/23 05:48, Orlando Chamberlain wrote:
> Currently it manually flips the byte order, but we can instead use
> cpu_to_be32(val) for this.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
>  drivers/platform/x86/apple-gmux.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 9333f82cfa8a..e8cb084cb81f 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -94,13 +94,7 @@ static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
>  static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
>  			     u32 val)
>  {
> -	int i;
> -	u8 tmpval;
> -
> -	for (i = 0; i < 4; i++) {
> -		tmpval = (val >> (i * 8)) & 0xff;
> -		outb(tmpval, gmux_data->iostart + port + i);
> -	}
> +	outl(cpu_to_be32(val), gmux_data->iostart + port);
>  }
>  
>  static int gmux_index_wait_ready(struct apple_gmux_data *gmux_data)

The ioport / indexed-ioport accessed apple_gmux-es likely are (part of?)
LPC bus devices . Looking at the bus level you are now changing 4 io
accesses with a size of 1 byte, to 1 32 bit io-access.

Depending on the decoding hw in the chip this may work fine,
or this may work not at all.

I realized that you have asked for more testing, but most surviving
macbooks from the older apple-gmux era appear to be models without
a discrete GPU (which are often the first thing to break) and thus
without a gmux.

Unless we get a bunch of testers to show up, which I doubt. I would
prefer slightly bigger / less pretty code and not change the functional
behavior of the driver on these older models.

Regards,

Hans



> @@ -177,16 +171,8 @@ static u32 gmux_index_read32(struct apple_gmux_data *gmux_data, int port)
>  static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>  			       u32 val)
>  {
> -	int i;
> -	u8 tmpval;
> -
>  	mutex_lock(&gmux_data->index_lock);
> -
> -	for (i = 0; i < 4; i++) {
> -		tmpval = (val >> (i * 8)) & 0xff;
> -		outb(tmpval, gmux_data->iostart + GMUX_PORT_VALUE + i);
> -	}
> -
> +	outl(cpu_to_be32(val), gmux_data->iostart + GMUX_PORT_VALUE);
>  	gmux_index_wait_ready(gmux_data);
>  	outb(port & 0xff, gmux_data->iostart + GMUX_PORT_WRITE);
>  	gmux_index_wait_complete(gmux_data);

Re: [RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[Hans de Goede]

Hi,

On 2/10/23 20:09, Hans de Goede wrote:
> Hi,
> 
> On 2/10/23 05:48, Orlando Chamberlain wrote:
>> Currently it manually flips the byte order, but we can instead use
>> cpu_to_be32(val) for this.
>>
>> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
>> ---
>>  drivers/platform/x86/apple-gmux.c | 18 ++----------------
>>  1 file changed, 2 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
>> index 9333f82cfa8a..e8cb084cb81f 100644
>> --- a/drivers/platform/x86/apple-gmux.c
>> +++ b/drivers/platform/x86/apple-gmux.c
>> @@ -94,13 +94,7 @@ static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
>>  static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
>>  			     u32 val)
>>  {
>> -	int i;
>> -	u8 tmpval;
>> -
>> -	for (i = 0; i < 4; i++) {
>> -		tmpval = (val >> (i * 8)) & 0xff;
>> -		outb(tmpval, gmux_data->iostart + port + i);
>> -	}
>> +	outl(cpu_to_be32(val), gmux_data->iostart + port);
>>  }
>>  
>>  static int gmux_index_wait_ready(struct apple_gmux_data *gmux_data)
> 
> The ioport / indexed-ioport accessed apple_gmux-es likely are (part of?)
> LPC bus devices . Looking at the bus level you are now changing 4 io
> accesses with a size of 1 byte, to 1 32 bit io-access.
> 
> Depending on the decoding hw in the chip this may work fine,
> or this may work not at all.
> 
> I realized that you have asked for more testing, but most surviving
> macbooks from the older apple-gmux era appear to be models without
> a discrete GPU (which are often the first thing to break) and thus
> without a gmux.
> 
> Unless we get a bunch of testers to show up, which I doubt. I would
> prefer slightly bigger / less pretty code and not change the functional
> behavior of the driver on these older models.

A quick follow up on this, I just noticed that only the pio_write32
is doing the one byte at a time thing:

static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
{
        return inl(gmux_data->iostart + port);
}

static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
                             u32 val)
{
        int i;
        u8 tmpval;

        for (i = 0; i < 4; i++) {
                tmpval = (val >> (i * 8)) & 0xff;
                outb(tmpval, gmux_data->iostart + port + i);
        }
}

And if you look closely gmux_pio_write32() is not swapping
the order to be32 at all, it is just taking the bytes
in little-endian memory order, starting with the first
(index 0) byte which is the least significant byte of
the value.

On x86 the original code is no different then doing:

static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
                             u32 val)
{
        u8 *data = (u8 *)&val;
        int i;

        for (i = 0; i < 4; i++)
                outb(data[i], gmux_data->iostart + port + i);
}

So yeah this patch is definitely wrong, it actually swaps
the byte order compared to the original code. Which becomes
clear when you look the weird difference between the read32 and
write32 functions after this patch.

Presumably there is a specific reason why gmux_pio_write32()
is not already doing a single outl(..., val) and byte-ordering
is not the reason.

Regards,

Hans



>> @@ -177,16 +171,8 @@ static u32 gmux_index_read32(struct apple_gmux_data *gmux_data, int port)
>>  static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>>  			       u32 val)
>>  {
>> -	int i;
>> -	u8 tmpval;
>> -
>>  	mutex_lock(&gmux_data->index_lock);
>> -
>> -	for (i = 0; i < 4; i++) {
>> -		tmpval = (val >> (i * 8)) & 0xff;
>> -		outb(tmpval, gmux_data->iostart + GMUX_PORT_VALUE + i);
>> -	}
>> -
>> +	outl(cpu_to_be32(val), gmux_data->iostart + GMUX_PORT_VALUE);
>>  	gmux_index_wait_ready(gmux_data);
>>  	outb(port & 0xff, gmux_data->iostart + GMUX_PORT_WRITE);
>>  	gmux_index_wait_complete(gmux_data);
> 

Re: [RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[Hans de Goede]

Hi,

On 2/10/23 20:09, Hans de Goede wrote:
> Hi,
> 
> On 2/10/23 05:48, Orlando Chamberlain wrote:
>> Currently it manually flips the byte order, but we can instead use
>> cpu_to_be32(val) for this.
>>
>> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
>> ---
>>  drivers/platform/x86/apple-gmux.c | 18 ++----------------
>>  1 file changed, 2 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
>> index 9333f82cfa8a..e8cb084cb81f 100644
>> --- a/drivers/platform/x86/apple-gmux.c
>> +++ b/drivers/platform/x86/apple-gmux.c
>> @@ -94,13 +94,7 @@ static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
>>  static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
>>  			     u32 val)
>>  {
>> -	int i;
>> -	u8 tmpval;
>> -
>> -	for (i = 0; i < 4; i++) {
>> -		tmpval = (val >> (i * 8)) & 0xff;
>> -		outb(tmpval, gmux_data->iostart + port + i);
>> -	}
>> +	outl(cpu_to_be32(val), gmux_data->iostart + port);
>>  }
>>  
>>  static int gmux_index_wait_ready(struct apple_gmux_data *gmux_data)
> 
> The ioport / indexed-ioport accessed apple_gmux-es likely are (part of?)
> LPC bus devices . Looking at the bus level you are now changing 4 io
> accesses with a size of 1 byte, to 1 32 bit io-access.

Correction to myself, re-reading the LPC specification, then
if I'm right and this is a LPC device then all IO in/out accesses
are always 1 byte accesses. Since the LPC bus only supports 16 / 32
bit accesses for DMA cycles.

So presumably the outl() would get split into 4 separate 8 bit
(port) IO accesses.

Regards,

Hans





>> @@ -177,16 +171,8 @@ static u32 gmux_index_read32(struct apple_gmux_data *gmux_data, int port)
>>  static void gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>>  			       u32 val)
>>  {
>> -	int i;
>> -	u8 tmpval;
>> -
>>  	mutex_lock(&gmux_data->index_lock);
>> -
>> -	for (i = 0; i < 4; i++) {
>> -		tmpval = (val >> (i * 8)) & 0xff;
>> -		outb(tmpval, gmux_data->iostart + GMUX_PORT_VALUE + i);
>> -	}
>> -
>> +	outl(cpu_to_be32(val), gmux_data->iostart + GMUX_PORT_VALUE);
>>  	gmux_index_wait_ready(gmux_data);
>>  	outb(port & 0xff, gmux_data->iostart + GMUX_PORT_WRITE);
>>  	gmux_index_wait_complete(gmux_data);
> 

Re: [RFC PATCH 2/9] apple-gmux: consolidate version reading

[Hans de Goede]

Hi,

On 2/10/23 05:48, Orlando Chamberlain wrote:
> Read gmux version in one go as 32 bits on both indexed and classic
> gmux's.
> 
> Classic gmux's used to read the version as
> 
> major = inb(base + 0x4);
> minor = inb(base + 0x5);
> release = inb(base + 0x6);
> 
> but this can instead be done the same way as indexed gmux's with
> gmux_read32(), so the same version reading code is used for classic
> and indexed gmux's (as well as mmio gmux's that will be added to this
> driver).
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
>  drivers/platform/x86/apple-gmux.c | 14 ++++++--------
>  include/linux/apple-gmux.h        |  6 +-----
>  2 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index e8cb084cb81f..67628104f31a 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -580,15 +580,13 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  	if (indexed) {
>  		mutex_init(&gmux_data->index_lock);
>  		gmux_data->indexed = true;
> -		version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
> -		ver_major = (version >> 24) & 0xff;
> -		ver_minor = (version >> 16) & 0xff;
> -		ver_release = (version >> 8) & 0xff;
> -	} else {
> -		ver_major = gmux_read8(gmux_data, GMUX_PORT_VERSION_MAJOR);
> -		ver_minor = gmux_read8(gmux_data, GMUX_PORT_VERSION_MINOR);
> -		ver_release = gmux_read8(gmux_data, GMUX_PORT_VERSION_RELEASE);
>  	}
> +
> +	version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
> +	ver_major = (version >> 24) & 0xff;
> +	ver_minor = (version >> 16) & 0xff;
> +	ver_release = (version >> 8) & 0xff;
> +
>  	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major, ver_minor,
>  		ver_release, (gmux_data->indexed ? "indexed" : "classic"));
>  

The problem with this is that there is nothing (no known register)
at address base + 7 and now you are reading from address base + 7
here where before the code was not, we have no idea how the hw
will respond to this.  This should be pretty innocent but still ...

> diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> index 1f68b49bcd68..eb2caee04abd 100644
> --- a/include/linux/apple-gmux.h
> +++ b/include/linux/apple-gmux.h
> @@ -67,7 +67,6 @@ static inline bool apple_gmux_is_indexed(unsigned long iostart)
>   */
>  static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
>  {
> -	u8 ver_major, ver_minor, ver_release;
>  	struct device *dev = NULL;
>  	struct acpi_device *adev;
>  	struct resource *res;
> @@ -95,10 +94,7 @@ static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool *indexed_ret)
>  	 * Invalid version information may indicate either that the gmux
>  	 * device isn't present or that it's a new one that uses indexed io.
>  	 */
> -	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> -	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> -	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> -	if (ver_major == 0xff && ver_minor == 0xff && ver_release == 0xff) {
> +	if (!(~inl(res->start + GMUX_PORT_VERSION_MAJOR))) {

Assuming we can get this tested well enough that I'm ok with the change in general
please write this as:

	if (inl(res->start + GMUX_PORT_VERSION_MAJOR) == 0xffffffff) {

Which I believe is what you are trying to achieve here ?

Regards,

Hans



>  		indexed = apple_gmux_is_indexed(res->start);
>  		if (!indexed)
>  			goto out;

Re: [RFC PATCH 5/9] apple-gmux: Use GMSP acpi method for interrupt clear

[Hans de Goede]

Hi,

On 2/10/23 05:48, Orlando Chamberlain wrote:
> This is needed for interrupts to be cleared correctly on MMIO based
> gmux's. It is untested if this helps/hinders other gmux types, but I
> have seen the GMSP method in the acpi tables of a MacBook with an
> indexed gmux.
> 
> If this turns out to break support for older gmux's, this can instead
> be only done on MMIO gmux's.
> 
> There is also a "GMLV" acpi method, and the "GMSP" method can be called
> with 1 as its argument, but the purposes of these aren't known and they
> don't seem to be needed.
> 
> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
>  drivers/platform/x86/apple-gmux.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index 760434a527c1..c605f036ea0b 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -494,8 +494,29 @@ static const struct apple_gmux_config apple_gmux_index = {
>   * MCP79, on all following generations it's GPIO pin 6 of the Intel PCH.
>   * The GPE merely signals that an interrupt occurred, the actual type of event
>   * is identified by reading a gmux register.
> + *
> + * On MMIO gmux's, we also need to call the acpi method GMSP to properly clear
> + * interrupts. TODO: Do other types need this? Does this break other types?
>   */
>  
> +static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data, int arg)
> +{
> +	acpi_status status = AE_OK;
> +	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> +	struct acpi_object_list arg_list = { 1, &arg0 };
> +
> +	arg0.integer.value = arg;
> +
> +	status = acpi_evaluate_object(gmux_data->dhandle, "GMSP", &arg_list, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("GMSP call failed: %s\n",
> +		       acpi_format_exception(status));
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
>  static inline void gmux_disable_interrupts(struct apple_gmux_data *gmux_data)
>  {
>  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
> @@ -519,7 +540,10 @@ static void gmux_clear_interrupts(struct apple_gmux_data *gmux_data)
>  
>  	/* to clear interrupts write back current status */
>  	status = gmux_interrupt_get_status(gmux_data);
> -	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +	if (status) {
> +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> +		gmux_call_acpi_gmsp(gmux_data, 0);

Ugh no, please don't go around calling random ACPI methods from untested
firmware revisions / device models.

ACPI code (even Apple's I have learned) tends to be full of bugs. If we
did not need to call GMSP before then please lets keep not calling it
on the older models. Just because it is there does not mean that calling
it is useful, it might even be harmful.

Regards,

Hans






> +	}
>  }
>  
>  static void gmux_notify_handler(acpi_handle device, u32 value, void *context)

Re: [RFC PATCH 7/9] apple-gmux: add sysfs interface

[Hans de Goede]

Hi,

On 2/10/23 05:48, Orlando Chamberlain wrote:
> Allow reading gmux ports from userspace. When the unsafe module
> parameter allow_user_writes is true, writing 1 byte
> values is also allowed.
> 
> For example:
> 
> cd /sys/bus/acpi/devices/APP000B:00/physical_node/
> echo 4 > gmux_selected_port
> cat gmux_selected_port_data | xxd -p
> 
> Will show the gmux version information (00000005 in this case)

Please use debugfs for this and as part of the conversion
drop the #ifdef-s (debugfs has stubs for when not enabled)
and drop all the error checking of creating the files, debugfs
is deliberately designed to not have any error checking in
the setup / teardown code.

This also removes the need for the allow_user_writes parameter
replacing it with the new kernel lockdown mechanism. debugfs
will automatically block access to writable files when
the kernel is in lockdown mode.

Regards,

Hans




> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> ---
>  drivers/platform/x86/apple-gmux.c | 129 ++++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
> 
> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> index c38d6ef0c15a..756059d48393 100644
> --- a/drivers/platform/x86/apple-gmux.c
> +++ b/drivers/platform/x86/apple-gmux.c
> @@ -66,6 +66,11 @@ struct apple_gmux_data {
>  	enum vga_switcheroo_client_id switch_state_external;
>  	enum vga_switcheroo_state power_state;
>  	struct completion powerchange_done;
> +
> +#ifdef CONFIG_SYSFS
> +	/* sysfs data */
> +	int selected_port;
> +#endif /* CONFIG_SYSFS */
>  };
>  
>  static struct apple_gmux_data *apple_gmux_data;
> @@ -651,6 +656,121 @@ static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
>  		complete(&gmux_data->powerchange_done);
>  }
>  
> +/**
> + * DOC: Sysfs Interface
> + *
> + * gmux ports can be read from userspace as a sysfs interface. For example:
> + *
> + * # echo 4 > /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port
> + * # cat /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port_data | xxd -p
> + * 00000005
> + *
> + * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
> + *
> + * Single byte writes are also supported, however this must be enabled with the
> + * unsafe allow_user_writes module parameter.
> + *
> + */
> +
> +#ifdef CONFIG_SYSFS
> +
> +static bool allow_user_writes;
> +module_param_unsafe(allow_user_writes, bool, 0);
> +MODULE_PARM_DESC(allow_user_writes, "Allow userspace to write to gmux ports (default: false) (bool)");
> +
> +static ssize_t gmux_selected_port_store(struct device *dev,
> +		struct device_attribute *attr, const char *sysfsbuf, size_t count)
> +{
> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> +	u8 port;
> +
> +	if (kstrtou8(sysfsbuf, 10, &port) < 0)
> +		return -EINVAL;
> +
> +	/* On pio gmux's, make sure the user doesn't access too high of a port. */
> +	if ((gmux_data->config == &apple_gmux_pio) &&
> +		port > (gmux_data->iolen - 4))
> +		return -EINVAL;
> +
> +	gmux_data->selected_port = port;
> +	return count;
> +}
> +
> +static ssize_t gmux_selected_port_show(struct device *dev,
> +		struct device_attribute *attr, char *sysfsbuf)
> +{
> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(sysfsbuf, "%d\n", gmux_data->selected_port);
> +}
> +
> +DEVICE_ATTR_RW(gmux_selected_port);
> +
> +static ssize_t gmux_selected_port_data_store(struct device *dev,
> +		struct device_attribute *attr, const char *sysfsbuf, size_t count)
> +{
> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> +
> +	if (count == 1)
> +		gmux_write8(gmux_data, gmux_data->selected_port, *sysfsbuf);
> +	else
> +		return -EINVAL;
> +
> +	return count;
> +}
> +
> +static ssize_t gmux_selected_port_data_show(struct device *dev,
> +		struct device_attribute *attr, char *sysfsbuf)
> +{
> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> +	u32 data;
> +
> +	data = gmux_read32(gmux_data, gmux_data->selected_port);
> +	memcpy(sysfsbuf, &data, sizeof(data));
> +
> +	return sizeof(data);
> +}
> +
> +struct device_attribute dev_attr_gmux_selected_port_data_rw = __ATTR_RW(gmux_selected_port_data);
> +struct device_attribute dev_attr_gmux_selected_port_data_ro = __ATTR_RO(gmux_selected_port_data);
> +
> +static int gmux_init_sysfs(struct pnp_dev *pnp)
> +{
> +	int ret;
> +
> +	ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port);
> +	if (ret)
> +		return ret;
> +	if (allow_user_writes)
> +		ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port_data_rw);
> +	else
> +		ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port_data_ro);
> +	if (ret)
> +		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port);
> +	return ret;
> +}
> +
> +static void gmux_fini_sysfs(struct pnp_dev *pnp)
> +{
> +	device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port);
> +	if (allow_user_writes)
> +		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port_data_rw);
> +	else
> +		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port_data_ro);
> +}
> +
> +#else
> +
> +static int gmux_init_sysfs(struct pnp_dev *pnp)
> +{
> +	return 0;
> +}
> +static void gmux_fini_sysfs(struct pnp_dev *pnp)
> +{
> +}
> +
> +#endif /* CONFIG_SYSFS */
> +
>  static int gmux_suspend(struct device *dev)
>  {
>  	struct pnp_dev *pnp = to_pnp_dev(dev);
> @@ -846,8 +966,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>  		goto err_register_handler;
>  	}
>  
> +	ret = gmux_init_sysfs(pnp);
> +	if (ret) {
> +		pr_err("Failed to register gmux sysfs entries\n");
> +		goto err_sysfs;
> +	}
> +
>  	return 0;
>  
> +err_sysfs:
> +	vga_switcheroo_unregister_handler();
>  err_register_handler:
>  	gmux_disable_interrupts(gmux_data);
>  	apple_gmux_data = NULL;
> @@ -877,6 +1005,7 @@ static void gmux_remove(struct pnp_dev *pnp)
>  {
>  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
>  
> +	gmux_fini_sysfs(pnp);
>  	vga_switcheroo_unregister_handler();
>  	gmux_disable_interrupts(gmux_data);
>  	if (gmux_data->gpe >= 0) {

Re: [RFC PATCH 7/9] apple-gmux: add sysfs interface

[Hans de Goede]

Hi,

On 2/10/23 21:15, Hans de Goede wrote:
> Hi,
> 
> On 2/10/23 05:48, Orlando Chamberlain wrote:
>> Allow reading gmux ports from userspace. When the unsafe module
>> parameter allow_user_writes is true, writing 1 byte
>> values is also allowed.
>>
>> For example:
>>
>> cd /sys/bus/acpi/devices/APP000B:00/physical_node/
>> echo 4 > gmux_selected_port
>> cat gmux_selected_port_data | xxd -p
>>
>> Will show the gmux version information (00000005 in this case)
> 
> Please use debugfs for this and as part of the conversion
> drop the #ifdef-s (debugfs has stubs for when not enabled)
> and drop all the error checking of creating the files, debugfs
> is deliberately designed to not have any error checking in
> the setup / teardown code.
> 
> This also removes the need for the allow_user_writes parameter
> replacing it with the new kernel lockdown mechanism. debugfs
> will automatically block access to writable files when
> the kernel is in lockdown mode.
> 
> Regards,
> 
> Hans

p.s.

I just realized I forgot my usual thank you for contributing
to the kernel reply to the cover letter before diving into
the review (oops).

So let me correct that: thank you very much for your work on this!

Regards,

Hans






>> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
>> ---
>>  drivers/platform/x86/apple-gmux.c | 129 ++++++++++++++++++++++++++++++
>>  1 file changed, 129 insertions(+)
>>
>> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
>> index c38d6ef0c15a..756059d48393 100644
>> --- a/drivers/platform/x86/apple-gmux.c
>> +++ b/drivers/platform/x86/apple-gmux.c
>> @@ -66,6 +66,11 @@ struct apple_gmux_data {
>>  	enum vga_switcheroo_client_id switch_state_external;
>>  	enum vga_switcheroo_state power_state;
>>  	struct completion powerchange_done;
>> +
>> +#ifdef CONFIG_SYSFS
>> +	/* sysfs data */
>> +	int selected_port;
>> +#endif /* CONFIG_SYSFS */
>>  };
>>  
>>  static struct apple_gmux_data *apple_gmux_data;
>> @@ -651,6 +656,121 @@ static void gmux_notify_handler(acpi_handle device, u32 value, void *context)
>>  		complete(&gmux_data->powerchange_done);
>>  }
>>  
>> +/**
>> + * DOC: Sysfs Interface
>> + *
>> + * gmux ports can be read from userspace as a sysfs interface. For example:
>> + *
>> + * # echo 4 > /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port
>> + * # cat /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port_data | xxd -p
>> + * 00000005
>> + *
>> + * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
>> + *
>> + * Single byte writes are also supported, however this must be enabled with the
>> + * unsafe allow_user_writes module parameter.
>> + *
>> + */
>> +
>> +#ifdef CONFIG_SYSFS
>> +
>> +static bool allow_user_writes;
>> +module_param_unsafe(allow_user_writes, bool, 0);
>> +MODULE_PARM_DESC(allow_user_writes, "Allow userspace to write to gmux ports (default: false) (bool)");
>> +
>> +static ssize_t gmux_selected_port_store(struct device *dev,
>> +		struct device_attribute *attr, const char *sysfsbuf, size_t count)
>> +{
>> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
>> +	u8 port;
>> +
>> +	if (kstrtou8(sysfsbuf, 10, &port) < 0)
>> +		return -EINVAL;
>> +
>> +	/* On pio gmux's, make sure the user doesn't access too high of a port. */
>> +	if ((gmux_data->config == &apple_gmux_pio) &&
>> +		port > (gmux_data->iolen - 4))
>> +		return -EINVAL;
>> +
>> +	gmux_data->selected_port = port;
>> +	return count;
>> +}
>> +
>> +static ssize_t gmux_selected_port_show(struct device *dev,
>> +		struct device_attribute *attr, char *sysfsbuf)
>> +{
>> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
>> +
>> +	return sysfs_emit(sysfsbuf, "%d\n", gmux_data->selected_port);
>> +}
>> +
>> +DEVICE_ATTR_RW(gmux_selected_port);
>> +
>> +static ssize_t gmux_selected_port_data_store(struct device *dev,
>> +		struct device_attribute *attr, const char *sysfsbuf, size_t count)
>> +{
>> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
>> +
>> +	if (count == 1)
>> +		gmux_write8(gmux_data, gmux_data->selected_port, *sysfsbuf);
>> +	else
>> +		return -EINVAL;
>> +
>> +	return count;
>> +}
>> +
>> +static ssize_t gmux_selected_port_data_show(struct device *dev,
>> +		struct device_attribute *attr, char *sysfsbuf)
>> +{
>> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
>> +	u32 data;
>> +
>> +	data = gmux_read32(gmux_data, gmux_data->selected_port);
>> +	memcpy(sysfsbuf, &data, sizeof(data));
>> +
>> +	return sizeof(data);
>> +}
>> +
>> +struct device_attribute dev_attr_gmux_selected_port_data_rw = __ATTR_RW(gmux_selected_port_data);
>> +struct device_attribute dev_attr_gmux_selected_port_data_ro = __ATTR_RO(gmux_selected_port_data);
>> +
>> +static int gmux_init_sysfs(struct pnp_dev *pnp)
>> +{
>> +	int ret;
>> +
>> +	ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port);
>> +	if (ret)
>> +		return ret;
>> +	if (allow_user_writes)
>> +		ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port_data_rw);
>> +	else
>> +		ret = device_create_file(&pnp->dev, &dev_attr_gmux_selected_port_data_ro);
>> +	if (ret)
>> +		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port);
>> +	return ret;
>> +}
>> +
>> +static void gmux_fini_sysfs(struct pnp_dev *pnp)
>> +{
>> +	device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port);
>> +	if (allow_user_writes)
>> +		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port_data_rw);
>> +	else
>> +		device_remove_file(&pnp->dev, &dev_attr_gmux_selected_port_data_ro);
>> +}
>> +
>> +#else
>> +
>> +static int gmux_init_sysfs(struct pnp_dev *pnp)
>> +{
>> +	return 0;
>> +}
>> +static void gmux_fini_sysfs(struct pnp_dev *pnp)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_SYSFS */
>> +
>>  static int gmux_suspend(struct device *dev)
>>  {
>>  	struct pnp_dev *pnp = to_pnp_dev(dev);
>> @@ -846,8 +966,16 @@ static int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
>>  		goto err_register_handler;
>>  	}
>>  
>> +	ret = gmux_init_sysfs(pnp);
>> +	if (ret) {
>> +		pr_err("Failed to register gmux sysfs entries\n");
>> +		goto err_sysfs;
>> +	}
>> +
>>  	return 0;
>>  
>> +err_sysfs:
>> +	vga_switcheroo_unregister_handler();
>>  err_register_handler:
>>  	gmux_disable_interrupts(gmux_data);
>>  	apple_gmux_data = NULL;
>> @@ -877,6 +1005,7 @@ static void gmux_remove(struct pnp_dev *pnp)
>>  {
>>  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
>>  
>> +	gmux_fini_sysfs(pnp);
>>  	vga_switcheroo_unregister_handler();
>>  	gmux_disable_interrupts(gmux_data);
>>  	if (gmux_data->gpe >= 0) {
> 

RE: [RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[David Laight]

From: Hans de Goede
> Sent: 10 February 2023 19:33
> 
> Hi,
> 
> On 2/10/23 20:09, Hans de Goede wrote:
> > Hi,
> >
> > On 2/10/23 05:48, Orlando Chamberlain wrote:
> >> Currently it manually flips the byte order, but we can instead use
> >> cpu_to_be32(val) for this.
> >>
> >> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> >> ---
> >>  drivers/platform/x86/apple-gmux.c | 18 ++----------------
> >>  1 file changed, 2 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/apple-gmux.c b/drivers/platform/x86/apple-gmux.c
> >> index 9333f82cfa8a..e8cb084cb81f 100644
> >> --- a/drivers/platform/x86/apple-gmux.c
> >> +++ b/drivers/platform/x86/apple-gmux.c
> >> @@ -94,13 +94,7 @@ static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
> >>  static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int port,
> >>  			     u32 val)
> >>  {
> >> -	int i;
> >> -	u8 tmpval;
> >> -
> >> -	for (i = 0; i < 4; i++) {
> >> -		tmpval = (val >> (i * 8)) & 0xff;
> >> -		outb(tmpval, gmux_data->iostart + port + i);
> >> -	}
> >> +	outl(cpu_to_be32(val), gmux_data->iostart + port);
> >>  }
> >>
> >>  static int gmux_index_wait_ready(struct apple_gmux_data *gmux_data)
> >
> > The ioport / indexed-ioport accessed apple_gmux-es likely are (part of?)
> > LPC bus devices . Looking at the bus level you are now changing 4 io
> > accesses with a size of 1 byte, to 1 32 bit io-access.
> 
> Correction to myself, re-reading the LPC specification, then
> if I'm right and this is a LPC device then all IO in/out accesses
> are always 1 byte accesses. Since the LPC bus only supports 16 / 32
> bit accesses for DMA cycles.
> 
> So presumably the outl() would get split into 4 separate 8 bit
> (port) IO accesses.

I wonder if there is something obscure and the order of the
4 bytes writes matters?

In any case writing as:
	xxxx iostart = gmux_data->iostart + port;

	outb(val, iostart);
	outb(val >> 8, iostart + 1);
	outb(val >> 16, iostart + 2);
	outb(val >> 24, ioctart + 3);
almost certainly generates better code.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[Orlando Chamberlain]

On Fri, 10 Feb 2023 20:19:27 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/10/23 20:09, Hans de Goede wrote:
> > Hi,
> > 
> > On 2/10/23 05:48, Orlando Chamberlain wrote:  
> >> Currently it manually flips the byte order, but we can instead use
> >> cpu_to_be32(val) for this.
> >>
> >> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> >> ---
> >>  drivers/platform/x86/apple-gmux.c | 18 ++----------------
> >>  1 file changed, 2 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/apple-gmux.c
> >> b/drivers/platform/x86/apple-gmux.c index
> >> 9333f82cfa8a..e8cb084cb81f 100644 ---
> >> a/drivers/platform/x86/apple-gmux.c +++
> >> b/drivers/platform/x86/apple-gmux.c @@ -94,13 +94,7 @@ static u32
> >> gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
> >> static void gmux_pio_write32(struct apple_gmux_data *gmux_data,
> >> int port, u32 val) {
> >> -	int i;
> >> -	u8 tmpval;
> >> -
> >> -	for (i = 0; i < 4; i++) {
> >> -		tmpval = (val >> (i * 8)) & 0xff;
> >> -		outb(tmpval, gmux_data->iostart + port + i);
> >> -	}
> >> +	outl(cpu_to_be32(val), gmux_data->iostart + port);
> >>  }
> >>  
> >>  static int gmux_index_wait_ready(struct apple_gmux_data
> >> *gmux_data)  
> > 
> > The ioport / indexed-ioport accessed apple_gmux-es likely are (part
> > of?) LPC bus devices . Looking at the bus level you are now
> > changing 4 io accesses with a size of 1 byte, to 1 32 bit io-access.
> > 
> > Depending on the decoding hw in the chip this may work fine,
> > or this may work not at all.
> > 
> > I realized that you have asked for more testing, but most surviving
> > macbooks from the older apple-gmux era appear to be models without
> > a discrete GPU (which are often the first thing to break) and thus
> > without a gmux.
> > 
> > Unless we get a bunch of testers to show up, which I doubt. I would
> > prefer slightly bigger / less pretty code and not change the
> > functional behavior of the driver on these older models.  
> 
> A quick follow up on this, I just noticed that only the pio_write32
> is doing the one byte at a time thing:
> 
> static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int
> port) {
>         return inl(gmux_data->iostart + port);
> }
> 
> static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int
> port, u32 val)
> {
>         int i;
>         u8 tmpval;
> 
>         for (i = 0; i < 4; i++) {
>                 tmpval = (val >> (i * 8)) & 0xff;
>                 outb(tmpval, gmux_data->iostart + port + i);
>         }
> }
> 
> And if you look closely gmux_pio_write32() is not swapping
> the order to be32 at all, it is just taking the bytes
> in little-endian memory order, starting with the first
> (index 0) byte which is the least significant byte of
> the value.
> 
> On x86 the original code is no different then doing:
> 
> static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int
> port, u32 val)
> {
>         u8 *data = (u8 *)&val;
>         int i;
> 
>         for (i = 0; i < 4; i++)
>                 outb(data[i], gmux_data->iostart + port + i);
> }
> 
> So yeah this patch is definitely wrong, it actually swaps
> the byte order compared to the original code. Which becomes
> clear when you look the weird difference between the read32 and
> write32 functions after this patch.
> 
> Presumably there is a specific reason why gmux_pio_write32()
> is not already doing a single outl(..., val) and byte-ordering
> is not the reason.
> 
> Regards,
> 
> Hans

Sounds like it may be better to just drop this patch as there's very
little benefit for the risk of causing a regression.

> 
> 
> 
> >> @@ -177,16 +171,8 @@ static u32 gmux_index_read32(struct
> >> apple_gmux_data *gmux_data, int port) static void
> >> gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
> >> u32 val) {
> >> -	int i;
> >> -	u8 tmpval;
> >> -
> >>  	mutex_lock(&gmux_data->index_lock);
> >> -
> >> -	for (i = 0; i < 4; i++) {
> >> -		tmpval = (val >> (i * 8)) & 0xff;
> >> -		outb(tmpval, gmux_data->iostart + GMUX_PORT_VALUE
> >> + i);
> >> -	}
> >> -
> >> +	outl(cpu_to_be32(val), gmux_data->iostart +
> >> GMUX_PORT_VALUE); gmux_index_wait_ready(gmux_data);
> >>  	outb(port & 0xff, gmux_data->iostart + GMUX_PORT_WRITE);
> >>  	gmux_index_wait_complete(gmux_data);  
> >   
> 

Re: [RFC PATCH 2/9] apple-gmux: consolidate version reading

[Orlando Chamberlain]

On Fri, 10 Feb 2023 20:41:19 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/10/23 05:48, Orlando Chamberlain wrote:
> > Read gmux version in one go as 32 bits on both indexed and classic
> > gmux's.
> > 
> > Classic gmux's used to read the version as
> > 
> > major = inb(base + 0x4);
> > minor = inb(base + 0x5);
> > release = inb(base + 0x6);
> > 
> > but this can instead be done the same way as indexed gmux's with
> > gmux_read32(), so the same version reading code is used for classic
> > and indexed gmux's (as well as mmio gmux's that will be added to
> > this driver).
> > 
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> >  drivers/platform/x86/apple-gmux.c | 14 ++++++--------
> >  include/linux/apple-gmux.h        |  6 +-----
> >  2 files changed, 7 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > e8cb084cb81f..67628104f31a 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -580,15 +580,13 @@ static
> > int gmux_probe(struct pnp_dev *pnp, const struct pnp_device_id *id)
> > if (indexed) { mutex_init(&gmux_data->index_lock);
> >  		gmux_data->indexed = true;
> > -		version = gmux_read32(gmux_data,
> > GMUX_PORT_VERSION_MAJOR);
> > -		ver_major = (version >> 24) & 0xff;
> > -		ver_minor = (version >> 16) & 0xff;
> > -		ver_release = (version >> 8) & 0xff;
> > -	} else {
> > -		ver_major = gmux_read8(gmux_data,
> > GMUX_PORT_VERSION_MAJOR);
> > -		ver_minor = gmux_read8(gmux_data,
> > GMUX_PORT_VERSION_MINOR);
> > -		ver_release = gmux_read8(gmux_data,
> > GMUX_PORT_VERSION_RELEASE); }
> > +
> > +	version = gmux_read32(gmux_data, GMUX_PORT_VERSION_MAJOR);
> > +	ver_major = (version >> 24) & 0xff;
> > +	ver_minor = (version >> 16) & 0xff;
> > +	ver_release = (version >> 8) & 0xff;
> > +
> >  	pr_info("Found gmux version %d.%d.%d [%s]\n", ver_major,
> > ver_minor, ver_release, (gmux_data->indexed ? "indexed" :
> > "classic")); 
> 
> The problem with this is that there is nothing (no known register)
> at address base + 7 and now you are reading from address base + 7
> here where before the code was not, we have no idea how the hw
> will respond to this.  This should be pretty innocent but still ...

That makes sense, hopefully someone will be able to test it.

> 
> > diff --git a/include/linux/apple-gmux.h b/include/linux/apple-gmux.h
> > index 1f68b49bcd68..eb2caee04abd 100644
> > --- a/include/linux/apple-gmux.h
> > +++ b/include/linux/apple-gmux.h
> > @@ -67,7 +67,6 @@ static inline bool apple_gmux_is_indexed(unsigned
> > long iostart) */
> >  static inline bool apple_gmux_detect(struct pnp_dev *pnp_dev, bool
> > *indexed_ret) {
> > -	u8 ver_major, ver_minor, ver_release;
> >  	struct device *dev = NULL;
> >  	struct acpi_device *adev;
> >  	struct resource *res;
> > @@ -95,10 +94,7 @@ static inline bool apple_gmux_detect(struct
> > pnp_dev *pnp_dev, bool *indexed_ret)
> >  	 * Invalid version information may indicate either that
> > the gmux
> >  	 * device isn't present or that it's a new one that uses
> > indexed io. */
> > -	ver_major = inb(res->start + GMUX_PORT_VERSION_MAJOR);
> > -	ver_minor = inb(res->start + GMUX_PORT_VERSION_MINOR);
> > -	ver_release = inb(res->start + GMUX_PORT_VERSION_RELEASE);
> > -	if (ver_major == 0xff && ver_minor == 0xff && ver_release
> > == 0xff) {
> > +	if (!(~inl(res->start + GMUX_PORT_VERSION_MAJOR))) {  
> 
> Assuming we can get this tested well enough that I'm ok with the
> change in general please write this as:
> 
> 	if (inl(res->start + GMUX_PORT_VERSION_MAJOR) == 0xffffffff) {
> 
> Which I believe is what you are trying to achieve here ?

Yes that is a neater way of doing what I was trying to do, I'll use
that in v2.

> 
> Regards,
> 
> Hans
> 
> 
> 
> >  		indexed = apple_gmux_is_indexed(res->start);
> >  		if (!indexed)
> >  			goto out;  
> 

Re: [RFC PATCH 5/9] apple-gmux: Use GMSP acpi method for interrupt clear

[Orlando Chamberlain]

On Fri, 10 Feb 2023 20:43:58 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/10/23 05:48, Orlando Chamberlain wrote:
> > This is needed for interrupts to be cleared correctly on MMIO based
> > gmux's. It is untested if this helps/hinders other gmux types, but I
> > have seen the GMSP method in the acpi tables of a MacBook with an
> > indexed gmux.
> > 
> > If this turns out to break support for older gmux's, this can
> > instead be only done on MMIO gmux's.
> > 
> > There is also a "GMLV" acpi method, and the "GMSP" method can be
> > called with 1 as its argument, but the purposes of these aren't
> > known and they don't seem to be needed.
> > 
> > Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> > ---
> >  drivers/platform/x86/apple-gmux.c | 26 +++++++++++++++++++++++++-
> >  1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/x86/apple-gmux.c
> > b/drivers/platform/x86/apple-gmux.c index
> > 760434a527c1..c605f036ea0b 100644 ---
> > a/drivers/platform/x86/apple-gmux.c +++
> > b/drivers/platform/x86/apple-gmux.c @@ -494,8 +494,29 @@ static
> > const struct apple_gmux_config apple_gmux_index = {
> >   * MCP79, on all following generations it's GPIO pin 6 of the
> > Intel PCH.
> >   * The GPE merely signals that an interrupt occurred, the actual
> > type of event
> >   * is identified by reading a gmux register.
> > + *
> > + * On MMIO gmux's, we also need to call the acpi method GMSP to
> > properly clear
> > + * interrupts. TODO: Do other types need this? Does this break
> > other types? */
> >  
> > +static int gmux_call_acpi_gmsp(struct apple_gmux_data *gmux_data,
> > int arg) +{
> > +	acpi_status status = AE_OK;
> > +	union acpi_object arg0 = { ACPI_TYPE_INTEGER };
> > +	struct acpi_object_list arg_list = { 1, &arg0 };
> > +
> > +	arg0.integer.value = arg;
> > +
> > +	status = acpi_evaluate_object(gmux_data->dhandle, "GMSP",
> > &arg_list, NULL);
> > +	if (ACPI_FAILURE(status)) {
> > +		pr_err("GMSP call failed: %s\n",
> > +		       acpi_format_exception(status));
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static inline void gmux_disable_interrupts(struct apple_gmux_data
> > *gmux_data) {
> >  	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_ENABLE,
> > @@ -519,7 +540,10 @@ static void gmux_clear_interrupts(struct
> > apple_gmux_data *gmux_data) 
> >  	/* to clear interrupts write back current status */
> >  	status = gmux_interrupt_get_status(gmux_data);
> > -	gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS, status);
> > +	if (status) {
> > +		gmux_write8(gmux_data, GMUX_PORT_INTERRUPT_STATUS,
> > status);
> > +		gmux_call_acpi_gmsp(gmux_data, 0);  
> 
> Ugh no, please don't go around calling random ACPI methods from
> untested firmware revisions / device models.
> 
> ACPI code (even Apple's I have learned) tends to be full of bugs. If
> we did not need to call GMSP before then please lets keep not calling
> it on the older models. Just because it is there does not mean that
> calling it is useful, it might even be harmful.

I'll make it only use this ACPI method on MMIO gmux's in v2 then.

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> > +	}
> >  }
> >  
> >  static void gmux_notify_handler(acpi_handle device, u32 value,
> > void *context)  
> 

Re: [RFC PATCH 7/9] apple-gmux: add sysfs interface

[Orlando Chamberlain]

On Fri, 10 Feb 2023 21:23:15 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 2/10/23 21:15, Hans de Goede wrote:
> > Hi,
> > 
> > On 2/10/23 05:48, Orlando Chamberlain wrote:  
> >> Allow reading gmux ports from userspace. When the unsafe module
> >> parameter allow_user_writes is true, writing 1 byte
> >> values is also allowed.
> >>
> >> For example:
> >>
> >> cd /sys/bus/acpi/devices/APP000B:00/physical_node/
> >> echo 4 > gmux_selected_port
> >> cat gmux_selected_port_data | xxd -p
> >>
> >> Will show the gmux version information (00000005 in this case)  
> > 
> > Please use debugfs for this and as part of the conversion
> > drop the #ifdef-s (debugfs has stubs for when not enabled)
> > and drop all the error checking of creating the files, debugfs
> > is deliberately designed to not have any error checking in
> > the setup / teardown code.
> > 
> > This also removes the need for the allow_user_writes parameter
> > replacing it with the new kernel lockdown mechanism. debugfs
> > will automatically block access to writable files when
> > the kernel is in lockdown mode.

I'll change it to use debugfs instead of sysfs in v2.

> > 
> > Regards,
> > 
> > Hans  
> 
> p.s.
> 
> I just realized I forgot my usual thank you for contributing
> to the kernel reply to the cover letter before diving into
> the review (oops).
> 
> So let me correct that: thank you very much for your work on this!

thank you for maintaining and reviewing!
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> >> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
> >> ---
> >>  drivers/platform/x86/apple-gmux.c | 129
> >> ++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/apple-gmux.c
> >> b/drivers/platform/x86/apple-gmux.c index
> >> c38d6ef0c15a..756059d48393 100644 ---
> >> a/drivers/platform/x86/apple-gmux.c +++
> >> b/drivers/platform/x86/apple-gmux.c @@ -66,6 +66,11 @@ struct
> >> apple_gmux_data { enum vga_switcheroo_client_id
> >> switch_state_external; enum vga_switcheroo_state power_state;
> >>  	struct completion powerchange_done;
> >> +
> >> +#ifdef CONFIG_SYSFS
> >> +	/* sysfs data */
> >> +	int selected_port;
> >> +#endif /* CONFIG_SYSFS */
> >>  };
> >>  
> >>  static struct apple_gmux_data *apple_gmux_data;
> >> @@ -651,6 +656,121 @@ static void gmux_notify_handler(acpi_handle
> >> device, u32 value, void *context)
> >> complete(&gmux_data->powerchange_done); }
> >>  
> >> +/**
> >> + * DOC: Sysfs Interface
> >> + *
> >> + * gmux ports can be read from userspace as a sysfs interface.
> >> For example:
> >> + *
> >> + * # echo 4 >
> >> /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port
> >> + * # cat
> >> /sys/bus/acpi/devices/APP000B:00/physical_node/gmux_selected_port_data
> >> | xxd -p
> >> + * 00000005
> >> + *
> >> + * Reads 4 bytes from port 4 (GMUX_PORT_VERSION_MAJOR).
> >> + *
> >> + * Single byte writes are also supported, however this must be
> >> enabled with the
> >> + * unsafe allow_user_writes module parameter.
> >> + *
> >> + */
> >> +
> >> +#ifdef CONFIG_SYSFS
> >> +
> >> +static bool allow_user_writes;
> >> +module_param_unsafe(allow_user_writes, bool, 0);
> >> +MODULE_PARM_DESC(allow_user_writes, "Allow userspace to write to
> >> gmux ports (default: false) (bool)"); +
> >> +static ssize_t gmux_selected_port_store(struct device *dev,
> >> +		struct device_attribute *attr, const char
> >> *sysfsbuf, size_t count) +{
> >> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> >> +	u8 port;
> >> +
> >> +	if (kstrtou8(sysfsbuf, 10, &port) < 0)
> >> +		return -EINVAL;
> >> +
> >> +	/* On pio gmux's, make sure the user doesn't access too
> >> high of a port. */
> >> +	if ((gmux_data->config == &apple_gmux_pio) &&
> >> +		port > (gmux_data->iolen - 4))
> >> +		return -EINVAL;
> >> +
> >> +	gmux_data->selected_port = port;
> >> +	return count;
> >> +}
> >> +
> >> +static ssize_t gmux_selected_port_show(struct device *dev,
> >> +		struct device_attribute *attr, char *sysfsbuf)
> >> +{
> >> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> >> +
> >> +	return sysfs_emit(sysfsbuf, "%d\n",
> >> gmux_data->selected_port); +}
> >> +
> >> +DEVICE_ATTR_RW(gmux_selected_port);
> >> +
> >> +static ssize_t gmux_selected_port_data_store(struct device *dev,
> >> +		struct device_attribute *attr, const char
> >> *sysfsbuf, size_t count) +{
> >> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> >> +
> >> +	if (count == 1)
> >> +		gmux_write8(gmux_data, gmux_data->selected_port,
> >> *sysfsbuf);
> >> +	else
> >> +		return -EINVAL;
> >> +
> >> +	return count;
> >> +}
> >> +
> >> +static ssize_t gmux_selected_port_data_show(struct device *dev,
> >> +		struct device_attribute *attr, char *sysfsbuf)
> >> +{
> >> +	struct apple_gmux_data *gmux_data = dev_get_drvdata(dev);
> >> +	u32 data;
> >> +
> >> +	data = gmux_read32(gmux_data, gmux_data->selected_port);
> >> +	memcpy(sysfsbuf, &data, sizeof(data));
> >> +
> >> +	return sizeof(data);
> >> +}
> >> +
> >> +struct device_attribute dev_attr_gmux_selected_port_data_rw =
> >> __ATTR_RW(gmux_selected_port_data); +struct device_attribute
> >> dev_attr_gmux_selected_port_data_ro =
> >> __ATTR_RO(gmux_selected_port_data); + +static int
> >> gmux_init_sysfs(struct pnp_dev *pnp) +{
> >> +	int ret;
> >> +
> >> +	ret = device_create_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (allow_user_writes)
> >> +		ret = device_create_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port_data_rw);
> >> +	else
> >> +		ret = device_create_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port_data_ro);
> >> +	if (ret)
> >> +		device_remove_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port);
> >> +	return ret;
> >> +}
> >> +
> >> +static void gmux_fini_sysfs(struct pnp_dev *pnp)
> >> +{
> >> +	device_remove_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port);
> >> +	if (allow_user_writes)
> >> +		device_remove_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port_data_rw);
> >> +	else
> >> +		device_remove_file(&pnp->dev,
> >> &dev_attr_gmux_selected_port_data_ro); +}
> >> +
> >> +#else
> >> +
> >> +static int gmux_init_sysfs(struct pnp_dev *pnp)
> >> +{
> >> +	return 0;
> >> +}
> >> +static void gmux_fini_sysfs(struct pnp_dev *pnp)
> >> +{
> >> +}
> >> +
> >> +#endif /* CONFIG_SYSFS */
> >> +
> >>  static int gmux_suspend(struct device *dev)
> >>  {
> >>  	struct pnp_dev *pnp = to_pnp_dev(dev);
> >> @@ -846,8 +966,16 @@ static int gmux_probe(struct pnp_dev *pnp,
> >> const struct pnp_device_id *id) goto err_register_handler;
> >>  	}
> >>  
> >> +	ret = gmux_init_sysfs(pnp);
> >> +	if (ret) {
> >> +		pr_err("Failed to register gmux sysfs entries\n");
> >> +		goto err_sysfs;
> >> +	}
> >> +
> >>  	return 0;
> >>  
> >> +err_sysfs:
> >> +	vga_switcheroo_unregister_handler();
> >>  err_register_handler:
> >>  	gmux_disable_interrupts(gmux_data);
> >>  	apple_gmux_data = NULL;
> >> @@ -877,6 +1005,7 @@ static void gmux_remove(struct pnp_dev *pnp)
> >>  {
> >>  	struct apple_gmux_data *gmux_data = pnp_get_drvdata(pnp);
> >>  
> >> +	gmux_fini_sysfs(pnp);
> >>  	vga_switcheroo_unregister_handler();
> >>  	gmux_disable_interrupts(gmux_data);
> >>  	if (gmux_data->gpe >= 0) {  
> >   
> 

Re: [RFC PATCH 9/9] drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached

[Orlando Chamberlain]

On Fri, 10 Feb 2023 11:37:08 -0500
Alex Deucher <alexdeucher@gmail.com> wrote:

> On Fri, Feb 10, 2023 at 11:07 AM Hans de Goede <hdegoede@redhat.com>
> wrote:
> >
> > Hi,
> >
> > On 2/10/23 16:53, Alex Deucher wrote:  
> > > On Fri, Feb 10, 2023 at 3:04 AM Orlando Chamberlain
> > > <orlandoch.dev@gmail.com> wrote:  
> > >>
> > >> From: Kerem Karabay <kekrby@gmail.com>
> > >>
> > >> Commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> > >> vga_switcheroo") made amdgpu only register a vga_switcheroo
> > >> client for GPU's with PX, however AMD GPUs in dual gpu Apple
> > >> Macbooks do need to register, but don't have PX. Instead of
> > >> AMD's PX, they use apple-gmux.  
> > >
> > > Is there a way to detect apple-gmux instead?  Otherwise, we
> > > register vga_switcheroo on any system with multiple GPUs which is
> > > not what we want.  
> >
> > Yes since 6.1.y (either stable series or just take 6.2.0) the
> > apple-gmux detect code has been factored out into a stand-alone
> > apple_gmux_detect() helper inside:
> >
> > include/linux/apple-gmux.h
> >
> > For usage outside of the actual apple-gmux driver you can simply
> > pass NULL for both arguments.
> >
> > This was necessary to reliably check if the apple-gmux should be
> > used for backlight control.
> >
> > Note there also is the older apple_gmux_present() helper, which is
> > already used in some drm code. That function is not reliable though
> > it detects if the ACPI tables contain an ACPI device describing
> > the presence of a gmux, but it turns out even Apple has buggy ACPI
> > tables and the mere presence of that ACPI device is not a reliable
> > indicator the gmux is actually there.
> >
> > I have not changed over any of the existing apple_gmux_present()
> > users for fear of unwanted side effects...  
> 
> Looks like we could maybe use the PWRD ACPI check like patch 8 does
> as well.

I wasn't using apple_gmux_detect as I mistakenly thought
pnp_get_resource would fail if apple-gmux had bound to the resource but
it looks like I was wrong about that so we can use that to determine if
the system has a gmux. I think I'll do that in v2.

As far as I know there's only one internal (non
thunderbolt) amd gpu inside all Macbooks with gmux so we probably
wouldn't need to check for PWRD to ensure it's  the right gpu.

With PWRD, I don't know if its present on all Dual GPU Macbooks, I've
only found the acpi tables for Macbookpro14,x to Macbookpro16,x, so I
don't know if it will work on older Macs (I'm also not sure if those
macs are using radeon or amdgpu).

> Alex
> 
> >
> > Regards,
> >
> > Hans
> >
> >
> >
> >  
> > >> Revert to the old logic of registering for all non-thunderbolt
> > >> gpus, like radeon and nouveau.
> > >>
> > >> Fixes: 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and
> > >> vga_switcheroo") Signed-off-by: Kerem Karabay <kekrby@gmail.com>
> > >> [Orlando Chamberlain <orlandoch.dev@gmail.com>: add commit
> > >> description] Signed-off-by: Orlando Chamberlain
> > >> <orlandoch.dev@gmail.com> ---
> > >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18
> > >> +++++++++++------- 1 file changed, 11 insertions(+), 7
> > >> deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c index
> > >> 2f28a8c02f64..0bb553a61552 100644 ---
> > >> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +++
> > >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c @@ -3919,12
> > >> +3919,13 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> > >> if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> > >> vga_client_register(adev->pdev, amdgpu_device_vga_set_decode);
> > >>
> > >> -       if (amdgpu_device_supports_px(ddev)) {
> > >> -               px = true;
> > >> -               vga_switcheroo_register_client(adev->pdev,
> > >> -
> > >> &amdgpu_switcheroo_ops, px);
> > >> +       px = amdgpu_device_supports_px(ddev);
> > >> +
> > >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> > >> +               vga_switcheroo_register_client(adev->pdev,
> > >> &amdgpu_switcheroo_ops, px); +
> > >> +       if (px)
> > >>                 vga_switcheroo_init_domain_pm_ops(adev->dev,
> > >> &adev->vga_pm_domain);
> > >> -       }
> > >>
> > >>         if (adev->gmc.xgmi.pending_reset)
> > >>                 queue_delayed_work(system_wq,
> > >> &mgpu_info.delayed_reset_work, @@ -4048,10 +4049,13 @@ void
> > >> amdgpu_device_fini_sw(struct amdgpu_device *adev)
> > >>
> > >>         kfree(adev->bios);
> > >>         adev->bios = NULL;
> > >> -       if (amdgpu_device_supports_px(adev_to_drm(adev))) {
> > >> +
> > >> +       if (!pci_is_thunderbolt_attached(adev->pdev))
> > >>                 vga_switcheroo_unregister_client(adev->pdev);
> > >> +
> > >> +       if (amdgpu_device_supports_px(adev_to_drm(adev)))
> > >>                 vga_switcheroo_fini_domain_pm_ops(adev->dev);
> > >> -       }
> > >> +
> > >>         if ((adev->pdev->class >> 8) == PCI_CLASS_DISPLAY_VGA)
> > >>                 vga_client_unregister(adev->pdev);
> > >>
> > >> --
> > >> 2.39.1
> > >>  
> > >  
> >  

Re: [RFC PATCH 1/9] apple-gmux: use cpu_to_be32 instead of manual reorder

[Hans de Goede]

Hi,

On 2/11/23 00:30, Orlando Chamberlain wrote:
> On Fri, 10 Feb 2023 20:19:27 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi,
>>
>> On 2/10/23 20:09, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 2/10/23 05:48, Orlando Chamberlain wrote:  
>>>> Currently it manually flips the byte order, but we can instead use
>>>> cpu_to_be32(val) for this.
>>>>
>>>> Signed-off-by: Orlando Chamberlain <orlandoch.dev@gmail.com>
>>>> ---
>>>>  drivers/platform/x86/apple-gmux.c | 18 ++----------------
>>>>  1 file changed, 2 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/platform/x86/apple-gmux.c
>>>> b/drivers/platform/x86/apple-gmux.c index
>>>> 9333f82cfa8a..e8cb084cb81f 100644 ---
>>>> a/drivers/platform/x86/apple-gmux.c +++
>>>> b/drivers/platform/x86/apple-gmux.c @@ -94,13 +94,7 @@ static u32
>>>> gmux_pio_read32(struct apple_gmux_data *gmux_data, int port)
>>>> static void gmux_pio_write32(struct apple_gmux_data *gmux_data,
>>>> int port, u32 val) {
>>>> -	int i;
>>>> -	u8 tmpval;
>>>> -
>>>> -	for (i = 0; i < 4; i++) {
>>>> -		tmpval = (val >> (i * 8)) & 0xff;
>>>> -		outb(tmpval, gmux_data->iostart + port + i);
>>>> -	}
>>>> +	outl(cpu_to_be32(val), gmux_data->iostart + port);
>>>>  }
>>>>  
>>>>  static int gmux_index_wait_ready(struct apple_gmux_data
>>>> *gmux_data)  
>>>
>>> The ioport / indexed-ioport accessed apple_gmux-es likely are (part
>>> of?) LPC bus devices . Looking at the bus level you are now
>>> changing 4 io accesses with a size of 1 byte, to 1 32 bit io-access.
>>>
>>> Depending on the decoding hw in the chip this may work fine,
>>> or this may work not at all.
>>>
>>> I realized that you have asked for more testing, but most surviving
>>> macbooks from the older apple-gmux era appear to be models without
>>> a discrete GPU (which are often the first thing to break) and thus
>>> without a gmux.
>>>
>>> Unless we get a bunch of testers to show up, which I doubt. I would
>>> prefer slightly bigger / less pretty code and not change the
>>> functional behavior of the driver on these older models.  
>>
>> A quick follow up on this, I just noticed that only the pio_write32
>> is doing the one byte at a time thing:
>>
>> static u32 gmux_pio_read32(struct apple_gmux_data *gmux_data, int
>> port) {
>>         return inl(gmux_data->iostart + port);
>> }
>>
>> static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int
>> port, u32 val)
>> {
>>         int i;
>>         u8 tmpval;
>>
>>         for (i = 0; i < 4; i++) {
>>                 tmpval = (val >> (i * 8)) & 0xff;
>>                 outb(tmpval, gmux_data->iostart + port + i);
>>         }
>> }
>>
>> And if you look closely gmux_pio_write32() is not swapping
>> the order to be32 at all, it is just taking the bytes
>> in little-endian memory order, starting with the first
>> (index 0) byte which is the least significant byte of
>> the value.
>>
>> On x86 the original code is no different then doing:
>>
>> static void gmux_pio_write32(struct apple_gmux_data *gmux_data, int
>> port, u32 val)
>> {
>>         u8 *data = (u8 *)&val;
>>         int i;
>>
>>         for (i = 0; i < 4; i++)
>>                 outb(data[i], gmux_data->iostart + port + i);
>> }
>>
>> So yeah this patch is definitely wrong, it actually swaps
>> the byte order compared to the original code. Which becomes
>> clear when you look the weird difference between the read32 and
>> write32 functions after this patch.
>>
>> Presumably there is a specific reason why gmux_pio_write32()
>> is not already doing a single outl(..., val) and byte-ordering
>> is not the reason.
>>
>> Regards,
>>
>> Hans
> 
> Sounds like it may be better to just drop this patch as there's very
> little benefit for the risk of causing a regression.

Yes if it is easy to drop this please drop this.

And the same more or less applies to 2/9. I would rather have
an extra "if () ... else ..."  in the code in a couple of places
then change behavior on old hw where we cannot get proper test
coverage (but will likely eventually get regressions reports
if we break things).

Thanks & Regards,

Hans



>>>> @@ -177,16 +171,8 @@ static u32 gmux_index_read32(struct
>>>> apple_gmux_data *gmux_data, int port) static void
>>>> gmux_index_write32(struct apple_gmux_data *gmux_data, int port,
>>>> u32 val) {
>>>> -	int i;
>>>> -	u8 tmpval;
>>>> -
>>>>  	mutex_lock(&gmux_data->index_lock);
>>>> -
>>>> -	for (i = 0; i < 4; i++) {
>>>> -		tmpval = (val >> (i * 8)) & 0xff;
>>>> -		outb(tmpval, gmux_data->iostart + GMUX_PORT_VALUE
>>>> + i);
>>>> -	}
>>>> -
>>>> +	outl(cpu_to_be32(val), gmux_data->iostart +
>>>> GMUX_PORT_VALUE); gmux_index_wait_ready(gmux_data);
>>>>  	outb(port & 0xff, gmux_data->iostart + GMUX_PORT_WRITE);
>>>>  	gmux_index_wait_complete(gmux_data);  
>>>   
>>
>