Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Cc: stable@vger.kernel.org Signed-off-by: Evan Quan evan.quan@amd.com --- drivers/pci/quirks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index dea10d62d5b9..f0c5dd3406a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5338,7 +5338,7 @@ 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 xHCI Host + * Create device link for GPUs with integrated USB xHCI Host * controller to VGA. */ static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ static void quirk_gpu_usb(struct pci_dev *usb) } DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID, + PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);
/* - * Create device link for NVIDIA GPU with integrated Type-C UCSI controller + * Create device link for GPUs 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. @@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi) DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_SERIAL_UNKNOWN, 8, quirk_gpu_usb_typec_ucsi); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID, + PCI_CLASS_SERIAL_UNKNOWN, 8, + quirk_gpu_usb_typec_ucsi);
/* * Enable the NVIDIA GPU integrated HDA controller if the BIOS left it
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Please comment on https://git.kernel.org/linus/6d2e369f0d4c .
Is there something the PCI core is missing here? Or is there something that needs to be added to ACPI or the PCI firmware spec?
We want a generic way to discover dependencies like this.
A quirk should not be necessary for spec-compliant devices. Quirks are an ongoing maintenance burden, and they mean that new hardware won't work correctly until the OS is patched to know about it. That's not what we want.
I expect we'll still need *this* quirk, but first I'd like to know whether there's a plan to handle this more generically in the future.
When you repost this, please follow the subject line style of 6d2e369f0d4c ("PCI: Add NVIDIA GPU multi-function power dependencies") so similar patches look similar.
Cc: stable@vger.kernel.org Signed-off-by: Evan Quan evan.quan@amd.com
drivers/pci/quirks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index dea10d62d5b9..f0c5dd3406a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5338,7 +5338,7 @@ 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 xHCI Host
*/
- Create device link for GPUs with integrated USB xHCI Host
- controller to VGA.
static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ static void quirk_gpu_usb(struct pci_dev *usb) } DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);/*
- Create device link for NVIDIA GPU with integrated Type-C UCSI controller
- Create device link for GPUs 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.
@@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi) DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_SERIAL_UNKNOWN, 8, quirk_gpu_usb_typec_ucsi); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8,quirk_gpu_usb_typec_ucsi);/*
- Enable the NVIDIA GPU integrated HDA controller if the BIOS left it
-- 2.29.0
[Public]
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Friday, September 3, 2021 3:55 PM To: Quan, Evan Evan.Quan@amd.com Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher, Alexander Alexander.Deucher@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Please comment on https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k ernel.org%2Flinus%2F6d2e369f0d4c&data=04%7C01%7CAlexander.Deu cher%40amd.com%7C9fa0d66e5f29424df36b08d96f14c710%7C3dd8961fe488 4e608e11a82d994e183d%7C0%7C0%7C637662957313172831%7CUnknown%7 CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi LCJXVCI6Mn0%3D%7C1000&sdata=6IlPLWlcO7iptbTqfh71fe5wmHN7RN 13OvScyYaWyI8%3D&reserved=0 .
Is there something the PCI core is missing here? Or is there something that needs to be added to ACPI or the PCI firmware spec?
We want a generic way to discover dependencies like this.
A quirk should not be necessary for spec-compliant devices. Quirks are an ongoing maintenance burden, and they mean that new hardware won't work correctly until the OS is patched to know about it. That's not what we want.
I expect we'll still need *this* quirk, but first I'd like to know whether there's a plan to handle this more generically in the future.
The requirement here is that all of the additional endpoints are dependencies for powering down the GPU. E.g., the audio controller and USB endpoints need to be in d3 before you put the GPU into d3, otherwise the non-GPU endpoints will be powered down as well behind their drivers' backs. On newer AMD hardware there is logic in the hardware to wait for all dependent devices to go into d3 before powering down everything or power up everything if anything enters d0, but this requires additional software setup in the GPU driver as well and older versions of the driver didn't set this up correctly, instead relying on software logic via dependencies. Earlier hardware didn't have that logic and needed software help. That said, I think all of the relevant drivers expect the hardware state to be powered down when d3 is entered and they may not handle a wake up properly if not all devices entered d3 and hence all of the devices never entered a powered down state.
Alex
When you repost this, please follow the subject line style of 6d2e369f0d4c ("PCI: Add NVIDIA GPU multi-function power dependencies") so similar patches look similar.
Cc: stable@vger.kernel.org Signed-off-by: Evan Quan evan.quan@amd.com
drivers/pci/quirks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index dea10d62d5b9..f0c5dd3406a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5338,7 +5338,7 @@
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 xHCI Host
*/
- Create device link for GPUs with integrated USB xHCI Host
- controller to VGA.
static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ static void quirk_gpu_usb(struct pci_dev *usb) } DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA,
PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);/*
- Create device link for NVIDIA GPU with integrated Type-C UCSI
controller
- Create device link for GPUs 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.
@@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi)
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8, quirk_gpu_usb_typec_ucsi);+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8,quirk_gpu_usb_typec_ucsi);/*
- Enable the NVIDIA GPU integrated HDA controller if the BIOS left
it
2.29.0
[+cc Rafael, beginning of thread: https://lore.kernel.org/all/20210903063311.3606226-1-evan.quan@amd.com/]
On Tue, Sep 07, 2021 at 04:09:40PM +0000, Deucher, Alexander wrote:
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Friday, September 3, 2021 3:55 PM To: Quan, Evan Evan.Quan@amd.com Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher, Alexander Alexander.Deucher@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Please comment on https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.k ernel.org%2Flinus%2F6d2e369f0d4c&data=04%7C01%7CAlexander.Deu cher%40amd.com%7C9fa0d66e5f29424df36b08d96f14c710%7C3dd8961fe488 4e608e11a82d994e183d%7C0%7C0%7C637662957313172831%7CUnknown%7 CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi LCJXVCI6Mn0%3D%7C1000&sdata=6IlPLWlcO7iptbTqfh71fe5wmHN7RN 13OvScyYaWyI8%3D&reserved=0 .
Is there something the PCI core is missing here? Or is there something that needs to be added to ACPI or the PCI firmware spec?
We want a generic way to discover dependencies like this.
A quirk should not be necessary for spec-compliant devices. Quirks are an ongoing maintenance burden, and they mean that new hardware won't work correctly until the OS is patched to know about it. That's not what we want.
I expect we'll still need *this* quirk, but first I'd like to know whether there's a plan to handle this more generically in the future.
The requirement here is that all of the additional endpoints are dependencies for powering down the GPU. E.g., the audio controller and USB endpoints need to be in d3 before you put the GPU into d3, otherwise the non-GPU endpoints will be powered down as well behind their drivers' backs. On newer AMD hardware there is logic in the hardware to wait for all dependent devices to go into d3 before powering down everything or power up everything if anything enters d0, but this requires additional software setup in the GPU driver as well and older versions of the driver didn't set this up correctly, instead relying on software logic via dependencies. Earlier hardware didn't have that logic and needed software help. That said, I think all of the relevant drivers expect the hardware state to be powered down when d3 is entered and they may not handle a wake up properly if not all devices entered d3 and hence all of the devices never entered a powered down state.
I'm not sure whether this answered my question. Will we need more quirks for future devices?
You said "On newer AMD hardware there is logic to wait for all dependent devices to go to d3 ...," which sounds promising, but then it "requires additional setup in the GPU driver."
So maybe PM works as per PCIe spec, but only after the driver sets things up? I'm not sure what, if any, PM we do before a driver claims the device.
The above suggests that if we put some (but not all) functions, in D3, the new logic will keep them from entering D3 until later. That doesn't really *sound* spec-compliant -- if we write D3 to a function's PCI_PM_CTRL and then read it back, the function will remain in D0 indefinitely, until we put that last function in D3?
pci_raw_set_power_state() does this read then write, and it expects PCI_PM_CTRL to change to the new state after the delay prescribed by the spec, which of course has nothing to do with sibling functions.
And if all the functions are in D3, and we write D0 to one function's PCI_PM_CTRL, *all* the functions magically go to D0? That sounds potentially confusing.
Bjorn
[Public]
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Tuesday, September 7, 2021 1:35 PM To: Deucher, Alexander Alexander.Deucher@amd.com Cc: Quan, Evan Evan.Quan@amd.com; linux-pci@vger.kernel.org; bhelgaas@google.com; stable@vger.kernel.org; Rafael J. Wysocki rjw@rjwysocki.net Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
[+cc Rafael, beginning of thread: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. kernel.org%2Fall%2F20210903063311.3606226-1- evan.quan%40amd.com%2F&data=04%7C01%7CAlexander.Deucher%4 0amd.com%7C48f3a6f7639343fcad6208d97225da95%7C3dd8961fe4884e608e 11a82d994e183d%7C0%7C0%7C637666329177869121%7CUnknown%7CTWFp bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC I6Mn0%3D%7C1000&sdata=k%2FgSHhtLiS8xS5hNkaP%2BOWWMWqiM 4tgQk4bybfumvfM%3D&reserved=0]
On Tue, Sep 07, 2021 at 04:09:40PM +0000, Deucher, Alexander wrote:
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Friday, September 3, 2021 3:55 PM To: Quan, Evan Evan.Quan@amd.com Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher, Alexander Alexander.Deucher@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Please comment on
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
t.k%2F&data=04%7C01%7CAlexander.Deucher%40amd.com%7C48f3a6f 76393
43fcad6208d97225da95%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0 %7C63
7666329177869121%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD AiLCJQIjo
iV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=0yxZbS6o P56
rXpA5j1wvlMpkp9Ern%2FcSRCPELtv47lM%3D&reserved=0
ernel.org%2Flinus%2F6d2e369f0d4c&data=04%7C01%7CAlexander.Deu
cher%40amd.com%7C9fa0d66e5f29424df36b08d96f14c710%7C3dd8961fe488
4e608e11a82d994e183d%7C0%7C0%7C637662957313172831%7CUnknown%7
CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
LCJXVCI6Mn0%3D%7C1000&sdata=6IlPLWlcO7iptbTqfh71fe5wmHN7RN
13OvScyYaWyI8%3D&reserved=0 .
Is there something the PCI core is missing here? Or is there something that needs to be added to ACPI or the PCI firmware spec?
We want a generic way to discover dependencies like this.
A quirk should not be necessary for spec-compliant devices. Quirks are an ongoing maintenance burden, and they mean that new hardware won't work correctly until the OS is patched to know about it. That's not what we want.
I expect we'll still need *this* quirk, but first I'd like to know whether there's a plan to handle this more generically in the future.
The requirement here is that all of the additional endpoints are dependencies for powering down the GPU. E.g., the audio controller and USB endpoints need to be in d3 before you put the GPU into d3, otherwise the non-GPU endpoints will be powered down as well behind their drivers' backs. On newer AMD hardware there is logic in the hardware to wait for all dependent devices to go into d3 before powering down everything or power up everything if anything enters d0, but this requires additional software setup in the GPU driver as well and older versions of the driver didn't set this up correctly, instead relying on software logic via dependencies. Earlier hardware didn't have that logic and needed software help. That said, I think all of the relevant drivers expect the hardware state to be powered down when d3 is entered and they may not handle a wake up properly if not all devices entered d3 and hence all of the devices never entered a powered down state.
I'm not sure whether this answered my question. Will we need more quirks for future devices?
The existing quirks should cover all devices unless we add some new endpoint to the GPUs in the future. The quirk for the audio dependency was added years ago and hasn't needed to be extended since. The USB stuff was added more recently and requires adding a similar quirk.
You said "On newer AMD hardware there is logic to wait for all dependent devices to go to d3 ...," which sounds promising, but then it "requires additional setup in the GPU driver."
So maybe PM works as per PCIe spec, but only after the driver sets things up? I'm not sure what, if any, PM we do before a driver claims the device.
I'm not exactly sure what the expectation is with regards to the spec. There is a single power rail that controls all of the endpoints so all have to be in d3 before the power can be cut.
The above suggests that if we put some (but not all) functions, in D3, the new logic will keep them from entering D3 until later. That doesn't really *sound* spec-compliant -- if we write D3 to a function's PCI_PM_CTRL and then read it back, the function will remain in D0 indefinitely, until we put that last function in D3?
It will read back as if the card is in D3, but the power rail stays on until all devices have been put into D3.
pci_raw_set_power_state() does this read then write, and it expects PCI_PM_CTRL to change to the new state after the delay prescribed by the spec, which of course has nothing to do with sibling functions.
And if all the functions are in D3, and we write D0 to one function's PCI_PM_CTRL, *all* the functions magically go to D0? That sounds potentially confusing.
The will all individually report the correct D state, it's just that they will be using more power than if they were all in D3.
Alex
Bjorn
On Tue, Sep 14, 2021 at 12:59:08PM +0000, Deucher, Alexander wrote:
[Public]
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Tuesday, September 7, 2021 1:35 PM To: Deucher, Alexander Alexander.Deucher@amd.com Cc: Quan, Evan Evan.Quan@amd.com; linux-pci@vger.kernel.org; bhelgaas@google.com; stable@vger.kernel.org; Rafael J. Wysocki rjw@rjwysocki.net Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
[+cc Rafael, beginning of thread: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore. kernel.org%2Fall%2F20210903063311.3606226-1- evan.quan%40amd.com%2F&data=04%7C01%7CAlexander.Deucher%4 0amd.com%7C48f3a6f7639343fcad6208d97225da95%7C3dd8961fe4884e608e 11a82d994e183d%7C0%7C0%7C637666329177869121%7CUnknown%7CTWFp bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVC I6Mn0%3D%7C1000&sdata=k%2FgSHhtLiS8xS5hNkaP%2BOWWMWqiM 4tgQk4bybfumvfM%3D&reserved=0]
On Tue, Sep 07, 2021 at 04:09:40PM +0000, Deucher, Alexander wrote:
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Friday, September 3, 2021 3:55 PM To: Quan, Evan Evan.Quan@amd.com Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher, Alexander Alexander.Deucher@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Please comment on
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
t.k%2F&data=04%7C01%7CAlexander.Deucher%40amd.com%7C48f3a6f 76393
43fcad6208d97225da95%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0 %7C63
7666329177869121%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD AiLCJQIjo
iV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=0yxZbS6o P56
rXpA5j1wvlMpkp9Ern%2FcSRCPELtv47lM%3D&reserved=0
ernel.org%2Flinus%2F6d2e369f0d4c&data=04%7C01%7CAlexander.Deu
cher%40amd.com%7C9fa0d66e5f29424df36b08d96f14c710%7C3dd8961fe488
4e608e11a82d994e183d%7C0%7C0%7C637662957313172831%7CUnknown%7
CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwi
LCJXVCI6Mn0%3D%7C1000&sdata=6IlPLWlcO7iptbTqfh71fe5wmHN7RN
13OvScyYaWyI8%3D&reserved=0 .
Is there something the PCI core is missing here? Or is there something that needs to be added to ACPI or the PCI firmware spec?
We want a generic way to discover dependencies like this.
A quirk should not be necessary for spec-compliant devices. Quirks are an ongoing maintenance burden, and they mean that new hardware won't work correctly until the OS is patched to know about it. That's not what we want.
I expect we'll still need *this* quirk, but first I'd like to know whether there's a plan to handle this more generically in the future.
The requirement here is that all of the additional endpoints are dependencies for powering down the GPU. E.g., the audio controller and USB endpoints need to be in d3 before you put the GPU into d3, otherwise the non-GPU endpoints will be powered down as well behind their drivers' backs. On newer AMD hardware there is logic in the hardware to wait for all dependent devices to go into d3 before powering down everything or power up everything if anything enters d0, but this requires additional software setup in the GPU driver as well and older versions of the driver didn't set this up correctly, instead relying on software logic via dependencies. Earlier hardware didn't have that logic and needed software help. That said, I think all of the relevant drivers expect the hardware state to be powered down when d3 is entered and they may not handle a wake up properly if not all devices entered d3 and hence all of the devices never entered a powered down state.
I'm not sure whether this answered my question. Will we need more quirks for future devices?
The existing quirks should cover all devices unless we add some new endpoint to the GPUs in the future. The quirk for the audio dependency was added years ago and hasn't needed to be extended since. The USB stuff was added more recently and requires adding a similar quirk.
Oh, I see, these quirks don't actually have specific Device IDs in them; they are generic for *all* PCI_VENDOR_ID_ATI USB devices (and pci_create_device_link() checks for a PCI_BASE_CLASS_DISPLAY function in the same device. So it's only when you add a new *class* of device dependency. Thanks for clearing that up!
You said "On newer AMD hardware there is logic to wait for all dependent devices to go to d3 ...," which sounds promising, but then it "requires additional setup in the GPU driver."
So maybe PM works as per PCIe spec, but only after the driver sets things up? I'm not sure what, if any, PM we do before a driver claims the device.
I'm not exactly sure what the expectation is with regards to the spec. There is a single power rail that controls all of the endpoints so all have to be in d3 before the power can be cut.
The above suggests that if we put some (but not all) functions, in D3, the new logic will keep them from entering D3 until later. That doesn't really *sound* spec-compliant -- if we write D3 to a function's PCI_PM_CTRL and then read it back, the function will remain in D0 indefinitely, until we put that last function in D3?
It will read back as if the card is in D3, but the power rail stays on until all devices have been put into D3.
pci_raw_set_power_state() does this read then write, and it expects PCI_PM_CTRL to change to the new state after the delay prescribed by the spec, which of course has nothing to do with sibling functions.
And if all the functions are in D3, and we write D0 to one function's PCI_PM_CTRL, *all* the functions magically go to D0? That sounds potentially confusing.
The will all individually report the correct D state, it's just that they will be using more power than if they were all in D3.
OK, that all sounds fine, I think. Thanks!
Bjorn
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Cc: stable@vger.kernel.org Signed-off-by: Evan Quan evan.quan@amd.com
Applied to pci/pm for v5.16, thanks!
drivers/pci/quirks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index dea10d62d5b9..f0c5dd3406a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5338,7 +5338,7 @@ 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 xHCI Host
*/
- Create device link for GPUs with integrated USB xHCI Host
- controller to VGA.
static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ static void quirk_gpu_usb(struct pci_dev *usb) } DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);/*
- Create device link for NVIDIA GPU with integrated Type-C UCSI controller
- Create device link for GPUs 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.
@@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi) DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_SERIAL_UNKNOWN, 8, quirk_gpu_usb_typec_ucsi); +DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8,quirk_gpu_usb_typec_ucsi);/*
- Enable the NVIDIA GPU integrated HDA controller if the BIOS left it
-- 2.29.0
[Public]
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Tuesday, September 14, 2021 11:29 AM To: Quan, Evan Evan.Quan@amd.com Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher, Alexander Alexander.Deucher@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Cc: stable@vger.kernel.org Signed-off-by: Evan Quan evan.quan@amd.com
Applied to pci/pm for v5.16, thanks!
Any chance we can get this applied for 5.15/stable? This fixes runtime pm problems on GPU boards with integrated USB. https://gitlab.freedesktop.org/drm/amd/-/issues/1704
Thanks,
Alex
drivers/pci/quirks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index dea10d62d5b9..f0c5dd3406a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5338,7 +5338,7 @@
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 xHCI Host
*/
- Create device link for GPUs with integrated USB xHCI Host
- controller to VGA.
static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ static void quirk_gpu_usb(struct pci_dev *usb) } DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA,
PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);/*
- Create device link for NVIDIA GPU with integrated Type-C UCSI
controller
- Create device link for GPUs 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.
@@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi)
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8, quirk_gpu_usb_typec_ucsi);+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8,quirk_gpu_usb_typec_ucsi);/*
- Enable the NVIDIA GPU integrated HDA controller if the BIOS left
it
2.29.0
On Wed, Sep 15, 2021 at 04:39:36PM +0000, Deucher, Alexander wrote:
[Public]
-----Original Message----- From: Bjorn Helgaas helgaas@kernel.org Sent: Tuesday, September 14, 2021 11:29 AM To: Quan, Evan Evan.Quan@amd.com Cc: linux-pci@vger.kernel.org; bhelgaas@google.com; Deucher, Alexander Alexander.Deucher@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] PCI: Create device links for AMD integrated USB xHCI and UCSI controllers
On Fri, Sep 03, 2021 at 02:33:11PM +0800, Evan Quan wrote:
Latest AMD GPUs have built-in USB xHCI and UCSI controllers. Add device link support for them.
Cc: stable@vger.kernel.org Signed-off-by: Evan Quan evan.quan@amd.com
Applied to pci/pm for v5.16, thanks!
Any chance we can get this applied for 5.15/stable? This fixes runtime pm problems on GPU boards with integrated USB. https://gitlab.freedesktop.org/drm/amd/-/issues/1704
Thanks for the link. It's always nice to have a clue about what problems a quirk fixes.
I added the link to the commit log and moved the whole thing to for-linus for v5.15.
Thanks,
Alex
drivers/pci/quirks.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index dea10d62d5b9..f0c5dd3406a1 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -5338,7 +5338,7 @@
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 xHCI Host
*/
- Create device link for GPUs with integrated USB xHCI Host
- controller to VGA.
static void quirk_gpu_usb(struct pci_dev *usb) @@ -5347,9 +5347,11 @@ static void quirk_gpu_usb(struct pci_dev *usb) } DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA,
PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_USB, 8, quirk_gpu_usb);/*
- Create device link for NVIDIA GPU with integrated Type-C UCSI
controller
- Create device link for GPUs 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.
@@ -5362,6 +5364,9 @@ static void quirk_gpu_usb_typec_ucsi(struct pci_dev *ucsi)
DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8, quirk_gpu_usb_typec_ucsi);+DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_ATI, PCI_ANY_ID,
PCI_CLASS_SERIAL_UNKNOWN, 8,quirk_gpu_usb_typec_ucsi);/*
- Enable the NVIDIA GPU integrated HDA controller if the BIOS left
it
2.29.0
linux-stable-mirror@lists.linaro.org
-
Bjorn Helgaas -
Deucher, Alexander -
Evan Quan