Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 14 May 2014 00:32:30 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, May 13, 2014 at 02:53:22PM -0700, Jesse Barnes wrote:
> > On Tue, 13 May 2014 22:26:08 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Tue, May 13, 2014 at 11:51:11AM -0700, clinton.a.taylor@intel.com wrote:
> > > > From: Clint Taylor <Clinton.A.Taylor@intel.com>
> > > > 
> > > > The panel power sequencer on vlv doesn't appear to accept changes to its T12 power down duration during warm reboots. This change forces a delay for warm reboots to the T12 panel timing as defined in the VBT table for the connected panel.
> > > > 
> > > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |    1 +
> > > >  2 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 5ca68aa9..03ac64f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -28,6 +28,8 @@
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/export.h>
> > > > +#include <linux/notifier.h>
> > > > +#include <linux/reboot.h>
> > > >  #include <drm/drmP.h>
> > > >  #include <drm/drm_crtc.h>
> > > >  #include <drm/drm_crtc_helper.h>
> > > > @@ -302,6 +304,39 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
> > > >  		return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> > > >  }
> > > >  
> > > > +/* Reboot notifier handler to shutdown panel power to gaurantee T12 timing */
> > > > +static int edp_notify_handler(struct notifier_block *this, unsigned long code,
> > > > +							  void *unused)
> > > > +{
> > > > +	struct intel_dp *intel_dp = container_of(this, typeof(* intel_dp),
> > > > +						 edp_notifier);
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	u32 pp;
> > > > +	u32 pp_ctrl_reg, pp_div_reg;
> > > > +	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> > > > +
> > > > +	if (!is_edp(intel_dp))
> > > > +		return 0;
> > > > +
> > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > +		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> > > > +		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> > > > +		if (code == SYS_RESTART) {
> > > > +			pr_crit("eDP Notifier Handler\n");
> > > > +			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
> > > > +			pp &= PP_REFERENCE_DIVIDER_MASK;
> > > > +			I915_WRITE(pp_div_reg , pp | 0x1F);
> > > > +			I915_WRITE(pp_ctrl_reg,
> > > > +				   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> > > > +			/* VBT entries are in tenths of uS convert to mS */
> > > > +			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);
> > > 
> > > Imo just compute the desired delay before setting up this reboot handler
> > > and pass it as the argument.
> > > 
> > > But that leaves a bit the question why we don't need that everywhere else
> > > and what exactly this does? I'm a bit confused with the commit message.
> > > -Daniel
> > 
> > We could potentially do it on other platforms, but it's really only
> > needed on those that don't do any display stuff in the BIOS (e.g.
> > Chromebooks).  In that case, a warm reboot might immediately jump back
> > to driver init, and if we haven't full done a PPS, we'll get into
> > trouble and potentially confuse the panel in the worst case.
> > 
> > Previous BIOS-free systems may have had this problem too, but this was
> > the first one we got an actual eDP timing spec violation about, so it's
> > a good first step.  It can easily be extended as needed.
> > 
> > Computing the delay ahead of time in eDP init would make it more
> > platform agnostic, and we could key off a separate flag as to whether
> > the delay was needed, but the above is pretty simple too, albeit VLV
> > specific.
> > 
> > The pr_crit() could probably be dropped though, and the magic 0x1f
> > needs a comment.
> 
> In that case I think rolling this out everywhere can't hurt. Gives us a
> notch more testing and rebooting tends to not be a performance critical
> thing really.
> 
> Otoh I've thought the hardware ensure that the t12 timing is obeyed under
> all cases? Does that get lost in the reset?

Yeah on reset the PPS will get reset too, so the off->on delay may not
be honored.

-- 
Jesse Barnes, Intel Open Source Technology Center