On Wed, Aug 03, 2016 at 04:36:18PM +0300, 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.
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 <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);
> +
> + 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
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
