[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 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