On Fri, Nov 22, 2019 at 08:04:39PM +0100, Hans de Goede wrote:

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.

Feels like the spec was written by a politician. It's left to the reader to interpret each statement either one way or the other. But I can get behind your interpretation, especially if it makes the firmware stop doing silly things.

Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com

BTW at some point I was looking for other ways to get the firmware to stop messing things. I found a bunch of scratch registers which supposedly might do something like that:

git://github.com/vsyrjala/linux.git vbios_swf

but in the end I don't think that fixed the issue I was trying to sort out, which IIRC was the fact that some old laptops don't survive S4 if we put the GPU into D3.

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