| Message ID | 20230210044826.9834-1-orlandoch.dev@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | apple-gmux: support MMIO gmux type on T2 Macs | expand |
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); > > >
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 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) >
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) { > > >
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 > > >> > > > > >
Hi All, This patch series adds support for the MMIO based gmux present on these Dual GPU Apple T2 Macs: MacBookPro15,1, MacBookPro15,3, MacBookPro16,1, MacBookPro16,4 (although amdgpu isn't working on MacBookPro16,4 [1]). It's only been tested by people on T2 Macs with MMIO based gmux's using t2linux [2] kernels, but some changes may impact older port io and indexed gmux's so testing, especially on those older Macbooks, would be appreciated. # 1-2: refactor code to make it easier to add the 3rd gmux type. # 3: has a slight change in how the switch state is read, I don't expect this to cause issues for older models (but still, please test if you have one!) # 4: implements a system to support more than 2 gmux types # 5: start using the gmux's GMSP acpi method when handling interrupts. This is needed for the MMIO gmux's, and its present in the acpi tables of some indexed gmux's I could find so hopefully enabling this for all models will be fine, but if not it can be only used on MMIO gmux's. # 6: Adds support for the MMIO based gmux on T2 macs. # 7: Add a sysfs interface to apple-gmux so data from ports can be read from userspace, and written to if the user enables an unsafe kernel parameter. This can be used for more easily researching what unknown ports do, and switching gpus when vga_switcheroo isn't ready (e.g. when one gpu is bound to vfio-pci and in use by a Windows VM, I can use this to switch my internal display between Linux and Windows easily). # 8-9: These patches make amdgpu and snd_hda_intel register with vga_switcheroo on Macbooks. I would like advice from the AMD folks on how they want this to work, so that both PX and apple-gmux laptops work properly. For radeon and nouveau we just register for every non-thunderbolt device, but this was changed for AMD cards in commit 3840c5bcc245 ("drm/amdgpu: disentangle runtime pm and vga_switcheroo") and commit 586bc4aab878 ("ALSA: hda/hdmi - fix vgaswitcheroo detection for AMD"). This meant that only gpu's with PX register. Commit #8 makes amdgpu register for all non-thinderbolt cards, and commit #9 makes snd_hda_intel register for all amd cards with the PWRD (mentioned below) acpi method. An alternative would be using apple-gmux-detect(), but that won't work after apple-gmux has probed and claimed its memory resources. # Issues: 1. Switching gpus at runtime has the same issue as indexed gmux's: the inactive gpu can't probe the DDC lines for eDP [3] 2. Powering on the amdgpu with vga_switcheroo doesn't work well. I'm told on the MacBookPro15,1 it works sometimes, and adding delays helps, but on my MacBookPro16,1 I haven't been able to get it to work at all: snd_hda_intel 0000:03:00.1: Disabling via vga_switcheroo snd_hda_intel 0000:03:00.1: Cannot lock devices! amdgpu: switched off amdgpu: switched on amdgpu 0000:03:00.0: Unable to change power state from D3hot to D0, device inaccessible amdgpu 0000:03:00.0: Unable to change power state from D3cold to D0, device inaccessible [drm] PCIE GART of 512M enabled (table at 0x00000080FEE00000). [drm] PSP is resuming... [drm:psp_hw_start [amdgpu]] *ERROR* PSP create ring failed! [drm:psp_resume [amdgpu]] *ERROR* PSP resume failed [drm:amdgpu_device_fw_loading [amdgpu]] *ERROR* resume of IP block <psp> failed -62 amdgpu 0000:03:00.0: amdgpu: amdgpu_device_ip_resume failed (-62). snd_hda_intel 0000:03:00.1: Enabling via vga_switcheroo snd_hda_intel 0000:03:00.1: Unable to change power state from D3cold to D0, device inaccessible snd_hda_intel 0000:03:00.1: CORB reset timeout#2, CORBRP = 65535 snd_hda_codec_hdmi hdaudioC0D0: Unable to sync register 0x2f0d00. -5 There are some acpi methods (PWRD, PWG1 [4, 5]) that macOS calls when changing the amdgpu's power state, but we don't use them and that could be a cause. Additionally unlike previous generation Macbooks which work better, on MacBookPro16,1 the gpu is located behind 2 pci bridges: 01:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 XL Upstream Port of PCI Express Switch (rev 43) 02:00.0 PCI bridge: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 XL Downstream Port of PCI Express Switch 03:00.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] Navi 14 [Radeon RX 5500/5500M / Pro 5500M] (rev 43) 03:00.1 Audio device: Advanced Micro Devices, Inc. [AMD/ATI] Navi 10 HDMI Audio Upon attempting to power on the gpu with vga_switcheroo, all these devices except 01:00.0 have their config space in `lspci -x` filled with 0xff. `echo 1 > /sys/bus/pci/rescan` fixes that and the dmesg errors about changing power state, but "PSP create ring failed" still happens, and the gpu doesn't resume properly. [1]: https://lore.kernel.org/all/3AFB9142-2BD0-46F9-AEA9-C9C5D13E68E6@live.com/ [2]: https://t2linux.org [3]: https://lore.kernel.org/all/9eed8ede6f15a254ad578e783b050e1c585d5a15.1439288957.git.lukas@wunner.de/ [4]: https://gist.github.com/Redecorating/6c7136b7a4ac7ce3b77d8e41740dd87b [5]: https://lore.kernel.org/all/20120710160555.GA31562@srcf.ucam.org/ Kerem Karabay (1): drm/amdgpu: register a vga_switcheroo client for all GPUs that are not thunderbolt attached Orlando Chamberlain (8): apple-gmux: use cpu_to_be32 instead of manual reorder apple-gmux: consolidate version reading apple-gmux: use first bit to check switch state apple-gmux: refactor gmux types apple-gmux: Use GMSP acpi method for interrupt clear apple-gmux: support MMIO gmux on T2 Macs apple-gmux: add sysfs interface hda/hdmi: Register with vga_switcheroo on Dual GPU Macbooks drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 18 +- drivers/platform/x86/apple-gmux.c | 416 +++++++++++++++++---- include/linux/apple-gmux.h | 50 ++- sound/pci/hda/hda_intel.c | 19 +- 4 files changed, 409 insertions(+), 94 deletions(-)