Re: [PATCH v2 08/11] drm/i915: Remove the unneeded AUX power ref from intel_dp_hpd_pulse()

[Imre Deak <imre.deak@intel.com>]

On Thu, May 09, 2019 at 06:50:42PM +0300, Ville Syrjälä wrote:
> On Thu, May 09, 2019 at 09:19:51AM +0300, Imre Deak wrote:
> > The power get/put was added in
> > 
> > commit 1c767b339b3938b19076ffdc9d70aa1e4235a45b
> > Author: Imre Deak <imre.deak@intel.com>
> > Date:   Mon Aug 18 14:42:42 2014 +0300
> > 
> >     drm/i915: take display port power domain in DP HPD handle
> > 
> > to account for the HW access in ibx_digital_port_connected(). This
> > latter call was in turn removed in
> > 
> > commit 7d23e3c37bb3fc6952dc84007ee60cb533fd2d5c
> > Author: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > Date:   Wed Mar 30 18:05:23 2016 +0530
> > 
> >     drm/i915: Cleaning up intel_dp_hpd_pulse
> > 
> > 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().
> > 
> > The other spot is in the PSR code doing register access, for that we can
> > use the DISPLAY_CORE power domain in a similar way done in the previous
> > patch.
> 
> I don't think the PSR code should touch the hardware unless the crtc is
> active. So not sure it really needs anything.

Ah, right, thanks for spotting it. I noticed the access in

intel_psr_short_pulse()->intel_psr_disable_locked()

only by code inspection, didn't think of checking the state in which it
can be called (even though the fn name should've been indicative). So
yes PSR is enabled at that point and the modeset should have already all
the power references needed for that access. I will remove the get/put
from intel_psr_short_pulse().

> 
> > 
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  | 20 ++++----------------
> >  drivers/gpu/drm/i915/intel_psr.c |  6 ++++++
> >  2 files changed, 10 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 24cca12a9a3e..de881bfea011 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -6308,9 +6308,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) {
> >  		/*
> > @@ -6333,9 +6330,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) {
> >  			/*
> > @@ -6347,7 +6341,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;
> >  		}
> >  	}
> >  
> > @@ -6357,17 +6352,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 */
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 963663ba0edf..856a39c7ee77 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -1251,10 +1251,13 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> >  	const u8 errors = DP_PSR_RFB_STORAGE_ERROR |
> >  			  DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR |
> >  			  DP_PSR_LINK_CRC_ERROR;
> > +	intel_wakeref_t wakeref;
> >  
> >  	if (!CAN_PSR(dev_priv) || !intel_dp_is_edp(intel_dp))
> >  		return;
> >  
> > +	wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > +
> >  	mutex_lock(&psr->lock);
> >  
> >  	if (!psr->enabled || psr->dp != intel_dp)
> > @@ -1294,6 +1297,9 @@ void intel_psr_short_pulse(struct intel_dp *intel_dp)
> >  	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_ERROR_STATUS, val);
> >  exit:
> >  	mutex_unlock(&psr->lock);
> > +
> > +	intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > +				      wakeref);
> >  }
> >  
> >  bool intel_psr_enabled(struct intel_dp *intel_dp)
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx