| Message ID | bfabf33fc63abbeb84e11e544456098dff0e4604.1484486499.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote: > We're about to allow runtime PM on Thunderbolt ports in > pci_bridge_d3_possible() and unblock runtime PM for Thunderbolt host > hotplug ports in pci_dev_check_d3cold(). In both cases we need to > uniquely identify if a PCI device belongs to a Thunderbolt controller. Sounds like "a device belongs to a Thunderbolt controller" means the device is part of a Thunderbolt controller or part of the hierarchy below it? > We also have the need to detect presence of a Thunderbolt controller in > drivers/platform/x86/apple-gmux.c because dual GPU MacBook Pros cannot > switch external DP/HDMI ports between GPUs if they have Thunderbolt. This series doesn't touch apple-gmux.c, and I don't know anything about this MacBook Pro topology, so I can't tell why Thunderbolt is relevant here. > Furthermore, in multiple places in the DRM subsystem we need to detect > whether a GPU is on-board or attached with Thunderbolt. As an example, > Thunderbolt-attached GPUs shall not be registered with vga_switcheroo. Why? The connection between vga_switcheroo and Thunderbolt is not obvious, at least to this non-GPU person. > Intel uses a Vendor-Specific Extended Capability (VSEC) with ID 0x1234 > on devices belonging to a Thunderbolt controller which allows us to > recognize them. > > Detect presence of this VSEC on device probe and cache it in a newly > added is_thunderbolt bit in struct pci_dev which can then be queried by > pci_bridge_d3_possible(), pci_dev_check_d3cold(), apple-gmux and others. > > Cc: Andreas Noever <andreas.noever@gmail.com> > Cc: Tomas Winkler <tomas.winkler@intel.com> > Cc: Amir Levy <amir.jer.levy@intel.com> > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/pci.h | 2 ++ > drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 1 + > 3 files changed, 37 insertions(+) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index cb17db242f30..45c2b8144911 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -3,6 +3,8 @@ > > #define PCI_FIND_CAP_TTL 48 > > +#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ > + > extern const unsigned char pcie_link_speed[]; > > bool pcie_cap_has_lnkctl(const struct pci_dev *dev); > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index e164b5c9f0f0..891a8fa1f9f6 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1206,6 +1206,37 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) > pdev->is_hotplug_bridge = 1; > } > > +static void set_pcie_vendor_specific(struct pci_dev *dev) This is very specific to Thunderbolt, so let's name it something that conveys that information. The fact that we use a vendor-specific capability to figure it out isn't really relevant in the caller. > +{ > + int vsec = 0; > + u32 header; > + > + while ((vsec = pci_find_next_ext_capability(dev, vsec, > + PCI_EXT_CAP_ID_VNDR))) { > + pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header); > + > + /* Is the device part of a Thunderbolt controller? */ > + if (dev->vendor == PCI_VENDOR_ID_INTEL && > + PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) > + dev->is_thunderbolt = 1; return; > + } > + > + /* > + * Is the device attached with Thunderbolt? Walk upwards and check for > + * each encountered bridge if it's part of a Thunderbolt controller. > + * Reaching the host bridge means dev is soldered to the mainboard. > + */ > + if (!dev->is_thunderbolt) { The "if" is unnecessary if you return above. > + struct pci_dev *parent = dev; > + > + while ((parent = pci_upstream_bridge(parent))) > + if (parent->is_thunderbolt) { > + dev->is_thunderbolt = 1; > + break; > + } > + } > +} > + > /** > * pci_ext_cfg_is_aliased - is ext config space just an alias of std config? > * @dev: PCI device > @@ -1358,6 +1389,9 @@ int pci_setup_device(struct pci_dev *dev) > /* need to have dev->class ready */ > dev->cfg_size = pci_cfg_space_size(dev); > > + /* need to have dev->cfg_size ready */ > + set_pcie_vendor_specific(dev); > + > /* "Unknown power state" */ > dev->current_state = PCI_UNKNOWN; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index e2d1a124216a..3c775e8498f1 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -358,6 +358,7 @@ struct pci_dev { > unsigned int is_virtfn:1; > unsigned int reset_fn:1; > unsigned int is_hotplug_bridge:1; > + unsigned int is_thunderbolt:1; /* part of Thunderbolt daisy chain */ > unsigned int __aer_firmware_first_valid:1; > unsigned int __aer_firmware_first:1; > unsigned int broken_intx_masking:1; > -- > 2.11.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 28, 2017 at 03:52:08PM -0600, Bjorn Helgaas wrote: > On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote: > > We're about to allow runtime PM on Thunderbolt ports in > > pci_bridge_d3_possible() and unblock runtime PM for Thunderbolt host > > hotplug ports in pci_dev_check_d3cold(). In both cases we need to > > uniquely identify if a PCI device belongs to a Thunderbolt controller. > > Sounds like "a device belongs to a Thunderbolt controller" means the > device is part of a Thunderbolt controller or part of the hierarchy > below it? The above paragraph and the following two in the commit message are intended to explain the need for this additional bit in struct pci_dev. Yes, the bit is set on all PCI devices that are part of the Thunderbolt controller (upstream bridge, downstream bridges, NHI and on Thunderbolt 3 there's also an XHCI) as well as on all PCI devices below it. That's why it says /* part of Thunderbolt daisy chain */ in the line added to pci.h at the bottom of this patch. > > We also have the need to detect presence of a Thunderbolt controller in > > drivers/platform/x86/apple-gmux.c because dual GPU MacBook Pros cannot > > switch external DP/HDMI ports between GPUs if they have Thunderbolt. > > This series doesn't touch apple-gmux.c, and I don't know anything > about this MacBook Pro topology, so I can't tell why Thunderbolt is > relevant here. It's just another example why this bit in struct pci_dev is needed: Dual GPU MacBook Pros introduced before 2011 are able to switch the external DisplayPort between GPUs. All newer models lost this ability and the external port can only be driven by the discrete GPU. That's because the port is no longer just used for DisplayPort, it's become a combined DP/Thunderbolt port. I guess the wiring would have been too complicated to keep the external port switchable between GPUs and also use it for Thunderbolt. They already had to go to great lengths and put various redrivers on the logic board to support the combined DP/thunderbolt port. We need to recognize if the model has Thunderbolt and in that case keep the external port switched to the discrete GPU. I have a patch for this in the pipeline but this one needs to go in first. > > Furthermore, in multiple places in the DRM subsystem we need to detect > > whether a GPU is on-board or attached with Thunderbolt. As an example, > > Thunderbolt-attached GPUs shall not be registered with vga_switcheroo. > > Why? The connection between vga_switcheroo and Thunderbolt is not > obvious, at least to this non-GPU person. nouveau, radeon and amdgpu register any GPU they find with vga_switcheroo, but vga_switcheroo only becomes enabled if the system has Optimus, AMD PowerXpress or an apple-gmux controller. If the user connects an external GPU to a dual GPU laptop, that external GPU will be registered with vga_switcheroo as well. When that external GPU runtime suspends, vga_switcheroo will invoke the callback to cut power to the internal discrete GPU and obviously things go south at that point. The solution is to not register external GPUs with vga_switcheroo at all. For this I need the is_thunderbolt bit. [snip] > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1206,6 +1206,37 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) > > pdev->is_hotplug_bridge = 1; > > } > > > > +static void set_pcie_vendor_specific(struct pci_dev *dev) > > This is very specific to Thunderbolt, so let's name it something that > conveys that information. The fact that we use a vendor-specific > capability to figure it out isn't really relevant in the caller. I thought that we may have the necessity in the future to parse other VSECs on device probe, so I gave the function this generic name. Think about it, every VSEC that needs to be parsed needs the while loop below. It's more efficient to have only a single while loop that handles *all* VSECs at once. If someone needs to parse another VSEC, they just add it to this function. So IMO the way I've solved it is preferable to just adding a Thunderbolt- specific function. Are you sure you want this renamed? (y/n) > > +{ > > + int vsec = 0; > > + u32 header; > > + > > + while ((vsec = pci_find_next_ext_capability(dev, vsec, > > + PCI_EXT_CAP_ID_VNDR))) { > > + pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header); > > + > > + /* Is the device part of a Thunderbolt controller? */ > > + if (dev->vendor == PCI_VENDOR_ID_INTEL && > > + PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) > > + dev->is_thunderbolt = 1; > > return; Well, see above. I don't want to return here to allow parsing other VSECs. Thanks, Lukas > > + } > > + > > + /* > > + * Is the device attached with Thunderbolt? Walk upwards and check for > > + * each encountered bridge if it's part of a Thunderbolt controller. > > + * Reaching the host bridge means dev is soldered to the mainboard. > > + */ > > + if (!dev->is_thunderbolt) { > > The "if" is unnecessary if you return above. > > > + struct pci_dev *parent = dev; > > + > > + while ((parent = pci_upstream_bridge(parent))) > > + if (parent->is_thunderbolt) { > > + dev->is_thunderbolt = 1; > > + break; > > + } > > + } > > +} > > + > > /** > > * pci_ext_cfg_is_aliased - is ext config space just an alias of std config? > > * @dev: PCI device > > @@ -1358,6 +1389,9 @@ int pci_setup_device(struct pci_dev *dev) > > /* need to have dev->class ready */ > > dev->cfg_size = pci_cfg_space_size(dev); > > > > + /* need to have dev->cfg_size ready */ > > + set_pcie_vendor_specific(dev); > > + > > /* "Unknown power state" */ > > dev->current_state = PCI_UNKNOWN; > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index e2d1a124216a..3c775e8498f1 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -358,6 +358,7 @@ struct pci_dev { > > unsigned int is_virtfn:1; > > unsigned int reset_fn:1; > > unsigned int is_hotplug_bridge:1; > > + unsigned int is_thunderbolt:1; /* part of Thunderbolt daisy chain */ > > unsigned int __aer_firmware_first_valid:1; > > unsigned int __aer_firmware_first:1; > > unsigned int broken_intx_masking:1; > > -- > > 2.11.0 > > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 29, 2017 at 01:26:16AM +0100, Lukas Wunner wrote: > On Sat, Jan 28, 2017 at 03:52:08PM -0600, Bjorn Helgaas wrote: > > On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote: > > > --- a/drivers/pci/probe.c > > > +++ b/drivers/pci/probe.c > > > @@ -1206,6 +1206,37 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) > > > pdev->is_hotplug_bridge = 1; > > > } > > > > > > +static void set_pcie_vendor_specific(struct pci_dev *dev) > > > > This is very specific to Thunderbolt, so let's name it something that > > conveys that information. The fact that we use a vendor-specific > > capability to figure it out isn't really relevant in the caller. > > I thought that we may have the necessity in the future to parse other > VSECs on device probe, so I gave the function this generic name. > > Think about it, every VSEC that needs to be parsed needs the while loop > below. It's more efficient to have only a single while loop that handles > *all* VSECs at once. > > If someone needs to parse another VSEC, they just add it to this function. > So IMO the way I've solved it is preferable to just adding a Thunderbolt- > specific function. > > Are you sure you want this renamed? (y/n) Bjorn, I'm taking the liberty to send a gentle ping already after a week: Could you give me a quick yes or no for the above question so that I get a chance to submit a rectified version of this patch before the merge window opens? Thanks! Lukas
On Sun, Jan 29, 2017 at 01:26:16AM +0100, Lukas Wunner wrote: > On Sat, Jan 28, 2017 at 03:52:08PM -0600, Bjorn Helgaas wrote: > > On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote: > > > +static void set_pcie_vendor_specific(struct pci_dev *dev) > > > > This is very specific to Thunderbolt, so let's name it something that > > conveys that information. The fact that we use a vendor-specific > > capability to figure it out isn't really relevant in the caller. > > I thought that we may have the necessity in the future to parse other > VSECs on device probe, so I gave the function this generic name. > > Think about it, every VSEC that needs to be parsed needs the while loop > below. It's more efficient to have only a single while loop that handles > *all* VSECs at once. > > If someone needs to parse another VSEC, they just add it to this function. > So IMO the way I've solved it is preferable to just adding a Thunderbolt- > specific function. > > Are you sure you want this renamed? (y/n) Sorry for the delay; I missed this question. If this has already been merged somewhere as-is, that's fine. If you repost it for any reason, I would prefer to rename it so it reflects the functionality rather than the source of the information. This isn't a performance path, so readability is more important than optimization. Bjorn
On Fri, Feb 10, 2017 at 11:42:50AM -0600, Bjorn Helgaas wrote: > On Sun, Jan 29, 2017 at 01:26:16AM +0100, Lukas Wunner wrote: > > On Sat, Jan 28, 2017 at 03:52:08PM -0600, Bjorn Helgaas wrote: > > > On Sun, Jan 15, 2017 at 09:03:45PM +0100, Lukas Wunner wrote: > > > > > +static void set_pcie_vendor_specific(struct pci_dev *dev) > > > > > > This is very specific to Thunderbolt, so let's name it something that > > > conveys that information. The fact that we use a vendor-specific > > > capability to figure it out isn't really relevant in the caller. > > > > I thought that we may have the necessity in the future to parse other > > VSECs on device probe, so I gave the function this generic name. > > > > Think about it, every VSEC that needs to be parsed needs the while loop > > below. It's more efficient to have only a single while loop that handles > > *all* VSECs at once. > > > > If someone needs to parse another VSEC, they just add it to this function. > > So IMO the way I've solved it is preferable to just adding a Thunderbolt- > > specific function. > > > > Are you sure you want this renamed? (y/n) > > Sorry for the delay; I missed this question. If this has already been > merged somewhere as-is, that's fine. No, none of the patches in this series have been merged so far. Greg's tree is closed for the duration of the merge window, which is expected to open today. Therefore, please merge whatever patches in this series you feel comfortable with. It would be good if you could merge at least patch [1/8] as it would allow me to fix the issues in apple-gmux and vga_switcheroo that I'm mentioning in the changelog in v4.12. Also, patch [2/8] seems uncontroversial. Greg has acked v5 of this series and suggested that you take it due to all the PCI changes: https://lkml.org/lkml/2017/1/19/177 Andreas Noever acked and tested v2 of this series: https://www.spinics.net/lists/linux-pci/msg51274.html > If you repost it for any reason, > I would prefer to rename it so it reflects the functionality rather > than the source of the information. This isn't a performance path, so > readability is more important than optimization. Sure, I've renamed it. Thanks, Lukas
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index cb17db242f30..45c2b8144911 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -3,6 +3,8 @@ #define PCI_FIND_CAP_TTL 48 +#define PCI_VSEC_ID_INTEL_TBT 0x1234 /* Thunderbolt */ + extern const unsigned char pcie_link_speed[]; bool pcie_cap_has_lnkctl(const struct pci_dev *dev); diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index e164b5c9f0f0..891a8fa1f9f6 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1206,6 +1206,37 @@ void set_pcie_hotplug_bridge(struct pci_dev *pdev) pdev->is_hotplug_bridge = 1; } +static void set_pcie_vendor_specific(struct pci_dev *dev) +{ + int vsec = 0; + u32 header; + + while ((vsec = pci_find_next_ext_capability(dev, vsec, + PCI_EXT_CAP_ID_VNDR))) { + pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header); + + /* Is the device part of a Thunderbolt controller? */ + if (dev->vendor == PCI_VENDOR_ID_INTEL && + PCI_VNDR_HEADER_ID(header) == PCI_VSEC_ID_INTEL_TBT) + dev->is_thunderbolt = 1; + } + + /* + * Is the device attached with Thunderbolt? Walk upwards and check for + * each encountered bridge if it's part of a Thunderbolt controller. + * Reaching the host bridge means dev is soldered to the mainboard. + */ + if (!dev->is_thunderbolt) { + struct pci_dev *parent = dev; + + while ((parent = pci_upstream_bridge(parent))) + if (parent->is_thunderbolt) { + dev->is_thunderbolt = 1; + break; + } + } +} + /** * pci_ext_cfg_is_aliased - is ext config space just an alias of std config? * @dev: PCI device @@ -1358,6 +1389,9 @@ int pci_setup_device(struct pci_dev *dev) /* need to have dev->class ready */ dev->cfg_size = pci_cfg_space_size(dev); + /* need to have dev->cfg_size ready */ + set_pcie_vendor_specific(dev); + /* "Unknown power state" */ dev->current_state = PCI_UNKNOWN; diff --git a/include/linux/pci.h b/include/linux/pci.h index e2d1a124216a..3c775e8498f1 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -358,6 +358,7 @@ struct pci_dev { unsigned int is_virtfn:1; unsigned int reset_fn:1; unsigned int is_hotplug_bridge:1; + unsigned int is_thunderbolt:1; /* part of Thunderbolt daisy chain */ unsigned int __aer_firmware_first_valid:1; unsigned int __aer_firmware_first:1; unsigned int broken_intx_masking:1;
We're about to allow runtime PM on Thunderbolt ports in pci_bridge_d3_possible() and unblock runtime PM for Thunderbolt host hotplug ports in pci_dev_check_d3cold(). In both cases we need to uniquely identify if a PCI device belongs to a Thunderbolt controller. We also have the need to detect presence of a Thunderbolt controller in drivers/platform/x86/apple-gmux.c because dual GPU MacBook Pros cannot switch external DP/HDMI ports between GPUs if they have Thunderbolt. Furthermore, in multiple places in the DRM subsystem we need to detect whether a GPU is on-board or attached with Thunderbolt. As an example, Thunderbolt-attached GPUs shall not be registered with vga_switcheroo. Intel uses a Vendor-Specific Extended Capability (VSEC) with ID 0x1234 on devices belonging to a Thunderbolt controller which allows us to recognize them. Detect presence of this VSEC on device probe and cache it in a newly added is_thunderbolt bit in struct pci_dev which can then be queried by pci_bridge_d3_possible(), pci_dev_check_d3cold(), apple-gmux and others. Cc: Andreas Noever <andreas.noever@gmail.com> Cc: Tomas Winkler <tomas.winkler@intel.com> Cc: Amir Levy <amir.jer.levy@intel.com> Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/pci.h | 2 ++ drivers/pci/probe.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/pci.h | 1 + 3 files changed, 37 insertions(+)