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

[Arkadiusz Hiler]

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 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.
> 
> 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