Re: [PATCH 2/4] drm/i915/opregion: Extract intel_opregion_runtime_{suspend, resume}()

[Imre Deak <imre.deak@intel.com>]

On Tue, Jul 08, 2025 at 07:03:18PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Extract the opregion runtime PM stuff to new functions. We'll
> need to add a bit more to the suspend side, and we don't want
> to clutter the top level runtime PM code with such details.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_opregion.c | 41 +++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_opregion.h | 11 +++++
>  drivers/gpu/drm/i915/i915_driver.c            | 25 +----------
>  3 files changed, 54 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.c b/drivers/gpu/drm/i915/display/intel_opregion.c
> index 81efdb17fc0c..9e39ab55a099 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.c
> @@ -1255,6 +1255,47 @@ void intel_opregion_suspend(struct intel_display *display, pci_power_t state)
>  		intel_opregion_suspend_display(display);
>  }
>  
> +void intel_opregion_runtime_resume(struct intel_display *display)
> +{
> +	struct intel_opregion *opregion = display->opregion;
> +
> +	if (!opregion)
> +		return;
> +
> +	intel_opregion_notify_adapter(display, PCI_D0);
> +}
> +
> +void intel_opregion_runtime_suspend(struct intel_display *display)
> +{
> +	struct intel_opregion *opregion = display->opregion;
> +
> +	if (!opregion)
> +		return;
> +
> +	/*
> +	 * 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 intel_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);
> +	}
> +}
> +
>  void intel_opregion_unregister(struct intel_display *display)
>  {
>  	struct intel_opregion *opregion = display->opregion;
> diff --git a/drivers/gpu/drm/i915/display/intel_opregion.h b/drivers/gpu/drm/i915/display/intel_opregion.h
> index 8101eeebfd8b..7067ffe07744 100644
> --- a/drivers/gpu/drm/i915/display/intel_opregion.h
> +++ b/drivers/gpu/drm/i915/display/intel_opregion.h
> @@ -44,6 +44,9 @@ void intel_opregion_resume(struct intel_display *display);
>  void intel_opregion_suspend(struct intel_display *display,
>  			    pci_power_t state);
>  
> +void intel_opregion_runtime_resume(struct intel_display *display);
> +void intel_opregion_runtime_suspend(struct intel_display *display);
> +
>  bool intel_opregion_asle_present(struct intel_display *display);
>  void intel_opregion_asle_intr(struct intel_display *display);
>  int intel_opregion_notify_encoder(struct intel_encoder *encoder,
> @@ -88,6 +91,14 @@ static inline void intel_opregion_suspend(struct intel_display *display,
>  {
>  }
>  
> +static inline void intel_opregion_runtime_resume(struct intel_display *display)
> +{
> +}
> +
> +static inline void intel_opregion_runtime_suspend(struct intel_display *display)
> +{
> +}
> +
>  static inline bool intel_opregion_asle_present(struct intel_display *display)
>  {
>  	return false;
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index c6263c6d3384..da0b7d9da3d5 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1553,28 +1553,7 @@ static int intel_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 intel_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);
> -	}
> +	intel_opregion_runtime_suspend(display);
>  
>  	assert_forcewakes_inactive(&dev_priv->uncore);
>  
> @@ -1603,7 +1582,7 @@ static int intel_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);
> +	intel_opregion_runtime_resume(display);
>  
>  	root_pdev = pcie_find_root_port(pdev);
>  	if (root_pdev)
> -- 
> 2.49.0
>