[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