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@xxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx > S: Maintained > F: drivers/usb/musb/ > > +MXDS GMUX DRIVER > +M: Daniel Dadap <ddadap@xxxxxxxxxx> > +L: platform-driver-x86@xxxxxxxxxxxxxxx > +S: Supported > +F: drivers/platform/x86/mxds-gmux.c > + > MXL301RF MEDIA DRIVER > M: Akihiro Tsukada <tskd08@xxxxxxxxx> > L: linux-media@xxxxxxxxxxxxxxx > 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@xxxxxxxxxx>"); > + > +/* > + * 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
