Re: [Intel-gfx] [PATCH v2 6/9] drm/i915: Reject unusablee power sequencers

["Manna, Animesh" <animesh.manna@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 6/9] drm/i915: Reject unusablee power sequencers
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On ICP-ADP the pins used by the second PPS can be alternatively muxed to
> some other function. In that case the second power sequencer is unusable.
> 
> Unfortunately (on my ADL Thinkpad T14 gen3 at least) the BIOS still likes to
> enable the VDD on the second PPS (due to the VBT declaring the second
> bogus eDP panel) even when not correctly muxed, so we need to deal with it
> somehow.
> For now let's just initialize the PPS as normal, and then use the normal eDP
> probe failure VDD off path to turn it off (and release the wakeref the PPS init
> grabbed). The alternative of just declaring that the platform has a single PPS
> doesn't really work since it would cause the second eDP probe to also try to
> use the first PPS and thus clobber the state for the first (real) eDP panel.
> 
> Cc: Animesh Manna <animesh.manna@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

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

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c  | 12 ++++++++-
> drivers/gpu/drm/i915/display/intel_pps.c | 34 +++++++++++++++++-------
> drivers/gpu/drm/i915/display/intel_pps.h |  2 +-
>  drivers/gpu/drm/i915/i915_reg.h          |  1 +
>  4 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index c1bebe77ed8e..9deaa5e3632a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5283,7 +5283,17 @@ static bool intel_edp_init_connector(struct
> intel_dp *intel_dp,
>  	intel_bios_init_panel_early(dev_priv, &intel_connector->panel,
>  				    encoder->devdata);
> 
> -	intel_pps_init(intel_dp);
> +	if (!intel_pps_init(intel_dp)) {
> +		drm_info(&dev_priv->drm,
> +			 "[ENCODER:%d:%s] unusable PPS, disabling eDP\n",
> +			 encoder->base.base.id, encoder->base.name);
> +		/*
> +		 * The BIOS may have still enabled VDD on the PPS even
> +		 * though it's unusable. Make sure we turn it back off
> +		 * and to release the power domain references/etc.
> +		 */
> +		goto out_vdd_off;
> +	}
> 
>  	/* Cache DPCD and EDID for edp. */
>  	has_dpcd = intel_edp_init_dpcd(intel_dp); diff --git
> a/drivers/gpu/drm/i915/display/intel_pps.c
> b/drivers/gpu/drm/i915/display/intel_pps.c
> index 77b0a4f27abc..d18c1c58dfcf 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -327,6 +327,18 @@ static int intel_num_pps(struct drm_i915_private
> *i915)
>  	return 1;
>  }
> 
> +static bool intel_pps_is_valid(struct intel_dp *intel_dp) {
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (intel_dp->pps.pps_idx == 1 &&
> +	    INTEL_PCH_TYPE(i915) >= PCH_ICP &&
> +	    INTEL_PCH_TYPE(i915) < PCH_MTP)
> +		return intel_de_read(i915, SOUTH_CHICKEN1) &
> +ICP_SECOND_PPS_IO_SELECT;
> +
> +	return true;
> +}
> +
>  static int
>  bxt_initial_pps_idx(struct drm_i915_private *i915, pps_check check)  { @@ -
> 340,7 +352,7 @@ bxt_initial_pps_idx(struct drm_i915_private *i915,
> pps_check check)
>  	return -1;
>  }
> 
> -static void
> +static bool
>  pps_initial_setup(struct intel_dp *intel_dp)  {
>  	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> @@ -351,7 +363,7 @@ pps_initial_setup(struct intel_dp *intel_dp)
> 
>  	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
>  		vlv_initial_power_sequencer_setup(intel_dp);
> -		return;
> +		return true;
>  	}
> 
>  	/* first ask the VBT */
> @@ -377,13 +389,14 @@ pps_initial_setup(struct intel_dp *intel_dp)
>  			    "[ENCODER:%d:%s] no initial power sequencer,
> assuming %d\n",
>  			    encoder->base.base.id, encoder->base.name,
>  			    intel_dp->pps.pps_idx);
> -		return;
> +	} else {
> +		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);
>  	}
> 
> -	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);
> +	return intel_pps_is_valid(intel_dp);
>  }
> 
>  void intel_pps_reset_all(struct drm_i915_private *dev_priv) @@ -1504,9
> +1517,10 @@ void intel_pps_encoder_reset(struct intel_dp *intel_dp)
>  	}
>  }
> 
> -void intel_pps_init(struct intel_dp *intel_dp)
> +bool intel_pps_init(struct intel_dp *intel_dp)
>  {
>  	intel_wakeref_t wakeref;
> +	bool ret;
> 
>  	intel_dp->pps.initializing = true;
>  	INIT_DELAYED_WORK(&intel_dp->pps.panel_vdd_work,
> edp_panel_vdd_work); @@ -1514,12 +1528,14 @@ void
> intel_pps_init(struct intel_dp *intel_dp)
>  	pps_init_timestamps(intel_dp);
> 
>  	with_intel_pps_lock(intel_dp, wakeref) {
> -		pps_initial_setup(intel_dp);
> +		ret = pps_initial_setup(intel_dp);
> 
>  		pps_init_delays(intel_dp);
>  		pps_init_registers(intel_dp, false);
>  		pps_vdd_init(intel_dp);
>  	}
> +
> +	return ret;
>  }
> 
>  static void pps_init_late(struct intel_dp *intel_dp) diff --git
> a/drivers/gpu/drm/i915/display/intel_pps.h
> b/drivers/gpu/drm/i915/display/intel_pps.h
> index a3a56f903f26..a2c2467e3c22 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.h
> +++ b/drivers/gpu/drm/i915/display/intel_pps.h
> @@ -40,7 +40,7 @@ void intel_pps_vdd_off_sync(struct intel_dp *intel_dp);
> bool intel_pps_have_panel_power_or_vdd(struct intel_dp *intel_dp);  void
> intel_pps_wait_power_cycle(struct intel_dp *intel_dp);
> 
> -void intel_pps_init(struct intel_dp *intel_dp);
> +bool intel_pps_init(struct intel_dp *intel_dp);
>  void intel_pps_init_late(struct intel_dp *intel_dp);  void
> intel_pps_encoder_reset(struct intel_dp *intel_dp);  void
> intel_pps_reset_all(struct drm_i915_private *i915); diff --git
> a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0b90fe6a28f7..ebb45935750c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6242,6 +6242,7 @@
>  #define  CHASSIS_CLK_REQ_DURATION_MASK	(0xf << 8)
>  #define  CHASSIS_CLK_REQ_DURATION(x)	((x) << 8)
>  #define  SBCLK_RUN_REFCLK_DIS		(1 << 7)
> +#define  ICP_SECOND_PPS_IO_SELECT	REG_BIT(2)
>  #define  SPT_PWM_GRANULARITY		(1 << 0)
>  #define SOUTH_CHICKEN2		_MMIO(0xc2004)
>  #define  FDI_MPHY_IOSFSB_RESET_STATUS	(1 << 13)
> --
> 2.37.4