| Message ID | 20190509173446.31095-9-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Add support for asynchronous display power disabling | expand |
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 */
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@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. 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@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 */
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 */
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(). 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(-)