[PATCH v4] platform/x86: Add new vga-switcheroo gmux driver for ACPI-driven muxes
Daniel Dadap
ddadap at nvidia.com
Tue Sep 8 19:33:06 UTC 2020
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 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?
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 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.
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
More information about the dri-devel
mailing list