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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxx>
> ---
> 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
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
