* [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
@ 2021-07-17 18:02 Ratchanan Srirattanamet
2021-07-22 16:36 ` Karol Herbst
2021-07-27 12:11 ` Karol Herbst
0 siblings, 2 replies; 6+ messages in thread
From: Ratchanan Srirattanamet @ 2021-07-17 18:02 UTC (permalink / raw)
To: bskeggs; +Cc: nouveau, Ratchanan Srirattanamet
The call site of nouveau_dsm_pci_probe() uses single set of output
variables for all invocations. So, we must not write anything to them
until we think this is an NVIDIA device of interest. Otherwise, if we
are called with another device after the NVIDIA device, we'll clober the
result of the NVIDIA device.
In this case, if the other device doesn't have _PR3 resources, the
detection later would miss the presence of power resource support, and
the rest of the code will keep using Optimus DSM, breaking power
management for that machine. In particular, this fixes power management
of the NVIDIA card in Lenovo Legion 5-15ARH05... well, at least
partially. New error shows up, but this patch is correct in itself
anyway.
As a bonus, we'll also stop preventing _PR3 usage from the bridge for
unrelated devices, which is always nice, I guess.
As noted in commit ccfc2d5cdb024 ("drm/nouveau: Use generic helper to
check _PR3 presence"), care is taken to leave the _PR3 detection outside
of the optimus_func condition.
https://gitlab.freedesktop.org/drm/nouveau/-/issues/79
Fixes: ccfc2d5cdb024 ("drm/nouveau: Use generic helper to check _PR3
presence")
Signed-off-by: Ratchanan Srirattanamet <peathot@hotmail.com>
---
Hello,
This is my first time submitting a Linux patch. I've done a
number of PR/MR workflows on GitHub or GitLab, but never done any
email-oriented development. So, please excuse me if I'm doing something
incorrectly.
drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
index 7c15f6448428..c88bda3ac820 100644
--- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
+++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
@@ -220,15 +220,6 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
int optimus_funcs;
struct pci_dev *parent_pdev;
- *has_pr3 = false;
- parent_pdev = pci_upstream_bridge(pdev);
- if (parent_pdev) {
- if (parent_pdev->bridge_d3)
- *has_pr3 = pci_pr3_present(parent_pdev);
- else
- pci_d3cold_disable(pdev);
- }
-
dhandle = ACPI_HANDLE(&pdev->dev);
if (!dhandle)
return;
@@ -249,6 +240,15 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
*has_opt = !!optimus_funcs;
*has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
+ *has_pr3 = false;
+ parent_pdev = pci_upstream_bridge(pdev);
+ if (parent_pdev) {
+ if (parent_pdev->bridge_d3)
+ *has_pr3 = pci_pr3_present(parent_pdev);
+ else
+ pci_d3cold_disable(pdev);
+ }
+
if (optimus_funcs) {
uint32_t result;
nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
--
2.25.1
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
2021-07-17 18:02 [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device Ratchanan Srirattanamet
@ 2021-07-22 16:36 ` Karol Herbst
2021-07-22 19:49 ` Ratchanan Srirattanamet
2021-07-27 12:11 ` Karol Herbst
1 sibling, 1 reply; 6+ messages in thread
From: Karol Herbst @ 2021-07-22 16:36 UTC (permalink / raw)
To: Ratchanan Srirattanamet; +Cc: nouveau, Ben Skeggs
hey, thanks for the patch. But I am a bit confused on why that patch
actually helps. It should only be called for nvidia GPUs, but are we
ending up checking it for AMD GPUs as well?
Mind posting the output of lspci -tvnn?
On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet
<peathot@hotmail.com> wrote:
>
> The call site of nouveau_dsm_pci_probe() uses single set of output
> variables for all invocations. So, we must not write anything to them
> until we think this is an NVIDIA device of interest. Otherwise, if we
> are called with another device after the NVIDIA device, we'll clober the
> result of the NVIDIA device.
>
> In this case, if the other device doesn't have _PR3 resources, the
> detection later would miss the presence of power resource support, and
> the rest of the code will keep using Optimus DSM, breaking power
> management for that machine. In particular, this fixes power management
> of the NVIDIA card in Lenovo Legion 5-15ARH05... well, at least
> partially. New error shows up, but this patch is correct in itself
> anyway.
>
> As a bonus, we'll also stop preventing _PR3 usage from the bridge for
> unrelated devices, which is always nice, I guess.
>
> As noted in commit ccfc2d5cdb024 ("drm/nouveau: Use generic helper to
> check _PR3 presence"), care is taken to leave the _PR3 detection outside
> of the optimus_func condition.
>
> https://gitlab.freedesktop.org/drm/nouveau/-/issues/79
>
> Fixes: ccfc2d5cdb024 ("drm/nouveau: Use generic helper to check _PR3
> presence")
> Signed-off-by: Ratchanan Srirattanamet <peathot@hotmail.com>
> ---
>
> Hello,
>
> This is my first time submitting a Linux patch. I've done a
> number of PR/MR workflows on GitHub or GitLab, but never done any
> email-oriented development. So, please excuse me if I'm doing something
> incorrectly.
>
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> index 7c15f6448428..c88bda3ac820 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> @@ -220,15 +220,6 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> int optimus_funcs;
> struct pci_dev *parent_pdev;
>
> - *has_pr3 = false;
> - parent_pdev = pci_upstream_bridge(pdev);
> - if (parent_pdev) {
> - if (parent_pdev->bridge_d3)
> - *has_pr3 = pci_pr3_present(parent_pdev);
> - else
> - pci_d3cold_disable(pdev);
> - }
> -
> dhandle = ACPI_HANDLE(&pdev->dev);
> if (!dhandle)
> return;
> @@ -249,6 +240,15 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> *has_opt = !!optimus_funcs;
> *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
>
> + *has_pr3 = false;
> + parent_pdev = pci_upstream_bridge(pdev);
> + if (parent_pdev) {
> + if (parent_pdev->bridge_d3)
> + *has_pr3 = pci_pr3_present(parent_pdev);
> + else
> + pci_d3cold_disable(pdev);
> + }
> +
> if (optimus_funcs) {
> uint32_t result;
> nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
> --
> 2.25.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
2021-07-22 16:36 ` Karol Herbst
@ 2021-07-22 19:49 ` Ratchanan Srirattanamet
2021-07-22 19:54 ` Karol Herbst
0 siblings, 1 reply; 6+ messages in thread
From: Ratchanan Srirattanamet @ 2021-07-22 19:49 UTC (permalink / raw)
To: Karol Herbst; +Cc: nouveau, Ben Skeggs
Hello,
เมื่อ 22/7/64 เวลา 23:36 Karol Herbst เขียนว่า:
> hey, thanks for the patch. But I am a bit confused on why that patch
> actually helps. It should only be called for nvidia GPUs, but are we
> ending up checking it for AMD GPUs as well?
This is the call site of nouveau_dsm_pci_probe() in nouveau_acpi.c:
/* now do DSM detection */
while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) {
vga_count++;
nouveau_dsm_pci_probe(pdev, &dhandle, &has_mux, &has_optimus,
&has_optimus_flags, &has_power_resources);
}
// And repeat with PCI_CLASS_DISPLAY_3D
So, all VGA and 3D cards, Nvidia or not, has this function called. Are
you suggesting that this should not happen?
> Mind posting the output of lspci -tvnn?
In case it helps, here it is:
$ lspci -tvnn
-[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Renoir Root
Complex [1022:1630]
+-00.2 Advanced Micro Devices, Inc. [AMD] Renoir IOMMU
[1022:1631]
+-01.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
Host Bridge [1022:1632]
+-01.1-[01]--+-00.0 NVIDIA Corporation Device [10de:1f95]
| \-00.1 NVIDIA Corporation Device [10de:10fa]
+-01.2-[02]----00.0 Micron Technology Inc Device [1344:5405]
+-02.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
Host Bridge [1022:1632]
+-02.1-[03]----00.0 Realtek Semiconductor Co., Ltd.
RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168]
+-02.2-[04]----00.0 Intel Corporation Wi-Fi 6 AX200 [8086:2723]
+-08.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
Host Bridge [1022:1632]
+-08.1-[05]--+-00.0 Advanced Micro Devices, Inc. [AMD/ATI]
Renoir [1002:1636]
| +-00.2 Advanced Micro Devices, Inc. [AMD]
Family 17h (Models 10h-1fh) Platform Security Processor [1022:15df]
| +-00.3 Advanced Micro Devices, Inc. [AMD]
Renoir USB 3.1 [1022:1639]
| +-00.4 Advanced Micro Devices, Inc. [AMD]
Renoir USB 3.1 [1022:1639]
| +-00.5 Advanced Micro Devices, Inc. [AMD]
Raven/Raven2/FireFlight/Renoir Audio Processor [1022:15e2]
| \-00.6 Advanced Micro Devices, Inc. [AMD]
Family 17h (Models 10h-1fh) HD Audio Controller [1022:15e3]
+-08.2-[06]--+-00.0 Advanced Micro Devices, Inc. [AMD] FCH
SATA Controller [AHCI mode] [1022:7901]
| \-00.1 Advanced Micro Devices, Inc. [AMD] FCH
SATA Controller [AHCI mode] [1022:7901]
+-14.0 Advanced Micro Devices, Inc. [AMD] FCH SMBus
Controller [1022:790b]
+-14.3 Advanced Micro Devices, Inc. [AMD] FCH LPC Bridge
[1022:790e]
+-18.0 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 0 [1022:1448]
+-18.1 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 1 [1022:1449]
+-18.2 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 2 [1022:144a]
+-18.3 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 3 [1022:144b]
+-18.4 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 4 [1022:144c]
+-18.5 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 5 [1022:144d]
+-18.6 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 6 [1022:144e]
\-18.7 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
Function 7 [1022:144f]
Ratchanan Srirattanamet
> On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet
> <peathot@hotmail.com> wrote:
>>
>> The call site of nouveau_dsm_pci_probe() uses single set of output
>> variables for all invocations. So, we must not write anything to them
>> until we think this is an NVIDIA device of interest. Otherwise, if we
>> are called with another device after the NVIDIA device, we'll clober the
>> result of the NVIDIA device.
>>
>> In this case, if the other device doesn't have _PR3 resources, the
>> detection later would miss the presence of power resource support, and
>> the rest of the code will keep using Optimus DSM, breaking power
>> management for that machine. In particular, this fixes power management
>> of the NVIDIA card in Lenovo Legion 5-15ARH05... well, at least
>> partially. New error shows up, but this patch is correct in itself
>> anyway.
>>
>> As a bonus, we'll also stop preventing _PR3 usage from the bridge for
>> unrelated devices, which is always nice, I guess.
>>
>> As noted in commit ccfc2d5cdb024 ("drm/nouveau: Use generic helper to
>> check _PR3 presence"), care is taken to leave the _PR3 detection outside
>> of the optimus_func condition.
>>
>> https://gitlab.freedesktop.org/drm/nouveau/-/issues/79
>>
>> Fixes: ccfc2d5cdb024 ("drm/nouveau: Use generic helper to check _PR3
>> presence")
>> Signed-off-by: Ratchanan Srirattanamet <peathot@hotmail.com>
>> ---
>>
>> Hello,
>>
>> This is my first time submitting a Linux patch. I've done a
>> number of PR/MR workflows on GitHub or GitLab, but never done any
>> email-oriented development. So, please excuse me if I'm doing something
>> incorrectly.
>>
>> drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +++++++++---------
>> 1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
>> index 7c15f6448428..c88bda3ac820 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
>> @@ -220,15 +220,6 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
>> int optimus_funcs;
>> struct pci_dev *parent_pdev;
>>
>> - *has_pr3 = false;
>> - parent_pdev = pci_upstream_bridge(pdev);
>> - if (parent_pdev) {
>> - if (parent_pdev->bridge_d3)
>> - *has_pr3 = pci_pr3_present(parent_pdev);
>> - else
>> - pci_d3cold_disable(pdev);
>> - }
>> -
>> dhandle = ACPI_HANDLE(&pdev->dev);
>> if (!dhandle)
>> return;
>> @@ -249,6 +240,15 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
>> *has_opt = !!optimus_funcs;
>> *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
>>
>> + *has_pr3 = false;
>> + parent_pdev = pci_upstream_bridge(pdev);
>> + if (parent_pdev) {
>> + if (parent_pdev->bridge_d3)
>> + *has_pr3 = pci_pr3_present(parent_pdev);
>> + else
>> + pci_d3cold_disable(pdev);
>> + }
>> +
>> if (optimus_funcs) {
>> uint32_t result;
>> nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
>> --
>> 2.25.1
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
>>
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
2021-07-22 19:49 ` Ratchanan Srirattanamet
@ 2021-07-22 19:54 ` Karol Herbst
2021-07-22 19:57 ` Karol Herbst
0 siblings, 1 reply; 6+ messages in thread
From: Karol Herbst @ 2021-07-22 19:54 UTC (permalink / raw)
To: Ratchanan Srirattanamet; +Cc: nouveau, Ben Skeggs
On Thu, Jul 22, 2021 at 9:49 PM Ratchanan Srirattanamet
<peathot@hotmail.com> wrote:
>
> Hello,
>
> เมื่อ 22/7/64 เวลา 23:36 Karol Herbst เขียนว่า:
> > hey, thanks for the patch. But I am a bit confused on why that patch
> > actually helps. It should only be called for nvidia GPUs, but are we
> > ending up checking it for AMD GPUs as well?
>
> This is the call site of nouveau_dsm_pci_probe() in nouveau_acpi.c:
>
> /* now do DSM detection */
> while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) {
> vga_count++;
>
> nouveau_dsm_pci_probe(pdev, &dhandle, &has_mux, &has_optimus,
> &has_optimus_flags, &has_power_resources);
> }
> // And repeat with PCI_CLASS_DISPLAY_3D
>
> So, all VGA and 3D cards, Nvidia or not, has this function called. Are
> you suggesting that this should not happen?
>
yeah.. it shouldn't. Although I am not quite sure how pci_get_class
works... I was under the impression it only touches children but I
guess it just iterates through the list of all devices? uhhhh mhhh...
Yeah then I'd see why we never hit this on intel systems as the intel
GPU is usually further up the list.
> > Mind posting the output of lspci -tvnn?
>
> In case it helps, here it is:
>
> $ lspci -tvnn
> -[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Renoir Root
> Complex [1022:1630]
> +-00.2 Advanced Micro Devices, Inc. [AMD] Renoir IOMMU
> [1022:1631]
> +-01.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
> Host Bridge [1022:1632]
> +-01.1-[01]--+-00.0 NVIDIA Corporation Device [10de:1f95]
> | \-00.1 NVIDIA Corporation Device [10de:10fa]
> +-01.2-[02]----00.0 Micron Technology Inc Device [1344:5405]
> +-02.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
> Host Bridge [1022:1632]
> +-02.1-[03]----00.0 Realtek Semiconductor Co., Ltd.
> RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168]
> +-02.2-[04]----00.0 Intel Corporation Wi-Fi 6 AX200 [8086:2723]
> +-08.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
> Host Bridge [1022:1632]
> +-08.1-[05]--+-00.0 Advanced Micro Devices, Inc. [AMD/ATI]
> Renoir [1002:1636]
> | +-00.2 Advanced Micro Devices, Inc. [AMD]
> Family 17h (Models 10h-1fh) Platform Security Processor [1022:15df]
> | +-00.3 Advanced Micro Devices, Inc. [AMD]
> Renoir USB 3.1 [1022:1639]
> | +-00.4 Advanced Micro Devices, Inc. [AMD]
> Renoir USB 3.1 [1022:1639]
> | +-00.5 Advanced Micro Devices, Inc. [AMD]
> Raven/Raven2/FireFlight/Renoir Audio Processor [1022:15e2]
> | \-00.6 Advanced Micro Devices, Inc. [AMD]
> Family 17h (Models 10h-1fh) HD Audio Controller [1022:15e3]
> +-08.2-[06]--+-00.0 Advanced Micro Devices, Inc. [AMD] FCH
> SATA Controller [AHCI mode] [1022:7901]
> | \-00.1 Advanced Micro Devices, Inc. [AMD] FCH
> SATA Controller [AHCI mode] [1022:7901]
> +-14.0 Advanced Micro Devices, Inc. [AMD] FCH SMBus
> Controller [1022:790b]
> +-14.3 Advanced Micro Devices, Inc. [AMD] FCH LPC Bridge
> [1022:790e]
> +-18.0 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 0 [1022:1448]
> +-18.1 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 1 [1022:1449]
> +-18.2 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 2 [1022:144a]
> +-18.3 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 3 [1022:144b]
> +-18.4 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 4 [1022:144c]
> +-18.5 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 5 [1022:144d]
> +-18.6 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 6 [1022:144e]
> \-18.7 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> Function 7 [1022:144f]
>
> Ratchanan Srirattanamet
>
> > On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet
> > <peathot@hotmail.com> wrote:
> >>
> >> The call site of nouveau_dsm_pci_probe() uses single set of output
> >> variables for all invocations. So, we must not write anything to them
> >> until we think this is an NVIDIA device of interest. Otherwise, if we
> >> are called with another device after the NVIDIA device, we'll clober the
> >> result of the NVIDIA device.
> >>
> >> In this case, if the other device doesn't have _PR3 resources, the
> >> detection later would miss the presence of power resource support, and
> >> the rest of the code will keep using Optimus DSM, breaking power
> >> management for that machine. In particular, this fixes power management
> >> of the NVIDIA card in Lenovo Legion 5-15ARH05... well, at least
> >> partially. New error shows up, but this patch is correct in itself
> >> anyway.
> >>
> >> As a bonus, we'll also stop preventing _PR3 usage from the bridge for
> >> unrelated devices, which is always nice, I guess.
> >>
> >> As noted in commit ccfc2d5cdb024 ("drm/nouveau: Use generic helper to
> >> check _PR3 presence"), care is taken to leave the _PR3 detection outside
> >> of the optimus_func condition.
> >>
> >> https://gitlab.freedesktop.org/drm/nouveau/-/issues/79
> >>
> >> Fixes: ccfc2d5cdb024 ("drm/nouveau: Use generic helper to check _PR3
> >> presence")
> >> Signed-off-by: Ratchanan Srirattanamet <peathot@hotmail.com>
> >> ---
> >>
> >> Hello,
> >>
> >> This is my first time submitting a Linux patch. I've done a
> >> number of PR/MR workflows on GitHub or GitLab, but never done any
> >> email-oriented development. So, please excuse me if I'm doing something
> >> incorrectly.
> >>
> >> drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +++++++++---------
> >> 1 file changed, 9 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> index 7c15f6448428..c88bda3ac820 100644
> >> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> >> @@ -220,15 +220,6 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> >> int optimus_funcs;
> >> struct pci_dev *parent_pdev;
> >>
> >> - *has_pr3 = false;
> >> - parent_pdev = pci_upstream_bridge(pdev);
> >> - if (parent_pdev) {
> >> - if (parent_pdev->bridge_d3)
> >> - *has_pr3 = pci_pr3_present(parent_pdev);
> >> - else
> >> - pci_d3cold_disable(pdev);
> >> - }
> >> -
> >> dhandle = ACPI_HANDLE(&pdev->dev);
> >> if (!dhandle)
> >> return;
> >> @@ -249,6 +240,15 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> >> *has_opt = !!optimus_funcs;
> >> *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
> >>
> >> + *has_pr3 = false;
> >> + parent_pdev = pci_upstream_bridge(pdev);
> >> + if (parent_pdev) {
> >> + if (parent_pdev->bridge_d3)
> >> + *has_pr3 = pci_pr3_present(parent_pdev);
> >> + else
> >> + pci_d3cold_disable(pdev);
> >> + }
> >> +
> >> if (optimus_funcs) {
> >> uint32_t result;
> >> nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
> >> --
> >> 2.25.1
> >>
> >> _______________________________________________
> >> Nouveau mailing list
> >> Nouveau@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/nouveau
> >>
> >
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
2021-07-22 19:54 ` Karol Herbst
@ 2021-07-22 19:57 ` Karol Herbst
0 siblings, 0 replies; 6+ messages in thread
From: Karol Herbst @ 2021-07-22 19:57 UTC (permalink / raw)
To: Ratchanan Srirattanamet; +Cc: nouveau, Ben Skeggs
On Thu, Jul 22, 2021 at 9:54 PM Karol Herbst <kherbst@redhat.com> wrote:
>
> On Thu, Jul 22, 2021 at 9:49 PM Ratchanan Srirattanamet
> <peathot@hotmail.com> wrote:
> >
> > Hello,
> >
> > เมื่อ 22/7/64 เวลา 23:36 Karol Herbst เขียนว่า:
> > > hey, thanks for the patch. But I am a bit confused on why that patch
> > > actually helps. It should only be called for nvidia GPUs, but are we
> > > ending up checking it for AMD GPUs as well?
> >
> > This is the call site of nouveau_dsm_pci_probe() in nouveau_acpi.c:
> >
> > /* now do DSM detection */
> > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, pdev)) != NULL) {
> > vga_count++;
> >
> > nouveau_dsm_pci_probe(pdev, &dhandle, &has_mux, &has_optimus,
> > &has_optimus_flags, &has_power_resources);
> > }
> > // And repeat with PCI_CLASS_DISPLAY_3D
> >
> > So, all VGA and 3D cards, Nvidia or not, has this function called. Are
> > you suggesting that this should not happen?
> >
>
> yeah.. it shouldn't. Although I am not quite sure how pci_get_class
> works... I was under the impression it only touches children but I
> guess it just iterates through the list of all devices? uhhhh mhhh...
> Yeah then I'd see why we never hit this on intel systems as the intel
> GPU is usually further up the list.
>
ohh, actually I missunderstood the entire code. Yeah.. seems you are
right and we are just stupid here.. okay.
> > > Mind posting the output of lspci -tvnn?
> >
> > In case it helps, here it is:
> >
> > $ lspci -tvnn
> > -[0000:00]-+-00.0 Advanced Micro Devices, Inc. [AMD] Renoir Root
> > Complex [1022:1630]
> > +-00.2 Advanced Micro Devices, Inc. [AMD] Renoir IOMMU
> > [1022:1631]
> > +-01.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
> > Host Bridge [1022:1632]
> > +-01.1-[01]--+-00.0 NVIDIA Corporation Device [10de:1f95]
> > | \-00.1 NVIDIA Corporation Device [10de:10fa]
> > +-01.2-[02]----00.0 Micron Technology Inc Device [1344:5405]
> > +-02.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
> > Host Bridge [1022:1632]
> > +-02.1-[03]----00.0 Realtek Semiconductor Co., Ltd.
> > RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller [10ec:8168]
> > +-02.2-[04]----00.0 Intel Corporation Wi-Fi 6 AX200 [8086:2723]
> > +-08.0 Advanced Micro Devices, Inc. [AMD] Renoir PCIe Dummy
> > Host Bridge [1022:1632]
> > +-08.1-[05]--+-00.0 Advanced Micro Devices, Inc. [AMD/ATI]
> > Renoir [1002:1636]
> > | +-00.2 Advanced Micro Devices, Inc. [AMD]
> > Family 17h (Models 10h-1fh) Platform Security Processor [1022:15df]
> > | +-00.3 Advanced Micro Devices, Inc. [AMD]
> > Renoir USB 3.1 [1022:1639]
> > | +-00.4 Advanced Micro Devices, Inc. [AMD]
> > Renoir USB 3.1 [1022:1639]
> > | +-00.5 Advanced Micro Devices, Inc. [AMD]
> > Raven/Raven2/FireFlight/Renoir Audio Processor [1022:15e2]
> > | \-00.6 Advanced Micro Devices, Inc. [AMD]
> > Family 17h (Models 10h-1fh) HD Audio Controller [1022:15e3]
> > +-08.2-[06]--+-00.0 Advanced Micro Devices, Inc. [AMD] FCH
> > SATA Controller [AHCI mode] [1022:7901]
> > | \-00.1 Advanced Micro Devices, Inc. [AMD] FCH
> > SATA Controller [AHCI mode] [1022:7901]
> > +-14.0 Advanced Micro Devices, Inc. [AMD] FCH SMBus
> > Controller [1022:790b]
> > +-14.3 Advanced Micro Devices, Inc. [AMD] FCH LPC Bridge
> > [1022:790e]
> > +-18.0 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 0 [1022:1448]
> > +-18.1 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 1 [1022:1449]
> > +-18.2 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 2 [1022:144a]
> > +-18.3 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 3 [1022:144b]
> > +-18.4 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 4 [1022:144c]
> > +-18.5 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 5 [1022:144d]
> > +-18.6 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 6 [1022:144e]
> > \-18.7 Advanced Micro Devices, Inc. [AMD] Renoir Device 24:
> > Function 7 [1022:144f]
> >
> > Ratchanan Srirattanamet
> >
> > > On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet
> > > <peathot@hotmail.com> wrote:
> > >>
> > >> The call site of nouveau_dsm_pci_probe() uses single set of output
> > >> variables for all invocations. So, we must not write anything to them
> > >> until we think this is an NVIDIA device of interest. Otherwise, if we
> > >> are called with another device after the NVIDIA device, we'll clober the
> > >> result of the NVIDIA device.
> > >>
> > >> In this case, if the other device doesn't have _PR3 resources, the
> > >> detection later would miss the presence of power resource support, and
> > >> the rest of the code will keep using Optimus DSM, breaking power
> > >> management for that machine. In particular, this fixes power management
> > >> of the NVIDIA card in Lenovo Legion 5-15ARH05... well, at least
> > >> partially. New error shows up, but this patch is correct in itself
> > >> anyway.
> > >>
> > >> As a bonus, we'll also stop preventing _PR3 usage from the bridge for
> > >> unrelated devices, which is always nice, I guess.
> > >>
> > >> As noted in commit ccfc2d5cdb024 ("drm/nouveau: Use generic helper to
> > >> check _PR3 presence"), care is taken to leave the _PR3 detection outside
> > >> of the optimus_func condition.
> > >>
> > >> https://gitlab.freedesktop.org/drm/nouveau/-/issues/79
> > >>
> > >> Fixes: ccfc2d5cdb024 ("drm/nouveau: Use generic helper to check _PR3
> > >> presence")
> > >> Signed-off-by: Ratchanan Srirattanamet <peathot@hotmail.com>
> > >> ---
> > >>
> > >> Hello,
> > >>
> > >> This is my first time submitting a Linux patch. I've done a
> > >> number of PR/MR workflows on GitHub or GitLab, but never done any
> > >> email-oriented development. So, please excuse me if I'm doing something
> > >> incorrectly.
> > >>
> > >> drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +++++++++---------
> > >> 1 file changed, 9 insertions(+), 9 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > >> index 7c15f6448428..c88bda3ac820 100644
> > >> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > >> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> > >> @@ -220,15 +220,6 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> > >> int optimus_funcs;
> > >> struct pci_dev *parent_pdev;
> > >>
> > >> - *has_pr3 = false;
> > >> - parent_pdev = pci_upstream_bridge(pdev);
> > >> - if (parent_pdev) {
> > >> - if (parent_pdev->bridge_d3)
> > >> - *has_pr3 = pci_pr3_present(parent_pdev);
> > >> - else
> > >> - pci_d3cold_disable(pdev);
> > >> - }
> > >> -
> > >> dhandle = ACPI_HANDLE(&pdev->dev);
> > >> if (!dhandle)
> > >> return;
> > >> @@ -249,6 +240,15 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> > >> *has_opt = !!optimus_funcs;
> > >> *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
> > >>
> > >> + *has_pr3 = false;
> > >> + parent_pdev = pci_upstream_bridge(pdev);
> > >> + if (parent_pdev) {
> > >> + if (parent_pdev->bridge_d3)
> > >> + *has_pr3 = pci_pr3_present(parent_pdev);
> > >> + else
> > >> + pci_d3cold_disable(pdev);
> > >> + }
> > >> +
> > >> if (optimus_funcs) {
> > >> uint32_t result;
> > >> nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
> > >> --
> > >> 2.25.1
> > >>
> > >> _______________________________________________
> > >> Nouveau mailing list
> > >> Nouveau@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/nouveau
> > >>
> > >
> >
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
2021-07-17 18:02 [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device Ratchanan Srirattanamet
2021-07-22 16:36 ` Karol Herbst
@ 2021-07-27 12:11 ` Karol Herbst
1 sibling, 0 replies; 6+ messages in thread
From: Karol Herbst @ 2021-07-27 12:11 UTC (permalink / raw)
To: Ratchanan Srirattanamet; +Cc: nouveau, Ben Skeggs
On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet
<peathot@hotmail.com> wrote:
>
> The call site of nouveau_dsm_pci_probe() uses single set of output
> variables for all invocations. So, we must not write anything to them
> until we think this is an NVIDIA device of interest. Otherwise, if we
> are called with another device after the NVIDIA device, we'll clober the
> result of the NVIDIA device.
>
> In this case, if the other device doesn't have _PR3 resources, the
> detection later would miss the presence of power resource support, and
> the rest of the code will keep using Optimus DSM, breaking power
> management for that machine. In particular, this fixes power management
> of the NVIDIA card in Lenovo Legion 5-15ARH05... well, at least
> partially. New error shows up, but this patch is correct in itself
> anyway.
>
> As a bonus, we'll also stop preventing _PR3 usage from the bridge for
> unrelated devices, which is always nice, I guess.
>
> As noted in commit ccfc2d5cdb024 ("drm/nouveau: Use generic helper to
> check _PR3 presence"), care is taken to leave the _PR3 detection outside
> of the optimus_func condition.
>
> https://gitlab.freedesktop.org/drm/nouveau/-/issues/79
>
> Fixes: ccfc2d5cdb024 ("drm/nouveau: Use generic helper to check _PR3
> presence")
> Signed-off-by: Ratchanan Srirattanamet <peathot@hotmail.com>
> ---
>
> Hello,
>
> This is my first time submitting a Linux patch. I've done a
> number of PR/MR workflows on GitHub or GitLab, but never done any
> email-oriented development. So, please excuse me if I'm doing something
> incorrectly.
>
> drivers/gpu/drm/nouveau/nouveau_acpi.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> index 7c15f6448428..c88bda3ac820 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c
> @@ -220,15 +220,6 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> int optimus_funcs;
> struct pci_dev *parent_pdev;
>
> - *has_pr3 = false;
> - parent_pdev = pci_upstream_bridge(pdev);
> - if (parent_pdev) {
> - if (parent_pdev->bridge_d3)
> - *has_pr3 = pci_pr3_present(parent_pdev);
> - else
> - pci_d3cold_disable(pdev);
> - }
> -
> dhandle = ACPI_HANDLE(&pdev->dev);
> if (!dhandle)
> return;
> @@ -249,6 +240,15 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out
> *has_opt = !!optimus_funcs;
> *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS);
>
> + *has_pr3 = false;
> + parent_pdev = pci_upstream_bridge(pdev);
> + if (parent_pdev) {
> + if (parent_pdev->bridge_d3)
> + *has_pr3 = pci_pr3_present(parent_pdev);
> + else
> + pci_d3cold_disable(pdev);
> + }
> +
given that we call this code for all GPUs, I _think_ it is better to
check for GPU Vendors or check if the GPU we touch is the secondary
one. We also have systems with two Nvidia GPUs. But I don't know how
the detection worked there and if that's fine in the end... I might
have to investigate this on all laptops with various hybrid GPU types,
but... mhh. Maybe checking for "this is a secondary GPU" is good
enough.
> if (optimus_funcs) {
> uint32_t result;
> nouveau_optimus_dsm(dhandle, NOUVEAU_DSM_OPTIMUS_CAPS, 0,
> --
> 2.25.1
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-27 12:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-17 18:02 [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device Ratchanan Srirattanamet
2021-07-22 16:36 ` Karol Herbst
2021-07-22 19:49 ` Ratchanan Srirattanamet
2021-07-22 19:54 ` Karol Herbst
2021-07-22 19:57 ` Karol Herbst
2021-07-27 12:11 ` Karol Herbst
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).