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

Submitted by Imre Deak on May 9, 2019, 6:19 a.m.

Details

Message ID 20190509061954.10379-9-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Add support for asynchronous display power disabling" ( rev: 3 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak May 9, 2019, 6:19 a.m.
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.

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(-)

Patch hide | download patch | download mbox

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)

Comments

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.

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