Re: [Nouveau] [PATCH] drm/nouveau: don't touch has_pr3 for likely-non-NVIDIA device

[Karol Herbst <kherbst@redhat.com>]

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