Ratchanan Srirattanamet
2021-Aug-08 19:23 UTC
[Nouveau] [PATCH] drm/nouveau: don't detect DSM for 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 unless it's an NVIDIA device. Otherwise, if we are called with another device after the NVIDIA device, we'll clober the result of the NVIDIA device. For example, 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. Also, because we're detecting NVIDIA's DSM, it doesn't make sense to run this detection on a non-NVIDIA device anyway. Thus, check at the beginning of the detection code if this is an NVIDIA card, and just return if it isn't. As a bonus, we'll also stop preventing _PR3 usage from the bridge for unrelated devices, which is always nice, I guess. https://gitlab.freedesktop.org/drm/nouveau/-/issues/79 Signed-off-by: Ratchanan Srirattanamet <peathot at hotmail.com> --- It's been discussed in the "previous" patch series ("[PATCH] drm/ nouveau: don't touch has_pr3 for likely-non-NVIDIA device" [1]) that, instead of changing how the detection works, it's better to check if the device's vendor is NVIDIA, or if it's a secondary device. I don't find a feasible way to detect the secondary-ness of a device, so I opted for checking the vendor code [2]. Thus, this makes the patch different enough to warant a different summary phrase and description. [1] https://lore.kernel.org/nouveau/CACO55tsYWW_dfAFWBFLrH51SVivUeN72Omc6DRTCtzVWSLE68w at mail.gmail.com/T/#mb710275dc1dd7e9d83028c218b886400d3a43bfc [2] It's been suggested to use `nouveau_dsm_get_client_id()`, however, on my machine it would return _DIS on both cards, so it can't be used. drivers/gpu/drm/nouveau/nouveau_acpi.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c index 7c15f6448428..9c55f205ab66 100644 --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -220,6 +220,9 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out int optimus_funcs; struct pci_dev *parent_pdev; + if (pdev->vendor != PCI_VENDOR_ID_NVIDIA) + return; + *has_pr3 = false; parent_pdev = pci_upstream_bridge(pdev); if (parent_pdev) { -- 2.25.1
Karol Herbst
2021-Sep-13 12:25 UTC
[Nouveau] [PATCH] drm/nouveau: don't detect DSM for non-NVIDIA device
Sorry for taking this long to take a look, it's just that I don't want this to get merged without testing, and I'd like to test it on the different kinds of hybrid GPU setups we have so that nothing unexpected happens here. I am not 100% sure how all of that works before optimus, so I have to find a laptop covering this generation of hardware. I think my oldest one here actually is, but the entire system on it is quite broken and it still has some private stuff on it so I would rather not touch that one :( But if others would be able to test that, that would be awesome. I will see if I can find a clean machine somewhere at work I can use for testing. On Sun, Aug 8, 2021 at 9:24 PM 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 > unless it's an NVIDIA device. Otherwise, if we are called with another > device after the NVIDIA device, we'll clober the result of the NVIDIA > device. > > For example, 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. > > Also, because we're detecting NVIDIA's DSM, it doesn't make sense to run > this detection on a non-NVIDIA device anyway. Thus, check at the > beginning of the detection code if this is an NVIDIA card, and just > return if it isn't. > > As a bonus, we'll also stop preventing _PR3 usage from the bridge for > unrelated devices, which is always nice, I guess. > > https://gitlab.freedesktop.org/drm/nouveau/-/issues/79 > > Signed-off-by: Ratchanan Srirattanamet <peathot at hotmail.com> > --- > > It's been discussed in the "previous" patch series ("[PATCH] drm/ > nouveau: don't touch has_pr3 for likely-non-NVIDIA device" [1]) that, > instead of changing how the detection works, it's better to check if > the device's vendor is NVIDIA, or if it's a secondary device. I don't > find a feasible way to detect the secondary-ness of a device, so I > opted for checking the vendor code [2]. > > Thus, this makes the patch different enough to warant a different > summary phrase and description. > > [1] https://lore.kernel.org/nouveau/CACO55tsYWW_dfAFWBFLrH51SVivUeN72Omc6DRTCtzVWSLE68w at mail.gmail.com/T/#mb710275dc1dd7e9d83028c218b886400d3a43bfc > [2] It's been suggested to use `nouveau_dsm_get_client_id()`, however, > on my machine it would return _DIS on both cards, so it can't be > used. > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > index 7c15f6448428..9c55f205ab66 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > @@ -220,6 +220,9 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, acpi_handle *dhandle_out > int optimus_funcs; > struct pci_dev *parent_pdev; > > + if (pdev->vendor != PCI_VENDOR_ID_NVIDIA) > + return; > + > *has_pr3 = false; > parent_pdev = pci_upstream_bridge(pdev); > if (parent_pdev) { > -- > 2.25.1 >