On Fri, Jul 21, 2017 at 02:14:33PM +0300, Arkadiusz Hiler wrote:
> 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.
Hm yea. Also skl_power_well_pre_disable() and
skl_power_well_post_enable() could be inlined the same way then. But
this is an incremental change to simplify things even more in the end by
removing this function and using the HSW counterpart instead. Should've
mentioned this in the commit log. Do you still want me to change this?
--Imre
>
> --
> 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
