[PATCH] drm/i915: opregion: set opregion chpd value to indicate the driver handles hotplug

* [PATCH] drm/i915: opregion: set opregion chpd value to indicate the driver handles hotplug
@ 2019-11-22 19:04 ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2019-11-22 19:04 UTC (permalink / raw)
  To: Maarten Lankhorst, Jani Nikula, Joonas Lahtinen,
	Ville Syrjälä,
	Rodrigo Vivi
  Cc: intel-gfx, dri-devel

According to both the old acpi_igd_opregion_spec_0.pdf and the newer
skl_opregion_rev0p5.pdf opregion specification documents, if a driver
handles hotplug events itself, it should set the opregion CHPD field to
1 to indicate this and the firmware should respond to this by no longer
sending ACPI 0x00 notification events on e.g. lid-state changes.

Specifically skl_opregion_rev0p5.pdf states thid in the documentation of
the CHPD word: "Re-enumeration trigger logic in System BIOS MUST be
disabled for all the Operating Systems supporting Hot-Plug
(e.g., Windows* Longhorn and above)." Note the MUST in there.

We ignore these notifications, so this should not be a problem but many
recent DSTDs seem to all have the same copy-pasted bug in the GNOT() AML
function which is used to send these notifications. Windows likely does not
hit this bug as it presumably correcty sets CHPD to 1.

Here is an example of the broken GNOT() method:

            Method (GNOT, 2, NotSerialized)
            {
                ...
                CEVT = Arg0
                CSTS = 0x03
                If (((CHPD == Zero) && (Arg1 == Zero)))
                {
                    If (((OSYS > 0x07D0) || (OSYS < 0x07D6)))
                    {
                        Notify (PCI0, Arg1)
                    }
                    Else
                    {
                        Notify (GFX0, Arg1)
                    }
                }
                ...

Notice that the condition for the If is always true I believe that the
|| like needs to be an &&, but there is nothing we can do about this and
in my own DSDT archive 55 of the 93 DSDTs have this issue.

When the if is true the notification gets send to the PCI root instead
of only to the GFX0 device. This causes Linux to re-enumerate PCI devices
whenever the LID opens / closes, leading to unexpected messages in dmesg:

Suspend through lid close:
[  313.598199] intel_atomisp2_pm 0000:00:03.0: Refused to change power state, currently in D3
[  313.664453] intel_atomisp2_pm 0000:00:03.0: Refused to change power state, currently in D3
[  313.737982] pci_bus 0000:01: Allocating resources
[  313.738036] pcieport 0000:00:1c.0: bridge window [io  0x1000-0x0fff] to [bus 01] add_size 1000
[  313.738051] pcieport 0000:00:1c.0: bridge window [mem 0x00100000-0x000fffff 64bit pref] to [bus 01] add_size 200000 add_align 100000
[  313.738111] pcieport 0000:00:1c.0: BAR 15: assigned [mem 0x91000000-0x911fffff 64bit pref]
[  313.738128] pcieport 0000:00:1c.0: BAR 13: assigned [io  0x1000-0x1fff]

Resume:
[  813.623894] pci 0000:00:03.0: [8086:22b8] type 00 class 0x048000
[  813.623955] pci 0000:00:03.0: reg 0x10: [mem 0x00000000-0x003fffff]
[  813.630477] pci 0000:00:03.0: BAR 0: assigned [mem 0x91c00000-0x91ffffff]
[  854.579101] intel_atomisp2_pm 0000:00:03.0: Refused to change power state, currently in D3

And more importantly this re-enumeration races with suspend/resume causing
enumeration to not be complete  when assert_isp_power_gated() from
drivers/gpu/drm/i915/display/intel_display_power.c runs. This causes
the !pci_dev_present(isp_ids) check in assert_isp_power_gated() to fail
making the condition for the WARN true, leading to:

[  813.327886] ------------[ cut here ]------------
[  813.327898] ISP not power gated
[  813.328028] WARNING: CPU: 2 PID: 2317 at drivers/gpu/drm/i915/display/intel_display_power.c:4870 intel_display_print_error_state+0x2b98/0x3a80 [i915]
...
[  813.328599] ---[ end trace f01e81b599596774 ]---

This commit fixes the unwanted ACPI notification on the PCI root device
by setting CHPD to 1, so that the broken if condition in the AML never
gets checked as notifications of type 0x00 are disabled altogether.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/display/intel_opregion.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
index 969ade623691..e59b4992ba1b 100644
--- a/drivers/gpu/drm/i915/display/intel_opregion.c
+++ b/drivers/gpu/drm/i915/display/intel_opregion.c
@@ -941,6 +941,13 @@ int intel_opregion_setup(struct drm_i915_private *dev_priv)
 	if (mboxes & MBOX_ACPI) {
 		DRM_DEBUG_DRIVER("Public ACPI methods supported\n");
 		opregion->acpi = base + OPREGION_ACPI_OFFSET;
+		/*
+		 * Indicate we handle monitor hotplug events ourselves so we do
+		 * not need ACPI notifications for them. Disabling these avoids
+		 * triggering the AML code doing the notifation, which may be
+		 * broken as Windows also seems to disable these.
+		 */
+		opregion->acpi->chpd = 1;
 	}
 
 	if (mboxes & MBOX_SWSCI) {
-- 
2.23.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread