[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: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 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 :-)

Made it to the part when you get rid of it completely.

Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler at intel.com>