[PATCH v3] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[PATCH v3] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

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.

This driver currently only supports systems with Intel integrated
graphics and NVIDIA discrete graphics. It will need to be updated if
designs are developed using the same interfaces which utilize GPUs
from other vendors.

v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>

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 | 267 +++++++++++++++++++++++++++++++
 4 files changed, 284 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..5d00ad1ffc0e 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..b79000733fae 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..070a64fc1b25
--- /dev/null
+++ b/drivers/platform/x86/mxds-gmux.c
@@ -0,0 +1,267 @@
+// 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>.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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 mxds_gmux_switchto(enum vga_switcheroo_client_id);
+static enum vga_switcheroo_client_id mxds_gmux_get_client_id(struct pci_dev *);
+
+static const struct vga_switcheroo_handler handler = {
+	.switchto = mxds_gmux_switchto,
+	.get_client_id = mxds_gmux_get_client_id,
+};
+
+enum acpi_method {
+	MXDM = 0,
+	MXDS,
+	NUM_ACPI_METHODS
+};
+
+static char *acpi_methods[NUM_ACPI_METHODS] = {
+	[MXDM] = "MXDM",
+	[MXDS] = "MXDS",
+};
+
+enum mux_mode_command {
+	MUX_MODE_GET = 0,
+};
+
+enum mux_mode {
+	MUX_MODE_DGPU_ONLY = 1,
+	MUX_MODE_IGPU_ONLY = 2,
+	MUX_MODE_MSHYBRID = 3,	/* Dual GPU, mux switched to iGPU */
+	MUX_MODE_DYNAMIC = 4,	/* Dual GPU, dynamic mux switching */
+};
+
+/*
+ * 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_IGPU = 1,
+	MUX_STATE_DGPU = 2,
+};
+
+enum mux_state_command {
+	MUX_STATE_GET = 0,
+	MUX_STATE_SET_IGPU = 0x01,
+	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};
+	acpi_integer ret;
+	acpi_status status;
+
+	arg.integer.type = ACPI_TYPE_INTEGER;
+	arg.integer.value = action;
+
+	status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
+
+	if (ACPI_FAILURE(status)) {
+		pr_err("ACPI %s failed: %s\n", acpi_methods[method],
+			acpi_format_exception(status));
+		return 0;
+	}
+
+	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)
+{
+	enum mux_state_command command;
+
+	switch (state) {
+	case MUX_STATE_IGPU:
+		command = MUX_STATE_SET_IGPU;
+		break;
+	case MUX_STATE_DGPU:
+		command = MUX_STATE_SET_DGPU;
+		break;
+	default:
+		return;
+	}
+
+	acpi_helper(handle, MXDS, command);
+}
+
+static int mxds_gmux_switchto(enum vga_switcheroo_client_id id)
+{
+	enum mux_state state;
+
+	switch (id) {
+	case VGA_SWITCHEROO_IGD:
+		state = MUX_STATE_IGPU;
+		break;
+	case VGA_SWITCHEROO_DIS:
+		state = MUX_STATE_DGPU;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (internal_mux_handle) {
+		set_mux_state(internal_mux_handle, state);
+		if (get_mux_state(internal_mux_handle) != state)
+			return -EAGAIN;
+	}
+
+	if (external_mux_handle) {
+		set_mux_state(external_mux_handle, state);
+		if (get_mux_state(external_mux_handle) != 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 __init mxds_gmux_init(void)
+{
+	int ret = 0;
+	struct pci_dev *dev = NULL;
+
+	/* Currently only supports Intel integrated and NVIDIA discrete GPUs */
+	while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
+		switch (dev->vendor) {
+		case PCI_VENDOR_ID_INTEL:
+			pci_dev_put(ig_dev);
+			ig_dev = pci_dev_get(dev);
+			break;
+		case PCI_VENDOR_ID_NVIDIA:
+			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);
+
+done:
+
+	if (ret) {
+		pci_dev_put(ig_dev);
+		pci_dev_put(dg_dev);
+	}
+
+	return ret;
+}
+module_init(mxds_gmux_init);
+
+static void __exit mxds_gmux_exit(void)
+{
+	vga_switcheroo_unregister_handler();
+	pci_dev_put(ig_dev);
+	pci_dev_put(dg_dev);
+}
+module_exit(mxds_gmux_exit);
-- 
2.18.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Lukas Wunner]

On Wed, Jul 29, 2020 at 04:05:57PM -0500, Daniel Dadap wrote:
> + * 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>.

This boilerplate is unnecessary, the SPDX identifier is sufficient.

> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id);
> +static enum vga_switcheroo_client_id mxds_gmux_get_client_id(struct pci_dev *);
> +
> +static const struct vga_switcheroo_handler handler = {
> +	.switchto = mxds_gmux_switchto,
> +	.get_client_id = mxds_gmux_get_client_id,
> +};

Move the handler struct further down to avoid the forward declarations.

> + * 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_command {
> +	MUX_STATE_GET = 0,
> +	MUX_STATE_SET_IGPU = 0x01,
> +	MUX_STATE_SET_DGPU = 0x11,
> +};

It looks like the code comment is wrong and bit 1 (instead of bit 4) is
used to select the GPU.

> +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};
> +	acpi_integer ret;
> +	acpi_status status;
> +
> +	arg.integer.type = ACPI_TYPE_INTEGER;
> +	arg.integer.value = action;

Hm, why not use an initializer for "arg", as you do for "in"?

> +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;
> +	}

That's a little odd.  Why not use "ig_dev == dev" and "dg_dev == dev"?

> +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;

Since this function returns an acpi_status, all the return statements
should use AE_OK intead of 0.

Otherwise LGTM.

Please cc dri-devel when respinning since this concerns vga_switcheroo.

I'm also cc'ing Peter Wu who has lots of experience with hybrid graphics
through his involvement with Bumblebee, hence might be interested.
Searching through his collection of ACPI dumps, it seems MXDS and MXMX
have been present for years, but not MXDM:

https://github.com/search?q=user%3ALekensteyn+MXDS&type=Code

Thanks,

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

On 8/10/20 3:37 AM, Lukas Wunner wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Jul 29, 2020 at 04:05:57PM -0500, Daniel Dadap wrote:
>> + * 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>.
> This boilerplate is unnecessary, the SPDX identifier is sufficient.


I don't doubt that it's unnecessary, but this is the recommended 
boilerplate license text for NVIDIA-copyrighted GPLv2-licensed code by 
NVIDIA legal. Unless there's a compelling reason to omit it, I'll leave 
it as-is. If anybody feels strongly about removing it, I can ask our 
legal team for guidance.


>
>> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id);
>> +static enum vga_switcheroo_client_id mxds_gmux_get_client_id(struct pci_dev *);
>> +
>> +static const struct vga_switcheroo_handler handler = {
>> +     .switchto = mxds_gmux_switchto,
>> +     .get_client_id = mxds_gmux_get_client_id,
>> +};
> Move the handler struct further down to avoid the forward declarations.
>

Sure.


>> + * 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_command {
>> +     MUX_STATE_GET = 0,
>> +     MUX_STATE_SET_IGPU = 0x01,
>> +     MUX_STATE_SET_DGPU = 0x11,
>> +};
> It looks like the code comment is wrong and bit 1 (instead of bit 4) is
> used to select the GPU.


The code comment is correct. The enum values are hexadecimal, not 
binary. Would it be clearer to write it out as something like 0 << 4 & 1 
<< 0 for MUX_STATE_SET_IGPU and 1 << 4 & 1 << 0 for MUX_STATE_SET_DGPU?


>> +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};
>> +     acpi_integer ret;
>> +     acpi_status status;
>> +
>> +     arg.integer.type = ACPI_TYPE_INTEGER;
>> +     arg.integer.value = action;
> Hm, why not use an initializer for "arg", as you do for "in"?
>

Sure.


>> +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;
>> +     }
> That's a little odd.  Why not use "ig_dev == dev" and "dg_dev == dev"?


No reason; that is indeed better. I think originally these comparisons 
got a vendor ID from some other means.


>
>> +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;
> Since this function returns an acpi_status, all the return statements
> should use AE_OK intead of 0.


Okay.


> Otherwise LGTM.
>
> Please cc dri-devel when respinning since this concerns vga_switcheroo.


Will do, after testing the updated change. I'll leave the GPL 
boilerplate text and hexadecimal enum definitions as-is unless I hear 
otherwise.


> I'm also cc'ing Peter Wu who has lots of experience with hybrid graphics
> through his involvement with Bumblebee, hence might be interested.
> Searching through his collection of ACPI dumps, it seems MXDS and MXMX
> have been present for years, but not MXDM:
>
> https://github.com/search?q=user%3ALekensteyn+MXDS&type=Code


Yes, MXMX and MXDS go back a ways, it seems. I'm not familiar enough 
with the MXMX+MXDS designs to know if MXDS uses the same API in those 
designs as it doesn in the MXDM+MXDS designs. I'm not aware of any 
already available designs with MXDM. MXMX was used for switching DDC 
lines independently back when LVDS panels were the norm. The upcoming 
MXDM+MXDS designs are eDP-based and do not support independent DDC muxing.


> Thanks,
>
> Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Lukas Wunner]

On Mon, Aug 10, 2020 at 01:44:58PM -0500, Daniel Dadap wrote:
> On 8/10/20 3:37 AM, Lukas Wunner wrote:
> > On Wed, Jul 29, 2020 at 04:05:57PM -0500, Daniel Dadap wrote:
> > > + * 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_command {
> > > +     MUX_STATE_GET = 0,
> > > +     MUX_STATE_SET_IGPU = 0x01,
> > > +     MUX_STATE_SET_DGPU = 0x11,
> > > +};
> > It looks like the code comment is wrong and bit 1 (instead of bit 4) is
> > used to select the GPU.
> 
> The code comment is correct. The enum values are hexadecimal, not binary.

Ugh, missed that, sorry for the noise.

> Would it be clearer to write it out as something like 0 << 4 & 1 << 0 for
> MUX_STATE_SET_IGPU and 1 << 4 & 1 << 0 for MUX_STATE_SET_DGPU?

BIT(4) | BIT(0) might be clearer, but that gives you an unsigned long
and I'm not sure if gcc accepts that as an enum (=int) initializer.

> > > +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;
> > > +     }
> > That's a little odd.  Why not use "ig_dev == dev" and "dg_dev == dev"?
> 
> No reason; that is indeed better. I think originally these comparisons got a
> vendor ID from some other means.

Perhaps it was necessary in case an eGPU is attached, but that shouldn't
be an issue if you filter out Thunderbolt devices with
pci_is_thunderbolt_attached().

> Yes, MXMX and MXDS go back a ways, it seems. I'm not familiar enough with
> the MXMX+MXDS designs to know if MXDS uses the same API in those designs as
> it doesn in the MXDM+MXDS designs. I'm not aware of any already available
> designs with MXDM. MXMX was used for switching DDC lines independently back
> when LVDS panels were the norm. The upcoming MXDM+MXDS designs are eDP-based
> and do not support independent DDC muxing.

Interesting, thank you for the explanation.

Lukas
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

On 8/10/20 10:43 PM, Lukas Wunner wrote:
> On Mon, Aug 10, 2020 at 01:44:58PM -0500, Daniel Dadap wrote:
>
>> Would it be clearer to write it out as something like 0 << 4 & 1 << 0 for
>> MUX_STATE_SET_IGPU and 1 << 4 & 1 << 0 for MUX_STATE_SET_DGPU?
> BIT(4) | BIT(0) might be clearer, but that gives you an unsigned long
> and I'm not sure if gcc accepts that as an enum (=int) initializer.


Ah yes, I forgot the BIT() macro. And it does seem to work just fine for 
initializing enum values, and there does seem to be precedent in other 
code in the kernel for doing so. And of course I meant | instead of & 
earlier. :)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

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.

This driver currently only supports systems with Intel integrated
graphics and NVIDIA discrete graphics. It will need to be updated if
designs are developed using the same interfaces which utilize GPUs
from other vendors.

v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>

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 | 265 +++++++++++++++++++++++++++++++
 4 files changed, 282 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..5d00ad1ffc0e 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..b79000733fae 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..dd7f6edaf0f3
--- /dev/null
+++ b/drivers/platform/x86/mxds-gmux.c
@@ -0,0 +1,265 @@
+// 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>.
+ *
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#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;
+
+enum acpi_method {
+	MXDM = 0,
+	MXDS,
+	NUM_ACPI_METHODS
+};
+
+static char *acpi_methods[NUM_ACPI_METHODS] = {
+	[MXDM] = "MXDM",
+	[MXDS] = "MXDS",
+};
+
+enum mux_mode_command {
+	MUX_MODE_GET = 0,
+};
+
+enum mux_mode {
+	MUX_MODE_DGPU_ONLY = 1,
+	MUX_MODE_IGPU_ONLY = 2,
+	MUX_MODE_MSHYBRID = 3,	/* Dual GPU, mux switched to iGPU */
+	MUX_MODE_DYNAMIC = 4,	/* Dual GPU, dynamic mux switching */
+};
+
+/*
+ * 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_IGPU = 1,
+	MUX_STATE_DGPU = 2,
+};
+
+enum mux_state_command {
+	MUX_STATE_GET = 0,
+	MUX_STATE_SET_IGPU = BIT(0),
+	MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
+};
+
+static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
+				acpi_integer action)
+{
+	union acpi_object arg = {
+		.integer = { .type = ACPI_TYPE_INTEGER, .value = action }
+	};
+	struct acpi_object_list in = {.count = 1, .pointer = &arg};
+	acpi_integer ret;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
+
+	if (ACPI_FAILURE(status)) {
+		pr_err("ACPI %s failed: %s\n", acpi_methods[method],
+			acpi_format_exception(status));
+		return 0;
+	}
+
+	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)
+{
+	enum mux_state_command command;
+
+	switch (state) {
+	case MUX_STATE_IGPU:
+		command = MUX_STATE_SET_IGPU;
+		break;
+	case MUX_STATE_DGPU:
+		command = MUX_STATE_SET_DGPU;
+		break;
+	default:
+		return;
+	}
+
+	acpi_helper(handle, MXDS, command);
+}
+
+static int mxds_gmux_switchto(enum vga_switcheroo_client_id id)
+{
+	enum mux_state state;
+
+	switch (id) {
+	case VGA_SWITCHEROO_IGD:
+		state = MUX_STATE_IGPU;
+		break;
+	case VGA_SWITCHEROO_DIS:
+		state = MUX_STATE_DGPU;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (internal_mux_handle) {
+		set_mux_state(internal_mux_handle, state);
+		if (get_mux_state(internal_mux_handle) != state)
+			return -EAGAIN;
+	}
+
+	if (external_mux_handle) {
+		set_mux_state(external_mux_handle, state);
+		if (get_mux_state(external_mux_handle) != 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 == ig_dev)
+		return VGA_SWITCHEROO_IGD;
+	if (dev == dg_dev)
+		return VGA_SWITCHEROO_DIS;
+
+	return VGA_SWITCHEROO_UNKNOWN_ID;
+}
+
+static const struct vga_switcheroo_handler handler = {
+	.switchto = mxds_gmux_switchto,
+	.get_client_id = mxds_gmux_get_client_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 AE_OK;
+	if (acpi_get_handle(object, "MXDS", &search))
+		return AE_OK;
+
+	/* MXDS only works when MXDM indicates dynamic mode */
+	if (get_mux_mode(object) != MUX_MODE_DYNAMIC)
+		return AE_OK;
+
+	/* Internal display has _BCL; external does not */
+	if (acpi_get_handle(object, "_BCL", &search))
+		external_mux_handle = object;
+	else
+		internal_mux_handle = object;
+
+	return AE_OK;
+}
+
+static int __init mxds_gmux_init(void)
+{
+	int ret = 0;
+	struct pci_dev *dev = NULL;
+
+	/* Currently only supports Intel integrated and NVIDIA discrete GPUs */
+	while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
+		/* Ignore eGPU */
+		if (pci_is_thunderbolt_attached(dev))
+			continue;
+
+		switch (dev->vendor) {
+		case PCI_VENDOR_ID_INTEL:
+			pci_dev_put(ig_dev);
+			ig_dev = pci_dev_get(dev);
+			break;
+		case PCI_VENDOR_ID_NVIDIA:
+			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);
+
+done:
+
+	if (ret) {
+		pci_dev_put(ig_dev);
+		pci_dev_put(dg_dev);
+	}
+
+	return ret;
+}
+module_init(mxds_gmux_init);
+
+static void __exit mxds_gmux_exit(void)
+{
+	vga_switcheroo_unregister_handler();
+	pci_dev_put(ig_dev);
+	pci_dev_put(dg_dev);
+}
+module_exit(mxds_gmux_exit);
-- 
2.18.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Andy Shevchenko]

On Wed, Sep 2, 2020 at 8:37 PM Daniel Dadap <ddadap@nvidia.com> wrote:
>
> 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.
>
> This driver currently only supports systems with Intel integrated
> graphics and NVIDIA discrete graphics. It will need to be updated if
> designs are developed using the same interfaces which utilize GPUs
> from other vendors.

Thanks for an update. My comments below.

> v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
> v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>

This should go after the cutter '---' line below.

> 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 | 265 +++++++++++++++++++++++++++++++

...

> +config MXDS_GMUX
> +       tristate "ACPI MXDS Gmux Driver"
> +       depends on ACPI_WMI

> +       depends on ACPI

Is not this implied by the above?

> +       depends on VGA_SWITCHEROO
> +       help
> +         This driver provides support for ACPI-driven gmux devices which are
> +         present on some notebook designs with hybrid graphics.

The stuff in Kconfig and Makefile is grouped and sorted by alphabet in
each group. Please, follow.

...

> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * mxds-gmux: vga_switcheroo mux handler for ACPI MXDS muxes

Please, remove the file name from the file.

> + * 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>.
> + *

Above should be removed since we use SPDX.

> + */

...

> +#include <linux/module.h>
> +#include <linux/acpi.h>
> +#include <linux/pci.h>
> +#include <linux/vga_switcheroo.h>
> +#include <linux/delay.h>

Sorted is easier to read.

...

> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("vga_switcheroo mux handler for ACPI MXDS muxes");
> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>");

Usually we put this at the end of the file.

> +/*
> + * 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");

But this one depends.

...

> +static struct pci_dev *ig_dev, *dg_dev;
> +static acpi_handle internal_mux_handle;
> +static acpi_handle external_mux_handle;

Global variables?! Please get rid of them.

...

> +enum acpi_method {
> +       MXDM = 0,
> +       MXDS,
> +       NUM_ACPI_METHODS
> +};

Hmm... any real need for this enum?

...

> +enum mux_state_command {
> +       MUX_STATE_GET = 0,
> +       MUX_STATE_SET_IGPU = BIT(0),
> +       MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
> +};

#include <linux/bits.h>

...

> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
> +                               acpi_integer action)
> +{
> +       union acpi_object arg = {
> +               .integer = { .type = ACPI_TYPE_INTEGER, .value = action }
> +       };
> +       struct acpi_object_list in = {.count = 1, .pointer = &arg};

Be consistent with spaces surrounding the structure content.

> +       acpi_integer ret;
> +       acpi_status status;
> +
> +       status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);

> +

Redundant blank line.

> +       if (ACPI_FAILURE(status)) {

> +               pr_err("ACPI %s failed: %s\n", acpi_methods[method],
> +                       acpi_format_exception(status));

Why not acpi_handle_err() ?

> +               return 0;
> +       }
> +
> +       return ret;
> +}

...

> +static enum vga_switcheroo_client_id mxds_gmux_get_client_id(
> +       struct pci_dev *dev)

One line, please.

...

> +static acpi_status find_acpi_methods(
> +       acpi_handle object, u32 nesting_level, void *context,
> +       void **return_value)

Fix indentation here as well.

...

> +static int __init mxds_gmux_init(void)
> +{
> +       int ret = 0;

> +       struct pci_dev *dev = NULL;

Redundant assignment. And keep it in reversed xmas tree order.

> +       /* Currently only supports Intel integrated and NVIDIA discrete GPUs */

> +       while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {

(Mostly as TODO for somebody else)
arch/alpha/kernel/console.c-47-
arch/x86/kernel/sysfb_efi.c-117-
drivers/acpi/acpi_video.c-2139-
drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:616
drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c:288
drivers/gpu/drm/i915/display/intel_acpi.c:136
drivers/gpu/drm/nouveau/nouveau_acpi.c:284
drivers/gpu/drm/radeon/radeon_bios.c:202
drivers/vfio/pci/vfio_pci.c:136
sound/pci/hda/hda_intel.c:1434

(slightly different story) drivers/gpu/drm/qxl/qxl_drv.c-67-static
drivers/vfio/pci/vfio_pci.c-153-static

So, what  I think is better to do in this case is

#define for_each_pci_vga(dev) ...

in pci.h (at least here for the future use)

> +       /* 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);

It's a bit too much. Can we reduce scope somehow?

> +       /* Require at least one mux */
> +       if (!internal_mux_handle && !external_mux_handle) {
> +               ret = -ENODEV;
> +               goto done;
> +       }
> +
> +       ret = vga_switcheroo_register_handler(&handler, 0);
> +
> +done:
> +
> +       if (ret) {
> +               pci_dev_put(ig_dev);
> +               pci_dev_put(dg_dev);
> +       }
> +
> +       return ret;

Can we use usual pattern:

  ret = ...
  if (ret)
    goto error_put_devices;

  return 0;

error_put_devices:
    pci_dev_put(ig_dev);
    pci_dev_put(dg_dev);
    return ret;

?

> +}
> +module_init(mxds_gmux_init);
> +
> +static void __exit mxds_gmux_exit(void)
> +{
> +       vga_switcheroo_unregister_handler();
> +       pci_dev_put(ig_dev);
> +       pci_dev_put(dg_dev);
> +}
> +module_exit(mxds_gmux_exit);


-- 
With Best Regards,
Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

Thanks. I'm retesting with these changes now; I'll also apply analogous 
changes where applicable to the other x86 platform driver I have out for 
review.

On 9/2/20 1:37 PM, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
>
>
> On Wed, Sep 2, 2020 at 8:37 PM Daniel Dadap <ddadap@nvidia.com> wrote:
>> 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.
>>
>> This driver currently only supports systems with Intel integrated
>> graphics and NVIDIA discrete graphics. It will need to be updated if
>> designs are developed using the same interfaces which utilize GPUs
>> from other vendors.
> Thanks for an update. My comments below.
>
>> v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
>> v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>
> This should go after the cutter '---' line below.
>
>> 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 | 265 +++++++++++++++++++++++++++++++
> ...
>
>> +config MXDS_GMUX
>> +       tristate "ACPI MXDS Gmux Driver"
>> +       depends on ACPI_WMI
>> +       depends on ACPI
> Is not this implied by the above?


Yes, it is.


>
>> +       depends on VGA_SWITCHEROO
>> +       help
>> +         This driver provides support for ACPI-driven gmux devices which are
>> +         present on some notebook designs with hybrid graphics.
> The stuff in Kconfig and Makefile is grouped and sorted by alphabet in
> each group. Please, follow.


Alright, I hadn't seen any apparent ordering to the items in Kconfig, so 
I just put the new items at the end. However, looking at the Makefile I 
can see the grouping. I'm not really certain which of the existing 
groups this would belong to, as many of them seem to be 
manufacturer-specific, and this driver applies to hardware that will be 
available on systems from multiple manufacturers, so I've put it in 
"Platform drivers" for now; please let me know if there's a better place.


> ...
>
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * mxds-gmux: vga_switcheroo mux handler for ACPI MXDS muxes
> Please, remove the file name from the file.
>
>> + * 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>.
>> + *
> Above should be removed since we use SPDX.


Yes, it's already been pointed out that this is redundant. I wanted to 
keep it in since the usual guidance for GPL-licensed contributions from 
NVIDIA is to include the boilerplate text, but it does seem that the 
SPDX metadata supersedes that guidance, so I'll remove it.


>
>> + */
> ...
>
>> +#include <linux/module.h>
>> +#include <linux/acpi.h>
>> +#include <linux/pci.h>
>> +#include <linux/vga_switcheroo.h>
>> +#include <linux/delay.h>
> Sorted is easier to read.
>
> ...
>
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("vga_switcheroo mux handler for ACPI MXDS muxes");
>> +MODULE_AUTHOR("Daniel Dadap <ddadap@nvidia.com>");
> Usually we put this at the end of the file.
>
>> +/*
>> + * 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");
> But this one depends.
>
> ...
>
>> +static struct pci_dev *ig_dev, *dg_dev;
>> +static acpi_handle internal_mux_handle;
>> +static acpi_handle external_mux_handle;
> Global variables?! Please get rid of them.


Where would you propose stashing these instead? The vga-switcheroo 
callbacks this driver registers, whose implementations depend on these 
globals, don't pass in anything that would be useful for referencing 
this information. I suppose these could be wrapped in functions or the 
dev private storage of a struct device that we create just for storing 
things (it doesn't currently have an associated struct device), but then 
that struct device would have to be global.


>
> ...
>
>> +enum acpi_method {
>> +       MXDM = 0,
>> +       MXDS,
>> +       NUM_ACPI_METHODS
>> +};
> Hmm... any real need for this enum?


I suppose we could just pass in the method name string directly instead.


>
> ...
>
>> +enum mux_state_command {
>> +       MUX_STATE_GET = 0,
>> +       MUX_STATE_SET_IGPU = BIT(0),
>> +       MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
>> +};
> #include <linux/bits.h>
>
> ...
>
>> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
>> +                               acpi_integer action)
>> +{
>> +       union acpi_object arg = {
>> +               .integer = { .type = ACPI_TYPE_INTEGER, .value = action }
>> +       };
>> +       struct acpi_object_list in = {.count = 1, .pointer = &arg};
> Be consistent with spaces surrounding the structure content.
>
>> +       acpi_integer ret;
>> +       acpi_status status;
>> +
>> +       status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
>> +
> Redundant blank line.
>
>> +       if (ACPI_FAILURE(status)) {
>> +               pr_err("ACPI %s failed: %s\n", acpi_methods[method],
>> +                       acpi_format_exception(status));
> Why not acpi_handle_err() ?


Indeed. That was the only use of the pr_*() macros, so I've also removed 
the pr_fmt #define.


>> +               return 0;
>> +       }
>> +
>> +       return ret;
>> +}
> ...
>
>> +static enum vga_switcheroo_client_id mxds_gmux_get_client_id(
>> +       struct pci_dev *dev)
> One line, please.


Sure. I was just trying to keep under 80 characters, and didn't notice 
that the code in drivers/platform/x86 doesn't keep to the 80 character 
limit.


>
> ...
>
>> +static acpi_status find_acpi_methods(
>> +       acpi_handle object, u32 nesting_level, void *context,
>> +       void **return_value)
> Fix indentation here as well.
>
> ...
>
>> +static int __init mxds_gmux_init(void)
>> +{
>> +       int ret = 0;
>> +       struct pci_dev *dev = NULL;
> Redundant assignment. And keep it in reversed xmas tree order.


Not redundant. It needs to be NULL for the first iteration of the while 
loop, since pci_get_class() takes it as an argument for the previous 
result. The other assignment here for ret, however, does become 
redundant after changing the goto cleanup flow.


>
>> +       /* Currently only supports Intel integrated and NVIDIA discrete GPUs */
>> +       while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
> (Mostly as TODO for somebody else)
> arch/alpha/kernel/console.c-47-
> arch/x86/kernel/sysfb_efi.c-117-
> drivers/acpi/acpi_video.c-2139-
> drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c:616
> drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c:288
> drivers/gpu/drm/i915/display/intel_acpi.c:136
> drivers/gpu/drm/nouveau/nouveau_acpi.c:284
> drivers/gpu/drm/radeon/radeon_bios.c:202
> drivers/vfio/pci/vfio_pci.c:136
> sound/pci/hda/hda_intel.c:1434
>
> (slightly different story) drivers/gpu/drm/qxl/qxl_drv.c-67-static
> drivers/vfio/pci/vfio_pci.c-153-static
>
> So, what  I think is better to do in this case is
>
> #define for_each_pci_vga(dev) ...
>
> in pci.h (at least here for the future use)
>
>> +       /* 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);
> It's a bit too much. Can we reduce scope somehow?


Yeah, we can start at the dGPU's ACPI node, since that's where these 
methods are implemented, and walk with depth 2. If that's still too 
broad, we can retrieve the _DOD table from the dGPU and then walk each 
child of the dGPU's ACPI node that occurs in _DOD with depth 1.


>
>> +       /* Require at least one mux */
>> +       if (!internal_mux_handle && !external_mux_handle) {
>> +               ret = -ENODEV;
>> +               goto done;
>> +       }
>> +
>> +       ret = vga_switcheroo_register_handler(&handler, 0);
>> +
>> +done:
>> +
>> +       if (ret) {
>> +               pci_dev_put(ig_dev);
>> +               pci_dev_put(dg_dev);
>> +       }
>> +
>> +       return ret;
> Can we use usual pattern:
>
>    ret = ...
>    if (ret)
>      goto error_put_devices;
> https://wiki.nvidia.com/nvwiki/index.php/SW_IP_Audit_Team/Licenses
>    return 0;
>
> error_put_devices:
>      pci_dev_put(ig_dev);
>      pci_dev_put(dg_dev);
>      return ret;
>
> ?
>
>> +}
>> +module_init(mxds_gmux_init);
>> +
>> +static void __exit mxds_gmux_exit(void)
>> +{
>> +       vga_switcheroo_unregister_handler();
>> +       pci_dev_put(ig_dev);
>> +       pci_dev_put(dg_dev);
>> +}
>> +module_exit(mxds_gmux_exit);
>
> --
> With Best Regards,
> Andy Shevchenko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Hans de Goede]

Hi Daniel,

Quick self intro: I have take over drivers/platform/x86
maintainership from Andy; and I'm working my way through
the backlog of old patches in patchwork:
https://patchwork.kernel.org/project/platform-driver-x86/list/

On 9/2/20 7:38 PM, Daniel Dadap wrote:
> 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.
> 
> This driver currently only supports systems with Intel integrated
> graphics and NVIDIA discrete graphics. It will need to be updated if
> designs are developed using the same interfaces which utilize GPUs
> from other vendors.
> 
> v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
> v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>

According to the discussion archived here:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20200902173851.224368-1-ddadap@nvidia.com/

Andy did a review and you promised a new version, but I don't see
a newer version in patchwork. Can you please submit a new version
addressing Andy's remarks ? Then I will review and merge the new
version.

In that discussion you also promised a new version of this patch:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20200731202154.11382-1-ddadap@nvidia.com/

So please also submit a new version of that series.

Thanks & Regards,

Hans





> 
> 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 | 265 +++++++++++++++++++++++++++++++
>  4 files changed, 282 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..5d00ad1ffc0e 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..b79000733fae 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..dd7f6edaf0f3
> --- /dev/null
> +++ b/drivers/platform/x86/mxds-gmux.c
> @@ -0,0 +1,265 @@
> +// 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>.
> + *
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#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;
> +
> +enum acpi_method {
> +	MXDM = 0,
> +	MXDS,
> +	NUM_ACPI_METHODS
> +};
> +
> +static char *acpi_methods[NUM_ACPI_METHODS] = {
> +	[MXDM] = "MXDM",
> +	[MXDS] = "MXDS",
> +};
> +
> +enum mux_mode_command {
> +	MUX_MODE_GET = 0,
> +};
> +
> +enum mux_mode {
> +	MUX_MODE_DGPU_ONLY = 1,
> +	MUX_MODE_IGPU_ONLY = 2,
> +	MUX_MODE_MSHYBRID = 3,	/* Dual GPU, mux switched to iGPU */
> +	MUX_MODE_DYNAMIC = 4,	/* Dual GPU, dynamic mux switching */
> +};
> +
> +/*
> + * 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_IGPU = 1,
> +	MUX_STATE_DGPU = 2,
> +};
> +
> +enum mux_state_command {
> +	MUX_STATE_GET = 0,
> +	MUX_STATE_SET_IGPU = BIT(0),
> +	MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
> +};
> +
> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
> +				acpi_integer action)
> +{
> +	union acpi_object arg = {
> +		.integer = { .type = ACPI_TYPE_INTEGER, .value = action }
> +	};
> +	struct acpi_object_list in = {.count = 1, .pointer = &arg};
> +	acpi_integer ret;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
> +
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("ACPI %s failed: %s\n", acpi_methods[method],
> +			acpi_format_exception(status));
> +		return 0;
> +	}
> +
> +	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)
> +{
> +	enum mux_state_command command;
> +
> +	switch (state) {
> +	case MUX_STATE_IGPU:
> +		command = MUX_STATE_SET_IGPU;
> +		break;
> +	case MUX_STATE_DGPU:
> +		command = MUX_STATE_SET_DGPU;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	acpi_helper(handle, MXDS, command);
> +}
> +
> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id)
> +{
> +	enum mux_state state;
> +
> +	switch (id) {
> +	case VGA_SWITCHEROO_IGD:
> +		state = MUX_STATE_IGPU;
> +		break;
> +	case VGA_SWITCHEROO_DIS:
> +		state = MUX_STATE_DGPU;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (internal_mux_handle) {
> +		set_mux_state(internal_mux_handle, state);
> +		if (get_mux_state(internal_mux_handle) != state)
> +			return -EAGAIN;
> +	}
> +
> +	if (external_mux_handle) {
> +		set_mux_state(external_mux_handle, state);
> +		if (get_mux_state(external_mux_handle) != 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 == ig_dev)
> +		return VGA_SWITCHEROO_IGD;
> +	if (dev == dg_dev)
> +		return VGA_SWITCHEROO_DIS;
> +
> +	return VGA_SWITCHEROO_UNKNOWN_ID;
> +}
> +
> +static const struct vga_switcheroo_handler handler = {
> +	.switchto = mxds_gmux_switchto,
> +	.get_client_id = mxds_gmux_get_client_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 AE_OK;
> +	if (acpi_get_handle(object, "MXDS", &search))
> +		return AE_OK;
> +
> +	/* MXDS only works when MXDM indicates dynamic mode */
> +	if (get_mux_mode(object) != MUX_MODE_DYNAMIC)
> +		return AE_OK;
> +
> +	/* Internal display has _BCL; external does not */
> +	if (acpi_get_handle(object, "_BCL", &search))
> +		external_mux_handle = object;
> +	else
> +		internal_mux_handle = object;
> +
> +	return AE_OK;
> +}
> +
> +static int __init mxds_gmux_init(void)
> +{
> +	int ret = 0;
> +	struct pci_dev *dev = NULL;
> +
> +	/* Currently only supports Intel integrated and NVIDIA discrete GPUs */
> +	while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
> +		/* Ignore eGPU */
> +		if (pci_is_thunderbolt_attached(dev))
> +			continue;
> +
> +		switch (dev->vendor) {
> +		case PCI_VENDOR_ID_INTEL:
> +			pci_dev_put(ig_dev);
> +			ig_dev = pci_dev_get(dev);
> +			break;
> +		case PCI_VENDOR_ID_NVIDIA:
> +			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);
> +
> +done:
> +
> +	if (ret) {
> +		pci_dev_put(ig_dev);
> +		pci_dev_put(dg_dev);
> +	}
> +
> +	return ret;
> +}
> +module_init(mxds_gmux_init);
> +
> +static void __exit mxds_gmux_exit(void)
> +{
> +	vga_switcheroo_unregister_handler();
> +	pci_dev_put(ig_dev);
> +	pci_dev_put(dg_dev);
> +}
> +module_exit(mxds_gmux_exit);
> 

Re: [PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Hans de Goede]

Hi Daniel,

Quick self intro: I have take over drivers/platform/x86
maintainership from Andy; and I'm working my way through
the backlog of old patches in patchwork:
https://patchwork.kernel.org/project/platform-driver-x86/list/

On 9/2/20 7:38 PM, Daniel Dadap wrote:
> 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.
> 
> This driver currently only supports systems with Intel integrated
> graphics and NVIDIA discrete graphics. It will need to be updated if
> designs are developed using the same interfaces which utilize GPUs
> from other vendors.
> 
> v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
> v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>

According to the discussion archived here:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20200902173851.224368-1-ddadap@nvidia.com/

Andy did a review and you promised a new version, but I don't see
a newer version in patchwork. Can you please submit a new version
addressing Andy's remarks ? Then I will review and merge the new
version.

In that discussion you also promised a new version of this patch:
https://patchwork.kernel.org/project/platform-driver-x86/patch/20200731202154.11382-1-ddadap@nvidia.com/

So please also submit a new version of that series.

Thanks & Regards,

Hans





> 
> 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 | 265 +++++++++++++++++++++++++++++++
>  4 files changed, 282 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..5d00ad1ffc0e 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..b79000733fae 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..dd7f6edaf0f3
> --- /dev/null
> +++ b/drivers/platform/x86/mxds-gmux.c
> @@ -0,0 +1,265 @@
> +// 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>.
> + *
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#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;
> +
> +enum acpi_method {
> +	MXDM = 0,
> +	MXDS,
> +	NUM_ACPI_METHODS
> +};
> +
> +static char *acpi_methods[NUM_ACPI_METHODS] = {
> +	[MXDM] = "MXDM",
> +	[MXDS] = "MXDS",
> +};
> +
> +enum mux_mode_command {
> +	MUX_MODE_GET = 0,
> +};
> +
> +enum mux_mode {
> +	MUX_MODE_DGPU_ONLY = 1,
> +	MUX_MODE_IGPU_ONLY = 2,
> +	MUX_MODE_MSHYBRID = 3,	/* Dual GPU, mux switched to iGPU */
> +	MUX_MODE_DYNAMIC = 4,	/* Dual GPU, dynamic mux switching */
> +};
> +
> +/*
> + * 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_IGPU = 1,
> +	MUX_STATE_DGPU = 2,
> +};
> +
> +enum mux_state_command {
> +	MUX_STATE_GET = 0,
> +	MUX_STATE_SET_IGPU = BIT(0),
> +	MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
> +};
> +
> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
> +				acpi_integer action)
> +{
> +	union acpi_object arg = {
> +		.integer = { .type = ACPI_TYPE_INTEGER, .value = action }
> +	};
> +	struct acpi_object_list in = {.count = 1, .pointer = &arg};
> +	acpi_integer ret;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
> +
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("ACPI %s failed: %s\n", acpi_methods[method],
> +			acpi_format_exception(status));
> +		return 0;
> +	}
> +
> +	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)
> +{
> +	enum mux_state_command command;
> +
> +	switch (state) {
> +	case MUX_STATE_IGPU:
> +		command = MUX_STATE_SET_IGPU;
> +		break;
> +	case MUX_STATE_DGPU:
> +		command = MUX_STATE_SET_DGPU;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	acpi_helper(handle, MXDS, command);
> +}
> +
> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id)
> +{
> +	enum mux_state state;
> +
> +	switch (id) {
> +	case VGA_SWITCHEROO_IGD:
> +		state = MUX_STATE_IGPU;
> +		break;
> +	case VGA_SWITCHEROO_DIS:
> +		state = MUX_STATE_DGPU;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (internal_mux_handle) {
> +		set_mux_state(internal_mux_handle, state);
> +		if (get_mux_state(internal_mux_handle) != state)
> +			return -EAGAIN;
> +	}
> +
> +	if (external_mux_handle) {
> +		set_mux_state(external_mux_handle, state);
> +		if (get_mux_state(external_mux_handle) != 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 == ig_dev)
> +		return VGA_SWITCHEROO_IGD;
> +	if (dev == dg_dev)
> +		return VGA_SWITCHEROO_DIS;
> +
> +	return VGA_SWITCHEROO_UNKNOWN_ID;
> +}
> +
> +static const struct vga_switcheroo_handler handler = {
> +	.switchto = mxds_gmux_switchto,
> +	.get_client_id = mxds_gmux_get_client_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 AE_OK;
> +	if (acpi_get_handle(object, "MXDS", &search))
> +		return AE_OK;
> +
> +	/* MXDS only works when MXDM indicates dynamic mode */
> +	if (get_mux_mode(object) != MUX_MODE_DYNAMIC)
> +		return AE_OK;
> +
> +	/* Internal display has _BCL; external does not */
> +	if (acpi_get_handle(object, "_BCL", &search))
> +		external_mux_handle = object;
> +	else
> +		internal_mux_handle = object;
> +
> +	return AE_OK;
> +}
> +
> +static int __init mxds_gmux_init(void)
> +{
> +	int ret = 0;
> +	struct pci_dev *dev = NULL;
> +
> +	/* Currently only supports Intel integrated and NVIDIA discrete GPUs */
> +	while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
> +		/* Ignore eGPU */
> +		if (pci_is_thunderbolt_attached(dev))
> +			continue;
> +
> +		switch (dev->vendor) {
> +		case PCI_VENDOR_ID_INTEL:
> +			pci_dev_put(ig_dev);
> +			ig_dev = pci_dev_get(dev);
> +			break;
> +		case PCI_VENDOR_ID_NVIDIA:
> +			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);
> +
> +done:
> +
> +	if (ret) {
> +		pci_dev_put(ig_dev);
> +		pci_dev_put(dg_dev);
> +	}
> +
> +	return ret;
> +}
> +module_init(mxds_gmux_init);
> +
> +static void __exit mxds_gmux_exit(void)
> +{
> +	vga_switcheroo_unregister_handler();
> +	pci_dev_put(ig_dev);
> +	pci_dev_put(dg_dev);
> +}
> +module_exit(mxds_gmux_exit);
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

On 11/10/20 3:29 AM, Hans de Goede wrote:
> Hi Daniel,


Hi Hans,


> Quick self intro: I have take over drivers/platform/x86
> maintainership from Andy; and I'm working my way through
> the backlog of old patches in patchwork:
> https://patchwork.kernel.org/project/platform-driver-x86/list/


Thanks for the introduction. I believe we have corresponded in the past 
regarding other issues. Thank you for following up on this patch set.


> On 9/2/20 7:38 PM, Daniel Dadap wrote:
>> 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.
>>
>> This driver currently only supports systems with Intel integrated
>> graphics and NVIDIA discrete graphics. It will need to be updated if
>> designs are developed using the same interfaces which utilize GPUs
>> from other vendors.
>>
>> v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
>> v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>
> According to the discussion archived here:
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20200902173851.224368-1-ddadap@nvidia.com/
>
> Andy did a review and you promised a new version, but I don't see
> a newer version in patchwork. Can you please submit a new version
> addressing Andy's remarks ? Then I will review and merge the new
> version.
>
> In that discussion you also promised a new version of this patch:
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20200731202154.11382-1-ddadap@nvidia.com/
>
> So please also submit a new version of that series.


You are right. I do have updated versions of those patches, but enough 
time has passed that I do not recall if I tested them and failed to 
follow up on the e-mail threads, or had yet to test them. So I will test 
the updated patches and get them to you. Thanks.


> Thanks & Regards,
>
> Hans
>
>
>
>
>
>> 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 | 265 +++++++++++++++++++++++++++++++
>>   4 files changed, 282 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..5d00ad1ffc0e 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..b79000733fae 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..dd7f6edaf0f3
>> --- /dev/null
>> +++ b/drivers/platform/x86/mxds-gmux.c
>> @@ -0,0 +1,265 @@
>> +// 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>.
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#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;
>> +
>> +enum acpi_method {
>> +     MXDM = 0,
>> +     MXDS,
>> +     NUM_ACPI_METHODS
>> +};
>> +
>> +static char *acpi_methods[NUM_ACPI_METHODS] = {
>> +     [MXDM] = "MXDM",
>> +     [MXDS] = "MXDS",
>> +};
>> +
>> +enum mux_mode_command {
>> +     MUX_MODE_GET = 0,
>> +};
>> +
>> +enum mux_mode {
>> +     MUX_MODE_DGPU_ONLY = 1,
>> +     MUX_MODE_IGPU_ONLY = 2,
>> +     MUX_MODE_MSHYBRID = 3,  /* Dual GPU, mux switched to iGPU */
>> +     MUX_MODE_DYNAMIC = 4,   /* Dual GPU, dynamic mux switching */
>> +};
>> +
>> +/*
>> + * 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_IGPU = 1,
>> +     MUX_STATE_DGPU = 2,
>> +};
>> +
>> +enum mux_state_command {
>> +     MUX_STATE_GET = 0,
>> +     MUX_STATE_SET_IGPU = BIT(0),
>> +     MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
>> +};
>> +
>> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
>> +                             acpi_integer action)
>> +{
>> +     union acpi_object arg = {
>> +             .integer = { .type = ACPI_TYPE_INTEGER, .value = action }
>> +     };
>> +     struct acpi_object_list in = {.count = 1, .pointer = &arg};
>> +     acpi_integer ret;
>> +     acpi_status status;
>> +
>> +     status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
>> +
>> +     if (ACPI_FAILURE(status)) {
>> +             pr_err("ACPI %s failed: %s\n", acpi_methods[method],
>> +                     acpi_format_exception(status));
>> +             return 0;
>> +     }
>> +
>> +     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)
>> +{
>> +     enum mux_state_command command;
>> +
>> +     switch (state) {
>> +     case MUX_STATE_IGPU:
>> +             command = MUX_STATE_SET_IGPU;
>> +             break;
>> +     case MUX_STATE_DGPU:
>> +             command = MUX_STATE_SET_DGPU;
>> +             break;
>> +     default:
>> +             return;
>> +     }
>> +
>> +     acpi_helper(handle, MXDS, command);
>> +}
>> +
>> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id)
>> +{
>> +     enum mux_state state;
>> +
>> +     switch (id) {
>> +     case VGA_SWITCHEROO_IGD:
>> +             state = MUX_STATE_IGPU;
>> +             break;
>> +     case VGA_SWITCHEROO_DIS:
>> +             state = MUX_STATE_DGPU;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (internal_mux_handle) {
>> +             set_mux_state(internal_mux_handle, state);
>> +             if (get_mux_state(internal_mux_handle) != state)
>> +                     return -EAGAIN;
>> +     }
>> +
>> +     if (external_mux_handle) {
>> +             set_mux_state(external_mux_handle, state);
>> +             if (get_mux_state(external_mux_handle) != 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 == ig_dev)
>> +             return VGA_SWITCHEROO_IGD;
>> +     if (dev == dg_dev)
>> +             return VGA_SWITCHEROO_DIS;
>> +
>> +     return VGA_SWITCHEROO_UNKNOWN_ID;
>> +}
>> +
>> +static const struct vga_switcheroo_handler handler = {
>> +     .switchto = mxds_gmux_switchto,
>> +     .get_client_id = mxds_gmux_get_client_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 AE_OK;
>> +     if (acpi_get_handle(object, "MXDS", &search))
>> +             return AE_OK;
>> +
>> +     /* MXDS only works when MXDM indicates dynamic mode */
>> +     if (get_mux_mode(object) != MUX_MODE_DYNAMIC)
>> +             return AE_OK;
>> +
>> +     /* Internal display has _BCL; external does not */
>> +     if (acpi_get_handle(object, "_BCL", &search))
>> +             external_mux_handle = object;
>> +     else
>> +             internal_mux_handle = object;
>> +
>> +     return AE_OK;
>> +}
>> +
>> +static int __init mxds_gmux_init(void)
>> +{
>> +     int ret = 0;
>> +     struct pci_dev *dev = NULL;
>> +
>> +     /* Currently only supports Intel integrated and NVIDIA discrete GPUs */
>> +     while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
>> +             /* Ignore eGPU */
>> +             if (pci_is_thunderbolt_attached(dev))
>> +                     continue;
>> +
>> +             switch (dev->vendor) {
>> +             case PCI_VENDOR_ID_INTEL:
>> +                     pci_dev_put(ig_dev);
>> +                     ig_dev = pci_dev_get(dev);
>> +                     break;
>> +             case PCI_VENDOR_ID_NVIDIA:
>> +                     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);
>> +
>> +done:
>> +
>> +     if (ret) {
>> +             pci_dev_put(ig_dev);
>> +             pci_dev_put(dg_dev);
>> +     }
>> +
>> +     return ret;
>> +}
>> +module_init(mxds_gmux_init);
>> +
>> +static void __exit mxds_gmux_exit(void)
>> +{
>> +     vga_switcheroo_unregister_handler();
>> +     pci_dev_put(ig_dev);
>> +     pci_dev_put(dg_dev);
>> +}
>> +module_exit(mxds_gmux_exit);
>>

Re: [PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes

[Daniel Dadap]

On 11/10/20 3:29 AM, Hans de Goede wrote:
> Hi Daniel,


Hi Hans,


> Quick self intro: I have take over drivers/platform/x86
> maintainership from Andy; and I'm working my way through
> the backlog of old patches in patchwork:
> https://patchwork.kernel.org/project/platform-driver-x86/list/


Thanks for the introduction. I believe we have corresponded in the past 
regarding other issues. Thank you for following up on this patch set.


> On 9/2/20 7:38 PM, Daniel Dadap wrote:
>> 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.
>>
>> This driver currently only supports systems with Intel integrated
>> graphics and NVIDIA discrete graphics. It will need to be updated if
>> designs are developed using the same interfaces which utilize GPUs
>> from other vendors.
>>
>> v2,v3: misc. fixes suggested by Barnabás Pőcze <pobrn@protonmail.com>
>> v4: misc. changes suggested by Lukas Wunner <lukas@wunner.de>
> According to the discussion archived here:
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20200902173851.224368-1-ddadap@nvidia.com/
>
> Andy did a review and you promised a new version, but I don't see
> a newer version in patchwork. Can you please submit a new version
> addressing Andy's remarks ? Then I will review and merge the new
> version.
>
> In that discussion you also promised a new version of this patch:
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20200731202154.11382-1-ddadap@nvidia.com/
>
> So please also submit a new version of that series.


You are right. I do have updated versions of those patches, but enough 
time has passed that I do not recall if I tested them and failed to 
follow up on the e-mail threads, or had yet to test them. So I will test 
the updated patches and get them to you. Thanks.


> Thanks & Regards,
>
> Hans
>
>
>
>
>
>> 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 | 265 +++++++++++++++++++++++++++++++
>>   4 files changed, 282 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..5d00ad1ffc0e 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..b79000733fae 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..dd7f6edaf0f3
>> --- /dev/null
>> +++ b/drivers/platform/x86/mxds-gmux.c
>> @@ -0,0 +1,265 @@
>> +// 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>.
>> + *
>> + */
>> +
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> +
>> +#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;
>> +
>> +enum acpi_method {
>> +     MXDM = 0,
>> +     MXDS,
>> +     NUM_ACPI_METHODS
>> +};
>> +
>> +static char *acpi_methods[NUM_ACPI_METHODS] = {
>> +     [MXDM] = "MXDM",
>> +     [MXDS] = "MXDS",
>> +};
>> +
>> +enum mux_mode_command {
>> +     MUX_MODE_GET = 0,
>> +};
>> +
>> +enum mux_mode {
>> +     MUX_MODE_DGPU_ONLY = 1,
>> +     MUX_MODE_IGPU_ONLY = 2,
>> +     MUX_MODE_MSHYBRID = 3,  /* Dual GPU, mux switched to iGPU */
>> +     MUX_MODE_DYNAMIC = 4,   /* Dual GPU, dynamic mux switching */
>> +};
>> +
>> +/*
>> + * 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_IGPU = 1,
>> +     MUX_STATE_DGPU = 2,
>> +};
>> +
>> +enum mux_state_command {
>> +     MUX_STATE_GET = 0,
>> +     MUX_STATE_SET_IGPU = BIT(0),
>> +     MUX_STATE_SET_DGPU = BIT(4) | BIT(0),
>> +};
>> +
>> +static acpi_integer acpi_helper(acpi_handle handle, enum acpi_method method,
>> +                             acpi_integer action)
>> +{
>> +     union acpi_object arg = {
>> +             .integer = { .type = ACPI_TYPE_INTEGER, .value = action }
>> +     };
>> +     struct acpi_object_list in = {.count = 1, .pointer = &arg};
>> +     acpi_integer ret;
>> +     acpi_status status;
>> +
>> +     status = acpi_evaluate_integer(handle, acpi_methods[method], &in, &ret);
>> +
>> +     if (ACPI_FAILURE(status)) {
>> +             pr_err("ACPI %s failed: %s\n", acpi_methods[method],
>> +                     acpi_format_exception(status));
>> +             return 0;
>> +     }
>> +
>> +     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)
>> +{
>> +     enum mux_state_command command;
>> +
>> +     switch (state) {
>> +     case MUX_STATE_IGPU:
>> +             command = MUX_STATE_SET_IGPU;
>> +             break;
>> +     case MUX_STATE_DGPU:
>> +             command = MUX_STATE_SET_DGPU;
>> +             break;
>> +     default:
>> +             return;
>> +     }
>> +
>> +     acpi_helper(handle, MXDS, command);
>> +}
>> +
>> +static int mxds_gmux_switchto(enum vga_switcheroo_client_id id)
>> +{
>> +     enum mux_state state;
>> +
>> +     switch (id) {
>> +     case VGA_SWITCHEROO_IGD:
>> +             state = MUX_STATE_IGPU;
>> +             break;
>> +     case VGA_SWITCHEROO_DIS:
>> +             state = MUX_STATE_DGPU;
>> +             break;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (internal_mux_handle) {
>> +             set_mux_state(internal_mux_handle, state);
>> +             if (get_mux_state(internal_mux_handle) != state)
>> +                     return -EAGAIN;
>> +     }
>> +
>> +     if (external_mux_handle) {
>> +             set_mux_state(external_mux_handle, state);
>> +             if (get_mux_state(external_mux_handle) != 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 == ig_dev)
>> +             return VGA_SWITCHEROO_IGD;
>> +     if (dev == dg_dev)
>> +             return VGA_SWITCHEROO_DIS;
>> +
>> +     return VGA_SWITCHEROO_UNKNOWN_ID;
>> +}
>> +
>> +static const struct vga_switcheroo_handler handler = {
>> +     .switchto = mxds_gmux_switchto,
>> +     .get_client_id = mxds_gmux_get_client_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 AE_OK;
>> +     if (acpi_get_handle(object, "MXDS", &search))
>> +             return AE_OK;
>> +
>> +     /* MXDS only works when MXDM indicates dynamic mode */
>> +     if (get_mux_mode(object) != MUX_MODE_DYNAMIC)
>> +             return AE_OK;
>> +
>> +     /* Internal display has _BCL; external does not */
>> +     if (acpi_get_handle(object, "_BCL", &search))
>> +             external_mux_handle = object;
>> +     else
>> +             internal_mux_handle = object;
>> +
>> +     return AE_OK;
>> +}
>> +
>> +static int __init mxds_gmux_init(void)
>> +{
>> +     int ret = 0;
>> +     struct pci_dev *dev = NULL;
>> +
>> +     /* Currently only supports Intel integrated and NVIDIA discrete GPUs */
>> +     while ((dev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, dev))) {
>> +             /* Ignore eGPU */
>> +             if (pci_is_thunderbolt_attached(dev))
>> +                     continue;
>> +
>> +             switch (dev->vendor) {
>> +             case PCI_VENDOR_ID_INTEL:
>> +                     pci_dev_put(ig_dev);
>> +                     ig_dev = pci_dev_get(dev);
>> +                     break;
>> +             case PCI_VENDOR_ID_NVIDIA:
>> +                     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);
>> +
>> +done:
>> +
>> +     if (ret) {
>> +             pci_dev_put(ig_dev);
>> +             pci_dev_put(dg_dev);
>> +     }
>> +
>> +     return ret;
>> +}
>> +module_init(mxds_gmux_init);
>> +
>> +static void __exit mxds_gmux_exit(void)
>> +{
>> +     vga_switcheroo_unregister_handler();
>> +     pci_dev_put(ig_dev);
>> +     pci_dev_put(dg_dev);
>> +}
>> +module_exit(mxds_gmux_exit);
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel