Ratchanan Srirattanamet
2021-Jul-17 18:02 UTC
[Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
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 at 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
Karol Herbst
2021-Jul-22 16:36 UTC
[Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
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 at 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 at 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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau >
Karol Herbst
2021-Jul-27 12:11 UTC
[Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device
On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet <peathot at 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 at 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 at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/nouveau >