Re: [PATCH 2/4] drm/{i915, xe}: move intel_opregion_suspend() to intel_display_driver_pm_suspend_late()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/i915: Use-after-suspend of asle_work due to late intel_opregion_suspend()
- [High] drm/i915: Hard system hang risk from late ACPI SMI notification
- [High] drm/xe: ACPI state corruption via double intel_opregion_suspend()
--

commit fc3c0277d6ab46a7da2ef12128f2682dd8013140
Author: Jani Nikula <jani.nikula@intel.com>

drm/{i915, xe}: move intel_opregion_suspend() to intel_display_driver_pm_suspend_late()

Try to unify the suspend paths and behaviour by moving the
intel_opregion_suspend() calls to
intel_display_driver_pm_suspend_late().

> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 77009dca7d0d9..abab457bce3a3 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -783,6 +783,8 @@ void intel_display_driver_pm_suspend_late(struct intel_display *display, bool s2
>  	if (!HAS_DISPLAY(display))
>  		return;
>  
> +	intel_opregion_suspend(display, s2idle ? PCI_D1 : PCI_D3cold);
> +
>  	intel_display_power_suspend_late(display, s2idle);
>  }

[Severity: High]
Does moving intel_opregion_suspend() here cause it to execute too late in the
suspend sequence?

In i915_drm_suspend_late(), the GTs and uncore are suspended before calling
intel_display_driver_pm_suspend_late():

drivers/gpu/drm/i915/i915_driver.c:i915_drm_suspend_late() {
	...
	for_each_gt(gt, dev_priv, i)
		intel_uncore_suspend(gt->uncore);

	intel_display_driver_pm_suspend_late(display, s2idle);
	...
}

Because intel_opregion_suspend() calls cancel_work_sync(&opregion->asle_work),
delaying this sync until after intel_uncore_suspend() could allow asle_work
to execute concurrently with or after uncore suspension, risking MMIO accesses
on suspended hardware.

Additionally, could triggering a System Management Interrupt (SMI) via
intel_opregion_notify_adapter() while the GT/uncore is already suspended cause
the BIOS to access powered-down GPU registers and result in a hard system hang?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/drm/xe/display/xe_display.c
> index 184a5a2378805..b013ec00b75da 100644
> --- a/drivers/gpu/drm/xe/display/xe_display.c
> +++ b/drivers/gpu/drm/xe/display/xe_display.c
> @@ -246,14 +246,11 @@ static bool suspend_to_idle(void)
>  void xe_display_pm_suspend(struct xe_device *xe)
>  {
>  	struct intel_display *display = xe->display;
> -	bool s2idle = suspend_to_idle();
>  
>  	if (!xe->info.probe_display)
>  		return;
>  
>  	intel_display_driver_pm_suspend(display);
> -
> -	intel_opregion_suspend(display, s2idle ? PCI_D1 : PCI_D3cold);
>  }

[Severity: High]
Will this lead to a double intel_opregion_suspend() and ACPI state corruption
during runtime suspend?

When xe->d3cold.allowed is true, xe_display_pm_runtime_suspend() calls
xe_display_enable_d3cold() which explicitly suspends the OpRegion to PCI_D3cold:

drivers/gpu/drm/xe/display/xe_display.c:xe_display_enable_d3cold() {
	...
	intel_opregion_suspend(display, PCI_D3cold);
	...
}

Then xe_display_pm_runtime_suspend_late() calls xe_display_pm_suspend_late():

drivers/gpu/drm/xe/display/xe_display.c:xe_display_pm_runtime_suspend_late() {
	...
	if (xe->d3cold.allowed) {
		xe_display_pm_suspend_late(xe);
	...
}

Since xe_display_pm_suspend_late() calls intel_display_driver_pm_suspend_late(),
which now includes the new intel_opregion_suspend() call, it will be executed a
second time. Because suspend_to_idle() evaluates to true during runtime suspend
(S0), it will be called with PCI_D1 instead of PCI_D3cold.

Will this overwrite the ACPI power state from PCI_D3cold to PCI_D1 and corrupt
the BIOS state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1790089118.git.jani.nikula@intel.com?part=2