[Intel-gfx] [PATCH v3 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()

[Imre Deak]

On Thu, May 09, 2019 at 08:43:25PM +0300, Souza, Jose wrote:
> On Thu, 2019-05-09 at 20:34 +0300, Imre Deak wrote:
> > The power get/put was added in
> > 
> > commit 1c767b339b39 ("drm/i915: take display port power domain in DP
> > HPD handler")
> > Author: Imre Deak <imre.deak at intel.com>
> > Date:   Mon Aug 18 14:42:42 2014 +0300
> > 
> > to account for the HW access in ibx_digital_port_connected(). This
> > latter call was in turn removed in
> > 
> > commit 7d23e3c37bb3 ("drm/i915: Cleaning up intel_dp_hpd_pulse")
> > Author: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
> > Date:   Wed Mar 30 18:05:23 2016 +0530
> > 
> > after which we didn't actually need the power reference.
> > 
> > One way we are accessing the HW during HPD pulse handling is via DP
> > AUX
> > transfers, but the transfer function takes its own reference, so
> > doesn't
> > need the reference in intel_dp_hpd_pulse().
> 
> <Did not look at the other patches />
> 
> The problem of removing that reference is that every aux transfer will
> take a little bit more of time because it will need to wait the aux
> power well to be enabled/disabled, taking one reference before hand
> save us that.

That is solved by disabling the power with a delay. But we could only
claim that there would be any problem (even in the lack of the delayed
disabling) with actual numbers for the enabling/disabling delay. Please
check the discussion on patch 4 for more details.

> 
> > 
> > The other spot is in
> > 
> > 	intel_psr_short_pulse()->intel_psr_disable_locked()
> > 
> > but that can only happen when the panel is enabled with the
> > corresponding modeset already holding the required power reference.
> > 
> > v2:
> > - Remove the unneeded power get/put from intel_psr_disable_locked().
> >   (Ville)
> > - Checkpatch commit quoting format fix in the commit log.
> > 
> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 20 ++++----------------
> >  1 file changed, 4 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 553071812f69..8a91b453b2e9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6302,9 +6302,6 @@ enum irqreturn
> >  intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool
> > long_hpd)
> >  {
> >  	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > -	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > -	enum irqreturn ret = IRQ_NONE;
> > -	intel_wakeref_t wakeref;
> >  
> >  	if (long_hpd && intel_dig_port->base.type == INTEL_OUTPUT_EDP)
> > {
> >  		/*
> > @@ -6327,9 +6324,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  		return IRQ_NONE;
> >  	}
> >  
> > -	wakeref = intel_display_power_get(dev_priv,
> > -					  intel_aux_power_domain(intel_
> > dig_port));
> > -
> >  	if (intel_dp->is_mst) {
> >  		if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> >  			/*
> > @@ -6341,7 +6335,8 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  			intel_dp->is_mst = false;
> >  			drm_dp_mst_topology_mgr_set_mst(&intel_dp-
> > >mst_mgr,
> >  							intel_dp-
> > >is_mst);
> > -			goto put_power;
> > +
> > +			return IRQ_NONE;
> >  		}
> >  	}
> >  
> > @@ -6351,17 +6346,10 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  		handled = intel_dp_short_pulse(intel_dp);
> >  
> >  		if (!handled)
> > -			goto put_power;
> > +			return IRQ_NONE;
> >  	}
> >  
> > -	ret = IRQ_HANDLED;
> > -
> > -put_power:
> > -	intel_display_power_put(dev_priv,
> > -				intel_aux_power_domain(intel_dig_port),
> > -				wakeref);
> > -
> > -	return ret;
> > +	return IRQ_HANDLED;
> >  }
> >  
> >  /* check the VBT to see whether the eDP is on another port */