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