| Message ID | 1464130381-4797-5-git-send-email-peter@lekensteyn.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote: > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > can be runtime-suspended which disables power resources via ACPI. This > is incompatible with DSM, resulting in a GPU device which is still in D3 > and locks up the kernel on resume. > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > debugger trace) and stop using the DSM functions for D3cold when power > resources are available on the parent PCIe port. > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > --- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > index df9f73e..e469df7 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > bool dsm_detected; > bool optimus_detected; > bool optimus_flags_detected; > + bool optimus_skip_dsm; > acpi_handle dhandle; > acpi_handle rom_handle; > } nouveau_dsm_priv; > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > .get_client_id = nouveau_dsm_get_client_id, > }; > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > + * D3cold, they instead rely on disabling power resources on the parent. */ > +static bool nouveau_pr3_present(struct pci_dev *pdev) > +{ > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > + struct acpi_device *ad; Nit: please call this adev instead of ad. > + > + if (!parent_pdev) > + return false; > + > + ad = ACPI_COMPANION(&parent_pdev->dev); > + if (!ad) > + return false; > + > + return ad->power.flags.power_resources; Is this sufficient to tell if the parent device has _PR3? I thought it returns true if it has power resources in general, not necessarily _PR3. Otherwise this looks okay to me.
On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote: > On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote: > > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > > can be runtime-suspended which disables power resources via ACPI. This > > is incompatible with DSM, resulting in a GPU device which is still in D3 > > and locks up the kernel on resume. > > > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > > debugger trace) and stop using the DSM functions for D3cold when power > > resources are available on the parent PCIe port. > > > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > --- > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > index df9f73e..e469df7 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > > bool dsm_detected; > > bool optimus_detected; > > bool optimus_flags_detected; > > + bool optimus_skip_dsm; > > acpi_handle dhandle; > > acpi_handle rom_handle; > > } nouveau_dsm_priv; > > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > > .get_client_id = nouveau_dsm_get_client_id, > > }; > > > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > > + * D3cold, they instead rely on disabling power resources on the parent. */ > > +static bool nouveau_pr3_present(struct pci_dev *pdev) > > +{ > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > + struct acpi_device *ad; > > Nit: please call this adev instead of ad. Will do. > > + > > + if (!parent_pdev) > > + return false; > > + > > + ad = ACPI_COMPANION(&parent_pdev->dev); > > + if (!ad) > > + return false; > > + > > + return ad->power.flags.power_resources; > > Is this sufficient to tell if the parent device has _PR3? I thought it > returns true if it has power resources in general, not necessarily _PR3. > > Otherwise this looks okay to me. It is indeed set whenever there is any _PRx method. I wonder if it is appropriate to access fields directly like this, perhaps this would be more accurate (based on device_pm.c): /* Check whether the _PR3 method is available. */ return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; I am also considering adding a check in case the pcieport driver does not support D3cold via runtime PM, what do you think of this? if (!parent_pdev) return false; /* If the PCIe port does not support D3cold via runtime PM, allow a * fallback to the Optimus DSM method to put the device in D3cold. */ if (parent_pdev->no_d3cold) return false; This is needed to avoid the regression reported in the cover letter, but also allows pre-2015 systems to (still) have the D3cold possibility. Out of curiosity I looked up an pre-2015 laptop (found Acer V5-573G, apparently from November 2013, Windows 8.1) and extracted the ACPI tables from the BIOS images. BIOS 2.28 (2014/05/13) introduces support for power resources on the parent devicea(\_SB.PCI0.PEG0._PR3 and a related NVP3 device) when _OSI("Windows 2013") is true. (This is added as alternative for the old DSM interface.) Maybe 2014 is also an appropriate cutoff date? I wonder if it is feasible to detect firmware use of _OSI("Windows 2013") and use that instead of the BIOS year.
Hi, On 27-05-16 13:10, Peter Wu wrote: > On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote: >> On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote: >>> Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port >>> can be runtime-suspended which disables power resources via ACPI. This >>> is incompatible with DSM, resulting in a GPU device which is still in D3 >>> and locks up the kernel on resume. >>> >>> Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi >>> debugger trace) and stop using the DSM functions for D3cold when power >>> resources are available on the parent PCIe port. >>> >>> [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold >>> >>> Signed-off-by: Peter Wu <peter@lekensteyn.nl> >>> --- >>> drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- >>> 1 file changed, 30 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c >>> index df9f73e..e469df7 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c >>> +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c >>> @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { >>> bool dsm_detected; >>> bool optimus_detected; >>> bool optimus_flags_detected; >>> + bool optimus_skip_dsm; >>> acpi_handle dhandle; >>> acpi_handle rom_handle; >>> } nouveau_dsm_priv; >>> @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { >>> .get_client_id = nouveau_dsm_get_client_id, >>> }; >>> >>> +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into >>> + * D3cold, they instead rely on disabling power resources on the parent. */ >>> +static bool nouveau_pr3_present(struct pci_dev *pdev) >>> +{ >>> + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); >>> + struct acpi_device *ad; >> >> Nit: please call this adev instead of ad. > > Will do. > >>> + >>> + if (!parent_pdev) >>> + return false; >>> + >>> + ad = ACPI_COMPANION(&parent_pdev->dev); >>> + if (!ad) >>> + return false; >>> + >>> + return ad->power.flags.power_resources; >> >> Is this sufficient to tell if the parent device has _PR3? I thought it >> returns true if it has power resources in general, not necessarily _PR3. >> >> Otherwise this looks okay to me. > > It is indeed set whenever there is any _PRx method. I wonder if it is > appropriate to access fields directly like this, perhaps this would be > more accurate (based on device_pm.c): > > /* Check whether the _PR3 method is available. */ > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; > > I am also considering adding a check in case the pcieport driver does > not support D3cold via runtime PM, what do you think of this? > > if (!parent_pdev) > return false; > /* If the PCIe port does not support D3cold via runtime PM, allow a > * fallback to the Optimus DSM method to put the device in D3cold. */ > if (parent_pdev->no_d3cold) > return false; > > This is needed to avoid the regression reported in the cover letter, but > also allows pre-2015 systems to (still) have the D3cold possibility. > > > Out of curiosity I looked up an pre-2015 laptop (found Acer V5-573G, > apparently from November 2013, Windows 8.1) and extracted the ACPI > tables from the BIOS images. BIOS 2.28 (2014/05/13) introduces support > for power resources on the parent devicea(\_SB.PCI0.PEG0._PR3 and a > related NVP3 device) when _OSI("Windows 2013") is true. (This is added > as alternative for the old DSM interface.) > > Maybe 2014 is also an appropriate cutoff date? I wonder if it is > feasible to detect firmware use of _OSI("Windows 2013") and use that > instead of the BIOS year. It is definitely possible to check if the firmware uses _OSI("Windows 2013") we do something similar to check for windows-8 ready laptops in the backlight code, see acpi_osi_is_win8() in drivers/acpi/osl.c, or if you actually want to test for Windows 8 or newer, just use acpi_osi_is_win8() :) Regards, Hans
Hi Peter, On 24 May 2016 at 23:53, Peter Wu <peter@lekensteyn.nl> wrote: > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > can be runtime-suspended which disables power resources via ACPI. This > is incompatible with DSM, resulting in a GPU device which is still in D3 > and locks up the kernel on resume. > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > debugger trace) and stop using the DSM functions for D3cold when power > resources are available on the parent PCIe port. > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > --- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > 1 file changed, 30 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > index df9f73e..e469df7 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > bool dsm_detected; > bool optimus_detected; > bool optimus_flags_detected; > + bool optimus_skip_dsm; > acpi_handle dhandle; > acpi_handle rom_handle; > } nouveau_dsm_priv; > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > .get_client_id = nouveau_dsm_get_client_id, > }; > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > + * D3cold, they instead rely on disabling power resources on the parent. */ > +static bool nouveau_pr3_present(struct pci_dev *pdev) > +{ > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > + struct acpi_device *ad; > + > + if (!parent_pdev) > + return false; > + > + ad = ACPI_COMPANION(&parent_pdev->dev); > + if (!ad) > + return false; > + > + return ad->power.flags.power_resources; > +} > + > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > - bool *has_opt, bool *has_opt_flags) > + bool *has_opt, bool *has_opt_flags, > + bool *has_power_resources) > { > acpi_handle dhandle; > bool supports_mux; > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > *has_mux = supports_mux; > *has_opt = !!optimus_funcs; > *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS); > + *has_power_resources = false; > > if (optimus_funcs) { > uint32_t result; > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > (result & OPTIMUS_ENABLED) ? "enabled" : "disabled", > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, " : "", > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec supported" : ""); > + > + *has_power_resources = nouveau_pr3_present(pdev); > } > } > > @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void) > bool has_mux = false; > bool has_optimus = false; > bool has_optimus_flags = false; > + bool has_power_resources = false; > int vga_count = 0; > bool guid_valid; > bool ret = false; > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) > vga_count++; > > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > - &has_optimus_flags); > + &has_optimus_flags, &has_power_resources); > } > > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { > vga_count++; > > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > - &has_optimus_flags); > + &has_optimus_flags, &has_power_resources); > } > This and earlier patch break things in a subtle way. Namely: upon the second (and any later) call into the nouveau_dsm_pci_probe() function, the had_foo flags are reset. Thus only the specifics of the _final_ device are being used (at a later stage). IMHO one should change that to "_any_ device", which will match the original code and the actual intent further down in the file. Regards, Emil
On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote: > Hi Peter, > > On 24 May 2016 at 23:53, Peter Wu <peter@lekensteyn.nl> wrote: > > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > > can be runtime-suspended which disables power resources via ACPI. This > > is incompatible with DSM, resulting in a GPU device which is still in D3 > > and locks up the kernel on resume. > > > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > > debugger trace) and stop using the DSM functions for D3cold when power > > resources are available on the parent PCIe port. > > > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > --- > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > index df9f73e..e469df7 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > > bool dsm_detected; > > bool optimus_detected; > > bool optimus_flags_detected; > > + bool optimus_skip_dsm; > > acpi_handle dhandle; > > acpi_handle rom_handle; > > } nouveau_dsm_priv; > > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > > .get_client_id = nouveau_dsm_get_client_id, > > }; > > > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > > + * D3cold, they instead rely on disabling power resources on the parent. */ > > +static bool nouveau_pr3_present(struct pci_dev *pdev) > > +{ > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > + struct acpi_device *ad; > > + > > + if (!parent_pdev) > > + return false; > > + > > + ad = ACPI_COMPANION(&parent_pdev->dev); > > + if (!ad) > > + return false; > > + > > + return ad->power.flags.power_resources; > > +} > > + > > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > > - bool *has_opt, bool *has_opt_flags) > > + bool *has_opt, bool *has_opt_flags, > > + bool *has_power_resources) > > { > > acpi_handle dhandle; > > bool supports_mux; > > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > > *has_mux = supports_mux; > > *has_opt = !!optimus_funcs; > > *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS); > > + *has_power_resources = false; > > > > if (optimus_funcs) { > > uint32_t result; > > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > > (result & OPTIMUS_ENABLED) ? "enabled" : "disabled", > > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, " : "", > > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec supported" : ""); > > + > > + *has_power_resources = nouveau_pr3_present(pdev); > > } > > } > > > > @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void) > > bool has_mux = false; > > bool has_optimus = false; > > bool has_optimus_flags = false; > > + bool has_power_resources = false; > > int vga_count = 0; > > bool guid_valid; > > bool ret = false; > > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) > > vga_count++; > > > > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > > - &has_optimus_flags); > > + &has_optimus_flags, &has_power_resources); > > } > > > > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { > > vga_count++; > > > > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > > - &has_optimus_flags); > > + &has_optimus_flags, &has_power_resources); > > } > > > This and earlier patch break things in a subtle way. > > Namely: upon the second (and any later) call into the > nouveau_dsm_pci_probe() function, the had_foo flags are reset. Thus > only the specifics of the _final_ device are being used (at a later > stage). IMHO one should change that to "_any_ device", which will > match the original code and the actual intent further down in the > file. The flags are only reset if any of the MUX or Optimus handles are found. If both are missing, the flags are not overridden. This is from patch 1: + /* Does not look like a Nvidia device. */ + if (!supports_mux && !supports_opt) + return; The reason why later calls override early ones is because some Optimus laptops have the _DSM method on both the Intel GPU (00:02.0) and the Nvidia one (01:00.0). The previous detection method would fail in this scenario: 1. One device reports support for X and Y (has_x = 1, has_y = 1). Write ACPI handle A to nouveau_dsm_priv.dhandle. 2. Another device reports support for X only (has_x = 1). Write ACPI handle B to nouveau_dsm_priv.dhandle. 3. End result: has_x = 1, has_y = 1, dhandle = B. But ACPI handle B does not really support Y! This is theoretical since I don't think that such firmware occurs in practice, but then imo it would be better to enforce in code that the case cannot occur (as is done in patch 1). Unless the MUX DSM has some magic I am not aware of.
Hi Peter, On Fri, May 27, 2016 at 11:31:23PM +0200, Peter Wu wrote: > On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote: > > On 24 May 2016 at 23:53, Peter Wu <peter@lekensteyn.nl> wrote: [snip] > > > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) > > > vga_count++; > > > > > > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > > > - &has_optimus_flags); > > > + &has_optimus_flags, &has_power_resources); > > > } > > > > > > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { > > > vga_count++; > > > > > > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > > > - &has_optimus_flags); > > > + &has_optimus_flags, &has_power_resources); > > > } > > > > > This and earlier patch break things in a subtle way. > > > > Namely: upon the second (and any later) call into the > > nouveau_dsm_pci_probe() function, the had_foo flags are reset. Thus > > only the specifics of the _final_ device are being used (at a later > > stage). IMHO one should change that to "_any_ device", which will > > match the original code and the actual intent further down in the > > file. > > The flags are only reset if any of the MUX or Optimus handles are found. > If both are missing, the flags are not overridden. This is from patch 1: > > + /* Does not look like a Nvidia device. */ > + if (!supports_mux && !supports_opt) > + return; > > The reason why later calls override early ones is because some Optimus > laptops have the _DSM method on both the Intel GPU (00:02.0) and the > Nvidia one (01:00.0). Sounds like you may want to check for pdev->vendor == PCI_VENDOR_ID_NVIDIA or export pci_get_dev_by_id() and use that to match for class and vendor. Best regards, Lukas
+Rafael On Fri, May 27, 2016 at 01:10:37PM +0200, Peter Wu wrote: > On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote: > > On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote: > > > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > > > can be runtime-suspended which disables power resources via ACPI. This > > > is incompatible with DSM, resulting in a GPU device which is still in D3 > > > and locks up the kernel on resume. > > > > > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > > > debugger trace) and stop using the DSM functions for D3cold when power > > > resources are available on the parent PCIe port. > > > > > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > > > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > > --- > > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > index df9f73e..e469df7 100644 > > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > > > bool dsm_detected; > > > bool optimus_detected; > > > bool optimus_flags_detected; > > > + bool optimus_skip_dsm; > > > acpi_handle dhandle; > > > acpi_handle rom_handle; > > > } nouveau_dsm_priv; > > > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > > > .get_client_id = nouveau_dsm_get_client_id, > > > }; > > > > > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > > > + * D3cold, they instead rely on disabling power resources on the parent. */ > > > +static bool nouveau_pr3_present(struct pci_dev *pdev) > > > +{ > > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > > + struct acpi_device *ad; > > > > Nit: please call this adev instead of ad. > > Will do. > > > > + > > > + if (!parent_pdev) > > > + return false; > > > + > > > + ad = ACPI_COMPANION(&parent_pdev->dev); > > > + if (!ad) > > > + return false; > > > + > > > + return ad->power.flags.power_resources; > > > > Is this sufficient to tell if the parent device has _PR3? I thought it > > returns true if it has power resources in general, not necessarily _PR3. > > > > Otherwise this looks okay to me. > > It is indeed set whenever there is any _PRx method. I wonder if it is > appropriate to access fields directly like this, perhaps this would be > more accurate (based on device_pm.c): > > /* Check whether the _PR3 method is available. */ > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; > > I am also considering adding a check in case the pcieport driver does > not support D3cold via runtime PM, what do you think of this? > > if (!parent_pdev) > return false; > /* If the PCIe port does not support D3cold via runtime PM, allow a > * fallback to the Optimus DSM method to put the device in D3cold. */ > if (parent_pdev->no_d3cold) > return false; > > This is needed to avoid the regression reported in the cover letter, but > also allows pre-2015 systems to (still) have the D3cold possibility. The _DSM method with 0 as index parameter should return a bit field telling which functions are supported. Sane BIOS disables that particular function if it detects Windows 8 and newer. Have you checked if that's the case? Then you can call _DSM only if it is supported and otherwise expect the parent device's power resources to turn off power when runtime suspended. > Out of curiosity I looked up an pre-2015 laptop (found Acer V5-573G, > apparently from November 2013, Windows 8.1) and extracted the ACPI > tables from the BIOS images. BIOS 2.28 (2014/05/13) introduces support > for power resources on the parent devicea(\_SB.PCI0.PEG0._PR3 and a > related NVP3 device) when _OSI("Windows 2013") is true. (This is added > as alternative for the old DSM interface.) > > Maybe 2014 is also an appropriate cutoff date? I wonder if it is > feasible to detect firmware use of _OSI("Windows 2013") and use that > instead of the BIOS year. Using BIOS year works even if there is no ACPI available. What comes to the cutoff date, I discussed with Rafael and it was decided that we use the same year Windows 10 was released to be on the safe side. Reading the links you provided here: https://msdn.microsoft.com/fi-fi/windows/hardware/drivers/bringup/device-power-management https://msdn.microsoft.com/en-us/library/windows/hardware/hh967709(v=vs.85).aspx it seems that from Windows 8 they started transitioning devices into D3cold during runtime as well.
On 27 May 2016 at 22:31, Peter Wu <peter@lekensteyn.nl> wrote: > On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote: >> Hi Peter, >> >> On 24 May 2016 at 23:53, Peter Wu <peter@lekensteyn.nl> wrote: >> > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port >> > can be runtime-suspended which disables power resources via ACPI. This >> > is incompatible with DSM, resulting in a GPU device which is still in D3 >> > and locks up the kernel on resume. >> > >> > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi >> > debugger trace) and stop using the DSM functions for D3cold when power >> > resources are available on the parent PCIe port. >> > >> > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold >> > >> > Signed-off-by: Peter Wu <peter@lekensteyn.nl> >> > --- >> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- >> > 1 file changed, 30 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c >> > index df9f73e..e469df7 100644 >> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c >> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c >> > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { >> > bool dsm_detected; >> > bool optimus_detected; >> > bool optimus_flags_detected; >> > + bool optimus_skip_dsm; >> > acpi_handle dhandle; >> > acpi_handle rom_handle; >> > } nouveau_dsm_priv; >> > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { >> > .get_client_id = nouveau_dsm_get_client_id, >> > }; >> > >> > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into >> > + * D3cold, they instead rely on disabling power resources on the parent. */ >> > +static bool nouveau_pr3_present(struct pci_dev *pdev) >> > +{ >> > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); >> > + struct acpi_device *ad; >> > + >> > + if (!parent_pdev) >> > + return false; >> > + >> > + ad = ACPI_COMPANION(&parent_pdev->dev); >> > + if (!ad) >> > + return false; >> > + >> > + return ad->power.flags.power_resources; >> > +} >> > + >> > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, >> > - bool *has_opt, bool *has_opt_flags) >> > + bool *has_opt, bool *has_opt_flags, >> > + bool *has_power_resources) >> > { >> > acpi_handle dhandle; >> > bool supports_mux; >> > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, >> > *has_mux = supports_mux; >> > *has_opt = !!optimus_funcs; >> > *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS); >> > + *has_power_resources = false; >> > >> > if (optimus_funcs) { >> > uint32_t result; >> > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, >> > (result & OPTIMUS_ENABLED) ? "enabled" : "disabled", >> > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, " : "", >> > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec supported" : ""); >> > + >> > + *has_power_resources = nouveau_pr3_present(pdev); >> > } >> > } >> > >> > @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void) >> > bool has_mux = false; >> > bool has_optimus = false; >> > bool has_optimus_flags = false; >> > + bool has_power_resources = false; >> > int vga_count = 0; >> > bool guid_valid; >> > bool ret = false; >> > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) >> > vga_count++; >> > >> > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, >> > - &has_optimus_flags); >> > + &has_optimus_flags, &has_power_resources); >> > } >> > >> > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { >> > vga_count++; >> > >> > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, >> > - &has_optimus_flags); >> > + &has_optimus_flags, &has_power_resources); >> > } >> > >> This and earlier patch break things in a subtle way. >> >> Namely: upon the second (and any later) call into the >> nouveau_dsm_pci_probe() function, the had_foo flags are reset. Thus >> only the specifics of the _final_ device are being used (at a later >> stage). IMHO one should change that to "_any_ device", which will >> match the original code and the actual intent further down in the >> file. > > The flags are only reset if any of the MUX or Optimus handles are found. > If both are missing, the flags are not overridden. This is from patch 1: > > + /* Does not look like a Nvidia device. */ > + if (!supports_mux && !supports_opt) > + return; > This is precisely what I'm saying, and I think it's wrong/strange. If you've detected that device A support_{X,Y}, you'll reset the support_{X,Y} flag anyway if device B is present... (continues further down) > The reason why later calls override early ones is because some Optimus > laptops have the _DSM method on both the Intel GPU (00:02.0) and the > Nvidia one (01:00.0). > I agree with Lukas idea that one could/should be checking for nvidia devices (perhaps in nouveau_dsm_pci_probe() or just before calling it ?). > The previous detection method would fail in this scenario: > 1. One device reports support for X and Y (has_x = 1, has_y = 1). Write > ACPI handle A to nouveau_dsm_priv.dhandle. > 2. Another device reports support for X only (has_x = 1). Write > ACPI handle B to nouveau_dsm_priv.dhandle. > 3. End result: has_x = 1, has_y = 1, dhandle = B. But ACPI handle B > does not really support Y! ... so to avoid the above case and preserve the original ideas ('do not discard earlier device caps' and 'Optimus takes precedence over DSM v1') one could do the following: - decouple the "feature check" and "set the dhandle" - pick the 'ideal' one based on the feature set provided. if multiple pick one based on $insert_heuristics - set the dhandle What do you think ? Regards, Emil
On Mon, May 30, 2016 at 11:48:34AM +0100, Emil Velikov wrote: > On 27 May 2016 at 22:31, Peter Wu <peter@lekensteyn.nl> wrote: > > On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote: > >> Hi Peter, > >> > >> On 24 May 2016 at 23:53, Peter Wu <peter@lekensteyn.nl> wrote: > >> > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > >> > can be runtime-suspended which disables power resources via ACPI. This > >> > is incompatible with DSM, resulting in a GPU device which is still in D3 > >> > and locks up the kernel on resume. > >> > > >> > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > >> > debugger trace) and stop using the DSM functions for D3cold when power > >> > resources are available on the parent PCIe port. > >> > > >> > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > >> > > >> > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > >> > --- > >> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > >> > 1 file changed, 30 insertions(+), 4 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > >> > index df9f73e..e469df7 100644 > >> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > >> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > >> > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > >> > bool dsm_detected; > >> > bool optimus_detected; > >> > bool optimus_flags_detected; > >> > + bool optimus_skip_dsm; > >> > acpi_handle dhandle; > >> > acpi_handle rom_handle; > >> > } nouveau_dsm_priv; > >> > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > >> > .get_client_id = nouveau_dsm_get_client_id, > >> > }; > >> > > >> > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > >> > + * D3cold, they instead rely on disabling power resources on the parent. */ > >> > +static bool nouveau_pr3_present(struct pci_dev *pdev) > >> > +{ > >> > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > >> > + struct acpi_device *ad; > >> > + > >> > + if (!parent_pdev) > >> > + return false; > >> > + > >> > + ad = ACPI_COMPANION(&parent_pdev->dev); > >> > + if (!ad) > >> > + return false; > >> > + > >> > + return ad->power.flags.power_resources; > >> > +} > >> > + > >> > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > >> > - bool *has_opt, bool *has_opt_flags) > >> > + bool *has_opt, bool *has_opt_flags, > >> > + bool *has_power_resources) > >> > { > >> > acpi_handle dhandle; > >> > bool supports_mux; > >> > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > >> > *has_mux = supports_mux; > >> > *has_opt = !!optimus_funcs; > >> > *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS); > >> > + *has_power_resources = false; > >> > > >> > if (optimus_funcs) { > >> > uint32_t result; > >> > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, > >> > (result & OPTIMUS_ENABLED) ? "enabled" : "disabled", > >> > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, " : "", > >> > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec supported" : ""); > >> > + > >> > + *has_power_resources = nouveau_pr3_present(pdev); > >> > } > >> > } > >> > > >> > @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void) > >> > bool has_mux = false; > >> > bool has_optimus = false; > >> > bool has_optimus_flags = false; > >> > + bool has_power_resources = false; > >> > int vga_count = 0; > >> > bool guid_valid; > >> > bool ret = false; > >> > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) > >> > vga_count++; > >> > > >> > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > >> > - &has_optimus_flags); > >> > + &has_optimus_flags, &has_power_resources); > >> > } > >> > > >> > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { > >> > vga_count++; > >> > > >> > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, > >> > - &has_optimus_flags); > >> > + &has_optimus_flags, &has_power_resources); > >> > } > >> > > >> This and earlier patch break things in a subtle way. > >> > >> Namely: upon the second (and any later) call into the > >> nouveau_dsm_pci_probe() function, the had_foo flags are reset. Thus > >> only the specifics of the _final_ device are being used (at a later > >> stage). IMHO one should change that to "_any_ device", which will > >> match the original code and the actual intent further down in the > >> file. > > > > The flags are only reset if any of the MUX or Optimus handles are found. > > If both are missing, the flags are not overridden. This is from patch 1: > > > > + /* Does not look like a Nvidia device. */ > > + if (!supports_mux && !supports_opt) > > + return; > > > This is precisely what I'm saying, and I think it's wrong/strange. If > you've detected that device A support_{X,Y}, you'll reset the > support_{X,Y} flag anyway if device B is present... (continues further > down) The flags will only be reset when device B supports at least one function. > > The reason why later calls override early ones is because some Optimus > > laptops have the _DSM method on both the Intel GPU (00:02.0) and the > > Nvidia one (01:00.0). > > > I agree with Lukas idea that one could/should be checking for nvidia > devices (perhaps in nouveau_dsm_pci_probe() or just before calling it > ?). That could break PM on at least two Acer laptops. The Acer Travelmate 8472TG from 2011 (acpidump[1]) has two DSM on the Nvidia and Intel ACPI handles: - Nvidia: supports MXM methods only. - Intel: supports the older Nvidia UUID (for toggling power and possibly other things). [1]: https://github.com/Bumblebee-Project/bbswitch/issues/4#issuecomment-219988501 There is also an Acer Aspire 5742G which possibly breaks (linked in the above issue), but that could be a configuration issue that disabled Optimus in BIOS (unconfirmed). If it matters, both of these laptops have a MXMX method (Select Display Data Channel), but their MXMI (Return Specification Support Level) and MXMS (Return MXM Structure) functions are disfunctional. There is also a MXDS function on both ACPI handles, but these are not hooked to the WMI interface for some reason. No idea of Acer has hacked up some drivers to work with this, outside these models I do not know others that are also affected by this issue. > > The previous detection method would fail in this scenario: > > 1. One device reports support for X and Y (has_x = 1, has_y = 1). Write > > ACPI handle A to nouveau_dsm_priv.dhandle. > > 2. Another device reports support for X only (has_x = 1). Write > > ACPI handle B to nouveau_dsm_priv.dhandle. > > 3. End result: has_x = 1, has_y = 1, dhandle = B. But ACPI handle B > > does not really support Y! > > ... so to avoid the above case and preserve the original ideas ('do > not discard earlier device caps' and 'Optimus takes precedence over > DSM v1') one could do the following: > > - decouple the "feature check" and "set the dhandle" > > - pick the 'ideal' one based on the feature set provided. if multiple > pick one based on $insert_heuristics > - set the dhandle > > What do you think ? The dhandle is only set when at least one valid DSM was found on the device. The dhandle assignment could indeed be moved to the caller, making it more obvious that the dhandle is only valid when the capabilities are detected (this does not have a functional change though). I'll do it in the next version.
On Mon, May 30, 2016 at 12:57:09PM +0300, Mika Westerberg wrote: > +Rafael > > On Fri, May 27, 2016 at 01:10:37PM +0200, Peter Wu wrote: > > On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote: > > > On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote: > > > > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > > > > can be runtime-suspended which disables power resources via ACPI. This > > > > is incompatible with DSM, resulting in a GPU device which is still in D3 > > > > and locks up the kernel on resume. > > > > > > > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > > > > debugger trace) and stop using the DSM functions for D3cold when power > > > > resources are available on the parent PCIe port. > > > > > > > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > > > > > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > > > --- > > > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > > index df9f73e..e469df7 100644 > > > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > > > > bool dsm_detected; > > > > bool optimus_detected; > > > > bool optimus_flags_detected; > > > > + bool optimus_skip_dsm; > > > > acpi_handle dhandle; > > > > acpi_handle rom_handle; > > > > } nouveau_dsm_priv; > > > > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > > > > .get_client_id = nouveau_dsm_get_client_id, > > > > }; > > > > > > > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > > > > + * D3cold, they instead rely on disabling power resources on the parent. */ > > > > +static bool nouveau_pr3_present(struct pci_dev *pdev) > > > > +{ > > > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > > > + struct acpi_device *ad; > > > > > > Nit: please call this adev instead of ad. > > > > Will do. > > > > > > + > > > > + if (!parent_pdev) > > > > + return false; > > > > + > > > > + ad = ACPI_COMPANION(&parent_pdev->dev); > > > > + if (!ad) > > > > + return false; > > > > + > > > > + return ad->power.flags.power_resources; > > > > > > Is this sufficient to tell if the parent device has _PR3? I thought it > > > returns true if it has power resources in general, not necessarily _PR3. > > > > > > Otherwise this looks okay to me. > > > > It is indeed set whenever there is any _PRx method. I wonder if it is > > appropriate to access fields directly like this, perhaps this would be > > more accurate (based on device_pm.c): > > > > /* Check whether the _PR3 method is available. */ > > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; > > > > I am also considering adding a check in case the pcieport driver does > > not support D3cold via runtime PM, what do you think of this? > > > > if (!parent_pdev) > > return false; > > /* If the PCIe port does not support D3cold via runtime PM, allow a > > * fallback to the Optimus DSM method to put the device in D3cold. */ > > if (parent_pdev->no_d3cold) > > return false; > > > > This is needed to avoid the regression reported in the cover letter, but > > also allows pre-2015 systems to (still) have the D3cold possibility. > > The _DSM method with 0 as index parameter should return a bit field > telling which functions are supported. Sane BIOS disables that > particular function if it detects Windows 8 and newer. Have you checked > if that's the case? > > Then you can call _DSM only if it is supported and otherwise expect the > parent device's power resources to turn off power when runtime > suspended. The _DSM methods (for the Nvidia device) are often still included and functions are reported as supported. I guess that vendors just check whether it is working and do not bother removing legacy functions. The Acer case below seems exceptional. I suggested the no_d3cold check such that DSM can still be called even though the runtime PM on the PCIe port does nothing. > > Out of curiosity I looked up an pre-2015 laptop (found Acer V5-573G, > > apparently from November 2013, Windows 8.1) and extracted the ACPI > > tables from the BIOS images. BIOS 2.28 (2014/05/13) introduces support > > for power resources on the parent devicea(\_SB.PCI0.PEG0._PR3 and a > > related NVP3 device) when _OSI("Windows 2013") is true. (This is added > > as alternative for the old DSM interface.) > > > > Maybe 2014 is also an appropriate cutoff date? I wonder if it is > > feasible to detect firmware use of _OSI("Windows 2013") and use that > > instead of the BIOS year. > > Using BIOS year works even if there is no ACPI available. I thought that you need support from ACPI to put a device in D3cold? > What comes to the cutoff date, I discussed with Rafael and it was > decided that we use the same year Windows 10 was released to be on the > safe side. Reading the links you provided here: > > https://msdn.microsoft.com/fi-fi/windows/hardware/drivers/bringup/device-power-management > https://msdn.microsoft.com/en-us/library/windows/hardware/hh967709(v=vs.85).aspx > > it seems that from Windows 8 they started transitioning devices into > D3cold during runtime as well. My impression from the ACPI tables I have seen so far is that power resources support is enabled for Windows 2012 (Win8) or newer.
On 30 May 2016 at 12:23, Peter Wu <peter@lekensteyn.nl> wrote: > On Mon, May 30, 2016 at 11:48:34AM +0100, Emil Velikov wrote: >> On 27 May 2016 at 22:31, Peter Wu <peter@lekensteyn.nl> wrote: >> > On Fri, May 27, 2016 at 02:01:39PM +0100, Emil Velikov wrote: >> >> Hi Peter, >> >> >> >> On 24 May 2016 at 23:53, Peter Wu <peter@lekensteyn.nl> wrote: >> >> > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port >> >> > can be runtime-suspended which disables power resources via ACPI. This >> >> > is incompatible with DSM, resulting in a GPU device which is still in D3 >> >> > and locks up the kernel on resume. >> >> > >> >> > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi >> >> > debugger trace) and stop using the DSM functions for D3cold when power >> >> > resources are available on the parent PCIe port. >> >> > >> >> > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold >> >> > >> >> > Signed-off-by: Peter Wu <peter@lekensteyn.nl> >> >> > --- >> >> > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- >> >> > 1 file changed, 30 insertions(+), 4 deletions(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c >> >> > index df9f73e..e469df7 100644 >> >> > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c >> >> > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c >> >> > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { >> >> > bool dsm_detected; >> >> > bool optimus_detected; >> >> > bool optimus_flags_detected; >> >> > + bool optimus_skip_dsm; >> >> > acpi_handle dhandle; >> >> > acpi_handle rom_handle; >> >> > } nouveau_dsm_priv; >> >> > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { >> >> > .get_client_id = nouveau_dsm_get_client_id, >> >> > }; >> >> > >> >> > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into >> >> > + * D3cold, they instead rely on disabling power resources on the parent. */ >> >> > +static bool nouveau_pr3_present(struct pci_dev *pdev) >> >> > +{ >> >> > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); >> >> > + struct acpi_device *ad; >> >> > + >> >> > + if (!parent_pdev) >> >> > + return false; >> >> > + >> >> > + ad = ACPI_COMPANION(&parent_pdev->dev); >> >> > + if (!ad) >> >> > + return false; >> >> > + >> >> > + return ad->power.flags.power_resources; >> >> > +} >> >> > + >> >> > static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, >> >> > - bool *has_opt, bool *has_opt_flags) >> >> > + bool *has_opt, bool *has_opt_flags, >> >> > + bool *has_power_resources) >> >> > { >> >> > acpi_handle dhandle; >> >> > bool supports_mux; >> >> > @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, >> >> > *has_mux = supports_mux; >> >> > *has_opt = !!optimus_funcs; >> >> > *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS); >> >> > + *has_power_resources = false; >> >> > >> >> > if (optimus_funcs) { >> >> > uint32_t result; >> >> > @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, >> >> > (result & OPTIMUS_ENABLED) ? "enabled" : "disabled", >> >> > (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, " : "", >> >> > (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec supported" : ""); >> >> > + >> >> > + *has_power_resources = nouveau_pr3_present(pdev); >> >> > } >> >> > } >> >> > >> >> > @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void) >> >> > bool has_mux = false; >> >> > bool has_optimus = false; >> >> > bool has_optimus_flags = false; >> >> > + bool has_power_resources = false; >> >> > int vga_count = 0; >> >> > bool guid_valid; >> >> > bool ret = false; >> >> > @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) >> >> > vga_count++; >> >> > >> >> > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, >> >> > - &has_optimus_flags); >> >> > + &has_optimus_flags, &has_power_resources); >> >> > } >> >> > >> >> > while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { >> >> > vga_count++; >> >> > >> >> > nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, >> >> > - &has_optimus_flags); >> >> > + &has_optimus_flags, &has_power_resources); >> >> > } >> >> > >> >> This and earlier patch break things in a subtle way. >> >> >> >> Namely: upon the second (and any later) call into the >> >> nouveau_dsm_pci_probe() function, the had_foo flags are reset. Thus >> >> only the specifics of the _final_ device are being used (at a later >> >> stage). IMHO one should change that to "_any_ device", which will >> >> match the original code and the actual intent further down in the >> >> file. >> > >> > The flags are only reset if any of the MUX or Optimus handles are found. >> > If both are missing, the flags are not overridden. This is from patch 1: >> > >> > + /* Does not look like a Nvidia device. */ >> > + if (!supports_mux && !supports_opt) >> > + return; >> > >> This is precisely what I'm saying, and I think it's wrong/strange. If >> you've detected that device A support_{X,Y}, you'll reset the >> support_{X,Y} flag anyway if device B is present... (continues further >> down) > > The flags will only be reset when device B supports at least one > function. > Indeed. Seems like I completely misread the code on multiple occasions. Sorry about the noise. >> > The reason why later calls override early ones is because some Optimus >> > laptops have the _DSM method on both the Intel GPU (00:02.0) and the >> > Nvidia one (01:00.0). >> > >> I agree with Lukas idea that one could/should be checking for nvidia >> devices (perhaps in nouveau_dsm_pci_probe() or just before calling it >> ?). > > That could break PM on at least two Acer laptops. The Acer Travelmate > 8472TG from 2011 (acpidump[1]) has two DSM on the Nvidia and Intel ACPI > handles: > > - Nvidia: supports MXM methods only. > - Intel: supports the older Nvidia UUID (for toggling power and > possibly other things). > > [1]: https://github.com/Bumblebee-Project/bbswitch/issues/4#issuecomment-219988501 > > There is also an Acer Aspire 5742G which possibly breaks (linked in the > above issue), but that could be a configuration issue that disabled > Optimus in BIOS (unconfirmed). > > If it matters, both of these laptops have a MXMX method (Select Display > Data Channel), but their MXMI (Return Specification Support Level) and > MXMS (Return MXM Structure) functions are disfunctional. There is also a > MXDS function on both ACPI handles, but these are not hooked to the WMI > interface for some reason. No idea of Acer has hacked up some drivers to > work with this, outside these models I do not know others that are also > affected by this issue. > /me takes a sigh "Why Acer why ..." :-) >> > The previous detection method would fail in this scenario: >> > 1. One device reports support for X and Y (has_x = 1, has_y = 1). Write >> > ACPI handle A to nouveau_dsm_priv.dhandle. >> > 2. Another device reports support for X only (has_x = 1). Write >> > ACPI handle B to nouveau_dsm_priv.dhandle. >> > 3. End result: has_x = 1, has_y = 1, dhandle = B. But ACPI handle B >> > does not really support Y! >> >> ... so to avoid the above case and preserve the original ideas ('do >> not discard earlier device caps' and 'Optimus takes precedence over >> DSM v1') one could do the following: >> >> - decouple the "feature check" and "set the dhandle" >> >> - pick the 'ideal' one based on the feature set provided. if multiple >> pick one based on $insert_heuristics >> - set the dhandle >> >> What do you think ? > > The dhandle is only set when at least one valid DSM was found on the > device. The dhandle assignment could indeed be moved to the caller, > making it more obvious that the dhandle is only valid when the > capabilities are detected (this does not have a functional change > though). I'll do it in the next version. That will be amazing, thanks. Emil
On Mon, May 30, 2016 at 02:20:10PM +0200, Peter Wu wrote: > On Mon, May 30, 2016 at 12:57:09PM +0300, Mika Westerberg wrote: > > +Rafael > > > > On Fri, May 27, 2016 at 01:10:37PM +0200, Peter Wu wrote: > > > On Wed, May 25, 2016 at 04:55:35PM +0300, Mika Westerberg wrote: > > > > On Wed, May 25, 2016 at 12:53:01AM +0200, Peter Wu wrote: > > > > > Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port > > > > > can be runtime-suspended which disables power resources via ACPI. This > > > > > is incompatible with DSM, resulting in a GPU device which is still in D3 > > > > > and locks up the kernel on resume. > > > > > > > > > > Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi > > > > > debugger trace) and stop using the DSM functions for D3cold when power > > > > > resources are available on the parent PCIe port. > > > > > > > > > > [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold > > > > > > > > > > Signed-off-by: Peter Wu <peter@lekensteyn.nl> > > > > > --- > > > > > drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- > > > > > 1 file changed, 30 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > > > index df9f73e..e469df7 100644 > > > > > --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > > > +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c > > > > > @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { > > > > > bool dsm_detected; > > > > > bool optimus_detected; > > > > > bool optimus_flags_detected; > > > > > + bool optimus_skip_dsm; > > > > > acpi_handle dhandle; > > > > > acpi_handle rom_handle; > > > > > } nouveau_dsm_priv; > > > > > @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { > > > > > .get_client_id = nouveau_dsm_get_client_id, > > > > > }; > > > > > > > > > > +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into > > > > > + * D3cold, they instead rely on disabling power resources on the parent. */ > > > > > +static bool nouveau_pr3_present(struct pci_dev *pdev) > > > > > +{ > > > > > + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); > > > > > + struct acpi_device *ad; > > > > > > > > Nit: please call this adev instead of ad. > > > > > > Will do. > > > > > > > > + > > > > > + if (!parent_pdev) > > > > > + return false; > > > > > + > > > > > + ad = ACPI_COMPANION(&parent_pdev->dev); > > > > > + if (!ad) > > > > > + return false; > > > > > + > > > > > + return ad->power.flags.power_resources; > > > > > > > > Is this sufficient to tell if the parent device has _PR3? I thought it > > > > returns true if it has power resources in general, not necessarily _PR3. > > > > > > > > Otherwise this looks okay to me. > > > > > > It is indeed set whenever there is any _PRx method. I wonder if it is > > > appropriate to access fields directly like this, perhaps this would be > > > more accurate (based on device_pm.c): > > > > > > /* Check whether the _PR3 method is available. */ > > > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; > > > > > > I am also considering adding a check in case the pcieport driver does > > > not support D3cold via runtime PM, what do you think of this? > > > > > > if (!parent_pdev) > > > return false; > > > /* If the PCIe port does not support D3cold via runtime PM, allow a > > > * fallback to the Optimus DSM method to put the device in D3cold. */ > > > if (parent_pdev->no_d3cold) > > > return false; > > > > > > This is needed to avoid the regression reported in the cover letter, but > > > also allows pre-2015 systems to (still) have the D3cold possibility. > > > > The _DSM method with 0 as index parameter should return a bit field > > telling which functions are supported. Sane BIOS disables that > > particular function if it detects Windows 8 and newer. Have you checked > > if that's the case? > > > > Then you can call _DSM only if it is supported and otherwise expect the > > parent device's power resources to turn off power when runtime > > suspended. > > The _DSM methods (for the Nvidia device) are often still included and > functions are reported as supported. I guess that vendors just check > whether it is working and do not bother removing legacy functions. The > Acer case below seems exceptional. > > I suggested the no_d3cold check such that DSM can still be called even > though the runtime PM on the PCIe port does nothing. Somehow it does not feel right to poke parent device's fields directly. What if you just check if it has the method like: bool no_dsm = acpi_has_method(parent_adev->handle, "_PR3"); That should follow what Windows is doing. > > > Out of curiosity I looked up an pre-2015 laptop (found Acer V5-573G, > > > apparently from November 2013, Windows 8.1) and extracted the ACPI > > > tables from the BIOS images. BIOS 2.28 (2014/05/13) introduces support > > > for power resources on the parent devicea(\_SB.PCI0.PEG0._PR3 and a > > > related NVP3 device) when _OSI("Windows 2013") is true. (This is added > > > as alternative for the old DSM interface.) > > > > > > Maybe 2014 is also an appropriate cutoff date? I wonder if it is > > > feasible to detect firmware use of _OSI("Windows 2013") and use that > > > instead of the BIOS year. > > > > Using BIOS year works even if there is no ACPI available. > > I thought that you need support from ACPI to put a device in D3cold? It is not just about D3cold but D3 in general (which includes also D3hot). Yes, you need platform support to put the device into D3cold. > > What comes to the cutoff date, I discussed with Rafael and it was > > decided that we use the same year Windows 10 was released to be on the > > safe side. Reading the links you provided here: > > > > https://msdn.microsoft.com/fi-fi/windows/hardware/drivers/bringup/device-power-management > > https://msdn.microsoft.com/en-us/library/windows/hardware/hh967709(v=vs.85).aspx > > > > it seems that from Windows 8 they started transitioning devices into > > D3cold during runtime as well. > > My impression from the ACPI tables I have seen so far is that power > resources support is enabled for Windows 2012 (Win8) or newer.
On Mon, May 30, 2016 at 04:09:09PM +0300, Mika Westerberg wrote: ... > > > > > > + > > > > > > + if (!parent_pdev) > > > > > > + return false; > > > > > > + > > > > > > + ad = ACPI_COMPANION(&parent_pdev->dev); > > > > > > + if (!ad) > > > > > > + return false; > > > > > > + > > > > > > + return ad->power.flags.power_resources; > > > > > > > > > > Is this sufficient to tell if the parent device has _PR3? I thought it > > > > > returns true if it has power resources in general, not necessarily _PR3. > > > > > > > > > > Otherwise this looks okay to me. > > > > > > > > It is indeed set whenever there is any _PRx method. I wonder if it is > > > > appropriate to access fields directly like this, perhaps this would be > > > > more accurate (based on device_pm.c): > > > > > > > > /* Check whether the _PR3 method is available. */ > > > > return adev->power.states[ACPI_STATE_D3_COLD].flags.valid; > > > > > > > > I am also considering adding a check in case the pcieport driver does > > > > not support D3cold via runtime PM, what do you think of this? > > > > > > > > if (!parent_pdev) > > > > return false; > > > > /* If the PCIe port does not support D3cold via runtime PM, allow a > > > > * fallback to the Optimus DSM method to put the device in D3cold. */ > > > > if (parent_pdev->no_d3cold) > > > > return false; > > > > > > > > This is needed to avoid the regression reported in the cover letter, but > > > > also allows pre-2015 systems to (still) have the D3cold possibility. > > > > > > The _DSM method with 0 as index parameter should return a bit field > > > telling which functions are supported. Sane BIOS disables that > > > particular function if it detects Windows 8 and newer. Have you checked > > > if that's the case? > > > > > > Then you can call _DSM only if it is supported and otherwise expect the > > > parent device's power resources to turn off power when runtime > > > suspended. > > > > The _DSM methods (for the Nvidia device) are often still included and > > functions are reported as supported. I guess that vendors just check > > whether it is working and do not bother removing legacy functions. The > > Acer case below seems exceptional. > > > > I suggested the no_d3cold check such that DSM can still be called even > > though the runtime PM on the PCIe port does nothing. > > Somehow it does not feel right to poke parent device's fields directly. > > What if you just check if it has the method like: > > bool no_dsm = acpi_has_method(parent_adev->handle, "_PR3"); > > That should follow what Windows is doing. Checking for _PR3 was the intention, but it seems that the ACPI core does not really store it somewhere. Your check should be simple enough, I'll use that in the next version. Do you have any suggestions for the case where the pcieport driver refuses to put the bridge in D3 (because the BIOS is too old)? In that case the nouveau driver needs to fallback to the DSM method (but not when runtime PM is deliberately disabled by writing control=on).
On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > Do you have any suggestions for the case where the pcieport driver > refuses to put the bridge in D3 (because the BIOS is too old)? In that > case the nouveau driver needs to fallback to the DSM method (but not > when runtime PM is deliberately disabled by writing control=on). Do you know what Windows does then? I think we should do the same if possible. If user has disabled runtime PM from the root port deliberately, there might be good reason to do so. Why we want to fallback to something that could cause problems? I mean _DSM on such systems is probably not that much tested because everybody runs Windows 8+ and using standard ACPI power resources.
On Tue, May 31, 2016 at 11:43:56AM +0300, Mika Westerberg wrote: > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > > Do you have any suggestions for the case where the pcieport driver > > refuses to put the bridge in D3 (because the BIOS is too old)? In that > > case the nouveau driver needs to fallback to the DSM method (but not > > when runtime PM is deliberately disabled by writing control=on). > > Do you know what Windows does then? I think we should do the same if > possible. If the BIOS is too old, then it probably does not have _PR3 objects nor calls to _OSI("Windows 2013"). See below. > If user has disabled runtime PM from the root port deliberately, there > might be good reason to do so. Why we want to fallback to something that > could cause problems? I mean _DSM on such systems is probably not that > much tested because everybody runs Windows 8+ and using standard ACPI > power resources. I agree that when runtime PM on the root port is disabled (control=on), then there should be no fallback to DSM. For devices without _PR3 it is clear that DSM will always be used (if available). In other cases (where _PR3 is available) we can distinguish: - pre-Windows 8 machines. I have never seen this combination. Firmware writers seems to prefer sticking to reference code which did not use power resources before. - Machines targeting Windows 8 or newer. (Note that there exist machines with Windows 8 support that do not have _PR3, DSM is used in that case.) If Windows 7 is running on a Windows 8 machine, PR3 will not be used anyway. If the Linux kernel claims support for Windows 8, but does not use PR3, then we are probably approaching an untested area. So far firmware seems fine with using *only* DSM *or* PR3, but at least my laptop gets confused when you use both at the same time. The latter happens on pci/pm (8b71f565) without other patches: 1. nouveau invokes _DSM and _PS3, device is put in D3cold. 2. pcieport driver calls PG00._OFF (PG00 is returned by _PR3). 3. Wake up Nvidia device (e.g. by power=on). 4. This will trigger PG00._ON (via pcieport) and _PS0 (via nouveau). 5. Nvidia card is not really ready (observed via "restoring config space at offset ... (was 0xffffffff, writing ...)", a soft lockup and RCU stall after that requiring a reboot to recover). nouveau could be patched not to invoke DSM when PR3 is detected (proposal is ready) but will keep the device powered on in these cases: - nouveau is patched, but pci/pm patches are not. - PR3 is supported but due to the cutoff date (2015) it is not used. - Boot option pcie_port_pm=off. - runtime PM is disabled for pcieport (should be fine). There is a wealth of acpidumps on Launchpad bug 752542 (https://bugs.launchpad.net/bugs/752542). Search for example for comments in early 2015 or before, those will likely be machine from 2014 or before. Interesting to see is the _PR3 method of a HP Envy TS 15 (11/20/2014): Method (_PR3, 0, NotSerialized) { If (\_OSI ("Windows 2013")) { Return (Package (0x01) { \NVP3 }) } Else { Return (Package (0x00) {}) } } (Note for self: just checking for the _PR3 handle in the nouveau patch is apparently not sufficient, it must really be evaluated.) Other machines with _PR3: - Dell Inspiron 3543 (11/04/2014), comment 757. - Dell XPS 15 9530 (03/28/2014), comment 711. - Novatech 15.6 NSPIRE Laptop (01/20/2014), comment 695. - Lenovo ThinkPad T440p (10/27/2013), comment 659. There were many models from 2013 without _PR3 method but still checking for _OSI("Windows 2013"). Maybe some heuristics based on _PR3 would be more helpful than just a cutoff date?
On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > Do you have any suggestions for the case where the pcieport driver > refuses to put the bridge in D3 (because the BIOS is too old)? In that > case the nouveau driver needs to fallback to the DSM method (but not > when runtime PM is deliberately disabled by writing control=on). The BIOS cut-off date is meant to avoid issues when suspending ports on older chipsets. However if the port is used for an Optimus GPU and we can clearly identify that, and there's a _PR3 method provided, it's probably safe to say that the port is *intended* to be suspended. So you may want to consider amending pci_bridge_d3_possible() to allow D3 for such ports regardless of the BIOS date, as I've done for Thunderbolt in this commit: https://github.com/l1k/linux/commit/3cb8549cd4e5 Not sure how to uniquely identify such ports though. Perhaps check if there's a device in slot 0 below the port which has (pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY && (pdev->vendor == PCI_VENDOR_ID_NVIDIA || pdev->vendor == PCI_VENDOR_ID_ATI) Best regards, Lukas
On Tue, May 31, 2016 at 01:02:31PM +0200, Peter Wu wrote: > On Tue, May 31, 2016 at 11:43:56AM +0300, Mika Westerberg wrote: > > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > > > Do you have any suggestions for the case where the pcieport driver > > > refuses to put the bridge in D3 (because the BIOS is too old)? In that > > > case the nouveau driver needs to fallback to the DSM method (but not > > > when runtime PM is deliberately disabled by writing control=on). > > > > Do you know what Windows does then? I think we should do the same if > > possible. > > If the BIOS is too old, then it probably does not have _PR3 objects nor > calls to _OSI("Windows 2013"). See below. > > > If user has disabled runtime PM from the root port deliberately, there > > might be good reason to do so. Why we want to fallback to something that > > could cause problems? I mean _DSM on such systems is probably not that > > much tested because everybody runs Windows 8+ and using standard ACPI > > power resources. > > I agree that when runtime PM on the root port is disabled (control=on), > then there should be no fallback to DSM. For devices without _PR3 it is > clear that DSM will always be used (if available). > > In other cases (where _PR3 is available) we can distinguish: > - pre-Windows 8 machines. I have never seen this combination. Firmware > writers seems to prefer sticking to reference code which did not use > power resources before. > - Machines targeting Windows 8 or newer. (Note that there exist > machines with Windows 8 support that do not have _PR3, DSM is used in > that case.) > > If Windows 7 is running on a Windows 8 machine, PR3 will not be used > anyway. If the Linux kernel claims support for Windows 8, but does not > use PR3, then we are probably approaching an untested area. So far > firmware seems fine with using *only* DSM *or* PR3, but at least my > laptop gets confused when you use both at the same time. > > The latter happens on pci/pm (8b71f565) without other patches: > > 1. nouveau invokes _DSM and _PS3, device is put in D3cold. > 2. pcieport driver calls PG00._OFF (PG00 is returned by _PR3). > 3. Wake up Nvidia device (e.g. by power=on). > 4. This will trigger PG00._ON (via pcieport) and _PS0 (via nouveau). > 5. Nvidia card is not really ready (observed via "restoring config > space at offset ... (was 0xffffffff, writing ...)", a soft lockup > and RCU stall after that requiring a reboot to recover). > > nouveau could be patched not to invoke DSM when PR3 is detected > (proposal is ready) but will keep the device powered on in these cases: > - nouveau is patched, but pci/pm patches are not. > - PR3 is supported but due to the cutoff date (2015) it is not used. > - Boot option pcie_port_pm=off. > - runtime PM is disabled for pcieport (should be fine). Since using only _DSM has been the only method to power down the card currently inńLinux (even if the root port has had _PR3), and it has been working fine, why not stick with that when _DSM is supported? In other words, something like this: nouveau_dsm_pci_probe() { ... if (retval & (NOUVEAU_DSM_HAS_OPT | NOUVEAU_DSM_HAS_MUX)) { /* * We have custom _DSM method to power down the card so * prevent the PCI core from transitioning the * card into D3cold. */ pci_d3cold_disable(pdev); } } (Not sure about those flags above, though). Yes, it does not follow Windows 8+ but if it works... ;-) > There is a wealth of acpidumps on Launchpad bug 752542 > (https://bugs.launchpad.net/bugs/752542). Search for example for > comments in early 2015 or before, those will likely be machine from 2014 > or before. > > Interesting to see is the _PR3 method of a HP Envy TS 15 (11/20/2014): > > Method (_PR3, 0, NotSerialized) { > If (\_OSI ("Windows 2013")) { > Return (Package (0x01) { > \NVP3 > }) > } Else { > Return (Package (0x00) {}) > } > } > > (Note for self: just checking for the _PR3 handle in the nouveau patch > is apparently not sufficient, it must really be evaluated.) > > Other machines with _PR3: > - Dell Inspiron 3543 (11/04/2014), comment 757. > - Dell XPS 15 9530 (03/28/2014), comment 711. > - Novatech 15.6 NSPIRE Laptop (01/20/2014), comment 695. > - Lenovo ThinkPad T440p (10/27/2013), comment 659. > > There were many models from 2013 without _PR3 method but still checking > for _OSI("Windows 2013"). Maybe some heuristics based on _PR3 would be > more helpful than just a cutoff date? You mean for allowing bridge_d3? I don't think checking _PR3 helps us in any way. We can put PCIe port into D3hot just fine without any help from ACPI. Only thing that matters here is that we should be able to do that safely without causing problems to hardware which does not support it properly.
On Tue, May 31, 2016 at 02:20:26PM +0200, Lukas Wunner wrote: > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > > Do you have any suggestions for the case where the pcieport driver > > refuses to put the bridge in D3 (because the BIOS is too old)? In that > > case the nouveau driver needs to fallback to the DSM method (but not > > when runtime PM is deliberately disabled by writing control=on). > > The BIOS cut-off date is meant to avoid issues when suspending ports > on older chipsets. However if the port is used for an Optimus GPU > and we can clearly identify that, and there's a _PR3 method provided, > it's probably safe to say that the port is *intended* to be suspended. > > So you may want to consider amending pci_bridge_d3_possible() to > allow D3 for such ports regardless of the BIOS date, as I've done > for Thunderbolt in this commit: > https://github.com/l1k/linux/commit/3cb8549cd4e5 Then we have heuristics based on BIOS year, on whether it is TB or not, and next to it whether it is an Optimus laptop? Maybe the PCI core needs to export a function that allows drivers to override the detection if this becomes more common. > Not sure how to uniquely identify such ports though. Perhaps check > if there's a device in slot 0 below the port which has > (pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY && > (pdev->vendor == PCI_VENDOR_ID_NVIDIA || > pdev->vendor == PCI_VENDOR_ID_ATI) Seems fragile, there are desktop setups satisfying this match.
On Wed, Jun 01, 2016 at 12:28:47PM +0300, Mika Westerberg wrote: > On Tue, May 31, 2016 at 01:02:31PM +0200, Peter Wu wrote: > > On Tue, May 31, 2016 at 11:43:56AM +0300, Mika Westerberg wrote: > > > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > > > > Do you have any suggestions for the case where the pcieport driver > > > > refuses to put the bridge in D3 (because the BIOS is too old)? In that > > > > case the nouveau driver needs to fallback to the DSM method (but not > > > > when runtime PM is deliberately disabled by writing control=on). > > > > > > Do you know what Windows does then? I think we should do the same if > > > possible. > > > > If the BIOS is too old, then it probably does not have _PR3 objects nor > > calls to _OSI("Windows 2013"). See below. > > > > > If user has disabled runtime PM from the root port deliberately, there > > > might be good reason to do so. Why we want to fallback to something that > > > could cause problems? I mean _DSM on such systems is probably not that > > > much tested because everybody runs Windows 8+ and using standard ACPI > > > power resources. > > > > I agree that when runtime PM on the root port is disabled (control=on), > > then there should be no fallback to DSM. For devices without _PR3 it is > > clear that DSM will always be used (if available). > > > > In other cases (where _PR3 is available) we can distinguish: > > - pre-Windows 8 machines. I have never seen this combination. Firmware > > writers seems to prefer sticking to reference code which did not use > > power resources before. > > - Machines targeting Windows 8 or newer. (Note that there exist > > machines with Windows 8 support that do not have _PR3, DSM is used in > > that case.) > > > > If Windows 7 is running on a Windows 8 machine, PR3 will not be used > > anyway. If the Linux kernel claims support for Windows 8, but does not > > use PR3, then we are probably approaching an untested area. So far > > firmware seems fine with using *only* DSM *or* PR3, but at least my > > laptop gets confused when you use both at the same time. > > > > The latter happens on pci/pm (8b71f565) without other patches: > > > > 1. nouveau invokes _DSM and _PS3, device is put in D3cold. > > 2. pcieport driver calls PG00._OFF (PG00 is returned by _PR3). > > 3. Wake up Nvidia device (e.g. by power=on). > > 4. This will trigger PG00._ON (via pcieport) and _PS0 (via nouveau). > > 5. Nvidia card is not really ready (observed via "restoring config > > space at offset ... (was 0xffffffff, writing ...)", a soft lockup > > and RCU stall after that requiring a reboot to recover). > > > > nouveau could be patched not to invoke DSM when PR3 is detected > > (proposal is ready) but will keep the device powered on in these cases: > > - nouveau is patched, but pci/pm patches are not. > > - PR3 is supported but due to the cutoff date (2015) it is not used. > > - Boot option pcie_port_pm=off. > > - runtime PM is disabled for pcieport (should be fine). > > Since using only _DSM has been the only method to power down the card > currently inńLinux (even if the root port has had _PR3), and it has been > working fine, why not stick with that when _DSM is supported? Maybe it is not really working, people have been reporting memory corruption[1] for example on certain Lenovo models that was gone after hacking the bbswitch module to disable the root port: https://bugs.freedesktop.org/show_bug.cgi?id=78530 https://github.com/Bumblebee-Project/bbswitch/issues/78 https://github.com/Bumblebee-Project/bbswitch/issues/115 I'll try to solicit some feedback from the affected people on these patch series, whether it solves their memory corruption issue. Dave also said "This fixes GPU auto powerdown on the Lenovo W541," when he added PR3 support in https://patchwork.freedesktop.org/patch/76313/ So apparently it did not work with just DSM. > In other words, something like this: > > nouveau_dsm_pci_probe() > { > ... > if (retval & (NOUVEAU_DSM_HAS_OPT | NOUVEAU_DSM_HAS_MUX)) { > /* > * We have custom _DSM method to power down the card so > * prevent the PCI core from transitioning the > * card into D3cold. > */ > pci_d3cold_disable(pdev); > } > } > > (Not sure about those flags above, though). > > Yes, it does not follow Windows 8+ but if it works... ;-) > > > There is a wealth of acpidumps on Launchpad bug 752542 > > (https://bugs.launchpad.net/bugs/752542). Search for example for > > comments in early 2015 or before, those will likely be machine from 2014 > > or before. > > > > Interesting to see is the _PR3 method of a HP Envy TS 15 (11/20/2014): > > > > Method (_PR3, 0, NotSerialized) { > > If (\_OSI ("Windows 2013")) { > > Return (Package (0x01) { > > \NVP3 > > }) > > } Else { > > Return (Package (0x00) {}) > > } > > } > > > > (Note for self: just checking for the _PR3 handle in the nouveau patch > > is apparently not sufficient, it must really be evaluated.) > > > > Other machines with _PR3: > > - Dell Inspiron 3543 (11/04/2014), comment 757. > > - Dell XPS 15 9530 (03/28/2014), comment 711. > > - Novatech 15.6 NSPIRE Laptop (01/20/2014), comment 695. > > - Lenovo ThinkPad T440p (10/27/2013), comment 659. > > > > There were many models from 2013 without _PR3 method but still checking > > for _OSI("Windows 2013"). Maybe some heuristics based on _PR3 would be > > more helpful than just a cutoff date? > > You mean for allowing bridge_d3? I don't think checking _PR3 helps us in > any way. We can put PCIe port into D3hot just fine without any help from > ACPI. Only thing that matters here is that we should be able to do that > safely without causing problems to hardware which does not support it > properly. Currently bridge_d3 will always be false when pci_bridge_d3_possible fails (that is, on BIOSes older than 2015). The idea is to add an additional whitelist condition when _PR3 exists and _OSI("Windows 2013") is requested in ACPI. This should help modern Nvidia Optimus laptops which rely on ACPI to save power.
On Wed, Jun 01, 2016 at 06:51:51PM +0200, Peter Wu wrote: > On Tue, May 31, 2016 at 02:20:26PM +0200, Lukas Wunner wrote: > > On Mon, May 30, 2016 at 06:13:51PM +0200, Peter Wu wrote: > > > Do you have any suggestions for the case where the pcieport driver > > > refuses to put the bridge in D3 (because the BIOS is too old)? In that > > > case the nouveau driver needs to fallback to the DSM method (but not > > > when runtime PM is deliberately disabled by writing control=on). > > > > The BIOS cut-off date is meant to avoid issues when suspending ports > > on older chipsets. However if the port is used for an Optimus GPU > > and we can clearly identify that, and there's a _PR3 method provided, > > it's probably safe to say that the port is *intended* to be suspended. > > > > So you may want to consider amending pci_bridge_d3_possible() to > > allow D3 for such ports regardless of the BIOS date, as I've done > > for Thunderbolt in this commit: > > https://github.com/l1k/linux/commit/3cb8549cd4e5 > > Then we have heuristics based on BIOS year, on whether it is TB or not, > and next to it whether it is an Optimus laptop? Maybe the PCI core needs > to export a function that allows drivers to override the detection if > this becomes more common. Well I consider the TB and Optimus whitelisting as a stop-gap until the BIOS date is lowered. Rafael wrote: Some time around when machines with Windows 10 started to ship should be relatively safe. I guess we can just pick a reasonable date in the initial patch and then try to move it back to the past subsequently and see if that breaks things for anyone. Source: http://permalink.gmane.org/gmane.linux.power-management.general/75133 > > > Not sure how to uniquely identify such ports though. Perhaps check > > if there's a device in slot 0 below the port which has > > (pdev->class >> 16) == PCI_BASE_CLASS_DISPLAY && > > (pdev->vendor == PCI_VENDOR_ID_NVIDIA || > > pdev->vendor == PCI_VENDOR_ID_ATI) > > Seems fragile, there are desktop setups satisfying this match. Of course, I didn't mean this to be used as is, you'd have to augment this with checks e.g. for presence of _PR3 and (if possible) Optimus, but I'm not familiar enough with Optimus to write down working code for it, I'm only familiar with apple-gmux switching. Best regards, Lukas
diff --git a/drivers/gpu/drm/nouveau/nouveau_acpi.c b/drivers/gpu/drm/nouveau/nouveau_acpi.c index df9f73e..e469df7 100644 --- a/drivers/gpu/drm/nouveau/nouveau_acpi.c +++ b/drivers/gpu/drm/nouveau/nouveau_acpi.c @@ -46,6 +46,7 @@ static struct nouveau_dsm_priv { bool dsm_detected; bool optimus_detected; bool optimus_flags_detected; + bool optimus_skip_dsm; acpi_handle dhandle; acpi_handle rom_handle; } nouveau_dsm_priv; @@ -212,8 +213,26 @@ static const struct vga_switcheroo_handler nouveau_dsm_handler = { .get_client_id = nouveau_dsm_get_client_id, }; +/* Firmware supporting Windows 8 or later do not use _DSM to put the device into + * D3cold, they instead rely on disabling power resources on the parent. */ +static bool nouveau_pr3_present(struct pci_dev *pdev) +{ + struct pci_dev *parent_pdev = pci_upstream_bridge(pdev); + struct acpi_device *ad; + + if (!parent_pdev) + return false; + + ad = ACPI_COMPANION(&parent_pdev->dev); + if (!ad) + return false; + + return ad->power.flags.power_resources; +} + static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, - bool *has_opt, bool *has_opt_flags) + bool *has_opt, bool *has_opt_flags, + bool *has_power_resources) { acpi_handle dhandle; bool supports_mux; @@ -238,6 +257,7 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, *has_mux = supports_mux; *has_opt = !!optimus_funcs; *has_opt_flags = optimus_funcs & (1 << NOUVEAU_DSM_OPTIMUS_FLAGS); + *has_power_resources = false; if (optimus_funcs) { uint32_t result; @@ -247,6 +267,8 @@ static void nouveau_dsm_pci_probe(struct pci_dev *pdev, bool *has_mux, (result & OPTIMUS_ENABLED) ? "enabled" : "disabled", (result & OPTIMUS_DYNAMIC_PWR_CAP) ? "dynamic power, " : "", (result & OPTIMUS_HDA_CODEC_MASK) ? "hda bios codec supported" : ""); + + *has_power_resources = nouveau_pr3_present(pdev); } } @@ -258,6 +280,7 @@ static bool nouveau_dsm_detect(void) bool has_mux = false; bool has_optimus = false; bool has_optimus_flags = false; + bool has_power_resources = false; int vga_count = 0; bool guid_valid; bool ret = false; @@ -273,14 +296,14 @@ static bool nouveau_dsm_detect(void) vga_count++; nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, - &has_optimus_flags); + &has_optimus_flags, &has_power_resources); } while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_3D << 8, pdev)) != NULL) { vga_count++; nouveau_dsm_pci_probe(pdev, &has_mux, &has_optimus, - &has_optimus_flags); + &has_optimus_flags, &has_power_resources); } /* find the optimus DSM or the old v1 DSM */ @@ -289,8 +312,11 @@ static bool nouveau_dsm_detect(void) &buffer); printk(KERN_INFO "VGA switcheroo: detected Optimus DSM method %s handle\n", acpi_method_name); + if (has_power_resources) + pr_info("nouveau: detected PR support, will not use DSM\n"); nouveau_dsm_priv.optimus_detected = true; nouveau_dsm_priv.optimus_flags_detected = has_optimus_flags; + nouveau_dsm_priv.optimus_skip_dsm = has_power_resources; ret = true; } else if (vga_count == 2 && has_mux && guid_valid) { acpi_get_name(nouveau_dsm_priv.dhandle, ACPI_FULL_PATHNAME, @@ -320,7 +346,7 @@ void nouveau_register_dsm_handler(void) void nouveau_switcheroo_optimus_dsm(void) { u32 result = 0; - if (!nouveau_dsm_priv.optimus_detected) + if (!nouveau_dsm_priv.optimus_detected || nouveau_dsm_priv.optimus_skip_dsm) return; if (nouveau_dsm_priv.optimus_flags_detected)
Since "PCI: Add runtime PM support for PCIe ports", the parent PCIe port can be runtime-suspended which disables power resources via ACPI. This is incompatible with DSM, resulting in a GPU device which is still in D3 and locks up the kernel on resume. Mirror the behavior of Windows 8 and newer[1] (as observed via an AMLi debugger trace) and stop using the DSM functions for D3cold when power resources are available on the parent PCIe port. [1]: https://msdn.microsoft.com/windows/hardware/drivers/bringup/firmware-requirements-for-d3cold Signed-off-by: Peter Wu <peter@lekensteyn.nl> --- drivers/gpu/drm/nouveau/nouveau_acpi.c | 34 ++++++++++++++++++++++++++++++---- 1 file changed, 30 insertions(+), 4 deletions(-)