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