On Tue, Dec 20, 2016 at 06:51:17PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote: > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Apparently some VLV BIOSen like to leave the VDD force bit enabled > even for power seqeuncers that aren't properly hooked up to any > port. That will result in a imbalance in the AUX power domain > refcount when we stat to use said power sequencer as edp_panel_vdd_on() > will not grab the power domain reference if it sees that the VDD is > already on. > > To fix this let's make sure we turn off the VDD force bit when we > initialize the power sequencer registers. That is, unless it's > being done from the init path since there we are actually > initializing the registers for the current power sequencer and > we don't want to turn VDD off needlessly as that would require > waiting for the power cycle delay before we turn it back on. > > This fixes the following kind of warnings: > WARNING: CPU: 0 PID: 123 at ../drivers/gpu/drm/i915/intel_runtime_pm.c:1455 intel_display_power_put+0x13a/0x170 [i915]() > WARN_ON(!power_domains->domain_use_count[domain]) > ... > > Cc: stable@xxxxxxxxxxxxxxx > Cc: Matwey V. Kornilov <matwey.kornilov@xxxxxxxxx> > Tested-by: Matwey V. Kornilov <matwey.kornilov@xxxxxxxxx> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98695 > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_dp.c | 42 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 66b5bc80b781..4139122704b3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -383,7 +383,8 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev, > struct intel_dp *intel_dp); > static void > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, > - struct intel_dp *intel_dp); > + struct intel_dp *intel_dp, > + bool force_disable_vdd); > static void > intel_dp_pps_init(struct drm_device *dev, struct intel_dp *intel_dp); > > @@ -567,7 +568,7 @@ vlv_power_sequencer_pipe(struct intel_dp *intel_dp) > > /* init power sequencer on this pipe and port */ > intel_dp_init_panel_power_sequencer(dev, intel_dp); > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, true); > > /* > * Even vdd force doesn't work until we've made > @@ -604,7 +605,7 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp) > * Only the HW needs to be reprogrammed, the SW state is fixed and > * has been setup during connector init. > */ > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, false); > > return 0; > } > @@ -687,7 +688,7 @@ vlv_initial_power_sequencer_setup(struct intel_dp *intel_dp) > port_name(port), pipe_name(intel_dp->pps_pipe)); > > intel_dp_init_panel_power_sequencer(dev, intel_dp); > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, false); > } > > void intel_power_sequencer_reset(struct drm_i915_private *dev_priv) > @@ -2981,7 +2982,7 @@ static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp) > > /* init power sequencer on this pipe and port */ > intel_dp_init_panel_power_sequencer(dev, intel_dp); > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, true); > } > > static void vlv_pre_enable_dp(struct intel_encoder *encoder, > @@ -5152,7 +5153,8 @@ intel_dp_init_panel_power_sequencer(struct drm_device *dev, > > static void > intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, > - struct intel_dp *intel_dp) > + struct intel_dp *intel_dp, > + bool force_disable_vdd) > { > struct drm_i915_private *dev_priv = to_i915(dev); > u32 pp_on, pp_off, pp_div, port_sel = 0; > @@ -5165,6 +5167,32 @@ intel_dp_init_panel_power_sequencer_registers(struct drm_device *dev, > > intel_pps_get_registers(dev_priv, intel_dp, ®s); > > + /* > + * One some VLV machines the BIOS can leave the VDD On > + * enabled even on power seqeuencers which aren't > + * even hooked up to any port. This would mess up Remove even here > + * the power domain tracking the first time we pick > + * one of these power sequencers for use since > + * edp_panel_vdd_on() would notice that the VDD was > + * already on and therefore wouldn't even grab the And here. > + * power domain reference. Disable VDD first to avoid > + * this. This also avoids spuriously turning the VDD > + * on as soon as the new power seqeuencer gets > + * initialized. > + */ > + if (force_disable_vdd) { > + u32 pp = ironlake_get_pp_control(intel_dp); > + > + WARN(pp & PANEL_POWER_ON, "Panel power already on\n"); Wouldn't this just replace one warning with another? Or is this a subset of the other warning? > + > + if (pp & EDP_FORCE_VDD) > + DRM_DEBUG_KMS("VDD already on, disabling first\n"); > + > + pp &= ~EDP_FORCE_VDD; > + > + I915_WRITE(regs.pp_ctrl, pp); > + } > + > pp_on = (seq->t1_t3 << PANEL_POWER_UP_DELAY_SHIFT) | > (seq->t8 << PANEL_LIGHT_ON_DELAY_SHIFT); > pp_off = (seq->t9 << PANEL_LIGHT_OFF_DELAY_SHIFT) | > @@ -5219,7 +5247,7 @@ static void intel_dp_pps_init(struct drm_device *dev, > vlv_initial_power_sequencer_setup(intel_dp); > } else { > intel_dp_init_panel_power_sequencer(dev, intel_dp); > - intel_dp_init_panel_power_sequencer_registers(dev, intel_dp); > + intel_dp_init_panel_power_sequencer_registers(dev, intel_dp, false); > } > } > > -- > 2.10.2 Kind regards, David -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html