On Fri, Aug 05, 2016 at 03:45:19PM -0700, Clint Taylor wrote:
> On 08/03/2016 06:36 AM, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Dell UP2414Q doesn't like it when we're driving it in MST mode, and then
> > reboot the machine. After reboot, the monitor is somehow confused and
> > refuses to do the payload allocation:
> > [drm:drm_dp_mst_allocate_vcpi] initing vcpi for 282 8
> > [drm:drm_dp_dpcd_write_payload] status not set after read payload table status 0
> > and thus we're left with a black screen until the monitor power is
> > toggled.
> >
> > It seems that if we shut down things gracefully prior to rebooting, the
> > monitor doesn't get confused like this. So let's try to shut down all
> > the displays gracefully on reboot. The downside is that we will
> > introduce the reboot notifier to all the modesetl locks. So if there's
> > a locking bug around, we might not be able to reboot the machine
> > gracefully. sysrq reboot will still work though, as it will not
> > call the notifiers.
> >
> > While we do this, we can eliminate the eDP reboot notifier, which is
> > there to shut down the panel power and make sure the firmware won't
> > violate the panel power cycle delay post-reboot. Since we're now
> > shutting eDP down properly, we can mostly just rip out the eDP notifier.
> > We still need to do the panel power cycle wait though, as that would
> > normally get postponed until the next modeset. So let's move that part
> > into intel_panel so that other display types will get the same treatment
> > as a bonus.
> >
> > The Dell UP2414Q can often get even more confused, and sometimes what
> > you have to do is: switch to another input on the monitor, toggle the
> > monitor power, switch the input back to the original setting. And
> > sometimes it seems you just have to yank the power cable entirely. I'm
> > not sure if this reboot notifier might avoid some of these other
> > failure modes as well, but I'm pretty sure it can't hurt at least.
> >
>
> While testing this change on SKL if 2 crtc's are enabled resume from
> suspend is 500ms longer than a single crtc resume. Are we calling the
> T12 msleep(500) for non eDP panels?
Seems unlikely, unless we misdetect DP as eDP. 500ms sure sounds a lot
for non-eDP though.
>
> During testing VDD to the panel, the T12 panel timeout requirement is
> being meet at >500ms during reboot and suspend/resume.
>
> > Cc: Clint Taylor <clinton.a.taylor@xxxxxxxxx>
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 ++
> > drivers/gpu/drm/i915/intel_display.c | 24 ++++++++++++++++++
> > drivers/gpu/drm/i915/intel_dp.c | 49 ++----------------------------------
> > drivers/gpu/drm/i915/intel_drv.h | 7 +++---
> > drivers/gpu/drm/i915/intel_dsi.c | 3 ++-
> > drivers/gpu/drm/i915/intel_dvo.c | 2 +-
> > drivers/gpu/drm/i915/intel_lvds.c | 3 ++-
> > drivers/gpu/drm/i915/intel_panel.c | 29 ++++++++++++++++++++-
> > 8 files changed, 65 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 65ada5d2f88c..f8eb8c7de007 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1727,6 +1727,8 @@ struct intel_wm_config {
> > struct drm_i915_private {
> > struct drm_device drm;
> >
> > + struct notifier_block reboot_notifier;
> > +
> > struct kmem_cache *objects;
> > struct kmem_cache *vmas;
> > struct kmem_cache *requests;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a8e8cc8dfae9..2192a21aa6c9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -31,6 +31,8 @@
> > #include <linux/kernel.h>
> > #include <linux/slab.h>
> > #include <linux/vgaarb.h>
> > +#include <linux/notifier.h>
> > +#include <linux/reboot.h>
> > #include <drm/drm_edid.h>
> > #include <drm/drmP.h>
> > #include "intel_drv.h"
> > @@ -15578,6 +15580,23 @@ fail:
> > drm_modeset_acquire_fini(&ctx);
> > }
> >
> > +
> > +static int intel_reboot_notifier(struct notifier_block *nb,
> > + unsigned long code, void *unused)
> > +{
> > + struct drm_i915_private *dev_priv =
> > + container_of(nb, typeof(*dev_priv), reboot_notifier);
> > +
> > + if (code != SYS_RESTART)
> > + return 0;
> > +
> > + intel_display_suspend(&dev_priv->drm);
> > +
>
> Need to check to make sure there is a panel before calling
> intel_panel_reboot_notifier().
>
> > + intel_panel_reboot_notifier(dev_priv);
> > +
> > + return 0;
> > +}
> > +
> > void intel_modeset_init(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > @@ -15706,6 +15725,9 @@ void intel_modeset_init(struct drm_device *dev)
> > * since the watermark calculation done here will use pstate->fb.
> > */
> > sanitize_watermarks(dev);
> > +
> > + dev_priv->reboot_notifier.notifier_call = intel_reboot_notifier;
> > + register_reboot_notifier(&dev_priv->reboot_notifier);
> > }
> >
> > static void intel_enable_pipe_a(struct drm_device *dev)
> > @@ -16291,6 +16313,8 @@ void intel_modeset_cleanup(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = to_i915(dev);
> >
> > + unregister_reboot_notifier(&dev_priv->reboot_notifier);
> > +
> > intel_disable_gt_powersave(dev_priv);
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 001f74fc0ce5..d8d13a9e33e5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -28,8 +28,6 @@
> > #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_atomic_helper.h>
> > #include <drm/drm_crtc.h>
> > @@ -631,42 +629,6 @@ _pp_stat_reg(struct intel_dp *intel_dp)
> > return regs.pp_stat;
> > }
> >
> > -/* Reboot notifier handler to shutdown panel power to guarantee T12 timing
> > - This function only applicable when panel PM state is not to be tracked */
> > -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 drm_device *dev = intel_dp_to_dev(intel_dp);
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > -
> > - if (!is_edp(intel_dp) || code != SYS_RESTART)
> > - return 0;
> > -
> > - pps_lock(intel_dp);
> > -
> > - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > - enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> > - i915_reg_t pp_ctrl_reg, pp_div_reg;
> > - u32 pp_div;
> > -
> > - pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> > - pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> > - pp_div = I915_READ(pp_div_reg);
> > - pp_div &= PP_REFERENCE_DIVIDER_MASK;
> > -
> > - /* 0x1F write to PP_DIV_REG sets max cycle delay */
> > - I915_WRITE(pp_div_reg, pp_div | 0x1F);
> > - I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> > - msleep(intel_dp->panel_power_cycle_delay);
> > - }
> > -
> > - pps_unlock(intel_dp);
> > -
> > - return 0;
> > -}
> > -
> > static bool edp_have_panel_power(struct intel_dp *intel_dp)
> > {
> > struct drm_device *dev = intel_dp_to_dev(intel_dp);
> > @@ -4562,11 +4524,6 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
> > pps_lock(intel_dp);
> > edp_panel_vdd_off_sync(intel_dp);
> > pps_unlock(intel_dp);
> > -
> > - if (intel_dp->edp_notifier.notifier_call) {
> > - unregister_reboot_notifier(&intel_dp->edp_notifier);
> > - intel_dp->edp_notifier.notifier_call = NULL;
> > - }
> > }
> >
> > intel_dp_aux_fini(intel_dp);
> > @@ -5465,9 +5422,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> > mutex_unlock(&dev->mode_config.mutex);
> >
> > if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) {
> > - intel_dp->edp_notifier.notifier_call = edp_notify_handler;
> > - register_reboot_notifier(&intel_dp->edp_notifier);
> > -
> > /*
> > * Figure out the current pipe for the initial backlight setup.
> > * If the current pipe isn't valid, try the PPS pipe, and if that
> > @@ -5488,7 +5442,8 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> > pipe_name(pipe));
> > }
> >
> > - intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> > + intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode,
> > + intel_dp->panel_power_cycle_delay);
> > intel_connector->panel.backlight.power = intel_edp_backlight_power;
> > intel_panel_setup_backlight(connector, pipe);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 50cdc890d956..92fa66a02ca5 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -226,6 +226,7 @@ struct intel_panel {
> > struct drm_display_mode *fixed_mode;
> > struct drm_display_mode *downclock_mode;
> > int fitting_mode;
> > + int reboot_power_cycle_delay;
> >
> > /* backlight */
> > struct {
> > @@ -877,8 +878,6 @@ struct intel_dp {
> > unsigned long last_backlight_off;
> > ktime_t panel_power_off_time;
> >
> > - struct notifier_block edp_notifier;
> > -
> > /*
> > * Pipe whose power sequencer is currently locked into
> > * this port. Only relevant on VLV/CHV.
> > @@ -1521,8 +1520,10 @@ void intel_overlay_reset(struct drm_i915_private *dev_priv);
> > /* intel_panel.c */
> > int intel_panel_init(struct intel_panel *panel,
> > struct drm_display_mode *fixed_mode,
> > - struct drm_display_mode *downclock_mode);
> > + struct drm_display_mode *downclock_mode,
> > + int reboot_power_cycle_delay);
> > void intel_panel_fini(struct intel_panel *panel);
> > +void intel_panel_reboot_notifier(struct drm_i915_private *dev_priv);
> > void intel_fixed_panel_mode(const struct drm_display_mode *fixed_mode,
> > struct drm_display_mode *adjusted_mode);
> > void intel_pch_panel_fitting(struct intel_crtc *crtc,
> > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> > index de8e9fb51595..5b722ec23381 100644
> > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > @@ -1596,7 +1596,8 @@ void intel_dsi_init(struct drm_device *dev)
> > connector->display_info.width_mm = fixed_mode->width_mm;
> > connector->display_info.height_mm = fixed_mode->height_mm;
> >
> > - intel_panel_init(&intel_connector->panel, fixed_mode, NULL);
> > + intel_panel_init(&intel_connector->panel, fixed_mode, NULL,
> > + intel_dsi->panel_pwr_cycle_delay);
> > intel_panel_setup_backlight(connector, INVALID_PIPE);
> >
> > intel_dsi_add_properties(intel_connector);
> > diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
> > index 47bdf9dad0d3..c2c9c922590c 100644
> > --- a/drivers/gpu/drm/i915/intel_dvo.c
> > +++ b/drivers/gpu/drm/i915/intel_dvo.c
> > @@ -547,7 +547,7 @@ void intel_dvo_init(struct drm_device *dev)
> > */
> > intel_panel_init(&intel_connector->panel,
> > intel_dvo_get_current_mode(connector),
> > - NULL);
> > + NULL, 0);
> > intel_dvo->panel_wants_dither = true;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 49550470483e..07d7ac2c1520 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -1120,7 +1120,8 @@ void intel_lvds_init(struct drm_device *dev)
> > out:
> > mutex_unlock(&dev->mode_config.mutex);
> >
> > - intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> > + /* FIXME fill in an accurate power cycle delay */
> > + intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode, 0);
> > intel_panel_setup_backlight(connector, INVALID_PIPE);
> >
> > lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> > index 96c65d77e886..fe57a743ad21 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1781,14 +1781,41 @@ intel_panel_init_backlight_funcs(struct intel_panel *panel)
> > }
> > }
> >
> > +/*
> > + * Make sure the power cycle delay is respected. The PPS
> > + * does supposedly initiate a power cycle on reset, but it
> > + * also resets the power cycle delay register value to the
> > + * default value, and hence may not wait long enough if the
> > + * firmware attempts to power up the panel immediately.
> > + * Also eg. DSI doesn't use the PPS.
> > + */
> > +void intel_panel_reboot_notifier(struct drm_i915_private *dev_priv)
> > +{
> > + struct drm_device *dev = &dev_priv->drm;
> > + struct intel_connector *connector;
> > + int reboot_power_cycle_delay = 0;
> > +
> > + for_each_intel_connector(dev, connector) {
> > + struct intel_panel *panel = &connector->panel;
> > +
> > + reboot_power_cycle_delay = max(reboot_power_cycle_delay,
> > + panel->reboot_power_cycle_delay);
> > + }
> > +
> > + if (reboot_power_cycle_delay)
> > + msleep(reboot_power_cycle_delay);
> > +}
> > +
> > int intel_panel_init(struct intel_panel *panel,
> > struct drm_display_mode *fixed_mode,
> > - struct drm_display_mode *downclock_mode)
> > + struct drm_display_mode *downclock_mode,
> > + int reboot_power_cycle_delay)
> > {
> > intel_panel_init_backlight_funcs(panel);
> >
> > panel->fixed_mode = fixed_mode;
> > panel->downclock_mode = downclock_mode;
> > + panel->reboot_power_cycle_delay = reboot_power_cycle_delay;
> >
> > return 0;
> > }
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
