[PATCH 0/4] nouveau fixes for RPM/Optimus-related hangs

[PATCH 0/4] nouveau fixes for RPM/Optimus-related hangs

[Peter Wu]

Hi,

Here are two patches to fix an issue reported on kernel bugzilla (infinite loop
due to unchecked function) and a more important fix to fix hanging Optimus
machines when runtime PM is enabled (with pm/pci patches).

An older (obsolete) patch for the first issue was tested by the reporter:
https://bugzilla.kernel.org/show_bug.cgi?id=104791#c11
(it is replaced by "check for function 0x1B before using it").

The second issue will occur when:
 - A modern Optimus laptop is in use (designed for Windows 8 or newer).
 - nouveau runtime PM is enabled (1 or the default -1).
 - The patch "PCI: Add runtime PM support for PCIe ports" from Mika is pulled
   into v4.7 (or v4.8[1]?) via the pci/pm branch,
   https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pm&id=8b71f5652eeac561acf883da01ab4810f763ee42
(see also the discussion for "[PATCH] PCI: Power on bridges before scanning new
devices" at http://article.gmane.org/gmane.linux.power-management.general/76411)

The first two patches are just refactoring to reduce code duplication (and
scratch an itch) and make the following patches possible. The next two patches
fix the problems reported above.

I intend to get these patches in 4.7 (or the first version where pci/pm gets
merged) to avoid a lockup when runpm is enabled. Note:
 - If the fourth patch is merged before/without Mika's PCIe port patch, then
   those modern Optimus machines above will not be put into D3cold.
 - If the fourth patch is not merged (or merged after Mika's patch), then under
   the above conditions the affected machine can lock up.
 - The three other patches are unrelated to this issue and can safely be merged.

Tested with:
 - Linux v4.6 + pci/pm + these four patches
 - Hardware: Clevo P651RA with acpi_osi="!Windows 2015" (the latter is a
   workaround for another PCIe issue).
 - Card is asleep, woke up with lspci, waited a bit and retried/suspended:
   - # lspci -xxxxnnvvvv >/dev/null; sleep 5
   - # lspci -xxxxnnvvvv >/dev/null; sleep 5; systemctl suspend
   - # lspci -xxxxnnvvvv >/dev/null; systemctl suspend

Kind regards,
Peter

 [1]: https://lkml.kernel.org/r/20160524211309.GH1789@lahna.fi.intel.com

Peter Wu (4):
  drm/nouveau/acpi: ensure matching ACPI handle and supported functions
  drm/nouveau/acpi: return supported DSM functions
  drm/nouveau/acpi: check for function 0x1B before using it
  drm/nouveau/acpi: fix lockup with PCIe runtime PM

 drivers/gpu/drm/nouveau/nouveau_acpi.c | 100 +++++++++++++++++++++------------
 1 file changed, 63 insertions(+), 37 deletions(-)

-- 
2.8.2

[PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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);
 	}
 
 	/* 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)
-- 
2.8.2

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Mika Westerberg]

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.

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Hans de Goede]

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

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Mika Westerberg]

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

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Mika Westerberg]

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.

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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).
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Mika Westerberg]

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.

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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?
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Mika Westerberg]

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.

Re: [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Lukas Wunner]

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

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Lukas Wunner]

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

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Emil Velikov]

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

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Lukas Wunner]

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

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Emil Velikov]

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

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Peter Wu]

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.
-- 
Kind regards,
Peter Wu
https://lekensteyn.nl

Re: [Nouveau] [PATCH 4/4] drm/nouveau/acpi: fix lockup with PCIe runtime PM

[Emil Velikov]

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

Re: [Nouveau] [PATCH 0/4] nouveau fixes for RPM/Optimus-related hangs

[Hans de Goede]

Hi,

On 25-05-16 00:52, Peter Wu wrote:
> Hi,
>
> Here are two patches to fix an issue reported on kernel bugzilla (infinite loop
> due to unchecked function) and a more important fix to fix hanging Optimus
> machines when runtime PM is enabled (with pm/pci patches).
>
> An older (obsolete) patch for the first issue was tested by the reporter:
> https://bugzilla.kernel.org/show_bug.cgi?id=104791#c11
> (it is replaced by "check for function 0x1B before using it").
>
> The second issue will occur when:
>  - A modern Optimus laptop is in use (designed for Windows 8 or newer).
>  - nouveau runtime PM is enabled (1 or the default -1).
>  - The patch "PCI: Add runtime PM support for PCIe ports" from Mika is pulled
>    into v4.7 (or v4.8[1]?) via the pci/pm branch,
>    https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?h=pci/pm&id=8b71f5652eeac561acf883da01ab4810f763ee42
> (see also the discussion for "[PATCH] PCI: Power on bridges before scanning new
> devices" at http://article.gmane.org/gmane.linux.power-management.general/76411)
>
> The first two patches are just refactoring to reduce code duplication (and
> scratch an itch) and make the following patches possible. The next two patches
> fix the problems reported above.
>
> I intend to get these patches in 4.7 (or the first version where pci/pm gets
> merged) to avoid a lockup when runpm is enabled. Note:
>  - If the fourth patch is merged before/without Mika's PCIe port patch, then
>    those modern Optimus machines above will not be put into D3cold.
>  - If the fourth patch is not merged (or merged after Mika's patch), then under
>    the above conditions the affected machine can lock up.
>  - The three other patches are unrelated to this issue and can safely be merged.
>
> Tested with:
>  - Linux v4.6 + pci/pm + these four patches
>  - Hardware: Clevo P651RA with acpi_osi="!Windows 2015" (the latter is a
>    workaround for another PCIe issue).
>  - Card is asleep, woke up with lspci, waited a bit and retried/suspended:
>    - # lspci -xxxxnnvvvv >/dev/null; sleep 5
>    - # lspci -xxxxnnvvvv >/dev/null; sleep 5; systemctl suspend
>    - # lspci -xxxxnnvvvv >/dev/null; systemctl suspend

Series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





>
> Kind regards,
> Peter
>
>  [1]: https://lkml.kernel.org/r/20160524211309.GH1789@lahna.fi.intel.com
>
> Peter Wu (4):
>   drm/nouveau/acpi: ensure matching ACPI handle and supported functions
>   drm/nouveau/acpi: return supported DSM functions
>   drm/nouveau/acpi: check for function 0x1B before using it
>   drm/nouveau/acpi: fix lockup with PCIe runtime PM
>
>  drivers/gpu/drm/nouveau/nouveau_acpi.c | 100 +++++++++++++++++++++------------
>  1 file changed, 63 insertions(+), 37 deletions(-)
>