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

Souza, Jose
2019-05-09T17:43:26Z

[-- Attachment #1.1: Type: text/plain, Size: 3656 bytes --]

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

>
> 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@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>
> Reviewed-by: Ville Syrjälä <ville.syrjala@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 */

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]