Re: [PATCH 3/6] drm/i915/lvds: Restore initial HW state during encoder enabling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Aug 09, 2016 at 05:40:32PM +0300, Imre Deak wrote:
> On ti, 2016-08-09 at 15:48 +0300, Ville Syrjälä wrote:
> > On Tue, Aug 09, 2016 at 02:34:09PM +0300, Imre Deak wrote:
> > > Atm the LVDS encoder depends on the PPS HW context being saved/restored
> > > from generic suspend/resume code. Since the PPS is specific to the LVDS
> > > and eDP encoders a cleaner way is to reinitialize it during encoder
> > > enabling, so do this here for LVDS. Follow-up patches will init the PPS
> > > for the eDP encoder similarly and remove the suspend/resume time save /
> > > restore.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h   |   1 +
> > >  drivers/gpu/drm/i915/intel_lvds.c | 103 +++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 91 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index 889508f..da82744 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -3710,6 +3710,7 @@ enum {
> > >  
> > >  #define _PP_ON_DELAYS			0x61208
> > >  #define PP_ON_DELAYS(pps_idx)		_MMIO_PPS(pps_idx, _PP_ON_DELAYS)
> > > +#define  PANEL_PORT_SELECT_SHIFT	30
> > >  #define  PANEL_PORT_SELECT_MASK		(3 << 30)
> > >  #define  PANEL_PORT_SELECT_LVDS		(0 << 30)
> > >  #define  PANEL_PORT_SELECT_DPA		(1 << 30)
> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > > index c5739fc..d5158e5 100644
> > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > @@ -48,6 +48,20 @@ struct intel_lvds_connector {
> > >  	struct notifier_block lid_notifier;
> > >  };
> > >  
> > > +struct intel_lvds_pps {
> > > +	/* 100us units */
> > > +	int t1_t2;
> > > +	int t3;
> > > +	int t4;
> > > +	int t5;
> > > +	int tx;
> > > +
> > > +	int divider;
> > > +
> > > +	int port;
> > > +	bool reset_on_powerdown;
> > > +};
> > > +
> > >  struct intel_lvds_encoder {
> > >  	struct intel_encoder base;
> > >  
> > > @@ -55,6 +69,9 @@ struct intel_lvds_encoder {
> > >  	i915_reg_t reg;
> > >  	u32 a3_power;
> > >  
> > > +	struct intel_lvds_pps init_pps;
> > > +	u32 init_lvds_val;
> > > +
> > >  	struct intel_lvds_connector *attached_connector;
> > >  };
> > >  
> > > @@ -136,6 +153,72 @@ static void intel_lvds_get_config(struct intel_encoder *encoder,
> > >  	pipe_config->base.adjusted_mode.crtc_clock = pipe_config->port_clock;
> > >  }
> > >  
> > > +static void intel_lvds_pps_get_hw_state(struct drm_i915_private *dev_priv,
> > > +					struct intel_lvds_pps *pps)
> > > +{
> > > +	u32 val;
> > > +
> > > +	pps->reset_on_powerdown = I915_READ(PP_CONTROL(0)) & PANEL_POWER_RESET;
> > > +
> > > +	val = I915_READ(PP_ON_DELAYS(0));
> > > +	pps->port = (val & PANEL_PORT_SELECT_MASK) >>
> > > +		    PANEL_PORT_SELECT_SHIFT;
> > > +	pps->t1_t2 = (val & PANEL_POWER_UP_DELAY_MASK) >>
> > > +		     PANEL_POWER_UP_DELAY_SHIFT;
> > > +	pps->t5 = (val & PANEL_LIGHT_ON_DELAY_MASK) >>
> > > +		  PANEL_LIGHT_ON_DELAY_SHIFT;
> > > +
> > > +	val = I915_READ(PP_OFF_DELAYS(0));
> > > +	pps->t3 = (val & PANEL_POWER_DOWN_DELAY_MASK) >>
> > > +		  PANEL_POWER_DOWN_DELAY_SHIFT;
> > > +	pps->tx = (val & PANEL_LIGHT_OFF_DELAY_MASK) >>
> > > +		  PANEL_LIGHT_OFF_DELAY_SHIFT;
> > > +
> > > +	val = I915_READ(PP_DIVISOR(0));
> > > +	pps->divider = (val & PP_REFERENCE_DIVIDER_MASK) >>
> > > +		       PP_REFERENCE_DIVIDER_SHIFT;
> > > +	/* Convert from 100ms to 100us units */
> > > +	pps->t4 = ((val & PANEL_POWER_CYCLE_DELAY_MASK) >>
> > > +		   PANEL_POWER_CYCLE_DELAY_SHIFT) * 1000;
> > 
> > This should have the +1 offset handling I think.
> 
> Ok, will add that.
> 
> > Hmm. Looks like we mess that up even for eDP :(
> > > +
> > > +	if (INTEL_INFO(dev_priv)->gen <= 4 &&
> > > +	    pps->t1_t2 == 0 && pps->t5 == 0 && pps->t3 == 0 && pps->tx == 0) {
> > > +		DRM_DEBUG_KMS("Panel power timings uninitialized, "
> > > +			      "setting defaults\n");
> > > +		/* Set T2 to 40ms and T5 to 200ms in 100 usec units */
> > > +		pps->t1_t2 = 40 * 10;
> > > +		pps->t5 = 200 * 10;
> > > +		/* Set T3 to 35ms and Tx to 200ms in 100 usec units */
> > > +		pps->t3 = 35 * 10;
> > > +		pps->tx = 200 * 10;
> > 
> > Not sure where these came from originally. The spgw spec tells us:
> > 
> > min	max
> > t1	0.5	10
> > t2	0	50
> > t3	0	50
> > t4	200	-
> > t5	200	-
> > t6	200	-
> > t7	0	10
> > 
> > So maybe we should at least set t4=200 here based on the minimum t4
> > delay in the spec, probably should add the max t7 time there as well
> > as that's what the power cycle delays seems to be really: t4+t7.
> > Bspec seems to claim that spgw spec has an upper limit of 400 for this,
> > but I can't see that at least in the copy I found.
> > 
> > And maybe bump up t1_t2=60 and t3=50 based on the max delay for those
> > in the spec?
> 
> Haven't found yet the spec, but the above seem like reasonable
> defaults.
> 
> > 
> > Anyways, this is all material for a separate patch for sure.
> 
> Yep, I kept the behavior here as-is.
> 
> > 
> > > +	}
> > > +
> > > +	DRM_DEBUG_DRIVER("LVDS PPS:t1+t2 %d t3 %d t4 %d t5 %d tx %d "
> > > +			 "divider %d port %d reset_on_powerdown %d\n",
> > > +			 pps->t1_t2, pps->t3, pps->t4, pps->t5, pps->tx,
> > > +			 pps->divider, pps->port, pps->reset_on_powerdown);
> > > +}
> > > +
> > > +static void intel_lvds_pps_init_hw(struct drm_i915_private *dev_priv,
> > > +				   struct intel_lvds_pps *pps)
> > > +{
> > > +	u32 val;
> > > +
> > > +	val = I915_READ(PP_CONTROL(0));
> > > +	WARN_ON((val & PANEL_UNLOCK_MASK) != PANEL_UNLOCK_REGS);
> > > +	if (pps->reset_on_powerdown)
> > > +		val |= PANEL_POWER_RESET;
> > 
> > Hmm. Maybe we can just always set this bit?
> 
> Seems reasonable (for GEN3+), but as also as a follow-up. I just wonder
> if the w/a below should be then also applied at the same time. BSpec
> doesn't specify the w/a except for IBX, but it seems like the safer
> thing to do.

The problem with the w/a is that it opens up a time window when a reset
will then not initiate a power down cycle. Not sure that's a huge
concern though since the hardware can't apparently guarantee that any
power down cycle initiated via a reset is actually long enough.

The notes in the spec do not indicate that the problem would have been
observed on any other platform.

> 
> > Hmm, we also totally fail to deal with the related IBX w/a
> > in the LVDS code. Should probably fix that up as well as a
> > followup.
> 
> Ok.
> 
> > > +	I915_WRITE(PP_CONTROL(0), val);
> > > +
> > > +	I915_WRITE(PP_ON_DELAYS(0), (pps->port << PANEL_PORT_SELECT_SHIFT) |
> > > +				    (pps->t1_t2 << PANEL_POWER_UP_DELAY_SHIFT) |
> > > +				    (pps->t5 << PANEL_LIGHT_ON_DELAY_SHIFT));
> > > +	I915_WRITE(PP_OFF_DELAYS(0), (pps->t3 << PANEL_POWER_DOWN_DELAY_SHIFT) |
> > > +				     (pps->tx << PANEL_LIGHT_OFF_DELAY_SHIFT));
> > > +	I915_WRITE(PP_DIVISOR(0), (pps->divider << PP_REFERENCE_DIVIDER_SHIFT) |
> > > +				  ((pps->t4 / 1000) << PANEL_POWER_CYCLE_DELAY_SHIFT));
> > 
> > DIV_ROUND_UP() I think, and maybe the +1 handling. Though I can't really
> > tell if the spec has intended the +1 to be a replacement for rounding
> > up, or do we need to do both. Both would seem like the safe choice. Gen2
> > Bspec doesn't mention the +1, but it says writing 0 will even abort any
> > ongoing delay.
> 
> Ok, will change it doing both rounding and +1.
> 
> > 
> > > +}
> > > +
> > >  static void intel_pre_enable_lvds(struct intel_encoder *encoder)
> > >  {
> > >  	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> > > @@ -154,7 +237,9 @@ static void intel_pre_enable_lvds(struct intel_encoder *encoder)
> > >  		assert_pll_disabled(dev_priv, pipe);
> > >  	}
> > >  
> > > -	temp = I915_READ(lvds_encoder->reg);
> > > +	intel_lvds_pps_init_hw(dev_priv, &lvds_encoder->init_pps);
> > > +
> > > +	temp = lvds_encoder->init_lvds_val;
> > >  	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> > >  
> > >  	if (HAS_PCH_CPT(dev)) {
> > > @@ -922,18 +1007,6 @@ void intel_lvds_init(struct drm_device *dev)
> > >  		DRM_DEBUG_KMS("LVDS is not present in VBT, but enabled anyway\n");
> > >  	}
> > >  
> > > -	 /* Set the Panel Power On/Off timings if uninitialized. */
> > > -	if (INTEL_INFO(dev_priv)->gen < 5 &&
> > > -	    I915_READ(PP_ON_DELAYS(0)) == 0 && I915_READ(PP_OFF_DELAYS(0)) == 0) {
> > > -		/* Set T2 to 40ms and T5 to 200ms */
> > > -		I915_WRITE(PP_ON_DELAYS(0), 0x019007d0);
> > > -
> > > -		/* Set T3 to 35ms and Tx to 200ms */
> > > -		I915_WRITE(PP_OFF_DELAYS(0), 0x015e07d0);
> > > -
> > > -		DRM_DEBUG_KMS("Panel power timings uninitialized, setting defaults\n");
> > > -	}
> > > -
> > >  	lvds_encoder = kzalloc(sizeof(*lvds_encoder), GFP_KERNEL);
> > >  	if (!lvds_encoder)
> > >  		return;
> > > @@ -999,6 +1072,10 @@ void intel_lvds_init(struct drm_device *dev)
> > >  				      dev->mode_config.scaling_mode_property,
> > >  				      DRM_MODE_SCALE_ASPECT);
> > >  	intel_connector->panel.fitting_mode = DRM_MODE_SCALE_ASPECT;
> > > +
> > > +	intel_lvds_pps_get_hw_state(dev_priv, &lvds_encoder->init_pps);
> > > +	lvds_encoder->init_lvds_val = lvds;
> > > +
> > >  	/*
> > >  	 * LVDS discovery:
> > >  	 * 1) check for EDID on DDC
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux