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 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