| Message ID | 1400007071-1289-1-git-send-email-clinton.a.taylor@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + } > + } > + return 0; > +} > + > static bool edp_have_panel_power(struct intel_dp *intel_dp) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > @@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) > mutex_lock(&dev->mode_config.mutex); > edp_panel_vdd_off_sync(intel_dp); > mutex_unlock(&dev->mode_config.mutex); > + if (intel_dp->edp_notifier.notifier_call) { > + unregister_reboot_notifier(&intel_dp->edp_notifier); > + intel_dp->edp_notifier.notifier_call = NULL; > + } > } > kfree(intel_dig_port); > } > @@ -3782,6 +3821,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > if (is_edp(intel_dp)) { > intel_dp_init_panel_power_timestamps(intel_dp); > intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); > + if (IS_VALLEYVIEW(dev)) { > + intel_dp->edp_notifier.notifier_call = edp_notify_handler; > + register_reboot_notifier(&intel_dp->edp_notifier); > + } > } > > intel_dp_aux_init(intel_dp, intel_connector); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 328b1a7..1ea193a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -512,6 +512,7 @@ struct intel_dp { > bool psr_setup_done; > bool use_tps3; > struct intel_connector *attached_connector; > + struct notifier_block edp_notifier; > > uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index); > /* > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. Thanks, -- Jesse Barnes, Intel Open Source Technology Center
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? -Daniel
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.
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); + } + } + return 0; +} + static bool edp_have_panel_power(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); @@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder) mutex_lock(&dev->mode_config.mutex); edp_panel_vdd_off_sync(intel_dp); mutex_unlock(&dev->mode_config.mutex); + if (intel_dp->edp_notifier.notifier_call) { + unregister_reboot_notifier(&intel_dp->edp_notifier); + intel_dp->edp_notifier.notifier_call = NULL; + } } kfree(intel_dig_port); } @@ -3782,6 +3821,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, if (is_edp(intel_dp)) { intel_dp_init_panel_power_timestamps(intel_dp); intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq); + if (IS_VALLEYVIEW(dev)) { + intel_dp->edp_notifier.notifier_call = edp_notify_handler; + register_reboot_notifier(&intel_dp->edp_notifier); + } } intel_dp_aux_init(intel_dp, intel_connector); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 328b1a7..1ea193a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -512,6 +512,7 @@ struct intel_dp { bool psr_setup_done; bool use_tps3; struct intel_connector *attached_connector; + struct notifier_block edp_notifier; uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index); /*