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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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