[PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes
Andy Shevchenko
andy.shevchenko at gmail.com
Wed Sep 2 18:37:09 UTC 2020
On Wed, Sep 2, 2020 at 8:37 PM Daniel Dadap <ddadap at 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 at protonmail.com>
> v4: misc. changes suggested by Lukas Wunner <lukas at wunner.de>
This should go after the cutter '---' line below.
> Signed-off-by: Daniel Dadap <ddadap at 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 at 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
More information about the dri-devel
mailing list