On Wed, Aug 03, 2016 at 04:36:18PM +0300, [email protected] wrote: > From: Ville Syrjälä <[email protected]> > > 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.
struct pci_driver has a ->shutdown hook which is currently not defined in i915_pci_driver. How about using that instead of reboot_notifier to avoid potential locking issues? Best regards, Lukas > > 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. > > Cc: Clint Taylor <[email protected]> > Cc: Paulo Zanoni <[email protected]> > Signed-off-by: Ville Syrjälä <[email protected]> > --- > 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); > + > + 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; > } > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
