Re: [PATCH 1/4] drm/i915/runtime_pm: Consolidate runtime_pm functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 25, 2021 at 11:22:30AM -0400, Rodrigo Vivi wrote:
> No functional changes. Just revamping the functions with
> s/dev_priv/i915
> and consolidating along with other runtime_pm functions.
> 
> v2: avoid the extra redirection (Imre)
> 
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Tilak Tangudu <tilak.tangudu@xxxxxxxxx>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>

Looks ok:
Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>

One nit below.

> ---
>  drivers/gpu/drm/i915/i915_drv.c         | 145 +-----------------------
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 145 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.h |   2 +
>  3 files changed, 149 insertions(+), 143 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 59fb4c710c8c..a40b5d806321 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -64,7 +64,6 @@
>  #include "gem/i915_gem_mman.h"
>  #include "gem/i915_gem_pm.h"
>  #include "gt/intel_gt.h"
> -#include "gt/intel_gt_pm.h"
>  #include "gt/intel_rc6.h"
>  
>  #include "i915_debugfs.h"
> @@ -1517,146 +1516,6 @@ static int i915_pm_restore(struct device *kdev)
>  	return i915_pm_resume(kdev);
>  }
>  
> -static int intel_runtime_suspend(struct device *kdev)
> -{
> -	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> -	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
> -	int ret;
> -
> -	if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv)))
> -		return -ENODEV;
> -
> -	drm_dbg_kms(&dev_priv->drm, "Suspending device\n");
> -
> -	disable_rpm_wakeref_asserts(rpm);
> -
> -	/*
> -	 * We are safe here against re-faults, since the fault handler takes
> -	 * an RPM reference.
> -	 */
> -	i915_gem_runtime_suspend(dev_priv);
> -
> -	intel_gt_runtime_suspend(&dev_priv->gt);
> -
> -	intel_runtime_pm_disable_interrupts(dev_priv);
> -
> -	intel_uncore_suspend(&dev_priv->uncore);
> -
> -	intel_display_power_suspend(dev_priv);
> -
> -	ret = vlv_suspend_complete(dev_priv);
> -	if (ret) {
> -		drm_err(&dev_priv->drm,
> -			"Runtime suspend failed, disabling it (%d)\n", ret);
> -		intel_uncore_runtime_resume(&dev_priv->uncore);
> -
> -		intel_runtime_pm_enable_interrupts(dev_priv);
> -
> -		intel_gt_runtime_resume(&dev_priv->gt);
> -
> -		enable_rpm_wakeref_asserts(rpm);
> -
> -		return ret;
> -	}
> -
> -	enable_rpm_wakeref_asserts(rpm);
> -	intel_runtime_pm_driver_release(rpm);
> -
> -	if (intel_uncore_arm_unclaimed_mmio_detection(&dev_priv->uncore))
> -		drm_err(&dev_priv->drm,
> -			"Unclaimed access detected prior to suspending\n");
> -
> -	rpm->suspended = true;
> -
> -	/*
> -	 * 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(dev_priv, 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(dev_priv, PCI_D1);
> -	}
> -
> -	assert_forcewakes_inactive(&dev_priv->uncore);
> -
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> -		intel_hpd_poll_enable(dev_priv);
> -
> -	drm_dbg_kms(&dev_priv->drm, "Device suspended\n");
> -	return 0;
> -}
> -
> -static int intel_runtime_resume(struct device *kdev)
> -{
> -	struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> -	struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
> -	int ret;
> -
> -	if (drm_WARN_ON_ONCE(&dev_priv->drm, !HAS_RUNTIME_PM(dev_priv)))
> -		return -ENODEV;
> -
> -	drm_dbg_kms(&dev_priv->drm, "Resuming device\n");
> -
> -	drm_WARN_ON_ONCE(&dev_priv->drm, atomic_read(&rpm->wakeref_count));
> -	disable_rpm_wakeref_asserts(rpm);
> -
> -	intel_opregion_notify_adapter(dev_priv, PCI_D0);
> -	rpm->suspended = false;
> -	if (intel_uncore_unclaimed_mmio(&dev_priv->uncore))
> -		drm_dbg(&dev_priv->drm,
> -			"Unclaimed access during suspend, bios?\n");
> -
> -	intel_display_power_resume(dev_priv);
> -
> -	ret = vlv_resume_prepare(dev_priv, true);
> -
> -	intel_uncore_runtime_resume(&dev_priv->uncore);
> -
> -	intel_runtime_pm_enable_interrupts(dev_priv);
> -
> -	/*
> -	 * No point of rolling back things in case of an error, as the best
> -	 * we can do is to hope that things will still work (and disable RPM).
> -	 */
> -	intel_gt_runtime_resume(&dev_priv->gt);
> -
> -	/*
> -	 * 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(dev_priv);
> -		intel_hpd_poll_disable(dev_priv);
> -	}
> -
> -	intel_enable_ipc(dev_priv);
> -
> -	enable_rpm_wakeref_asserts(rpm);
> -
> -	if (ret)
> -		drm_err(&dev_priv->drm,
> -			"Runtime resume failed, disabling it (%d)\n", ret);
> -	else
> -		drm_dbg_kms(&dev_priv->drm, "Device resumed\n");
> -
> -	return ret;
> -}
> -
>  const struct dev_pm_ops i915_pm_ops = {
>  	/*
>  	 * S0ix (via system suspend) and S3 event handlers [PMSG_SUSPEND,
> @@ -1693,8 +1552,8 @@ const struct dev_pm_ops i915_pm_ops = {
>  	.restore = i915_pm_restore,
>  
>  	/* S0ix (via runtime suspend) event handlers */
> -	.runtime_suspend = intel_runtime_suspend,
> -	.runtime_resume = intel_runtime_resume,
> +	.runtime_suspend = intel_runtime_pm_suspend,
> +	.runtime_resume = intel_runtime_pm_resume,
>  };
>  
>  static const struct file_operations i915_driver_fops = {
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index eaf7688f517d..f28b5bab61b4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -32,6 +32,11 @@
>  
>  #include "i915_drv.h"
>  #include "i915_trace.h"
> +#include "gt/intel_gt.h"
> +#include "gt/intel_gt_pm.h"
> +#include "intel_pm.h"
> +#include "vlv_suspend.h"
> +#include "display/intel_hotplug.h"

I suppose these should be in the following order:

display/*.h

gt/*.h

i915_*.h
...

(taken from intel_display.c)

Could add docs for the exported funcs at one point.

>  
>  /**
>   * DOC: runtime pm
> @@ -652,3 +657,143 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm)
>  
>  	init_intel_runtime_pm_wakeref(rpm);
>  }
> +
> +int intel_runtime_pm_suspend(struct device *kdev)
> +{
> +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> +	int ret;
> +
> +	if (drm_WARN_ON_ONCE(&i915->drm, !HAS_RUNTIME_PM(i915)))
> +		return -ENODEV;
> +
> +	drm_dbg_kms(&i915->drm, "Suspending device\n");
> +
> +	disable_rpm_wakeref_asserts(rpm);
> +
> +	/*
> +	 * We are safe here against re-faults, since the fault handler takes
> +	 * an RPM reference.
> +	 */
> +	i915_gem_runtime_suspend(i915);
> +
> +	intel_gt_runtime_suspend(&i915->gt);
> +
> +	intel_runtime_pm_disable_interrupts(i915);
> +
> +	intel_uncore_suspend(&i915->uncore);
> +
> +	intel_display_power_suspend(i915);
> +
> +	ret = vlv_suspend_complete(i915);
> +	if (ret) {
> +		drm_err(&i915->drm,
> +			"Runtime suspend failed, disabling it (%d)\n", ret);
> +		intel_uncore_runtime_resume(&i915->uncore);
> +
> +		intel_runtime_pm_enable_interrupts(i915);
> +
> +		intel_gt_runtime_resume(&i915->gt);
> +
> +		enable_rpm_wakeref_asserts(rpm);
> +
> +		return ret;
> +	}
> +
> +	enable_rpm_wakeref_asserts(rpm);
> +	intel_runtime_pm_driver_release(rpm);
> +
> +	if (intel_uncore_arm_unclaimed_mmio_detection(&i915->uncore))
> +		drm_err(&i915->drm,
> +			"Unclaimed access detected prior to suspending\n");
> +
> +	rpm->suspended = true;
> +
> +	/*
> +	 * FIXME: We really should find a document that references the arguments
> +	 * used below!
> +	 */
> +	if (IS_BROADWELL(i915)) {
> +		/*
> +		 * 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(i915, 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(i915, PCI_D1);
> +	}
> +
> +	assert_forcewakes_inactive(&i915->uncore);
> +
> +	if (!IS_VALLEYVIEW(i915) && !IS_CHERRYVIEW(i915))
> +		intel_hpd_poll_enable(i915);
> +
> +	drm_dbg_kms(&i915->drm, "Device suspended\n");
> +	return 0;
> +}
> +
> +int intel_runtime_pm_resume(struct device *kdev)
> +{
> +	struct drm_i915_private *i915 = kdev_to_i915(kdev);
> +	struct intel_runtime_pm *rpm = &i915->runtime_pm;
> +	int ret;
> +
> +	if (drm_WARN_ON_ONCE(&i915->drm, !HAS_RUNTIME_PM(i915)))
> +		return -ENODEV;
> +
> +	drm_dbg_kms(&i915->drm, "Resuming device\n");
> +
> +	drm_WARN_ON_ONCE(&i915->drm, atomic_read(&rpm->wakeref_count));
> +	disable_rpm_wakeref_asserts(rpm);
> +
> +	intel_opregion_notify_adapter(i915, PCI_D0);
> +	rpm->suspended = false;
> +	if (intel_uncore_unclaimed_mmio(&i915->uncore))
> +		drm_dbg(&i915->drm,
> +			"Unclaimed access during suspend, bios?\n");
> +
> +	intel_display_power_resume(i915);
> +
> +	ret = vlv_resume_prepare(i915, true);
> +
> +	intel_uncore_runtime_resume(&i915->uncore);
> +
> +	intel_runtime_pm_enable_interrupts(i915);
> +
> +	/*
> +	 * No point of rolling back things in case of an error, as the best
> +	 * we can do is to hope that things will still work (and disable RPM).
> +	 */
> +	intel_gt_runtime_resume(&i915->gt);
> +
> +	/*
> +	 * 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(i915) && !IS_CHERRYVIEW(i915)) {
> +		intel_hpd_init(i915);
> +		intel_hpd_poll_disable(i915);
> +	}
> +
> +	intel_enable_ipc(i915);
> +
> +	enable_rpm_wakeref_asserts(rpm);
> +
> +	if (ret)
> +		drm_err(&i915->drm,
> +			"Runtime resume failed, disabling it (%d)\n", ret);
> +	else
> +		drm_dbg_kms(&i915->drm, "Device resumed\n");
> +
> +	return ret;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h b/drivers/gpu/drm/i915/intel_runtime_pm.h
> index 47a85fab4130..88ca531165f7 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> @@ -172,6 +172,8 @@ void intel_runtime_pm_init_early(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_enable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_disable(struct intel_runtime_pm *rpm);
>  void intel_runtime_pm_driver_release(struct intel_runtime_pm *rpm);
> +int intel_runtime_pm_suspend(struct device *kdev);
> +int intel_runtime_pm_resume(struct device *kdev);
>  
>  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
>  intel_wakeref_t intel_runtime_pm_get_if_in_use(struct intel_runtime_pm *rpm);
> -- 
> 2.31.1
> 



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux