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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx