From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S967782AbdAHNrR (ORCPT ); Sun, 8 Jan 2017 08:47:17 -0500 Received: from mailout2.hostsharing.net ([83.223.90.233]:35611 "EHLO mailout2.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932078AbdAHNrG (ORCPT ); Sun, 8 Jan 2017 08:47:06 -0500 Date: Sun, 8 Jan 2017 14:47:51 +0100 From: Lukas Wunner To: "Winkler, Tomas" Cc: Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" , Andreas Noever , "linux-pci@vger.kernel.org" , "linux-pm@vger.kernel.org" , "Chen, Yu C" , "Levy, Amir (Jer)" , Bjorn Helgaas Subject: Re: [PATCH v4 1/8] PCI: Recognize Thunderbolt devices Message-ID: <20170108134751.GA23120@wunner.de> References: <5B8DA87D05A7694D9FA63FD143655C1B54376D27@hasmsx108.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5B8DA87D05A7694D9FA63FD143655C1B54376D27@hasmsx108.ger.corp.intel.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jan 08, 2017 at 10:23:03AM +0000, Winkler, Tomas 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. > > > > 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 > > Cc: Tomas Winkler > > Cc: Amir Levy > > Signed-off-by: Lukas Wunner > > --- > > 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 cb17db2..45c2b81 > > 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 */ > > Shouldn't bet this defined in pci_ids.h ? That file is solely intended for IDs used in multiple places, as explained in the comment at its top. This particular ID is only used in the PCI core, hence it's in the PCI core's private drivers/pci/pci.h. However the line is formatted such that it can just be moved to the global include/linux/pci_ids.h should it be needed someplace else in the future. > > 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 e164b5c..891a8fa 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) { > > > Why don't u implement this in quirk.c ? The is_thunderbolt bit signifies whether a given PCI device is part of a Thunderbolt daisy chain. (As explained in the comment in include/linux/pci.h, see below.) So it is set on all the PCI devices that comprise a Thunderbolt controller, but also on all endpoint devices that are attached via Thunderbolt. Consequently this function needs to be executed for all PCI devices, not just for a subset that could be identified by a vendor, device or class ID. In the DRM drivers nouveau, radeon and amdgpu I need to prevent calls to vga_switcheroo_register_client() and vga_switcheroo_init_domain_pm_ops() if the device in question is attached via Thunderbolt. That's because external GPUs can't drive the panel of a laptop or be powered down like an on-board Optimus/PowerXpress GPU. We've got a bug there right now that manifests itself once someone attaches an eGPU to a dual GPU laptop. With this patch I'll be able to just skip registration with vga_switcheroo if is_thunderbolt is set on a GPU's pci_dev, thereby fixing the bug. BTW do you have information on the contents/meaning of the VSEC that you would be able/allowed to share? What I've seen so far is that its length is 0x1c bytes on older controllers (e.g. Light Ridge, Port Ridge) and its contents are always the same, regardless if the controller is used in host mode or endpoint mode: 500: 0b 00 01 00 34 12 c1 01|50 09 00 00 39 00 00 00 510: 00 00 00 00 00 00 00 00 00 00 00 00 However on Alpine Ridge the size of the VSEC has increased significantly to 0xd8 bytes, even though the version stayed at 1 as before: 500: 0b 00 01 60 34 12 81 0d 50 09 b0 0c b9 06 18 08 510: 00 38 00 01 00 00 00 00 00 00 00 00 32 02 10 00 520: ef d3 00 40 00 00 00 00 f0 f0 30 c1 00 02 30 00 530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 590: 00 00 00 08 00 00 00 00 00 00 00 00 00 00 00 00 5a0: 00 00 00 00 00 00 00 00 00 00 00 00 38 24 01 00 5b0: 08 40 00 1c 00 00 01 18 04 03 00 f0 06 00 00 00 5c0: 00 00 08 40 06 00 00 01 90 00 08 1b 00 08 80 08 5d0: 80 7f 08 00 00 00 00 00 Thanks, Lukas > > > + 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 e2d1a12..3c775e8 > > 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