Re: [Intel-gfx] [PATCH] drm/i915: Force VDD off on the new power seqeuencer before starting to use it

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

On Thu, Dec 22, 2016 at 01:57:26PM +0200, David Weinehall wrote:
> On Thu, Dec 22, 2016 at 01:49:39PM +0200, Ville Syrjälä wrote:
> > On Thu, Dec 22, 2016 at 11:39:37AM +0200, David Weinehall wrote:
> > > On Tue, Dec 20, 2016 at 06:51:17PM +0200, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > 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@vger.kernel.org
> > > > Cc: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > > > Tested-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98695
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>

Typos and whatnot fixed and patch pushed to dinq. Thanks for the review.

> 
> > > > ---
> > > >  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, &regs);
> > > >  
> > > > +	/*
> > > > +	 * 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?
> > 
> > This should never happen. But I'd rather we get a bug report if it ever
> > does occur.
> 
> Fair enough!
> 
> > 
> > > 
> > > > +
> > > > +		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
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC