On Thu, Jul 22, 2021 at 5:10 AM Ratchanan Srirattanamet
<peathot@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
_______________________________________________
Nouveau mailing list
Nouveau@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/nouveau
