Re: [Intel-gfx] [PATCH v2 4/9] drm/i915: Try to use the correct power sequencer intiially on bxt/glk

From: "Manna, Animesh" <animesh.manna@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH v2 4/9] drm/i915: Try to use the correct power sequencer intiially on bxt/glk
Date: Thu, 8 Dec 2022 15:28:31 +0000	[thread overview]
Message-ID: <PH7PR11MB5981A649CD0BD7CC6546AA4AF91D9@PH7PR11MB5981.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20221125173156.31689-5-ville.syrjala@linux.intel.com>

> -----Original Message-----
> From: Ville Syrjala <ville.syrjala@linux.intel.com>
> Sent: Friday, November 25, 2022 11:02 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Manna, Animesh <animesh.manna@intel.com>
> Subject: [PATCH v2 4/9] drm/i915: Try to use the correct power sequencer
> intiially on bxt/glk
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Currently on bxt/glk we just grab the power sequencer index from the VBT
> data even though it may not have been parsed yet. That could lead us to
> using the incorrect power sequencer during the initial panel probe.
> 
> To avoid that let's try to read out the current state of the power sequencer
> from the hardware. Unfortunately the power sequencer no longer has
> anything in its registers to associate it with the port, so the best we can do is
> just iterate through the power sequencers and pick the first one. This should
> be sufficient for single panel cases.
> 
> For the dual panel cases we probably need to go back to parsing the VBT
> before the panel probe (and hope that panel_type=0xff is never a thing in
> those cases). To that end the code always prefers the VBT panel sequencer, if
> available.
> 
> v2: Restructure a bit for upcoming icp+ dual PPS support
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

LGTM.
Reviewed-by: Animesh Manna <animesh.manna@intel.com>

> ---
>  .../drm/i915/display/intel_display_types.h    | 22 +++--
>  drivers/gpu/drm/i915/display/intel_panel.c    |  1 +
>  drivers/gpu/drm/i915/display/intel_pps.c      | 96 +++++++++++++++++--
>  3 files changed, 102 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index cc64e787e401..32e8b2fc3cc6 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -330,7 +330,7 @@ struct intel_vbt_panel_data {
>  		bool present;
>  		bool active_low_pwm;
>  		u8 min_brightness;	/* min_brightness/255 of max */
> -		u8 controller;		/* brightness controller number */
> +		s8 controller;		/* brightness controller number */
>  		enum intel_backlight_type type;
>  	} backlight;
> 
> @@ -1570,11 +1570,19 @@ struct intel_pps {
>  	ktime_t panel_power_off_time;
>  	intel_wakeref_t vdd_wakeref;
> 
> -	/*
> -	 * Pipe whose power sequencer is currently locked into
> -	 * this port. Only relevant on VLV/CHV.
> -	 */
> -	enum pipe pps_pipe;
> +	union {
> +		/*
> +		 * Pipe whose power sequencer is currently locked into
> +		 * this port. Only relevant on VLV/CHV.
> +		 */
> +		enum pipe pps_pipe;
> +
> +		/*
> +		 * Power sequencer index. Only relevant on BXT+.
> +		 */
> +		int pps_idx;
> +	};
> +
>  	/*
>  	 * Pipe currently driving the port. Used for preventing
>  	 * the use of the PPS for any pipe currentrly driving @@ -1583,7
> +1591,7 @@ struct intel_pps {
>  	enum pipe active_pipe;
>  	/*
>  	 * Set if the sequencer may be reset due to a power transition,
> -	 * requiring a reinitialization. Only relevant on BXT.
> +	 * requiring a reinitialization. Only relevant on BXT+.
>  	 */
>  	bool pps_reset;
>  	struct edp_power_seq pps_delays;
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> b/drivers/gpu/drm/i915/display/intel_panel.c
> index 609fcdbd7d58..3b1004b019a8 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -666,6 +666,7 @@ void intel_panel_init_alloc(struct intel_connector
> *connector)
>  	struct intel_panel *panel = &connector->panel;
> 
>  	connector->panel.vbt.panel_type = -1;
> +	connector->panel.vbt.backlight.controller = -1;
>  	INIT_LIST_HEAD(&panel->fixed_modes);
>  }
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> b/drivers/gpu/drm/i915/display/intel_pps.c
> index 41ab12fcce0e..d8d2f22f3e0c 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -212,8 +212,7 @@ static int
>  bxt_power_sequencer_idx(struct intel_dp *intel_dp)  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> -	struct intel_connector *connector = intel_dp->attached_connector;
> -	int backlight_controller = connector->panel.vbt.backlight.controller;
> +	int pps_idx = intel_dp->pps.pps_idx;
> 
>  	lockdep_assert_held(&dev_priv->display.pps.mutex);
> 
> @@ -221,7 +220,7 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
>  	drm_WARN_ON(&dev_priv->drm, !intel_dp_is_edp(intel_dp));
> 
>  	if (!intel_dp->pps.pps_reset)
> -		return backlight_controller;
> +		return pps_idx;
> 
>  	intel_dp->pps.pps_reset = false;
> 
> @@ -231,7 +230,7 @@ bxt_power_sequencer_idx(struct intel_dp *intel_dp)
>  	 */
>  	pps_init_registers(intel_dp, false);
> 
> -	return backlight_controller;
> +	return pps_idx;
>  }
> 
>  typedef bool (*pps_check)(struct drm_i915_private *dev_priv, int pps_idx);
> @@ -311,6 +310,64 @@ vlv_initial_power_sequencer_setup(struct intel_dp
> *intel_dp)
>  		    pipe_name(intel_dp->pps.pps_pipe));
>  }
> 
> +static int
> +bxt_initial_pps_idx(struct drm_i915_private *i915, pps_check check) {
> +	int pps_idx, pps_num = 2;
> +
> +	for (pps_idx = 0; pps_idx < pps_num; pps_idx++) {
> +		if (check(i915, pps_idx))
> +			return pps_idx;
> +	}
> +
> +	return -1;
> +}
> +
> +static void
> +pps_initial_setup(struct intel_dp *intel_dp) {
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> +
> +	lockdep_assert_held(&i915->display.pps.mutex);
> +
> +	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
> +		vlv_initial_power_sequencer_setup(intel_dp);
> +		return;
> +	}
> +
> +	if (!IS_GEMINILAKE(i915) && !IS_BROXTON(i915))
> +		return;
> +
> +	/* first ask the VBT */
> +	intel_dp->pps.pps_idx = connector->panel.vbt.backlight.controller;
> +	if (drm_WARN_ON(&i915->drm, intel_dp->pps.pps_idx >= 2))
> +		intel_dp->pps.pps_idx = -1;
> +
> +	/* VBT wasn't parsed yet? pick one where the panel is on */
> +	if (intel_dp->pps.pps_idx < 0)
> +		intel_dp->pps.pps_idx = bxt_initial_pps_idx(i915,
> pps_has_pp_on);
> +	/* didn't find one? pick one where vdd is on */
> +	if (intel_dp->pps.pps_idx < 0)
> +		intel_dp->pps.pps_idx = bxt_initial_pps_idx(i915,
> pps_has_vdd_on);
> +	/* didn't find one? pick any */
> +	if (intel_dp->pps.pps_idx < 0) {
> +		intel_dp->pps.pps_idx = bxt_initial_pps_idx(i915, pps_any);
> +
> +		drm_dbg_kms(&i915->drm,
> +			    "[ENCODER:%d:%s] no initial power sequencer,
> assuming %d\n",
> +			    encoder->base.base.id, encoder->base.name,
> +			    intel_dp->pps.pps_idx);
> +		return;
> +	}
> +
> +	drm_dbg_kms(&i915->drm,
> +		    "[ENCODER:%d:%s] initial power sequencer: %d\n",
> +		    encoder->base.base.id, encoder->base.name,
> +		    intel_dp->pps.pps_idx);
> +}
> +
>  void intel_pps_reset_all(struct drm_i915_private *dev_priv)  {
>  	struct intel_encoder *encoder;
> @@ -363,10 +420,10 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
> 
>  	memset(regs, 0, sizeof(*regs));
> 
> -	if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> -		pps_idx = bxt_power_sequencer_idx(intel_dp);
> -	else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		pps_idx = vlv_power_sequencer_pipe(intel_dp);
> +	else if (IS_GEMINILAKE(dev_priv) || IS_BROXTON(dev_priv))
> +		pps_idx = bxt_power_sequencer_idx(intel_dp);
> 
>  	regs->pp_ctrl = PP_CONTROL(pps_idx);
>  	regs->pp_stat = PP_STATUS(pps_idx);
> @@ -1429,7 +1486,6 @@ void intel_pps_encoder_reset(struct intel_dp
> *intel_dp)
> 
>  void intel_pps_init(struct intel_dp *intel_dp)  {
> -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	intel_wakeref_t wakeref;
> 
>  	intel_dp->pps.initializing = true;
> @@ -1438,8 +1494,7 @@ void intel_pps_init(struct intel_dp *intel_dp)
>  	pps_init_timestamps(intel_dp);
> 
>  	with_intel_pps_lock(intel_dp, wakeref) {
> -		if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> -			vlv_initial_power_sequencer_setup(intel_dp);
> +		pps_initial_setup(intel_dp);
> 
>  		pps_init_delays(intel_dp);
>  		pps_init_registers(intel_dp, false);
> @@ -1447,12 +1502,33 @@ void intel_pps_init(struct intel_dp *intel_dp)
>  	}
>  }
> 
> +static void pps_init_late(struct intel_dp *intel_dp) {
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +
> +	if (!IS_GEMINILAKE(i915) && !IS_BROXTON(i915))
> +		return;
> +
> +	drm_WARN(&i915->drm, connector->panel.vbt.backlight.controller
> >= 0 &&
> +		 intel_dp->pps.pps_idx != connector-
> >panel.vbt.backlight.controller,
> +		 "[ENCODER:%d:%s] power sequencer mismatch: %d (initial)
> vs. %d (VBT)\n",
> +		 encoder->base.base.id, encoder->base.name,
> +		 intel_dp->pps.pps_idx, connector-
> >panel.vbt.backlight.controller);
> +
> +	if (connector->panel.vbt.backlight.controller >= 0)
> +		intel_dp->pps.pps_idx = connector-
> >panel.vbt.backlight.controller;
> +}
> +
>  void intel_pps_init_late(struct intel_dp *intel_dp)  {
>  	intel_wakeref_t wakeref;
> 
>  	with_intel_pps_lock(intel_dp, wakeref) {
>  		/* Reinit delays after per-panel info has been parsed from
> VBT */
> +		pps_init_late(intel_dp);
> +
>  		memset(&intel_dp->pps.pps_delays, 0, sizeof(intel_dp-
> >pps.pps_delays));
>  		pps_init_delays(intel_dp);
>  		pps_init_registers(intel_dp, false);
> --
> 2.37.4