Re: [PATCH 05/11] drm/i915: Guard rpm helpers in gt helpers functions

[Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>]

On Tue, 21 Jun 2022, Tilak Tangudu <tilak.tangudu@xxxxxxxxx> wrote:
> Guard rpm helpers in gt_sanitize and intel_gt_set_wedged
> with is_intel_rpm_allowed
>
> Acquire rpm wakeref for higherlevel function i915_gem_resume
>
> Signed-off-by: Tilak Tangudu <tilak.tangudu@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_pm.c | 12 ++++++------
>  drivers/gpu/drm/i915/gt/intel_reset.c | 10 +++++++---
>  drivers/gpu/drm/i915/i915_driver.c    |  4 +++-
>  3 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index be99b01a0984..9857b91194b7 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -163,12 +163,14 @@ static void gt_sanitize(struct intel_gt *gt, bool force)
>  {
>  	struct intel_engine_cs *engine;
>  	enum intel_engine_id id;
> -	intel_wakeref_t wakeref;
> +	intel_wakeref_t wakeref = 0;

We've got intel_wakeref_t to hide what it actually is. You shouldn't
assume you can assign 0 to it or use wakeref in an if condition. You
should treat it as opaque. You should assume the typedef could be
switched to a struct and you shouldn't have to change the code using it.

BR,
Jani.

>  
>  	GT_TRACE(gt, "force:%s", str_yes_no(force));
>  
>  	/* Use a raw wakeref to avoid calling intel_display_power_get early */
> -	wakeref = intel_runtime_pm_get(gt->uncore->rpm);
> +	if (is_intel_rpm_allowed(gt->uncore->rpm))
> +		wakeref = intel_runtime_pm_get(gt->uncore->rpm);
> +
>  	intel_uncore_forcewake_get(gt->uncore, FORCEWAKE_ALL);
>  
>  	intel_gt_check_clock_frequency(gt);
> @@ -207,7 +209,8 @@ static void gt_sanitize(struct intel_gt *gt, bool force)
>  	intel_rps_sanitize(&gt->rps);
>  
>  	intel_uncore_forcewake_put(gt->uncore, FORCEWAKE_ALL);
> -	intel_runtime_pm_put(gt->uncore->rpm, wakeref);
> +	if (wakeref)
> +		intel_runtime_pm_put(gt->uncore->rpm, wakeref);
>  }
>  
>  void intel_gt_pm_fini(struct intel_gt *gt)
> @@ -226,7 +229,6 @@ int intel_gt_resume(struct intel_gt *gt)
>  		return err;
>  
>  	GT_TRACE(gt, "\n");
> -
>  	/*
>  	 * After resume, we may need to poke into the pinned kernel
>  	 * contexts to paper over any damage caused by the sudden suspend.
> @@ -259,10 +261,8 @@ int intel_gt_resume(struct intel_gt *gt)
>  
>  	for_each_engine(engine, gt, id) {
>  		intel_engine_pm_get(engine);
> -
>  		engine->serial++; /* kernel context lost */
>  		err = intel_engine_resume(engine);
> -
>  		intel_engine_pm_put(engine);
>  		if (err) {
>  			drm_err(&gt->i915->drm,
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
> index c8e05b48c14f..55a1fd38c7c4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -901,12 +901,14 @@ static void __intel_gt_set_wedged(struct intel_gt *gt)
>  
>  void intel_gt_set_wedged(struct intel_gt *gt)
>  {
> -	intel_wakeref_t wakeref;
> +	intel_wakeref_t wakeref = 0;
>  
>  	if (test_bit(I915_WEDGED, &gt->reset.flags))
>  		return;
>  
> -	wakeref = intel_runtime_pm_get(gt->uncore->rpm);
> +	if (is_intel_rpm_allowed(gt->uncore->rpm))
> +		wakeref = intel_runtime_pm_get(gt->uncore->rpm);
> +
>  	mutex_lock(&gt->reset.mutex);
>  
>  	if (GEM_SHOW_DEBUG()) {
> @@ -926,7 +928,9 @@ void intel_gt_set_wedged(struct intel_gt *gt)
>  	__intel_gt_set_wedged(gt);
>  
>  	mutex_unlock(&gt->reset.mutex);
> -	intel_runtime_pm_put(gt->uncore->rpm, wakeref);
> +
> +	if (wakeref)
> +		intel_runtime_pm_put(gt->uncore->rpm, wakeref);
>  }
>  
>  static bool __intel_gt_unset_wedged(struct intel_gt *gt)
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index d26dcca7e654..60f6fcc6b71d 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1263,6 +1263,7 @@ int i915_driver_suspend_switcheroo(struct drm_i915_private *i915,
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	intel_wakeref_t wakeref;
>  	int ret;
>  
>  	disable_rpm_wakeref_asserts(&dev_priv->runtime_pm);
> @@ -1303,7 +1304,8 @@ static int i915_drm_resume(struct drm_device *dev)
>  	if (HAS_DISPLAY(dev_priv))
>  		drm_mode_config_reset(dev);
>  
> -	i915_gem_resume(dev_priv);
> +	with_intel_runtime_pm(&dev_priv->runtime_pm, wakeref)
> +		i915_gem_resume(dev_priv);
>  
>  	intel_modeset_init_hw(dev_priv);
>  	intel_init_clock_gating(dev_priv);

-- 
Jani Nikula, Intel Open Source Graphics Center