diff mbox

Acer Aspire V7-582PG (Haswell, GTX 750M) fails to power off GPU via Power Resources

Message ID 20161028123041.GB1521@al
State Not Applicable
Headers show

Commit Message

Peter Wu Oct. 28, 2016, 12:30 p.m. UTC 
On Fri, Oct 28, 2016 at 02:19:07PM +0300, Mika Westerberg wrote:
> On Fri, Oct 28, 2016 at 01:09:30PM +0200, Peter Wu wrote:
> > On Fri, Oct 28, 2016 at 11:56:30AM +0300, Mika Westerberg wrote:
> > > On Thu, Oct 27, 2016 at 06:06:48PM +0200, Peter Wu wrote:
> > > > On Thu, Oct 27, 2016 at 12:55:08PM +0300, Mika Westerberg wrote:
> > > > > On Thu, Oct 27, 2016 at 09:42:28AM +0000, Rick Kerkhof wrote:
> > > > > >    No, there are not. Here is the recursive directory listing:
> > > > > 
> > > > > Are you able to try the following patch and send me dmesg (or attach it
> > > > > to that bug)? It should show if the ACPI core even tries to add those
> > > > > power resources.
> > > > 
> > > > So Rick has tested this patch now on top of 4.8.4 (mainline fails to
> > > > boot due to a kbuild issue which I reported elsewhere), but the output
> > > > is empty. That seems to indicate that flags.power_resources is unset.
> > > 
> > > Is it completely empty or is it empty just for RP05? It should print out
> > > all devices with power resources.
> > 
> > \NVP2 and \NVP3 are the only power resources under RP05 and defined in
> > SSDT1, there are no others.
> 
> We should probably add a debug print before checking
> flags.power_resources just to be sure the patch is correctly applied.

It was correctly applied. I did some testing with QEMU, it seems that
the \_OSI check is problematic. Removing it makes things work again.

To reproduce:

 1. Build the kernel with the attached config (minconfig with ACPI and
    printk stuff enabled) and debug patch.
 2. Compile the attached ASL file to AML (iasl ssdt1.dsl).
    (You can remove the If(\_OSI(...)) check to see the difference.
 3. Boot QEMU and watch the dmesg for the debug prints.

Alternatively, just boot QEMU with this SSDT and check against any Linux
distro and inspect /sys/bus/pci/devices/0000:00:01.3/firmware_node/.

Tested with Linux v4.8.5, QEMU 2.7.0, iasl 20160831. The SSDT structure
is adapted from Ricks SSDT1 file and modified for the 00:01.3 device in
QEMU. The QEMU command I used was:

    qemu-system-x86_64 -m 1G -M pc,accel=kvm -nographic \
        -kernel arch/x86/boot/bzImage -serial file:/dev/stdout \
        -acpitable file=ssdt1.aml \
        -append 'console=ttyS0 acpi.aml_debug_output=1'

Comments

Mika Westerberg Oct. 28, 2016, 2:10 p.m. UTC | #1 
On Fri, Oct 28, 2016 at 02:30:41PM +0200, Peter Wu wrote:
> It was correctly applied. I did some testing with QEMU, it seems that
> the \_OSI check is problematic. Removing it makes things work again.

I hope Bob and Lv can answer why _OSI fails.

In the meantime I think we should check flags.power_resources in nouveau
driver (in addition to _PR3) so that it falls back to _DSM if there are
no power resources (or if we failed to evaluate them for some reason).
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng Oct. 29, 2016, 12:49 a.m. UTC | #2 
Hi,

> From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Mika
> Westerberg
> Subject: Re: Acer Aspire V7-582PG (Haswell, GTX 750M) fails to power off GPU via Power Resources
> 
> On Fri, Oct 28, 2016 at 02:30:41PM +0200, Peter Wu wrote:
> > It was correctly applied. I did some testing with QEMU, it seems that
> > the \_OSI check is problematic. Removing it makes things work again.
> 
> I hope Bob and Lv can answer why _OSI fails.
> 
> In the meantime I think we should check flags.power_resources in nouveau
> driver (in addition to _PR3) so that it falls back to _DSM if there are
> no power resources (or if we failed to evaluate them for some reason).

IMO, the problem wasn't _OSI, the problem was "If".
"If" is module level here.
So execution order of it in current Linux upstream may be different from Windows.

You can try to modify acpi_gbl_parse_table_as_term_list to TRUE.
To see if this can be solved.

Thanks and best regards
Lv

> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Wu Oct. 30, 2016, 2:18 p.m. UTC | #3 
(removing nouveau list since it is an ACPI core issue)

On Sat, Oct 29, 2016 at 12:49:22AM +0000, Zheng, Lv wrote:
> Hi,
> 
> > From: linux-acpi-owner@vger.kernel.org [mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Mika
> > Westerberg
> > Subject: Re: Acer Aspire V7-582PG (Haswell, GTX 750M) fails to power off GPU via Power Resources
> > 
> > On Fri, Oct 28, 2016 at 02:30:41PM +0200, Peter Wu wrote:
> > > It was correctly applied. I did some testing with QEMU, it seems that
> > > the \_OSI check is problematic. Removing it makes things work again.
> > 
> > I hope Bob and Lv can answer why _OSI fails.
> > 
> > In the meantime I think we should check flags.power_resources in nouveau
> > driver (in addition to _PR3) so that it falls back to _DSM if there are
> > no power resources (or if we failed to evaluate them for some reason).
> 
> IMO, the problem wasn't _OSI, the problem was "If".
> "If" is module level here.
> So execution order of it in current Linux upstream may be different from Windows.
> 
> You can try to modify acpi_gbl_parse_table_as_term_list to TRUE.
> To see if this can be solved.
> 
> Thanks and best regards
> Lv

It seems that acpi_gbl_parse_table_as_term_list is a new flag in
v4.9-rc1. Changing the flag to TRUE in in include/acpi/acpixf.h has no
effect other than:

    ACPI: Executed 2 blocks of module-level executable AML code

changing into:


    ACPI: 2 ACPI AML tables successfully acquired and loaded

That was tested using v4.9-rc2-40-g9fe68ca + debug patch from previous
mail, but with a slightly modified QEMU command (to allow the device to
become visible in Windows Device Manager) and slightly modified SSDT1:

    qemu-system-x86_64 -M pc,accel=kvm -m 2G -acpitable file=ssdt1.aml \
        -net none -device pci-bridge,addr=12.0,chassis_nr=2,id=head.2 \
        -device isa-debugcon,iobase=0x402,chardev=dbug \
        -chardev file,id=dbug,path=/dev/stdout ...

With this command and Windows 10 (-drive file=w10.qcow2), I can see that
the power resource methods are invoked (stdout | grep PR_TEST):

    PR_TEST: NVP3._ON
    PR_TEST: NVP2._OFF
    PR_TEST: _PS0
    PR_TEST: _S0W
    PR_TEST: NVP3._ON
    PR_TEST: _PS0

Using the above test setup with Linux (-kernel arch/x86/boot/bzImage):

    PR_TEST: _S0W

If you think that the default runtime PM state affects this, note that
the dmesg also shows:

    ACPI: \_SB_.PCI0.S90_: Adding power resources for device:14? 0
diff mbox

Patch

diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
index fcd4ce6..64d6308 100644
--- a/drivers/acpi/power.c
+++ b/drivers/acpi/power.c
@@ -143,20 +143,26 @@  int acpi_extract_power_resources(union acpi_object *package, unsigned int start,
 
 		if (element->type != ACPI_TYPE_LOCAL_REFERENCE) {
 			err = -ENODATA;
+			pr_warn("ACPI: Unsupported element type: %d\n", element->type);
 			break;
 		}
 		rhandle = element->reference.handle;
 		if (!rhandle) {
 			err = -ENODEV;
+			pr_warn("ACPI: referenced handle not found!\n");
 			break;
 		}
 		err = acpi_add_power_resource(rhandle);
-		if (err)
+		if (err) {
+			acpi_handle_warn(rhandle, "acpi_add_power_resource failed!\n");
 			break;
+		}
 
 		err = acpi_power_resources_list_add(rhandle, list);
-		if (err)
+		if (err) {
+			acpi_handle_warn(rhandle, "acpi_power_resources_list_add failed!\n");
 			break;
+		}
 	}
 	if (err)
 		acpi_power_resources_list_free(list);
@@ -441,6 +447,9 @@  void acpi_power_add_remove_device(struct acpi_device *adev, bool add)
 		acpi_power_expose_hide(adev, &adev->wakeup.resources,
 				       &wakeup_attr_group, add);
 
+	acpi_handle_info(adev->handle, "Adding power resources for %s? %d\n",
+			 dev_name(&adev->dev), adev->power.flags.power_resources);
+
 	if (!adev->power.flags.power_resources)
 		return;
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index e878fc7..d71673d 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -930,6 +930,9 @@  static void acpi_bus_init_power_state(struct acpi_device *device, int state)
 		    && package->package.count) {
 			int err = acpi_extract_power_resources(package, 0,
 							       &ps->resources);
+			acpi_handle_info(device->handle,
+					 "acpi_extract_power_resources result for %s: %d\n",
+					 dev_name(&device->dev), err);
 			if (!err)
 				device->power.flags.power_resources = 1;
 		}