[Intel-gfx] [PATCH 09/18] drm/i915/gen9+: Remove redundant state check during power well toggling

[Arkadiusz Hiler]

On Thu, Jul 06, 2017 at 05:40:31PM +0300, Imre Deak wrote:
> Atm we enable/disable a power well only if it wasn't already
> enabled/disabled respectively. The only reason for this I can think of
> is to save the extra MMIO writes. Since the HW state matches the power
> well's usage counter most of the time the overhead due to these MMIOs is
> insignificant. Let's simplify the code by making the writes
> unconditional.
> 
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 85c592d..28d2ea9 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -806,7 +806,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  {
>  	uint32_t tmp, fuse_status;
>  	uint32_t req_mask, state_mask;
> -	bool is_enabled, enable_requested, check_fuse_status = false;
> +	bool check_fuse_status = false;
>  
>  	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
>  	fuse_status = I915_READ(SKL_FUSE_STATUS);
> @@ -844,29 +844,22 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  	}
>  
>  	req_mask = SKL_POWER_WELL_REQ(power_well->id);
> -	enable_requested = tmp & req_mask;
>  	state_mask = SKL_POWER_WELL_STATE(power_well->id);
> -	is_enabled = tmp & state_mask;
>  
> -	if (!enable && enable_requested)
> +	if (!enable)
>  		skl_power_well_pre_disable(dev_priv, power_well);
>  
>  	if (enable) {
> -		if (!enable_requested)
> -			I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
> +		I915_WRITE(HSW_PWR_WELL_DRIVER, tmp | req_mask);
>  
> -		if (!is_enabled) {
> -			DRM_DEBUG_KMS("Enabling %s\n", power_well->name);
> -			check_fuse_status = true;
> -		}
> +		DRM_DEBUG_KMS("Enabling %s\n", power_well->name);
> +		check_fuse_status = true;


It's the only place we set check_fuse_status to true and now we do that
unconditionally, so the following  `if (check_fuse_status)` can be
inlined here, so we can drop the variable completely.

-- 
Cheers,
Arek


>  
>  		gen9_wait_for_power_well_enable(dev_priv, power_well);
>  	} else {
> -		if (enable_requested) {
> -			I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> -			POSTING_READ(HSW_PWR_WELL_DRIVER);
> -			DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
> -		}
> +		I915_WRITE(HSW_PWR_WELL_DRIVER,	tmp & ~req_mask);
> +		POSTING_READ(HSW_PWR_WELL_DRIVER);
> +		DRM_DEBUG_KMS("Disabling %s\n", power_well->name);
>  
>  		gen9_wait_for_power_well_disable(dev_priv, power_well);
>  	}
> @@ -889,7 +882,7 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
>  		}
>  	}
>  
> -	if (enable && !is_enabled)
> +	if (enable)
>  		skl_power_well_post_enable(dev_priv, power_well);
>  }
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx