From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH v2 10.1/14] drm/i915: Reset power sequencer pipe tracking when disp2d is off Date: Tue, 02 Sep 2014 16:47:06 +0300 Message-ID: <1409665626.15662.12.camel@intelbox> References: <1408470329-11990-1-git-send-email-ville.syrjala@linux.intel.com> <1408717310-20535-1-git-send-email-ville.syrjala@linux.intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1577034187==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id A0D0D6E02D for ; Tue, 2 Sep 2014 06:47:13 -0700 (PDT) In-Reply-To: <1408717310-20535-1-git-send-email-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: ville.syrjala@linux.intel.com Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1577034187== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-3iJf7bbWlEBNyUaSXNjD" --=-3iJf7bbWlEBNyUaSXNjD Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2014-08-22 at 17:21 +0300, ville.syrjala@linux.intel.com wrote: > From: Ville Syrj=C3=A4l=C3=A4 >=20 > The power sequencer loses its state when the disp2d power well is down. > Clear the dev_priv->pps_pipe tracking so that the power sequencer state > gets reinitialized the next time it's needed. >=20 > v2: Fix the pps_mutex vs. power_domain mutex deadlock by taking power > domain reference first >=20 > Signed-off-by: Ville Syrj=C3=A4l=C3=A4 > --- > Imre noticed my previouys attempt was utter garbage. Let's try again. Aga= in maybe > we want to squash with 10/14 or at least move the edp_pps_{lock,unlock}()= introduction > there to avoid churn. >=20 > The alternative of trying to plumb all the power domain calls out from un= der > pps_mutex seemed a bit too nasty to me, so I opted for this approach inst= ead. >=20 > drivers/gpu/drm/i915/intel_dp.c | 157 +++++++++++++++++++++++++--------= ------ > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_pm.c | 2 + > 3 files changed, 106 insertions(+), 54 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel= _dp.c > index d6fe1a2..3468315 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -389,6 +389,67 @@ vlv_initial_power_sequencer_setup(struct intel_dp *i= ntel_dp) > &power_seq); > } > =20 > +void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv) > +{ > + struct drm_device *dev =3D dev_priv->dev; > + struct intel_encoder *encoder; > + > + if (WARN_ON(!IS_VALLEYVIEW(dev))) > + return; > + > + /* > + * We can't grab pps_mutex here due to deadlock with power_domain > + * mutex when power_domain functions are called while holding pps_mutex= . > + * That also means that in order to use pps_pipe the code needs to > + * hold both a power domain reference and pps_mutex, and the power doma= in > + * reference get/put must be done while _not_ holding pps_mutex. > + * edp_pps_{lock,unlock}() do these steps in the correct order, so one > + * should use them always. > + */ > + > + list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)= { > + struct intel_dp *intel_dp; > + > + if (encoder->type !=3D INTEL_OUTPUT_EDP) > + continue; > + > + intel_dp =3D enc_to_intel_dp(&encoder->base); > + intel_dp->pps_pipe =3D INVALID_PIPE; > + } > +} > + > +static void edp_pps_lock(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port =3D dp_to_dig_port(intel_dp); > + struct intel_encoder *encoder =3D &intel_dig_port->base; > + struct drm_device *dev =3D encoder->base.dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + enum intel_display_power_domain power_domain; > + > + /* > + * See vlv_power_sequencer_reset() why we need > + * a power domain reference here. > + */ > + power_domain =3D intel_display_port_power_domain(encoder); > + intel_display_power_get(dev_priv, power_domain); > + > + mutex_lock(&dev_priv->pps_mutex); > +} > + > +static void edp_pps_unlock(struct intel_dp *intel_dp) > +{ > + struct intel_digital_port *intel_dig_port =3D dp_to_dig_port(intel_dp); > + struct intel_encoder *encoder =3D &intel_dig_port->base; > + struct drm_device *dev =3D encoder->base.dev; > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + enum intel_display_power_domain power_domain; > + > + mutex_unlock(&dev_priv->pps_mutex); > + > + power_domain =3D intel_display_port_power_domain(encoder); > + intel_display_power_put(dev_priv, power_domain); > +} > + > static u32 _pp_ctrl_reg(struct intel_dp *intel_dp) > { > struct drm_device *dev =3D intel_dp_to_dev(intel_dp); > @@ -425,7 +486,7 @@ static int edp_notify_handler(struct notifier_block *= this, unsigned long code, > if (!IS_VALLEYVIEW(dev) || !is_edp(intel_dp) || code !=3D SYS_RESTART) > return 0; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > =20 > pipe =3D vlv_power_sequencer_pipe(intel_dp); > =20 > @@ -439,7 +500,7 @@ static int edp_notify_handler(struct notifier_block *= this, unsigned long code, > I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | PANEL_POWER_OFF); > msleep(intel_dp->panel_power_cycle_delay); > =20 > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > =20 > return 0; > } > @@ -458,17 +519,13 @@ static bool edp_have_panel_vdd(struct intel_dp *int= el_dp) > { > struct drm_device *dev =3D intel_dp_to_dev(intel_dp); > struct drm_i915_private *dev_priv =3D dev->dev_private; > - struct intel_digital_port *intel_dig_port =3D dp_to_dig_port(intel_dp); > - struct intel_encoder *intel_encoder =3D &intel_dig_port->base; > - enum intel_display_power_domain power_domain; > =20 > lockdep_assert_held(&dev_priv->pps_mutex); > =20 > - power_domain =3D intel_display_port_power_domain(intel_encoder); > - return intel_display_power_enabled(dev_priv, power_domain) && > - (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) !=3D 0; > + return I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD; > } > =20 > + Redundant w/s change. > static void > intel_dp_check_edp(struct intel_dp *intel_dp) > { > @@ -614,7 +671,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > bool has_aux_irq =3D HAS_AUX_IRQ(dev); > bool vdd; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > =20 > /* > * We will be called with VDD already enabled for dpcd/edid/oui reads. > @@ -731,7 +788,7 @@ out: > if (vdd) > edp_panel_vdd_off(intel_dp, false); > =20 > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > =20 > return ret; > } > @@ -1312,16 +1369,14 @@ static bool edp_panel_vdd_on(struct intel_dp *int= el_dp) > =20 > void intel_edp_panel_vdd_on(struct intel_dp *intel_dp) > { > - struct drm_i915_private *dev_priv =3D > - intel_dp_to_dev(intel_dp)->dev_private; > bool vdd; > =20 > if (!is_edp(intel_dp)) > return; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > vdd =3D edp_panel_vdd_on(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > =20 > WARN(!vdd, "eDP VDD already requested on\n"); > } > @@ -1370,13 +1425,11 @@ static void edp_panel_vdd_work(struct work_struct= *__work) > { > struct intel_dp *intel_dp =3D container_of(to_delayed_work(__work), > struct intel_dp, panel_vdd_work); > - struct drm_i915_private *dev_priv =3D > - intel_dp_to_dev(intel_dp)->dev_private; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > if (!intel_dp->want_panel_vdd) > edp_panel_vdd_off_sync(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > static void edp_panel_vdd_schedule_off(struct intel_dp *intel_dp) > @@ -1414,15 +1467,12 @@ static void edp_panel_vdd_off(struct intel_dp *in= tel_dp, bool sync) > =20 > static void intel_edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync= ) > { > - struct drm_i915_private *dev_priv =3D > - intel_dp_to_dev(intel_dp)->dev_private; > - > if (!is_edp(intel_dp)) > return; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > edp_panel_vdd_off(intel_dp, sync); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > void intel_edp_panel_on(struct intel_dp *intel_dp) > @@ -1437,7 +1487,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp) > =20 > DRM_DEBUG_KMS("Turn eDP power on\n"); > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > =20 > if (edp_have_panel_power(intel_dp)) { > DRM_DEBUG_KMS("eDP power already on\n"); > @@ -1472,7 +1522,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp) > } > =20 > out: > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > void intel_edp_panel_off(struct intel_dp *intel_dp) > @@ -1490,7 +1540,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > =20 > DRM_DEBUG_KMS("Turn eDP power off\n"); > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > =20 > WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); > =20 > @@ -1514,7 +1564,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > power_domain =3D intel_display_port_power_domain(intel_encoder); > intel_display_power_put(dev_priv, power_domain); > =20 > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > void intel_edp_backlight_on(struct intel_dp *intel_dp) > @@ -1540,7 +1590,7 @@ void intel_edp_backlight_on(struct intel_dp *intel_= dp) > */ > wait_backlight_on(intel_dp); > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > =20 > pp =3D ironlake_get_pp_control(intel_dp); > pp |=3D EDP_BLC_ENABLE; > @@ -1550,7 +1600,7 @@ void intel_edp_backlight_on(struct intel_dp *intel_= dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > =20 > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > void intel_edp_backlight_off(struct intel_dp *intel_dp) > @@ -1563,7 +1613,7 @@ void intel_edp_backlight_off(struct intel_dp *intel= _dp) > if (!is_edp(intel_dp)) > return; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > =20 > DRM_DEBUG_KMS("\n"); > pp =3D ironlake_get_pp_control(intel_dp); > @@ -1574,7 +1624,7 @@ void intel_edp_backlight_off(struct intel_dp *intel= _dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > =20 > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > =20 > intel_dp->last_backlight_off =3D jiffies; > edp_wait_backlight_off(intel_dp); As in 9/14 the changes in the backlight functions and in edp_notifier_handler() need to be rebased, with that this patch looks ok: Reviewed-by: Imre Deak > @@ -2290,18 +2340,19 @@ static void vlv_steal_power_sequencer(struct drm_= device *dev, > =20 > list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)= { > struct intel_dp *intel_dp; > + enum port port; > =20 > if (encoder->type !=3D INTEL_OUTPUT_EDP) > continue; > =20 > intel_dp =3D enc_to_intel_dp(&encoder->base); > + port =3D dp_to_dig_port(intel_dp)->port; > =20 > if (intel_dp->pps_pipe !=3D pipe) > continue; > =20 > DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n", > - pipe_name(pipe), > - port_name(dp_to_dig_port(intel_dp)->port)); > + pipe_name(pipe), port_name(port)); > =20 > /* make sure vdd is off before we steal it */ > edp_panel_vdd_off_sync(intel_dp); > @@ -2377,9 +2428,9 @@ static void vlv_pre_enable_dp(struct intel_encoder = *encoder) > mutex_unlock(&dev_priv->dpio_lock); > =20 > if (is_edp(intel_dp)) { > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > vlv_init_panel_power_sequencer(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > intel_enable_dp(encoder); > @@ -2477,9 +2528,9 @@ static void chv_pre_enable_dp(struct intel_encoder = *encoder) > mutex_unlock(&dev_priv->dpio_lock); > =20 > if (is_edp(intel_dp)) { > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > vlv_init_panel_power_sequencer(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > intel_enable_dp(encoder); > @@ -4219,17 +4270,16 @@ void intel_dp_encoder_destroy(struct drm_encoder = *encoder) > { > struct intel_digital_port *intel_dig_port =3D enc_to_dig_port(encoder); > struct intel_dp *intel_dp =3D &intel_dig_port->dp; > - struct drm_device *dev =3D intel_dp_to_dev(intel_dp); > - struct drm_i915_private *dev_priv =3D dev->dev_private; > =20 > drm_dp_aux_unregister(&intel_dp->aux); > intel_dp_mst_encoder_cleanup(intel_dig_port); > drm_encoder_cleanup(encoder); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > edp_panel_vdd_off_sync(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > + > if (intel_dp->edp_notifier.notifier_call) { > unregister_reboot_notifier(&intel_dp->edp_notifier); > intel_dp->edp_notifier.notifier_call =3D NULL; > @@ -4241,15 +4291,13 @@ void intel_dp_encoder_destroy(struct drm_encoder = *encoder) > static void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder= ) > { > struct intel_dp *intel_dp =3D enc_to_intel_dp(&intel_encoder->base); > - struct drm_device *dev =3D intel_dp_to_dev(intel_dp); > - struct drm_i915_private *dev_priv =3D dev->dev_private; > =20 > if (!is_edp(intel_dp)) > return; > =20 > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > edp_panel_vdd_off_sync(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > static void intel_dp_encoder_reset(struct drm_encoder *encoder) > @@ -4727,9 +4775,10 @@ void intel_edp_panel_vdd_sanitize(struct intel_enc= oder *intel_encoder) > if (intel_encoder->type !=3D INTEL_OUTPUT_EDP) > return; > =20 > - mutex_lock(&dev_priv->pps_mutex); > - > intel_dp =3D enc_to_intel_dp(&intel_encoder->base); > + > + edp_pps_lock(intel_dp); > + > if (!edp_have_panel_vdd(intel_dp)) > goto out; > /* > @@ -4744,7 +4793,7 @@ void intel_edp_panel_vdd_sanitize(struct intel_enco= der *intel_encoder) > =20 > edp_panel_vdd_schedule_off(intel_dp); > out: > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > static bool intel_edp_init_connector(struct intel_dp *intel_dp, > @@ -4786,9 +4835,9 @@ static bool intel_edp_init_connector(struct intel_d= p *intel_dp, > } > =20 > /* We now know it's not a ghost, init power sequence regs. */ > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, power_seq)= ; > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > =20 > mutex_lock(&dev->mode_config.mutex); > edid =3D drm_get_edid(connector, &intel_dp->aux.ddc); > @@ -4922,14 +4971,14 @@ intel_dp_init_connector(struct intel_digital_port= *intel_dig_port, > } > =20 > if (is_edp(intel_dp)) { > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > if (IS_VALLEYVIEW(dev)) { > vlv_initial_power_sequencer_setup(intel_dp); > } else { > intel_dp_init_panel_power_timestamps(intel_dp); > intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); > } > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > =20 > intel_dp_aux_init(intel_dp, intel_connector); > @@ -4945,9 +4994,9 @@ intel_dp_init_connector(struct intel_digital_port *= intel_dig_port, > drm_dp_aux_unregister(&intel_dp->aux); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(&intel_dp->panel_vdd_work); > - mutex_lock(&dev_priv->pps_mutex); > + edp_pps_lock(intel_dp); > edp_panel_vdd_off_sync(intel_dp); > - mutex_unlock(&dev_priv->pps_mutex); > + edp_pps_unlock(intel_dp); > } > drm_connector_unregister(connector); > drm_connector_cleanup(connector); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_drv.h > index 849a447..32627ae 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -943,6 +943,7 @@ void intel_dp_mst_suspend(struct drm_device *dev); > void intel_dp_mst_resume(struct drm_device *dev); > int intel_dp_max_link_bw(struct intel_dp *intel_dp); > void intel_dp_hot_plug(struct intel_encoder *intel_encoder); > +void vlv_power_sequencer_reset(struct drm_i915_private *dev_priv); > /* intel_dp_mst.c */ > int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,= int conn_id); > void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_p= ort); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index 39dd066..d5ffda9 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -6328,6 +6328,8 @@ static void vlv_display_power_well_disable(struct d= rm_i915_private *dev_priv, > spin_unlock_irq(&dev_priv->irq_lock); > =20 > vlv_set_power_well(dev_priv, power_well, false); > + > + vlv_power_sequencer_reset(dev_priv); > } > =20 > static void vlv_dpio_cmn_power_well_enable(struct drm_i915_private *dev_= priv, --=-3iJf7bbWlEBNyUaSXNjD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJUBcpaAAoJEORIIAnNuWDFoLcH/3mTC/pB6iKrui4bvbC7l62C 3seW1BdSowHR1Izgq7TYdYZrVo81+Wb+SpZSacTD81kjsqCoKTSP027bFNTOb7ed /PdMfx0sCysD6kMIY/9AjZVU39dGXeCJ6mxRf2cXwJZgYj253VqwbbrdLhtnJF10 AW0nGlzhZOBwKPXd4oyaOTD6SKa4FSrt/XoblweGIdM3ao/Nfsk5v3M9FRx0CIyQ CPqoglSCUK7cxyxmRkxftf710BE6fuwegnhNJ+6o/sN5rkhNXVPnZ6k87onpukOA hbZFX4WpFewxpq09vifQVyAguGCdmmkR+F2FzCm0EDsqkJxCL5dy4RFdIocldpI= =Rsd5 -----END PGP SIGNATURE----- --=-3iJf7bbWlEBNyUaSXNjD-- --===============1577034187== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1577034187==--