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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
