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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 06 Oct 2020, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Tue, Oct 06, 2020 at 12:29:22PM +0300, Jani Nikula wrote:
>> On Thu, 01 Oct 2020, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
>> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >
>> > 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@xxxxxxxxx>
>> 
>> 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.
>
> I don't think it would be quite that simple. We'd have to also toss
> in some combination of panel_off() and vdd_off_sync() in there,
> depending on whether the panel power is currently enabled or not.
> Otherwise the bookkeeping needed by wait_panel_power_cycle() isn't
> going to be up to date.

Oh? So the calls via encoder hooks actually ensure that?

In that case, push away!

BR,
Jani.



>
>> 
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> > ---
>> >  .../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

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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux