On Fri, Jul 21, 2017 at 02:32:55PM +0300, Arkadiusz Hiler wrote:
> On Fri, Jul 21, 2017 at 02:25:18PM +0300, Imre Deak wrote:
> > 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
>
> Let me go through the rest of the patches. If I will be happy with the
> end result then I'll r-b this patch.
>
> There no use bashing on intermediary steps and force you to rebase the
> thing if the final result is good :-)
Made it to the part when you get rid of it completely.
Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
