[RFC PATCH 0/9] apple-gmux: support MMIO gmux type on T2 Macs
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.1439288... [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(-)
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);
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);
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);
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);
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);
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);
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)
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;
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;
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;
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;
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:
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)
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)
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)
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; }
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) {
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) {
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) {
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) {
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); }
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);
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
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
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
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
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:
- Switching gpus at runtime has the same issue as indexed gmux's: the
inactive gpu can't probe the DDC lines for eDP [3]
- Powering on the amdgpu with vga_switcheroo doesn't work well. I'm
told on the MacBookPro15,1 it works sometimes, and adding delays helps, but on my MacBookPro16,1 I haven't been able to get it to work at all:
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
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
participants (4)
-
Alex Deucher -
David Laight -
Hans de Goede -
Orlando Chamberlain