Re: [Intel-gfx] [PATCH v2 3/6] drm/i915: Replace the VLV/CHV eDP reboot notifier with the .shutdown() hook

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 01 Oct 2020, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently VLV/CHV use a reboot notifier to make sure the panel
> power cycle delay isn't violated across a system reboot. Replace
> that with the new encoder .shutdown() hook.
>
> And let's also stop overriding the power cycle delay with the
> max value. No idea why the current code does that. The already
> programmed delay should be correct.

I kind of have a little uneasy feeling about conflating these two
changes together. I think both are objectively good changes, just not
necessarily at once.

ISTR setting the max delay was, perhaps, somehow related to the hardware
losing its marbles after power is cut, effectively not ensuring any of
the delays at power-on. So it's possible we set the max here to account
for that. Maybe. ;)

Anyway,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

on the whole.

I'm leaving it up to you, but personally I'd lean towards switching
edp_notify_handler() to use wait_panel_power_cycle(intel_dp) first in a
separate patch, to help with potential bisect results, and then doing
the rest.

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  2 -
>  drivers/gpu/drm/i915/display/intel_dp.c       | 58 +++++--------------
>  2 files changed, 14 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 6f3e3d756383..9b9ed1a2f412 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -1319,8 +1319,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.
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 70e0b85442f9..e0f2e9236785 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -28,7 +28,6 @@
>  #include <linux/export.h>
>  #include <linux/i2c.h>
>  #include <linux/notifier.h>
> -#include <linux/reboot.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  
> @@ -1191,41 +1190,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_i915_private *dev_priv = dp_to_i915(intel_dp);
> -	intel_wakeref_t wakeref;
> -
> -	if (!intel_dp_is_edp(intel_dp) || code != SYS_RESTART)
> -		return 0;
> -
> -	with_pps_lock(intel_dp, wakeref) {
> -		if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -			enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> -			i915_reg_t pp_ctrl_reg, pp_div_reg;
> -			u32 pp_div;
> -
> -			pp_ctrl_reg = PP_CONTROL(pipe);
> -			pp_div_reg  = PP_DIVISOR(pipe);
> -			pp_div = intel_de_read(dev_priv, pp_div_reg);
> -			pp_div &= PP_REFERENCE_DIVIDER_MASK;
> -
> -			/* 0x1F write to PP_DIV_REG sets max cycle delay */
> -			intel_de_write(dev_priv, pp_div_reg, pp_div | 0x1F);
> -			intel_de_write(dev_priv, pp_ctrl_reg,
> -				       PANEL_UNLOCK_REGS);
> -			msleep(intel_dp->panel_power_cycle_delay);
> -		}
> -	}
> -
> -	return 0;
> -}
> -
>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> @@ -6690,11 +6654,6 @@ void intel_dp_encoder_flush_work(struct drm_encoder *encoder)
>  		 */
>  		with_pps_lock(intel_dp, wakeref)
>  			edp_panel_vdd_off_sync(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);
> @@ -6725,6 +6684,18 @@ void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder)
>  		edp_panel_vdd_off_sync(intel_dp);
>  }
>  
> +static void intel_dp_encoder_shutdown(struct intel_encoder *intel_encoder)
> +{
> +	struct intel_dp *intel_dp = enc_to_intel_dp(intel_encoder);
> +	intel_wakeref_t wakeref;
> +
> +	if (!intel_dp_is_edp(intel_dp))
> +		return;
> +
> +	with_pps_lock(intel_dp, wakeref)
> +		wait_panel_power_cycle(intel_dp);
> +}
> +
>  static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> @@ -7838,9 +7809,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	mutex_unlock(&dev->mode_config.mutex);
>  
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> -		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
> @@ -8061,6 +8029,8 @@ bool intel_dp_init(struct drm_i915_private *dev_priv,
>  	intel_encoder->get_config = intel_dp_get_config;
>  	intel_encoder->update_pipe = intel_panel_update_backlight;
>  	intel_encoder->suspend = intel_dp_encoder_suspend;
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +		intel_encoder->shutdown = intel_dp_encoder_shutdown;
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		intel_encoder->pre_pll_enable = chv_dp_pre_pll_enable;
>  		intel_encoder->pre_enable = chv_pre_enable_dp;

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx