Re: [PATCH] drm/i915: Shut down displays gracefully on reboot

[Lukas Wunner <lukas@wunner.de>]

On Wed, Aug 03, 2016 at 04:36:18PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx