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

["Manna, Animesh" <animesh.manna@intel.com>]

> -----Original Message-----
> From: Ville Syrjala <ville.syrjala@linux.intel.com>
> Sent: Wednesday, November 9, 2022 4:47 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Manna, Animesh <animesh.manna@intel.com>
> Subject: [PATCH 4/6] 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.
> 
> TODO: Deal with all the modern platforms too
>       Maybe add checks to make sure the same power
>       sequencer doesn't get assigned to multiple ports?
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../drm/i915/display/intel_display_types.h    |  8 +-
>  drivers/gpu/drm/i915/display/intel_panel.c    |  1 +
>  drivers/gpu/drm/i915/display/intel_pps.c      | 78 +++++++++++++++++--
>  3 files changed, 80 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index aec06cb24e23..25165110142b 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;
> 
> @@ -1571,9 +1571,13 @@ 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;
> +	/*
> +	 * Power sequencer index. Only relevant on BXT+.
> +	 */
> +	s8 pps_idx;
>  	struct edp_power_seq pps_delays;
>  	struct edp_power_seq bios_pps_delays;
>  };
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> b/drivers/gpu/drm/i915/display/intel_panel.c
> index 918b3b9d9ebe..1794e5eecf90 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -665,6 +665,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 84265096f751..ff4f1def59d2 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -211,8 +211,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);
> 
> @@ -220,7 +219,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;
> 
> @@ -230,7 +229,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); @@
> -310,6 +309,54 @@ 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
> +bxt_initial_power_sequencer_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);
> +
> +	/* first ask the VBT */
> +	intel_dp->pps.pps_idx = connector->panel.vbt.backlight.controller;
> +  
> +	/* 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);

Always we will get 0 here even if bios enabled correctly two pps instance for dual EDP.
Can pps be mapped with port number, like pps1 for portA and pps2 for portB?
 
> +	/* 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);

Same as above for vdd.

> +	/* 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);

pps_any() is returning bool, any specific reason? Can we return 0 from it. 

Regards,
Animesh 

> +
> +		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;
> @@ -1431,7 +1478,9 @@ 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))
> +		if (IS_GEMINILAKE(i915) || IS_BROXTON(i915))
> +			bxt_initial_power_sequencer_setup(intel_dp);
> +		else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
>  			vlv_initial_power_sequencer_setup(intel_dp);
> 
>  		pps_init_delays(intel_dp);
> @@ -1440,12 +1489,31 @@ void intel_pps_init(struct intel_dp *intel_dp)
>  	}
>  }
> 
> +static void bxt_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;
> +
> +	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)  {
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	intel_wakeref_t wakeref;
> 
>  	with_intel_pps_lock(intel_dp, wakeref) {
>  		/* Reinit delays after per-panel info has been parsed from VBT
> */
> +		if (IS_GEMINILAKE(i915) || IS_BROXTON(i915))
> +			bxt_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