| Message ID | 20200727205752.28224-1-ddadap@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes | expand |
I am no authority to say if this patch is good or bad, but I hope to help with my comments. > Some upcoming notebook designs utilize display muxes driven by a > pair of ACPI methods, MXDM to query and configure the operational > mode of the mux (integrated only, discrete only, hybrid non-muxed, > hybrid with dynamic mux switching), and MXDS to query and set the > mux state when running in dynamic switch mode. > > Add a vga-switcheroo driver to support switching the mux on systems > with the ACPI MXDM/MXDS interface. The mux mode cannot be changed > dynamically (calling MXDM to change the mode won't have effect until > the next boot, and calling MXDM to read the mux mode returns the > active mode, not the mode that will be enabled on next boot), and > MXDS only works when the mux mode is set to dynamic switch, so this > driver will fail to load when MXDM reports any non-dynamic mode. > > Signed-off-by: Daniel Dadap <ddadap@nvidia.com> > --- > MAINTAINERS | 6 + > drivers/platform/x86/Kconfig | 9 ++ > drivers/platform/x86/Makefile | 2 + > drivers/platform/x86/mxds-gmux.c | 268 +++++++++++++++++++++++++++++++ > 4 files changed, 285 insertions(+) > create mode 100644 drivers/platform/x86/mxds-gmux.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index eeff55560759..636c9259b345 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -11510,6 +11510,12 @@ L: linux-usb@vger.kernel.org > S: Maintained > F: drivers/usb/musb/ > > +MXDS GMUX DRIVER > +M: Daniel Dadap <ddadap@nvidia.com> > +L: platform-driver-x86@vger.kernel.org > +S: Supported > +F: drivers/platform/x86/mxds-gmux.c > + > MXL301RF MEDIA DRIVER > M: Akihiro Tsukada <tskd08@gmail.com> > L: linux-media@vger.kernel.org > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 0ad7ad8cf8e1..f2fef1e8e8d9 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -1368,6 +1368,15 @@ config INTEL_TELEMETRY > directly via debugfs files. Various tools may use > this interface for SoC state monitoring. > > +config MXDS_GMUX > + tristate "ACPI MXDS Gmux Driver" > + depends on ACPI_WMI > + depends on ACPI > + depends on VGA_SWITCHEROO > + ---help--- > + This driver provides support for ACPI-driven gmux devices which are > + present on some notebook designs with hybrid graphics. > + > endif # X86_PLATFORM_DEVICES > > config PMC_ATOM > diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile > index 53408d965874..bc75b1f42057 100644 > --- a/drivers/platform/x86/Makefile > +++ b/drivers/platform/x86/Makefile > @@ -146,3 +146,5 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o intel_telemetry_pltdrv.o intel_telemetry_debugfs.o > obj-$(CONFIG_PMC_ATOM) += pmc_atom.o > + > +obj-$(CONFIG_MXDS_GMUX) += mxds-gmux.o > diff --git a/drivers/platform/x86/mxds-gmux.c b/drivers/platform/x86/mxds-gmux.c > new file mode 100644 > index 000000000000..c6c5973bde80 > --- /dev/null > +++ b/drivers/platform/x86/mxds-gmux.c > @@ -0,0 +1,268 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * mxds-gmux: vga_switcheroo mux handler for ACPI MXDS muxes > + * > + * Copyright (C) 2020 NVIDIA Corporation > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > + * more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses>. > + * > + */ > + > +#include <linux/module.h> > +#include <linux/acpi.h> > +#include <linux/pci.h> > +#include <linux/vga_switcheroo.h> > +#include <linux/delay.h> > + > +MODULE_LICENSE("GPL v2"); > +MODULE_DESCRIPTION("vga_switcheroo mux handler for ACPI MXDS muxes"); > +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>"); > + > +/* > + * The mux doesn't have its own ACPI HID/CID, or WMI wrapper, so key off of ^ There is an extra space here in contrast to the next line. > + * the WMI wrapper for the related WMAA method for backlight control. > + */ > +MODULE_ALIAS("wmi:603E9613-EF25-4338-A3D0-C46177516DB7"); > + > +static struct pci_dev *ig_dev, *dg_dev; > +static acpi_handle internal_mux_handle; > +static acpi_handle external_mux_handle; > +static int vga_switcheroo_registered; > + Later in the code "true" is used, so why not use bool for the type of vga_switcheroo_registered? > +enum acpi_method { > + MXDM, > + MXDS, > +}; > + These constants are later used to index an array, and in such cases, - I don't know about everybody, but - I prefer if they are explicitly given a value, at least the first one. > +static char *acpi_methods[] = { > + [MXDM] = "MXDM", > + [MXDS] = "MXDS", > +}; > + > +enum mux_mode { > + MUX_MODE_GET = 0, > + MUX_MODE_DGPU_ONLY = 1, > + MUX_MODE_IGPU_ONLY = 2, > + MUX_MODE_MSHYBRID = 3, > + MUX_MODE_DYNAMIC = 4, > +}; > + I think this could be improved by separating the commands and returned values into their respective enums. > +/* > + * Call MXDS with argument value 0 to read the current state. > + * When reading, a return value of 1 means iGPU and 2 means dGPU. > + * Call MXDS with bit 0 set to change the current state. > + * When changing state, clear bit 4 for iGPU and set bit 4 for dGPU. > + */ > + > +enum mux_state { > + MUX_STATE_GET = 0, > + MUX_STATE_SET_IGPU = 0x01, > + MUX_STATE_IGPU = 1, > + MUX_STATE_DGPU = 2, > + MUX_STATE_SET_DGPU = 0x11, > +}; > + Same here, I think commands and returned values should be separated. > +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method, > + acpi_integer action) > +{ > + union acpi_object arg; > + struct acpi_object_list in = {.count = 1, .pointer = &arg}; > + struct acpi_buffer buf = { > + .length = ACPI_ALLOCATE_BUFFER, > + .pointer = NULL, > + }; > + acpi_integer ret = 0; > + > + arg.integer.type = ACPI_TYPE_INTEGER; > + arg.integer.value = action; > + > + if (!ACPI_FAILURE(acpi_evaluate_object(handle, acpi_methods[method], > + &in, &buf))) { > + union acpi_object *obj = buf.pointer; > + > + if (obj && obj->type == ACPI_TYPE_INTEGER) > + ret = obj->integer.value; > + } > + > + return ret; > +} > + The allocated buffer is not freed. Furthermore, wouldn't acpi_evalute_integer() be a better fit? > +static acpi_integer get_mux_mode(acpi_handle handle) > +{ > + return acpi_helper(handle, MXDM, MUX_MODE_GET); > +} > + > +static acpi_integer get_mux_state(acpi_handle handle) > +{ > + return acpi_helper(handle, MXDS, MUX_STATE_GET); > +} > + > +static void set_mux_state(acpi_handle handle, enum mux_state state) > +{ > + switch (state) { > + case MUX_STATE_IGPU: > + state = MUX_STATE_SET_IGPU; > + break; > + case MUX_STATE_DGPU: > + case MUX_STATE_SET_DGPU: > + state = MUX_STATE_SET_DGPU; > + break; What's the reason for this inconsistency? MUX_STATE_SET_DGPU is handled, but MUX_STATE_SET_IGPU is not. > + default: > + state = MUX_STATE_GET; > + break; > + } > + > + acpi_helper(handle, MXDS, state); > +} > + > +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id) > +{ > + enum mux_state state_set_cmd, target_state; > + > + if (!internal_mux_handle && !external_mux_handle) > + return -ENOTSUPP; > + This condition cannot be true, no? mxds_gmux_init() returns -ENODEV if this condition is true, so the module is not even loaded in that situation; and I don't see anything that could modify these handles after the module is loaded. Am I missing something? > + switch (id) { > + case VGA_SWITCHEROO_IGD: > + state_set_cmd = MUX_STATE_SET_IGPU; > + target_state = MUX_STATE_IGPU; > + break; > + case VGA_SWITCHEROO_DIS: > + state_set_cmd = MUX_STATE_SET_DGPU; > + target_state = MUX_STATE_DGPU; > + break; > + default: > + return -EINVAL; > + } > + > + if (internal_mux_handle) { > + set_mux_state(internal_mux_handle, state_set_cmd); > + if (get_mux_state(internal_mux_handle) != target_state) > + return -EAGAIN; > + } > + > + if (external_mux_handle) { > + set_mux_state(external_mux_handle, state_set_cmd); > + if (get_mux_state(external_mux_handle) != target_state) > + return -EAGAIN; > + } > + > + /* DP AUX can take up to 100ms to settle after mux switch */ > + mdelay(100); > + > + return 0; > +} > + > +static enum vga_switcheroo_client_id mxds_gmux_get_client_id( > + struct pci_dev *dev) > +{ > + if (dev) { > + if (ig_dev && dev->vendor == ig_dev->vendor) > + return VGA_SWITCHEROO_IGD; > + if (dg_dev && dev->vendor == dg_dev->vendor) > + return VGA_SWITCHEROO_DIS; > + } > + > + return VGA_SWITCHEROO_UNKNOWN_ID; > +} > + > +static acpi_status find_acpi_methods( > + acpi_handle object, u32 nesting_level, void *context, > + void **return_value) > +{ > + acpi_handle search; > + > + /* If either MXDM or MXDS is missing, we can't use this object */ > + if (acpi_get_handle(object, "MXDM", &search)) > + return 0; > + if (acpi_get_handle(object, "MXDS", &search)) > + return 0; > + > + /* MXDS only works when MXDM indicates dynamic mode */ > + if (get_mux_mode(object) != MUX_MODE_DYNAMIC) > + return 0; > + > + /* Internal display has _BCL; external does not */ > + if (acpi_get_handle(object, "_BCL", &search)) > + external_mux_handle = object; > + else > + internal_mux_handle = object; > + > + return 0; > +} > + > +static int mxds_gmux_init(void) > +{ > + int ret = 0; > + struct pci_dev *dev = NULL; > + static struct vga_switcheroo_handler handler = { > + .switchto = mxds_gmux_switchto, > + .get_client_id = mxds_gmux_get_client_id, > + }; > + Any reason why "handler" is inside the function and not const? > + while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) { > + switch (dev->vendor) { > + case 0x8086: > + pci_dev_put(ig_dev); > + ig_dev = pci_dev_get(dev); > + break; > + case 0x10de: > + pci_dev_put(dg_dev); > + dg_dev = pci_dev_get(dev); > + break; > + default: > + break; > + } > + } > + I think it should be mentioned somewhere that it only works with nvidia and intel GPUs (as far as I can see). Furthermore, maybe the PCI_VENDOR_ID_INTEL and PCI_VENDOR_ID_NVIDIA defines from include/linux/pci_ids.h could be used here. Regardless of how improbable, I am wondering what happens if an external GPU is connected to a dual-GPU laptop? Cannot that interfere with this device lookup logic? > + /* Require both integrated and discrete GPUs */ > + if (!ig_dev || !dg_dev) { > + ret = -ENODEV; > + goto done; > + } > + > + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, > + find_acpi_methods, NULL, NULL, NULL); > + > + /* Require at least one mux */ > + if (!internal_mux_handle && !external_mux_handle) { > + ret = -ENODEV; > + goto done; > + } > + > + ret = vga_switcheroo_register_handler(&handler, 0); > + > + if (ret) > + goto done; > + > + vga_switcheroo_registered = true; > + > +done: > + > + if (ret) { > + pci_dev_put(ig_dev); > + pci_dev_put(dg_dev); > + } > + > + return ret; > +} > +module_init(mxds_gmux_init); > + > +static void mxds_gmux_fini(void) > +{ > + if (vga_switcheroo_registered) > + vga_switcheroo_unregister_handler(); > + pci_dev_put(ig_dev); > + pci_dev_put(dg_dev); > +} > +module_exit(mxds_gmux_fini); > -- > 2.18.4 Subsystem maintainers CCd. Barnabás Pőcze
Thanks for your comments. A v2 patch will follow after I've had some time to test it more; in the meantime, I've responded to your suggestions inline: On 7/27/20 7:11 PM, Barnabás Pőcze wrote: > External email: Use caution opening links or attachments > > > I am no authority to say if this patch is good or bad, but I hope to help with my comments. > > >> Some upcoming notebook designs utilize display muxes driven by a >> pair of ACPI methods, MXDM to query and configure the operational >> mode of the mux (integrated only, discrete only, hybrid non-muxed, >> hybrid with dynamic mux switching), and MXDS to query and set the >> mux state when running in dynamic switch mode. >> >> Add a vga-switcheroo driver to support switching the mux on systems >> with the ACPI MXDM/MXDS interface. The mux mode cannot be changed >> dynamically (calling MXDM to change the mode won't have effect until >> the next boot, and calling MXDM to read the mux mode returns the >> active mode, not the mode that will be enabled on next boot), and >> MXDS only works when the mux mode is set to dynamic switch, so this >> driver will fail to load when MXDM reports any non-dynamic mode. >> >> Signed-off-by: Daniel Dadap <ddadap@nvidia.com> >> --- >> MAINTAINERS | 6 + >> drivers/platform/x86/Kconfig | 9 ++ >> drivers/platform/x86/Makefile | 2 + >> drivers/platform/x86/mxds-gmux.c | 268 +++++++++++++++++++++++++++++++ >> 4 files changed, 285 insertions(+) >> create mode 100644 drivers/platform/x86/mxds-gmux.c >> >> diff --git a/MAINTAINERS b/MAINTAINERS >> index eeff55560759..636c9259b345 100644 >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -11510,6 +11510,12 @@ L: linux-usb@vger.kernel.org >> S: Maintained >> F: drivers/usb/musb/ >> >> +MXDS GMUX DRIVER >> +M: Daniel Dadap <ddadap@nvidia.com> >> +L: platform-driver-x86@vger.kernel.org >> +S: Supported >> +F: drivers/platform/x86/mxds-gmux.c >> + >> MXL301RF MEDIA DRIVER >> M: Akihiro Tsukada <tskd08@gmail.com> >> L: linux-media@vger.kernel.org >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >> index 0ad7ad8cf8e1..f2fef1e8e8d9 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -1368,6 +1368,15 @@ config INTEL_TELEMETRY >> directly via debugfs files. Various tools may use >> this interface for SoC state monitoring. >> >> +config MXDS_GMUX >> + tristate "ACPI MXDS Gmux Driver" >> + depends on ACPI_WMI >> + depends on ACPI >> + depends on VGA_SWITCHEROO >> + ---help--- >> + This driver provides support for ACPI-driven gmux devices which are >> + present on some notebook designs with hybrid graphics. >> + >> endif # X86_PLATFORM_DEVICES >> >> config PMC_ATOM >> diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile >> index 53408d965874..bc75b1f42057 100644 >> --- a/drivers/platform/x86/Makefile >> +++ b/drivers/platform/x86/Makefile >> @@ -146,3 +146,5 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o intel_telemetry_pltdrv.o intel_telemetry_debugfs.o >> obj-$(CONFIG_PMC_ATOM) += pmc_atom.o >> + >> +obj-$(CONFIG_MXDS_GMUX) += mxds-gmux.o >> diff --git a/drivers/platform/x86/mxds-gmux.c b/drivers/platform/x86/mxds-gmux.c >> new file mode 100644 >> index 000000000000..c6c5973bde80 >> --- /dev/null >> +++ b/drivers/platform/x86/mxds-gmux.c >> @@ -0,0 +1,268 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * mxds-gmux: vga_switcheroo mux handler for ACPI MXDS muxes >> + * >> + * Copyright (C) 2020 NVIDIA Corporation >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see <http://www.gnu.org/licenses>. >> + * >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/acpi.h> >> +#include <linux/pci.h> >> +#include <linux/vga_switcheroo.h> >> +#include <linux/delay.h> >> + >> +MODULE_LICENSE("GPL v2"); >> +MODULE_DESCRIPTION("vga_switcheroo mux handler for ACPI MXDS muxes"); >> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>"); >> + >> +/* >> + * The mux doesn't have its own ACPI HID/CID, or WMI wrapper, so key off of > ^ > There is an extra space here in contrast to the next line. > > >> + * the WMI wrapper for the related WMAA method for backlight control. >> + */ >> +MODULE_ALIAS("wmi:603E9613-EF25-4338-A3D0-C46177516DB7"); >> + >> +static struct pci_dev *ig_dev, *dg_dev; >> +static acpi_handle internal_mux_handle; >> +static acpi_handle external_mux_handle; >> +static int vga_switcheroo_registered; >> + > Later in the code "true" is used, so why not use bool for the type of vga_switcheroo_registered? > > >> +enum acpi_method { >> + MXDM, >> + MXDS, >> +}; >> + > These constants are later used to index an array, and in such cases, - I don't know about everybody, but - I prefer if they are explicitly given a value, at least the first one. > > >> +static char *acpi_methods[] = { >> + [MXDM] = "MXDM", >> + [MXDS] = "MXDS", >> +}; >> + >> +enum mux_mode { >> + MUX_MODE_GET = 0, >> + MUX_MODE_DGPU_ONLY = 1, >> + MUX_MODE_IGPU_ONLY = 2, >> + MUX_MODE_MSHYBRID = 3, >> + MUX_MODE_DYNAMIC = 4, >> +}; >> + > I think this could be improved by separating the commands and returned values into their respective enums. > > >> +/* >> + * Call MXDS with argument value 0 to read the current state. >> + * When reading, a return value of 1 means iGPU and 2 means dGPU. >> + * Call MXDS with bit 0 set to change the current state. >> + * When changing state, clear bit 4 for iGPU and set bit 4 for dGPU. >> + */ >> + >> +enum mux_state { >> + MUX_STATE_GET = 0, >> + MUX_STATE_SET_IGPU = 0x01, >> + MUX_STATE_IGPU = 1, >> + MUX_STATE_DGPU = 2, >> + MUX_STATE_SET_DGPU = 0x11, >> +}; >> + > Same here, I think commands and returned values should be separated. > Good suggestions; thanks. >> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method, >> + acpi_integer action) >> +{ >> + union acpi_object arg; >> + struct acpi_object_list in = {.count = 1, .pointer = &arg}; >> + struct acpi_buffer buf = { >> + .length = ACPI_ALLOCATE_BUFFER, >> + .pointer = NULL, >> + }; >> + acpi_integer ret = 0; >> + >> + arg.integer.type = ACPI_TYPE_INTEGER; >> + arg.integer.value = action; >> + >> + if (!ACPI_FAILURE(acpi_evaluate_object(handle, acpi_methods[method], >> + &in, &buf))) { >> + union acpi_object *obj = buf.pointer; >> + >> + if (obj && obj->type == ACPI_TYPE_INTEGER) >> + ret = obj->integer.value; >> + } >> + >> + return ret; >> +} >> + > The allocated buffer is not freed. Furthermore, wouldn't acpi_evalute_integer() be a better fit? It would. > >> +static acpi_integer get_mux_mode(acpi_handle handle) >> +{ >> + return acpi_helper(handle, MXDM, MUX_MODE_GET); >> +} >> + >> +static acpi_integer get_mux_state(acpi_handle handle) >> +{ >> + return acpi_helper(handle, MXDS, MUX_STATE_GET); >> +} >> + >> +static void set_mux_state(acpi_handle handle, enum mux_state state) >> +{ >> + switch (state) { >> + case MUX_STATE_IGPU: >> + state = MUX_STATE_SET_IGPU; >> + break; >> + case MUX_STATE_DGPU: >> + case MUX_STATE_SET_DGPU: >> + state = MUX_STATE_SET_DGPU; >> + break; > What's the reason for this inconsistency? MUX_STATE_SET_DGPU is handled, but MUX_STATE_SET_IGPU is not. Thanks for catching this. It's an artifact of an earlier version of this driver, which had an additional interface separate from vga-switcheroo, and which accepted the _SET variant. I've simplified the implementation of set_mux_state() now that this no longer needs to be supported. > > >> + default: >> + state = MUX_STATE_GET; >> + break; >> + } >> + >> + acpi_helper(handle, MXDS, state); >> +} >> + >> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id) >> +{ >> + enum mux_state state_set_cmd, target_state; >> + >> + if (!internal_mux_handle && !external_mux_handle) >> + return -ENOTSUPP; >> + > This condition cannot be true, no? mxds_gmux_init() returns -ENODEV if this condition is true, so the module is not even loaded in that situation; and I don't see anything that could modify these handles after the module is loaded. Am I missing something? You're right. This check is unnecessary. In an earlier version of this driver it was possible to load the module in the absence of the mux handles. > >> + switch (id) { >> + case VGA_SWITCHEROO_IGD: >> + state_set_cmd = MUX_STATE_SET_IGPU; >> + target_state = MUX_STATE_IGPU; >> + break; >> + case VGA_SWITCHEROO_DIS: >> + state_set_cmd = MUX_STATE_SET_DGPU; >> + target_state = MUX_STATE_DGPU; >> + break; >> + default: >> + return -EINVAL; >> + } >> + >> + if (internal_mux_handle) { >> + set_mux_state(internal_mux_handle, state_set_cmd); >> + if (get_mux_state(internal_mux_handle) != target_state) >> + return -EAGAIN; >> + } >> + >> + if (external_mux_handle) { >> + set_mux_state(external_mux_handle, state_set_cmd); >> + if (get_mux_state(external_mux_handle) != target_state) >> + return -EAGAIN; >> + } >> + >> + /* DP AUX can take up to 100ms to settle after mux switch */ >> + mdelay(100); >> + >> + return 0; >> +} >> + >> +static enum vga_switcheroo_client_id mxds_gmux_get_client_id( >> + struct pci_dev *dev) >> +{ >> + if (dev) { >> + if (ig_dev && dev->vendor == ig_dev->vendor) >> + return VGA_SWITCHEROO_IGD; >> + if (dg_dev && dev->vendor == dg_dev->vendor) >> + return VGA_SWITCHEROO_DIS; >> + } >> + >> + return VGA_SWITCHEROO_UNKNOWN_ID; >> +} >> + >> +static acpi_status find_acpi_methods( >> + acpi_handle object, u32 nesting_level, void *context, >> + void **return_value) >> +{ >> + acpi_handle search; >> + >> + /* If either MXDM or MXDS is missing, we can't use this object */ >> + if (acpi_get_handle(object, "MXDM", &search)) >> + return 0; >> + if (acpi_get_handle(object, "MXDS", &search)) >> + return 0; >> + >> + /* MXDS only works when MXDM indicates dynamic mode */ >> + if (get_mux_mode(object) != MUX_MODE_DYNAMIC) >> + return 0; >> + >> + /* Internal display has _BCL; external does not */ >> + if (acpi_get_handle(object, "_BCL", &search)) >> + external_mux_handle = object; >> + else >> + internal_mux_handle = object; >> + >> + return 0; >> +} >> + >> +static int mxds_gmux_init(void) >> +{ >> + int ret = 0; >> + struct pci_dev *dev = NULL; >> + static struct vga_switcheroo_handler handler = { >> + .switchto = mxds_gmux_switchto, >> + .get_client_id = mxds_gmux_get_client_id, >> + }; >> + > Any reason why "handler" is inside the function and not const? > You're right, it can be const. I have it in the function (with static storage) because we don't need to reference it anywhere else. I'd think the static storage would allow the pointer to the struct to stay alive even after the init function exits, but if you think it would be better to have it out of the function's scope I can move it. >> + while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) { >> + switch (dev->vendor) { >> + case 0x8086: >> + pci_dev_put(ig_dev); >> + ig_dev = pci_dev_get(dev); >> + break; >> + case 0x10de: >> + pci_dev_put(dg_dev); >> + dg_dev = pci_dev_get(dev); >> + break; >> + default: >> + break; >> + } >> + } >> + > I think it should be mentioned somewhere that it only works with nvidia and intel GPUs (as far as I can see). Furthermore, maybe the PCI_VENDOR_ID_INTEL and PCI_VENDOR_ID_NVIDIA defines from include/linux/pci_ids.h could be used here. > > Regardless of how improbable, I am wondering what happens if an external GPU is connected to a dual-GPU laptop? Cannot that interfere with this device lookup logic? I don't think it'll be a problem, since an external GPU won't have an implementation of the MXDM/MXDS methods in its associated ACPI node, so even if the eGPU is plugged in at the time this module loads, it should fail to initialize unless there is also an internal discrete GPU which does support MXDM/MXDS. > > >> + /* Require both integrated and discrete GPUs */ >> + if (!ig_dev || !dg_dev) { >> + ret = -ENODEV; >> + goto done; >> + } >> + >> + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, >> + find_acpi_methods, NULL, NULL, NULL); >> + >> + /* Require at least one mux */ >> + if (!internal_mux_handle && !external_mux_handle) { >> + ret = -ENODEV; >> + goto done; >> + } >> + >> + ret = vga_switcheroo_register_handler(&handler, 0); >> + >> + if (ret) >> + goto done; >> + >> + vga_switcheroo_registered = true; >> + >> +done: >> + >> + if (ret) { >> + pci_dev_put(ig_dev); >> + pci_dev_put(dg_dev); >> + } >> + >> + return ret; >> +} >> +module_init(mxds_gmux_init); >> + >> +static void mxds_gmux_fini(void) >> +{ >> + if (vga_switcheroo_registered) >> + vga_switcheroo_unregister_handler(); >> + pci_dev_put(ig_dev); >> + pci_dev_put(dg_dev); >> +} >> +module_exit(mxds_gmux_fini); >> -- >> 2.18.4 > > Subsystem maintainers CCd. > > > Barnabás Pőcze
2020. július 29., szerda 5:17 keltezéssel, Daniel Dadap <ddadap@nvidia.com> írta: > Thanks for your comments. A v2 patch will follow after I've had some > time to test it more; in the meantime, I've responded to your > suggestions inline: > > [...] > >> + > >> +static int mxds_gmux_init(void) > >> +{ > >> + int ret = 0; > >> + struct pci_dev *dev = NULL; > >> + static struct vga_switcheroo_handler handler = { > >> + .switchto = mxds_gmux_switchto, > >> + .get_client_id = mxds_gmux_get_client_id, > >> + }; > >> + > > Any reason why "handler" is inside the function and not const? > > > > You're right, it can be const. I have it in the function (with static > storage) because we don't need to reference it anywhere else. I'd think > the static storage would allow the pointer to the struct to stay alive > even after the init function exits, but if you think it would be better > to have it out of the function's scope I can move it. > I see. I think having it out of the function better signals the intention that this variable is supposed to live as long as the module is loaded. Furthermore, - although I am not sure about this, but - I think having a function static variable here prevents mxds_gmux_init() from being marked __init. Also, I forgot to mention before, but mxds_gmux_fini() is not marked __exit. Any reason why? > [...] Barnabás Pőcze
On 7/29/20 5:29 AM, Barnabás Pőcze wrote: > 2020. július 29., szerda 5:17 keltezéssel, Daniel Dadap <ddadap@nvidia.com> írta: > >> Thanks for your comments. A v2 patch will follow after I've had some >> time to test it more; in the meantime, I've responded to your >> suggestions inline: >> >> [...] >>>> + >>>> +static int mxds_gmux_init(void) >>>> +{ >>>> + int ret = 0; >>>> + struct pci_dev *dev = NULL; >>>> + static struct vga_switcheroo_handler handler = { >>>> + .switchto = mxds_gmux_switchto, >>>> + .get_client_id = mxds_gmux_get_client_id, >>>> + }; >>>> + >>> Any reason why "handler" is inside the function and not const? >>> >> You're right, it can be const. I have it in the function (with static >> storage) because we don't need to reference it anywhere else. I'd think >> the static storage would allow the pointer to the struct to stay alive >> even after the init function exits, but if you think it would be better >> to have it out of the function's scope I can move it. >> > I see. I think having it out of the function better signals the intention that > this variable is supposed to live as long as the module is loaded. Furthermore, > - although I am not sure about this, but - I think having a function static variable > here prevents mxds_gmux_init() from being marked __init. > > Also, I forgot to mention before, but mxds_gmux_fini() is not marked __exit. > Any reason why? The same reason mxds_gmux_init() wasn't marked __init: an oversight.
On Tue, Jul 28, 2020 at 10:17:19PM -0500, Daniel Dadap wrote: > On 7/27/20 7:11 PM, Barnabás P??cze wrote: > > > + while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) { > > > + switch (dev->vendor) { > > > + case 0x8086: > > > + pci_dev_put(ig_dev); > > > + ig_dev = pci_dev_get(dev); > > > + break; > > > + case 0x10de: > > > + pci_dev_put(dg_dev); > > > + dg_dev = pci_dev_get(dev); > > > + break; > > > + default: > > > + break; > > > + } > > > + } > > > + > > > > Regardless of how improbable, I am wondering what happens if an > > external GPU is connected to a dual-GPU laptop? Cannot that > > interfere with this device lookup logic? > > I don't think it'll be a problem, since an external GPU won't have an > implementation of the MXDM/MXDS methods in its associated ACPI node, so even > if the eGPU is plugged in at the time this module loads, it should fail to > initialize unless there is also an internal discrete GPU which does support > MXDM/MXDS. Still, dg_dev may point to the wrong device. You can avoid it by adding the following at the top of the while loop: if (pci_is_thunderbolt_attached(dev)) continue; Thanks, Lukas
diff --git a/MAINTAINERS b/MAINTAINERS index eeff55560759..636c9259b345 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11510,6 +11510,12 @@ L: linux-usb@vger.kernel.org S: Maintained F: drivers/usb/musb/ +MXDS GMUX DRIVER +M: Daniel Dadap <ddadap@nvidia.com> +L: platform-driver-x86@vger.kernel.org +S: Supported +F: drivers/platform/x86/mxds-gmux.c + MXL301RF MEDIA DRIVER M: Akihiro Tsukada <tskd08@gmail.com> L: linux-media@vger.kernel.org diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 0ad7ad8cf8e1..f2fef1e8e8d9 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -1368,6 +1368,15 @@ config INTEL_TELEMETRY directly via debugfs files. Various tools may use this interface for SoC state monitoring. +config MXDS_GMUX + tristate "ACPI MXDS Gmux Driver" + depends on ACPI_WMI + depends on ACPI + depends on VGA_SWITCHEROO + ---help--- + This driver provides support for ACPI-driven gmux devices which are + present on some notebook designs with hybrid graphics. + endif # X86_PLATFORM_DEVICES config PMC_ATOM diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 53408d965874..bc75b1f42057 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -146,3 +146,5 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \ intel_telemetry_pltdrv.o \ intel_telemetry_debugfs.o obj-$(CONFIG_PMC_ATOM) += pmc_atom.o + +obj-$(CONFIG_MXDS_GMUX) += mxds-gmux.o diff --git a/drivers/platform/x86/mxds-gmux.c b/drivers/platform/x86/mxds-gmux.c new file mode 100644 index 000000000000..c6c5973bde80 --- /dev/null +++ b/drivers/platform/x86/mxds-gmux.c @@ -0,0 +1,268 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * mxds-gmux: vga_switcheroo mux handler for ACPI MXDS muxes + * + * Copyright (C) 2020 NVIDIA Corporation + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses>. + * + */ + +#include <linux/module.h> +#include <linux/acpi.h> +#include <linux/pci.h> +#include <linux/vga_switcheroo.h> +#include <linux/delay.h> + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("vga_switcheroo mux handler for ACPI MXDS muxes"); +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>"); + +/* + * The mux doesn't have its own ACPI HID/CID, or WMI wrapper, so key off of + * the WMI wrapper for the related WMAA method for backlight control. + */ +MODULE_ALIAS("wmi:603E9613-EF25-4338-A3D0-C46177516DB7"); + +static struct pci_dev *ig_dev, *dg_dev; +static acpi_handle internal_mux_handle; +static acpi_handle external_mux_handle; +static int vga_switcheroo_registered; + +enum acpi_method { + MXDM, + MXDS, +}; + +static char *acpi_methods[] = { + [MXDM] = "MXDM", + [MXDS] = "MXDS", +}; + +enum mux_mode { + MUX_MODE_GET = 0, + MUX_MODE_DGPU_ONLY = 1, + MUX_MODE_IGPU_ONLY = 2, + MUX_MODE_MSHYBRID = 3, + MUX_MODE_DYNAMIC = 4, +}; + +/* + * Call MXDS with argument value 0 to read the current state. + * When reading, a return value of 1 means iGPU and 2 means dGPU. + * Call MXDS with bit 0 set to change the current state. + * When changing state, clear bit 4 for iGPU and set bit 4 for dGPU. + */ + +enum mux_state { + MUX_STATE_GET = 0, + MUX_STATE_SET_IGPU = 0x01, + MUX_STATE_IGPU = 1, + MUX_STATE_DGPU = 2, + MUX_STATE_SET_DGPU = 0x11, +}; + +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method, + acpi_integer action) +{ + union acpi_object arg; + struct acpi_object_list in = {.count = 1, .pointer = &arg}; + struct acpi_buffer buf = { + .length = ACPI_ALLOCATE_BUFFER, + .pointer = NULL, + }; + acpi_integer ret = 0; + + arg.integer.type = ACPI_TYPE_INTEGER; + arg.integer.value = action; + + if (!ACPI_FAILURE(acpi_evaluate_object(handle, acpi_methods[method], + &in, &buf))) { + union acpi_object *obj = buf.pointer; + + if (obj && obj->type == ACPI_TYPE_INTEGER) + ret = obj->integer.value; + } + + return ret; +} + +static acpi_integer get_mux_mode(acpi_handle handle) +{ + return acpi_helper(handle, MXDM, MUX_MODE_GET); +} + +static acpi_integer get_mux_state(acpi_handle handle) +{ + return acpi_helper(handle, MXDS, MUX_STATE_GET); +} + +static void set_mux_state(acpi_handle handle, enum mux_state state) +{ + switch (state) { + case MUX_STATE_IGPU: + state = MUX_STATE_SET_IGPU; + break; + case MUX_STATE_DGPU: + case MUX_STATE_SET_DGPU: + state = MUX_STATE_SET_DGPU; + break; + default: + state = MUX_STATE_GET; + break; + } + + acpi_helper(handle, MXDS, state); +} + +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id) +{ + enum mux_state state_set_cmd, target_state; + + if (!internal_mux_handle && !external_mux_handle) + return -ENOTSUPP; + + switch (id) { + case VGA_SWITCHEROO_IGD: + state_set_cmd = MUX_STATE_SET_IGPU; + target_state = MUX_STATE_IGPU; + break; + case VGA_SWITCHEROO_DIS: + state_set_cmd = MUX_STATE_SET_DGPU; + target_state = MUX_STATE_DGPU; + break; + default: + return -EINVAL; + } + + if (internal_mux_handle) { + set_mux_state(internal_mux_handle, state_set_cmd); + if (get_mux_state(internal_mux_handle) != target_state) + return -EAGAIN; + } + + if (external_mux_handle) { + set_mux_state(external_mux_handle, state_set_cmd); + if (get_mux_state(external_mux_handle) != target_state) + return -EAGAIN; + } + + /* DP AUX can take up to 100ms to settle after mux switch */ + mdelay(100); + + return 0; +} + +static enum vga_switcheroo_client_id mxds_gmux_get_client_id( + struct pci_dev *dev) +{ + if (dev) { + if (ig_dev && dev->vendor == ig_dev->vendor) + return VGA_SWITCHEROO_IGD; + if (dg_dev && dev->vendor == dg_dev->vendor) + return VGA_SWITCHEROO_DIS; + } + + return VGA_SWITCHEROO_UNKNOWN_ID; +} + +static acpi_status find_acpi_methods( + acpi_handle object, u32 nesting_level, void *context, + void **return_value) +{ + acpi_handle search; + + /* If either MXDM or MXDS is missing, we can't use this object */ + if (acpi_get_handle(object, "MXDM", &search)) + return 0; + if (acpi_get_handle(object, "MXDS", &search)) + return 0; + + /* MXDS only works when MXDM indicates dynamic mode */ + if (get_mux_mode(object) != MUX_MODE_DYNAMIC) + return 0; + + /* Internal display has _BCL; external does not */ + if (acpi_get_handle(object, "_BCL", &search)) + external_mux_handle = object; + else + internal_mux_handle = object; + + return 0; +} + +static int mxds_gmux_init(void) +{ + int ret = 0; + struct pci_dev *dev = NULL; + static struct vga_switcheroo_handler handler = { + .switchto = mxds_gmux_switchto, + .get_client_id = mxds_gmux_get_client_id, + }; + + while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) { + switch (dev->vendor) { + case 0x8086: + pci_dev_put(ig_dev); + ig_dev = pci_dev_get(dev); + break; + case 0x10de: + pci_dev_put(dg_dev); + dg_dev = pci_dev_get(dev); + break; + default: + break; + } + } + + /* Require both integrated and discrete GPUs */ + if (!ig_dev || !dg_dev) { + ret = -ENODEV; + goto done; + } + + acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT, ACPI_UINT32_MAX, + find_acpi_methods, NULL, NULL, NULL); + + /* Require at least one mux */ + if (!internal_mux_handle && !external_mux_handle) { + ret = -ENODEV; + goto done; + } + + ret = vga_switcheroo_register_handler(&handler, 0); + + if (ret) + goto done; + + vga_switcheroo_registered = true; + +done: + + if (ret) { + pci_dev_put(ig_dev); + pci_dev_put(dg_dev); + } + + return ret; +} +module_init(mxds_gmux_init); + +static void mxds_gmux_fini(void) +{ + if (vga_switcheroo_registered) + vga_switcheroo_unregister_handler(); + pci_dev_put(ig_dev); + pci_dev_put(dg_dev); +} +module_exit(mxds_gmux_fini);
Some upcoming notebook designs utilize display muxes driven by a pair of ACPI methods, MXDM to query and configure the operational mode of the mux (integrated only, discrete only, hybrid non-muxed, hybrid with dynamic mux switching), and MXDS to query and set the mux state when running in dynamic switch mode. Add a vga-switcheroo driver to support switching the mux on systems with the ACPI MXDM/MXDS interface. The mux mode cannot be changed dynamically (calling MXDM to change the mode won't have effect until the next boot, and calling MXDM to read the mux mode returns the active mode, not the mode that will be enabled on next boot), and MXDS only works when the mux mode is set to dynamic switch, so this driver will fail to load when MXDM reports any non-dynamic mode. Signed-off-by: Daniel Dadap <ddadap@nvidia.com> --- MAINTAINERS | 6 + drivers/platform/x86/Kconfig | 9 ++ drivers/platform/x86/Makefile | 2 + drivers/platform/x86/mxds-gmux.c | 268 +++++++++++++++++++++++++++++++ 4 files changed, 285 insertions(+) create mode 100644 drivers/platform/x86/mxds-gmux.c