Re: [PATCH 22/24] drm/i915: add intel_display_driver_pm_runtime*() functions

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

On Fri, May 29, 2026 at 02:04:06PM +0300, Jani Nikula wrote:
> Add new functions intel_display_driver_pm_runtime_suspend(),
> intel_display_driver_pm_runtime_suspend_late(),
> intel_display_driver_pm_runtime_resume_early(), and
> intel_display_driver_pm_runtime_resume(). Initially, only migrate i915,
> as there are some differences with xe that will be addressed later.
> 
> There are a few functional changes, which should be benign:
> 
> - i915_pm_runtime_suspend() moves assert_forcewakes_inactive() call
>   before opregion calls.
> 
> - i915_pm_runtime_resume() moves intel_opregion_notify_adapter() call
>   slightly later.
> 
> In the interest of not introducing more severe functional changes, the
> calls become slightly asymmetric. We might want to address this later.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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

> ---
>  .../drm/i915/display/intel_display_driver.c   | 56 +++++++++++++++++++
>  .../drm/i915/display/intel_display_driver.h   |  5 ++
>  drivers/gpu/drm/i915/i915_driver.c            | 44 ++-------------
>  3 files changed, 65 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.c b/drivers/gpu/drm/i915/display/intel_display_driver.c
> index 84807a9bff2b..a27ddf21b08b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.c
> @@ -884,3 +884,59 @@ void intel_display_driver_pm_resume(struct intel_display *display)
>  
>  	intel_display_power_enable(display);
>  }
> +
> +void intel_display_driver_pm_runtime_suspend(struct intel_display *display)
> +{
> +	intel_display_power_runtime_suspend(display);
> +}
> +
> +void intel_display_driver_pm_runtime_suspend_late(struct intel_display *display)
> +{
> +	/*
> +	 * FIXME: We really should find a document that references the arguments
> +	 * used below!
> +	 */
> +	if (display->platform.broadwell) {
> +		/*
> +		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
> +		 * being detected, and the call we do at i915_pm_runtime_resume()
> +		 * won't be able to restore them. Since PCI_D3hot matches the
> +		 * actual specification and appears to be working, use it.
> +		 */
> +		intel_opregion_notify_adapter(display, PCI_D3hot);
> +	} else {
> +		/*
> +		 * current versions of firmware which depend on this opregion
> +		 * notification have repurposed the D1 definition to mean
> +		 * "runtime suspended" vs. what you would normally expect (D3)
> +		 * to distinguish it from notifications that might be sent via
> +		 * the suspend path.
> +		 */
> +		intel_opregion_notify_adapter(display, PCI_D1);
> +	}
> +
> +	if (!display->platform.valleyview && !display->platform.cherryview)
> +		intel_hpd_poll_enable(display);
> +}
> +
> +void intel_display_driver_pm_runtime_resume_early(struct intel_display *display)
> +{
> +	intel_opregion_notify_adapter(display, PCI_D0);
> +
> +	intel_display_power_runtime_resume(display);
> +}
> +
> +void intel_display_driver_pm_runtime_resume(struct intel_display *display)
> +{
> +	/*
> +	 * On VLV/CHV display interrupts are part of the display
> +	 * power well, so hpd is reinitialized from there. For
> +	 * everyone else do it here.
> +	 */
> +	if (!display->platform.valleyview && !display->platform.cherryview) {
> +		intel_hpd_init(display);
> +		intel_hpd_poll_disable(display);
> +	}
> +
> +	skl_watermark_ipc_update(display);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_display_driver.h b/drivers/gpu/drm/i915/display/intel_display_driver.h
> index 7eca3d17dd82..1b494337d629 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_driver.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_driver.h
> @@ -43,5 +43,10 @@ void intel_display_driver_suspend_access(struct intel_display *display);
>  void intel_display_driver_resume_access(struct intel_display *display);
>  bool intel_display_driver_check_access(struct intel_display *display);
>  
> +void intel_display_driver_pm_runtime_suspend(struct intel_display *display);
> +void intel_display_driver_pm_runtime_suspend_late(struct intel_display *display);
> +void intel_display_driver_pm_runtime_resume_early(struct intel_display *display);
> +void intel_display_driver_pm_runtime_resume(struct intel_display *display);
> +
>  #endif /* __INTEL_DISPLAY_DRIVER_H__ */
>  
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 6b9e1b268a89..519a519d2d96 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1521,7 +1521,7 @@ static int i915_pm_runtime_suspend(struct device *kdev)
>  	for_each_gt(gt, dev_priv, i)
>  		intel_uncore_suspend(gt->uncore);
>  
> -	intel_display_power_runtime_suspend(display);
> +	intel_display_driver_pm_runtime_suspend(display);
>  
>  	ret = vlv_suspend_complete(dev_priv);
>  	if (ret) {
> @@ -1555,33 +1555,9 @@ static int i915_pm_runtime_suspend(struct device *kdev)
>  	if (root_pdev)
>  		pci_d3cold_disable(root_pdev);
>  
> -	/*
> -	 * FIXME: We really should find a document that references the arguments
> -	 * used below!
> -	 */
> -	if (IS_BROADWELL(dev_priv)) {
> -		/*
> -		 * On Broadwell, if we use PCI_D1 the PCH DDI ports will stop
> -		 * being detected, and the call we do at i915_pm_runtime_resume()
> -		 * won't be able to restore them. Since PCI_D3hot matches the
> -		 * actual specification and appears to be working, use it.
> -		 */
> -		intel_opregion_notify_adapter(display, PCI_D3hot);
> -	} else {
> -		/*
> -		 * current versions of firmware which depend on this opregion
> -		 * notification have repurposed the D1 definition to mean
> -		 * "runtime suspended" vs. what you would normally expect (D3)
> -		 * to distinguish it from notifications that might be sent via
> -		 * the suspend path.
> -		 */
> -		intel_opregion_notify_adapter(display, PCI_D1);
> -	}
> -
>  	assert_forcewakes_inactive(&dev_priv->uncore);
>  
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> -		intel_hpd_poll_enable(display);
> +	intel_display_driver_pm_runtime_suspend_late(display);
>  
>  	drm_dbg(&dev_priv->drm, "Device suspended\n");
>  	return 0;
> @@ -1605,8 +1581,6 @@ static int i915_pm_runtime_resume(struct device *kdev)
>  	drm_WARN_ON_ONCE(&dev_priv->drm, atomic_read(&rpm->wakeref_count));
>  	disable_rpm_wakeref_asserts(rpm);
>  
> -	intel_opregion_notify_adapter(display, PCI_D0);
> -
>  	root_pdev = pcie_find_root_port(pdev);
>  	if (root_pdev)
>  		pci_d3cold_enable(root_pdev);
> @@ -1615,7 +1589,7 @@ static int i915_pm_runtime_resume(struct device *kdev)
>  		drm_dbg(&dev_priv->drm,
>  			"Unclaimed access during suspend, bios?\n");
>  
> -	intel_display_power_runtime_resume(display);
> +	intel_display_driver_pm_runtime_resume_early(display);
>  
>  	ret = vlv_resume_prepare(dev_priv, true);
>  
> @@ -1633,17 +1607,7 @@ static int i915_pm_runtime_resume(struct device *kdev)
>  
>  	intel_pxp_runtime_resume(dev_priv->pxp);
>  
> -	/*
> -	 * On VLV/CHV display interrupts are part of the display
> -	 * power well, so hpd is reinitialized from there. For
> -	 * everyone else do it here.
> -	 */
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
> -		intel_hpd_init(display);
> -		intel_hpd_poll_disable(display);
> -	}
> -
> -	skl_watermark_ipc_update(display);
> +	intel_display_driver_pm_runtime_resume(display);
>  
>  	enable_rpm_wakeref_asserts(rpm);
>  
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel