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

[Lukas Wunner <lukas@wunner.de>]

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