[PATCH 0/2] PCI: device link quirk for NVIDIA GPU

[PATCH 0/2] PCI: device link quirk for NVIDIA GPU

[Abhishek Sahu]

NVIDIA Turing GPU [1] has hardware support for USB Type-C and
VirtualLink [2]. The Turing GPU is a multi-function PCI device
which has the following four functions:

	- VGA display controller (Function 0)
	- Audio controller (Function 1)
	- USB xHCI Host controller (Function 2)
	- USB Type-C USCI controller (Function 3)

Currently NVIDIA and Nouveau GPU drivers only manage function 0.
Rest of the functions are managed by other drivers. These functions
internally in the hardware are tightly coupled. When function 0 goes
in runtime suspended state, then it will do power gating for most of
the hardware blocks. Some of these hardware blocks are used by
the other PCI functions, which leads to functional failure. In the
mainline kernel, the device link is present between
function 0 and function 1.  This patch series deals with creating
a similar kind of device link between function 0 and
functions 2 and 3.

[1] https://www.nvidia.com/content/dam/en-zz/Solutions/design-visualization/technologies/turing-architecture/NVIDIA-Turing-Architecture-Whitepaper.pdf
[2] https://en.wikipedia.org/wiki/VirtualLink

Abhishek Sahu (2):
  PCI: Code reorganization for VGA device link
  PCI: Create device link for NVIDIA GPU

 drivers/pci/quirks.c | 67 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/2] PCI: Code reorganization for VGA device link

[Abhishek Sahu]

This patch does minor code reorganization. It introduces a helper
function which creates device link from the non-VGA controller
(consumer) to the VGA (supplier) and uses this helper function for
creating device link from integrated HDA controller to VGA. It will
help in subsequent patches which require a similar kind of device
link from USB/Type-C USCI controller to VGA.

Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
---
 drivers/pci/quirks.c | 44 +++++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a077f67fe1da..a20f7771a323 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4916,36 +4916,50 @@ static void quirk_fsl_no_msi(struct pci_dev *pdev)
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, quirk_fsl_no_msi);
 
 /*
- * GPUs with integrated HDA controller for streaming audio to attached displays
- * need a device link from the HDA controller (consumer) to the GPU (supplier)
- * so that the GPU is powered up whenever the HDA controller is accessed.
- * The GPU and HDA controller are functions 0 and 1 of the same PCI device.
- * The device link stays in place until shutdown (or removal of the PCI device
- * if it's hotplugged).  Runtime PM is allowed by default on the HDA controller
- * to prevent it from permanently keeping the GPU awake.
+ * GPUs can be multi-function PCI device which can contain controllers other
+ * than VGA (like Audio, USB, etc.). Internally in the hardware, these non-VGA
+ * controllers are tightly coupled with VGA controller. Whenever these
+ * controllers are runtime active, the VGA controller should also be in active
+ * state. Normally, in these GPUs, the VGA controller is present at function 0.
+ *
+ * This is a helper function which creates device link from the non-VGA
+ * controller (consumer) to the VGA (supplier). The device link stays in place
+ * until shutdown (or removal of the PCI device if it's hotplugged).
+ * Runtime PM is allowed by default on these non-VGA controllers to prevent
+ * it from permanently keeping the GPU awake.
  */
-static void quirk_gpu_hda(struct pci_dev *hda)
+static void
+pci_create_device_link_with_vga(struct pci_dev *pdev, unsigned int devfn)
 {
 	struct pci_dev *gpu;
 
-	if (PCI_FUNC(hda->devfn) != 1)
+	if (PCI_FUNC(pdev->devfn) != devfn)
 		return;
 
-	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus),
-					  hda->bus->number,
-					  PCI_DEVFN(PCI_SLOT(hda->devfn), 0));
+	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
+					  pdev->bus->number,
+					  PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
 	if (!gpu || (gpu->class >> 16) != PCI_BASE_CLASS_DISPLAY) {
 		pci_dev_put(gpu);
 		return;
 	}
 
-	if (!device_link_add(&hda->dev, &gpu->dev,
+	if (!device_link_add(&pdev->dev, &gpu->dev,
 			     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME))
-		pci_err(hda, "cannot link HDA to GPU %s\n", pci_name(gpu));
+		pci_err(pdev, "cannot link with VGA %s\n", pci_name(gpu));
 
-	pm_runtime_allow(&hda->dev);
+	pm_runtime_allow(&pdev->dev);
 	pci_dev_put(gpu);
 }
+
+/*
+ * Create device link for GPUs with integrated HDA controller for streaming
+ * audio to attached displays.
+ */
+static void quirk_gpu_hda(struct pci_dev *hda)
+{
+	pci_create_device_link_with_vga(hda, 1);
+}
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
 			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID,
-- 
2.17.1

Re: [PATCH 1/2] PCI: Code reorganization for VGA device link

[Bjorn Helgaas]

[+cc Lukas]

On Fri, May 31, 2019 at 10:31:08AM +0530, Abhishek Sahu wrote:
> This patch does minor code reorganization. It introduces a helper
> function which creates device link from the non-VGA controller
> (consumer) to the VGA (supplier) and uses this helper function for
> creating device link from integrated HDA controller to VGA. It will
> help in subsequent patches which require a similar kind of device
> link from USB/Type-C USCI controller to VGA.
> 
> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
> ---
>  drivers/pci/quirks.c | 44 +++++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a077f67fe1da..a20f7771a323 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4916,36 +4916,50 @@ static void quirk_fsl_no_msi(struct pci_dev *pdev)
>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, quirk_fsl_no_msi);
>  
>  /*
> - * GPUs with integrated HDA controller for streaming audio to attached displays
> - * need a device link from the HDA controller (consumer) to the GPU (supplier)
> - * so that the GPU is powered up whenever the HDA controller is accessed.
> - * The GPU and HDA controller are functions 0 and 1 of the same PCI device.
> - * The device link stays in place until shutdown (or removal of the PCI device
> - * if it's hotplugged).  Runtime PM is allowed by default on the HDA controller
> - * to prevent it from permanently keeping the GPU awake.
> + * GPUs can be multi-function PCI device which can contain controllers other
> + * than VGA (like Audio, USB, etc.). Internally in the hardware, these non-VGA
> + * controllers are tightly coupled with VGA controller. Whenever these
> + * controllers are runtime active, the VGA controller should also be in active
> + * state. Normally, in these GPUs, the VGA controller is present at function 0.
> + *
> + * This is a helper function which creates device link from the non-VGA
> + * controller (consumer) to the VGA (supplier). The device link stays in place
> + * until shutdown (or removal of the PCI device if it's hotplugged).
> + * Runtime PM is allowed by default on these non-VGA controllers to prevent
> + * it from permanently keeping the GPU awake.
>   */
> -static void quirk_gpu_hda(struct pci_dev *hda)
> +static void
> +pci_create_device_link_with_vga(struct pci_dev *pdev, unsigned int devfn)

There's nothing in this functionality that depends on VGA, so let's
remove "GPU, "VGA", etc from the description, the function name, the
local variable name, and the log message.  Maybe you need to allow the
caller to supply the class type (PCI_BASE_CLASS_DISPLAY for current
users, but Lukas mentioned a NIC that might be able to use this too).

Follow the prevailing indentation style, with return type and function
name on the same line, i.e.,

  static void pci_create_device_link(...)

>  {
>  	struct pci_dev *gpu;
>  
> -	if (PCI_FUNC(hda->devfn) != 1)
> +	if (PCI_FUNC(pdev->devfn) != devfn)
>  		return;
>  
> -	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus),
> -					  hda->bus->number,
> -					  PCI_DEVFN(PCI_SLOT(hda->devfn), 0));
> +	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
> +					  pdev->bus->number,
> +					  PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
>  	if (!gpu || (gpu->class >> 16) != PCI_BASE_CLASS_DISPLAY) {
>  		pci_dev_put(gpu);
>  		return;
>  	}
>  
> -	if (!device_link_add(&hda->dev, &gpu->dev,
> +	if (!device_link_add(&pdev->dev, &gpu->dev,
>  			     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME))
> -		pci_err(hda, "cannot link HDA to GPU %s\n", pci_name(gpu));
> +		pci_err(pdev, "cannot link with VGA %s\n", pci_name(gpu));

I think we should emit a message in the success case, too.  There is
one in device_link_add(), but it's a dev_dbg() so we can't count on it
being in the log.  I'd like a pci_info() that we can count on.

> -	pm_runtime_allow(&hda->dev);
> +	pm_runtime_allow(&pdev->dev);
>  	pci_dev_put(gpu);
>  }
> +
> +/*
> + * Create device link for GPUs with integrated HDA controller for streaming
> + * audio to attached displays.
> + */
> +static void quirk_gpu_hda(struct pci_dev *hda)
> +{
> +	pci_create_device_link_with_vga(hda, 1);
> +}
>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>  			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID,
> -- 
> 2.17.1
> 

Re: [PATCH 1/2] PCI: Code reorganization for VGA device link

[Abhishek Sahu]

On 6/3/2019 10:45 PM, Bjorn Helgaas wrote:
> [+cc Lukas]
> 
> On Fri, May 31, 2019 at 10:31:08AM +0530, Abhishek Sahu wrote:
>> This patch does minor code reorganization. It introduces a helper
>> function which creates device link from the non-VGA controller
>> (consumer) to the VGA (supplier) and uses this helper function for
>> creating device link from integrated HDA controller to VGA. It will
>> help in subsequent patches which require a similar kind of device
>> link from USB/Type-C USCI controller to VGA.
>>
>> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
>> ---
>>  drivers/pci/quirks.c | 44 +++++++++++++++++++++++++++++---------------
>>  1 file changed, 29 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index a077f67fe1da..a20f7771a323 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -4916,36 +4916,50 @@ static void quirk_fsl_no_msi(struct pci_dev *pdev)
>>  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_FREESCALE, PCI_ANY_ID, quirk_fsl_no_msi);
>>  
>>  /*
>> - * GPUs with integrated HDA controller for streaming audio to attached displays
>> - * need a device link from the HDA controller (consumer) to the GPU (supplier)
>> - * so that the GPU is powered up whenever the HDA controller is accessed.
>> - * The GPU and HDA controller are functions 0 and 1 of the same PCI device.
>> - * The device link stays in place until shutdown (or removal of the PCI device
>> - * if it's hotplugged).  Runtime PM is allowed by default on the HDA controller
>> - * to prevent it from permanently keeping the GPU awake.
>> + * GPUs can be multi-function PCI device which can contain controllers other
>> + * than VGA (like Audio, USB, etc.). Internally in the hardware, these non-VGA
>> + * controllers are tightly coupled with VGA controller. Whenever these
>> + * controllers are runtime active, the VGA controller should also be in active
>> + * state. Normally, in these GPUs, the VGA controller is present at function 0.
>> + *
>> + * This is a helper function which creates device link from the non-VGA
>> + * controller (consumer) to the VGA (supplier). The device link stays in place
>> + * until shutdown (or removal of the PCI device if it's hotplugged).
>> + * Runtime PM is allowed by default on these non-VGA controllers to prevent
>> + * it from permanently keeping the GPU awake.
>>   */
>> -static void quirk_gpu_hda(struct pci_dev *hda)
>> +static void
>> +pci_create_device_link_with_vga(struct pci_dev *pdev, unsigned int devfn)
> 
> There's nothing in this functionality that depends on VGA, so let's
> remove "GPU, "VGA", etc from the description, the function name, the
> local variable name, and the log message.  Maybe you need to allow the

 Thanks. Then we can make this function generic where we can pass
 device link supplier and supplier pci class mask. It will help
 in creating device link from one function (other than 0 also) to
 any another function. Later on, same can be used by non
 GPUs devices also, if required.

> caller to supply the class type (PCI_BASE_CLASS_DISPLAY for current
> users, but Lukas mentioned a NIC that might be able to use this too).
> 
> Follow the prevailing indentation style, with return type and function
> name on the same line, i.e.,
> 

 I will fix in v2.

>   static void pci_create_device_link(...)
> 
>>  {
>>  	struct pci_dev *gpu;
>>  
>> -	if (PCI_FUNC(hda->devfn) != 1)
>> +	if (PCI_FUNC(pdev->devfn) != devfn)
>>  		return;
>>  
>> -	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(hda->bus),
>> -					  hda->bus->number,
>> -					  PCI_DEVFN(PCI_SLOT(hda->devfn), 0));
>> +	gpu = pci_get_domain_bus_and_slot(pci_domain_nr(pdev->bus),
>> +					  pdev->bus->number,
>> +					  PCI_DEVFN(PCI_SLOT(pdev->devfn), 0));
>>  	if (!gpu || (gpu->class >> 16) != PCI_BASE_CLASS_DISPLAY) {
>>  		pci_dev_put(gpu);
>>  		return;
>>  	}
>>  
>> -	if (!device_link_add(&hda->dev, &gpu->dev,
>> +	if (!device_link_add(&pdev->dev, &gpu->dev,
>>  			     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME))
>> -		pci_err(hda, "cannot link HDA to GPU %s\n", pci_name(gpu));
>> +		pci_err(pdev, "cannot link with VGA %s\n", pci_name(gpu));
> 
> I think we should emit a message in the success case, too.  There is
> one in device_link_add(), but it's a dev_dbg() so we can't count on it
> being in the log.  I'd like a pci_info() that we can count on.
> 

 I will add this in v2.

 Regards,
 Abhishek

>> -	pm_runtime_allow(&hda->dev);
>> +	pm_runtime_allow(&pdev->dev);
>>  	pci_dev_put(gpu);
>>  }
>> +
>> +/*
>> + * Create device link for GPUs with integrated HDA controller for streaming
>> + * audio to attached displays.
>> + */
>> +static void quirk_gpu_hda(struct pci_dev *hda)
>> +{
>> +	pci_create_device_link_with_vga(hda, 1);
>> +}
>>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
>>  			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
>>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID,
>> -- 
>> 2.17.1
>>

[PATCH 2/2] PCI: Create device link for NVIDIA GPU

[Abhishek Sahu]

NVIDIA Turing GPUs include hardware support for USB Type-C and
VirtualLink. It helps in delivering the power, display, and data
required to power VR headsets through a single USB Type-C connector.
The Turing GPU is a multi-function PCI device has the following
four functions:

	- VGA display controller (Function 0)
	- Audio controller (Function 1)
	- USB xHCI Host controller (Function 2)
	- USB Type-C USCI controller (Function 3)

The function 0 is tightly coupled with other functions in the
hardware. When function 0 goes in runtime suspended state,
then it will do power gating for most of the hardware blocks.
Some of these hardware blocks are used by other functions which
leads to functional failure. So if any of these functions (1/2/3)
are active, then function 0 should also be in active state.
'commit 07f4f97d7b4b ("vga_switcheroo: Use device link for
HDA controller")' creates the device link from function 1 to
function 0. A similar kind of device link needs to be created
between function 0 and functions 2 and 3 for NVIDIA Turing GPU.

This patch does the same and create the required device links. It
will make function 0 to be runtime PM active if other functions
are still active.

Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
---
 drivers/pci/quirks.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a20f7771a323..afdbc199efc5 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4967,6 +4967,29 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID,
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
 			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
 
+/* Create device link for NVIDIA GPU with integrated USB controller to VGA. */
+static void quirk_gpu_usb(struct pci_dev *usb)
+{
+	pci_create_device_link_with_vga(usb, 2);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+			      PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);
+
+/*
+ * Create device link for NVIDIA GPU with integrated Type-C UCSI controller
+ * to VGA. Currently there is no class code defined for UCSI device over PCI
+ * so using UNKNOWN class for now and it will be updated when UCSI
+ * over PCI gets a class code.
+ */
+#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
+static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi)
+{
+	pci_create_device_link_with_vga(ucsi, 3);
+}
+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
+			      PCI_CLASS_SERIAL_UNKNOWN, 8,
+			      quirk_gpu_usb_typec_ucsi);
+
 /*
  * Some IDT switches incorrectly flag an ACS Source Validation error on
  * completions for config read requests even though PCIe r4.0, sec
-- 
2.17.1

Re: [PATCH 2/2] PCI: Create device link for NVIDIA GPU

[Bjorn Helgaas]

[+cc Lukas, author of 07f4f97d7b4b ("vga_switcheroo: Use device link
for HDA controller")]

On Fri, May 31, 2019 at 10:31:09AM +0530, Abhishek Sahu wrote:
> NVIDIA Turing GPUs include hardware support for USB Type-C and
> VirtualLink. It helps in delivering the power, display, and data
> required to power VR headsets through a single USB Type-C connector.
> The Turing GPU is a multi-function PCI device has the following
> four functions:
> 
> 	- VGA display controller (Function 0)
> 	- Audio controller (Function 1)
> 	- USB xHCI Host controller (Function 2)
> 	- USB Type-C USCI controller (Function 3)
> 
> The function 0 is tightly coupled with other functions in the
> hardware. When function 0 goes in runtime suspended state,

"Runtime suspended" is a Linux concept, not a PCI concept.  Please
replace this with the appropriate PCI term, e.g., "D3hot" or whatever
it is.

> then it will do power gating for most of the hardware blocks.
> Some of these hardware blocks are used by other functions which
> leads to functional failure. So if any of these functions (1/2/3)
> are active, then function 0 should also be in active state.

Instead of "active" and "active state", please use the specific states
required in terms of PCI.

> 'commit 07f4f97d7b4b ("vga_switcheroo: Use device link for
> HDA controller")' creates the device link from function 1 to
> function 0. A similar kind of device link needs to be created
> between function 0 and functions 2 and 3 for NVIDIA Turing GPU.

I can't point to language that addresses this, but this sounds like a
case of the GPU not conforming to the PCI spec.  The general
assumption is that the OS should be able to discover everything it
needs to do power management directly from the architected PCI config
space.

It is definitely not ideal to have to add quirks like this for devices
designed this way.  Such quirks force us to do otherwise unnecessary
OS updates as new devices are released.

If all the devices in a multi-function device were connected
intimately enough that they all had to be managed by the same driver,
I could imagine putting these non-discoverable dependencies in the
driver.  But these devices don't seem to be related in that way.

If there *is* spec language that allows dependencies like this, please
include the citation in your commit log.

> This patch does the same and create the required device links. It
> will make function 0 to be runtime PM active if other functions
> are still active.
> 
> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
> ---
>  drivers/pci/quirks.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a20f7771a323..afdbc199efc5 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -4967,6 +4967,29 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID,
>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
>  			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
>  
> +/* Create device link for NVIDIA GPU with integrated USB controller to VGA. */
> +static void quirk_gpu_usb(struct pci_dev *usb)
> +{
> +	pci_create_device_link_with_vga(usb, 2);
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> +			      PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);
> +
> +/*
> + * Create device link for NVIDIA GPU with integrated Type-C UCSI controller
> + * to VGA. Currently there is no class code defined for UCSI device over PCI
> + * so using UNKNOWN class for now and it will be updated when UCSI
> + * over PCI gets a class code.

Ugh.  Here's a good example of having to do yet another OS update.

> + */
> +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> +static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi)
> +{
> +	pci_create_device_link_with_vga(ucsi, 3);
> +}
> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
> +			      PCI_CLASS_SERIAL_UNKNOWN, 8,
> +			      quirk_gpu_usb_typec_ucsi);
> +
>  /*
>   * Some IDT switches incorrectly flag an ACS Source Validation error on
>   * completions for config read requests even though PCIe r4.0, sec
> -- 
> 2.17.1
> 

Re: [PATCH 2/2] PCI: Create device link for NVIDIA GPU

[Abhishek Sahu]

 Thanks Bjorn for your review.

On 6/1/2019 2:09 AM, Bjorn Helgaas wrote:
> [+cc Lukas, author of 07f4f97d7b4b ("vga_switcheroo: Use device link
> for HDA controller")]
> 
> On Fri, May 31, 2019 at 10:31:09AM +0530, Abhishek Sahu wrote:
>> NVIDIA Turing GPUs include hardware support for USB Type-C and
>> VirtualLink. It helps in delivering the power, display, and data
>> required to power VR headsets through a single USB Type-C connector.
>> The Turing GPU is a multi-function PCI device has the following
>> four functions:
>>
>> 	- VGA display controller (Function 0)
>> 	- Audio controller (Function 1)
>> 	- USB xHCI Host controller (Function 2)
>> 	- USB Type-C USCI controller (Function 3)
>>
>> The function 0 is tightly coupled with other functions in the
>> hardware. When function 0 goes in runtime suspended state,
> 
> "Runtime suspended" is a Linux concept, not a PCI concept.  Please
> replace this with the appropriate PCI term, e.g., "D3hot" or whatever
> it is.

 Sure. I will change this.

> 
>> then it will do power gating for most of the hardware blocks.
>> Some of these hardware blocks are used by other functions which
>> leads to functional failure. So if any of these functions (1/2/3)
>> are active, then function 0 should also be in active state.
> 
> Instead of "active" and "active state", please use the specific states
> required in terms of PCI.

 Sure. I will use specific states name.

> 
>> 'commit 07f4f97d7b4b ("vga_switcheroo: Use device link for
>> HDA controller")' creates the device link from function 1 to
>> function 0. A similar kind of device link needs to be created
>> between function 0 and functions 2 and 3 for NVIDIA Turing GPU.
> 
> I can't point to language that addresses this, but this sounds like a
> case of the GPU not conforming to the PCI spec.  The general
> assumption is that the OS should be able to discover everything it
> needs to do power management directly from the architected PCI config
> space.

 The GPU is following PCIe spec but following is the implementation
 from HW side

 Normal GPU has VGA and Audio controller in which Audio is dependent
 upon VGA. For these GPU, the VGA is managed by GPU driver and Audio is
 managed by sound driver. Now the VGA driver can go to D3hot while
 Audio still in D0 if there was no device link (added in commit
 07f4f97d7b4b). It would lead to Audio functionality failure. The
 device link is making sure that GPU is in D0 while Audio is D0.

 Now the NVIDIA Turing GPU has one USB Type-C port mainly to support
 virtual reality headset. With default mode, this USB port will act as
 normal USB port and any USB device can be connected over it. It will
 be managed with PCI xHCI USB controller driver.  Now, to support VR
 headset, This USB Type-C alternate mode is going to be used. This
 alternate mode setting will be managed with [1] and [2] which is part
 of 5.2-rc1. More detail for this is available in [3] and virtual link
 open industry standard [4].  These VR frame-buffers are internally
 going to be rendered by GPU only and managed by function 0 (VGA)
 driver. Now, from HW side, we need to make sure VGA is in D0 while any
 of other functions are in D0.

 [1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/altmodes/nvidia.c
 [2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-nvidia-gpu.c
 [3]
https://fullcirclemagazine.org/2019/04/30/nvidia-creates-free-virtual-link-driver-for-linux/
 [4] https://sites.google.com/view/virtuallink-consortium/home

> 
> It is definitely not ideal to have to add quirks like this for devices
> designed this way.  Such quirks force us to do otherwise unnecessary
> OS updates as new devices are released.

 I can understand but this is the HW requirement. To support,
 VR headset in GPU, the HW has provided USB Type-C. Now, from
 SW side, we are working on supporting this with runtime PM.
 Currently runtime PM is not possible without adding the
 dependencies between different functions.

> 
> If all the devices in a multi-function device were connected
> intimately enough that they all had to be managed by the same driver,
> I could imagine putting these non-discoverable dependencies in the
> driver.  But these devices don't seem to be related in that way.
> > If there *is* spec language that allows dependencies like this, please
> include the citation in your commit log.
> 

 The PCIe specification treats each function separately but GPU case is
 different. So, it won't be part of PCIe spec. in GPU, the different
 kind of devices are internally coupled in HW but still needs to be
 managed by different driver.

>> This patch does the same and create the required device links. It
>> will make function 0 to be runtime PM active if other functions
>> are still active.
>>
>> Signed-off-by: Abhishek Sahu <abhsahu@nvidia.com>
>> ---
>>  drivers/pci/quirks.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index a20f7771a323..afdbc199efc5 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -4967,6 +4967,29 @@ DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_AMD, PCI_ANY_ID,
>>  DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
>>  			      PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda);
>>  
>> +/* Create device link for NVIDIA GPU with integrated USB controller to VGA. */
>> +static void quirk_gpu_usb(struct pci_dev *usb)
>> +{
>> +	pci_create_device_link_with_vga(usb, 2);
>> +}
>> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
>> +			      PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);
>> +
>> +/*
>> + * Create device link for NVIDIA GPU with integrated Type-C UCSI controller
>> + * to VGA. Currently there is no class code defined for UCSI device over PCI
>> + * so using UNKNOWN class for now and it will be updated when UCSI
>> + * over PCI gets a class code.
> 
> Ugh.  Here's a good example of having to do yet another OS update.
> 

 Correct. But currently we don't have any other way.
 Same thing we did for
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-nvidia-gpu.c#n248

 Once it gets a class code then we can replace at all the places.

 Regards,
 Abhishek

>> + */
>> +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
>> +static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi)
>> +{
>> +	pci_create_device_link_with_vga(ucsi, 3);
>> +}
>> +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
>> +			      PCI_CLASS_SERIAL_UNKNOWN, 8,
>> +			      quirk_gpu_usb_typec_ucsi);
>> +
>>  /*
>>   * Some IDT switches incorrectly flag an ACS Source Validation error on
>>   * completions for config read requests even though PCIe r4.0, sec
>> -- 
>> 2.17.1
>>

Re: [PATCH 2/2] PCI: Create device link for NVIDIA GPU

[Lukas Wunner]

On Mon, Jun 03, 2019 at 01:30:51PM +0530, Abhishek Sahu wrote:
> On 6/1/2019 2:09 AM, Bjorn Helgaas wrote:
> > > If there *is* spec language that allows dependencies like this, please
> > include the citation in your commit log.
> 
>  The PCIe specification treats each function separately but GPU case is
>  different. So, it won't be part of PCIe spec. in GPU, the different
>  kind of devices are internally coupled in HW but still needs to be
>  managed by different driver.

GPUs aren't the only devices which have these implicit dependencies.

E.g. Broadcom BCM57765 combines a Gigabit Ethernet chip in function 0
(14e4:16b4) with an SD card reader in function 1 (14e4:16bc) and AFAIK
for the latter to work, the former needs to be in D0.  I think we're
incorrectly not enforcing this dependency in the kernel yet.  That chip
was built into many MacBooks.

Indeed the inability to specify such inter-function power dependencies
in config space seems to be a gaping hole in the PCI spec.  Maybe we
should always assume that function 0 needs to be in D0 for any other
function to work?  Not sure if that would be safe.

Thanks,

Lukas

Re: [PATCH 2/2] PCI: Create device link for NVIDIA GPU

[Bjorn Helgaas]

[+cc Rafael, just FYI]

On Mon, Jun 03, 2019 at 01:30:51PM +0530, Abhishek Sahu wrote:
> On 6/1/2019 2:09 AM, Bjorn Helgaas wrote:
> > On Fri, May 31, 2019 at 10:31:09AM +0530, Abhishek Sahu wrote:
> >> NVIDIA Turing GPUs include hardware support for USB Type-C and
> >> VirtualLink. It helps in delivering the power, display, and data
> >> required to power VR headsets through a single USB Type-C connector.
> >> The Turing GPU is a multi-function PCI device has the following
> >> four functions:
> >>
> >> 	- VGA display controller (Function 0)
> >> 	- Audio controller (Function 1)
> >> 	- USB xHCI Host controller (Function 2)
> >> 	- USB Type-C USCI controller (Function 3)
> >>
> >> The function 0 is tightly coupled with other functions in the
> >> hardware. When function 0 goes in runtime suspended state,
> >> then it will do power gating for most of the hardware blocks.
> >> Some of these hardware blocks are used by other functions which
> >> leads to functional failure. So if any of these functions (1/2/3)
> >> are active, then function 0 should also be in active state.
> > 
> >> 'commit 07f4f97d7b4b ("vga_switcheroo: Use device link for
> >> HDA controller")' creates the device link from function 1 to
> >> function 0. A similar kind of device link needs to be created
> >> between function 0 and functions 2 and 3 for NVIDIA Turing GPU.
> > 
> > I can't point to language that addresses this, but this sounds like a
> > case of the GPU not conforming to the PCI spec.  The general
> > assumption is that the OS should be able to discover everything it
> > needs to do power management directly from the architected PCI config
> > space.
> 
>  The GPU is following PCIe spec but following is the implementation
>  from HW side

Unless you can find spec language that talks about D-state
dependencies between functions, I claim this is not following the
PCIe spec.  For example, PCIe r5.0, sec 1.4, says "the PCI/PCIe
hardware/software model includes architectural constructs necessary to
discover, configure, and use a Function, without needing Function-
specific knowledge." Sec 5.1 says "D states are associated with a
particular Function" and "PM provides ... a mechanism to identify
power management capabilities of a given Function [and] the ability to
transition a Function into a certain power management state."

If there *is* something about dependencies between functions in the
spec, we should improve the generic PCI core to pay attention to that,
and then we wouldn't need this quirk.

If the spec doesn't provide a way to discover them, these dependencies
are exceptions from the spec, and we have to handle them as hardware
defects, using quirks like this.  That's fine, but let's not pretend
that this is a conforming device and that adding quirks is the
expected process.  Just call a spade a spade and say we're working
around a defect in this particular device.

I think the best path forward would be to add this quirk for the
existing device, and then pursue a spec change to add something like
a new PCIe capability to describe the dependencies.  Then we could
enhance the PCI core once and power management for future devices
would "Just Work" without having to add quirks.

Bjorn

Re: [PATCH 2/2] PCI: Create device link for NVIDIA GPU

[Abhishek Sahu]

On 6/3/2019 10:52 PM, Bjorn Helgaas wrote:
> [+cc Rafael, just FYI]
> 
> On Mon, Jun 03, 2019 at 01:30:51PM +0530, Abhishek Sahu wrote:
>> On 6/1/2019 2:09 AM, Bjorn Helgaas wrote:
>>> On Fri, May 31, 2019 at 10:31:09AM +0530, Abhishek Sahu wrote:
>>>> NVIDIA Turing GPUs include hardware support for USB Type-C and
>>>> VirtualLink. It helps in delivering the power, display, and data
>>>> required to power VR headsets through a single USB Type-C connector.
>>>> The Turing GPU is a multi-function PCI device has the following
>>>> four functions:
>>>>
>>>> 	- VGA display controller (Function 0)
>>>> 	- Audio controller (Function 1)
>>>> 	- USB xHCI Host controller (Function 2)
>>>> 	- USB Type-C USCI controller (Function 3)
>>>>
>>>> The function 0 is tightly coupled with other functions in the
>>>> hardware. When function 0 goes in runtime suspended state,
>>>> then it will do power gating for most of the hardware blocks.
>>>> Some of these hardware blocks are used by other functions which
>>>> leads to functional failure. So if any of these functions (1/2/3)
>>>> are active, then function 0 should also be in active state.
>>>
>>>> 'commit 07f4f97d7b4b ("vga_switcheroo: Use device link for
>>>> HDA controller")' creates the device link from function 1 to
>>>> function 0. A similar kind of device link needs to be created
>>>> between function 0 and functions 2 and 3 for NVIDIA Turing GPU.
>>>
>>> I can't point to language that addresses this, but this sounds like a
>>> case of the GPU not conforming to the PCI spec.  The general
>>> assumption is that the OS should be able to discover everything it
>>> needs to do power management directly from the architected PCI config
>>> space.
>>
>>  The GPU is following PCIe spec but following is the implementation
>>  from HW side
> 
> Unless you can find spec language that talks about D-state
> dependencies between functions, I claim this is not following the
> PCIe spec.  For example, PCIe r5.0, sec 1.4, says "the PCI/PCIe
> hardware/software model includes architectural constructs necessary to
> discover, configure, and use a Function, without needing Function-
> specific knowledge." Sec 5.1 says "D states are associated with a
> particular Function" and "PM provides ... a mechanism to identify
> power management capabilities of a given Function [and] the ability to
> transition a Function into a certain power management state."
> 

 Thanks Bjorn. Here in case of GPU's these functions are not
 completely independent so it is not following PCIe spec in
 that aspect.

> If there *is* something about dependencies between functions in the
> spec, we should improve the generic PCI core to pay attention to that,
> and then we wouldn't need this quirk.
> 
> If the spec doesn't provide a way to discover them, these dependencies
> are exceptions from the spec, and we have to handle them as hardware
> defects, using quirks like this.  That's fine, but let's not pretend
> that this is a conforming device and that adding quirks is the
> expected process.  Just call a spade a spade and say we're working
> around a defect in this particular device.
> 

 Yes. I am agree with that we need to be very careful in
 adding quirks like this. I will communicate the same to
 HW team so they can explore other options to handle this
 in HW design side for future chips.

> I think the best path forward would be to add this quirk for the
> existing device, and then pursue a spec change to add something like
> a new PCIe capability to describe the dependencies.  Then we could
> enhance the PCI core once and power management for future devices
> would "Just Work" without having to add quirks.
> 

 Yes. It will be long term process. If other HW has similar
 requirement then it would be good to have this.

 Regards,
 Abhishek