Re: [Intel-gfx] [PATCH v2 5/9] drm/i915: Extend dual PPS handlind for ICP+

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 5/9] drm/i915: Extend dual PPS handlind for ICP+
Date: Thu, 8 Dec 2022 15:29:33 +0000	[thread overview]
Message-ID: <PH7PR11MB5981D0CF63A72A33A69CECADF91D9@PH7PR11MB5981.namprd11.prod.outlook.com> (raw)
In-Reply-To: <20221125173156.31689-6-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 5/9] drm/i915: Extend dual PPS handlind for ICP+
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On the PCH side the second PPS was introduced in ICP. Let's make sure we
> examine both power sequencer on ICP+ as well.
> 
> Note that DG1/2 south block only has the single PPS, so need to exclude the
> fake DG1/2 PCHs.
> 
> 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>

> ---
>  drivers/gpu/drm/i915/display/intel_pps.c | 44 +++++++++++++++++-------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_pps.c
> b/drivers/gpu/drm/i915/display/intel_pps.c
> index d8d2f22f3e0c..77b0a4f27abc 100644
> --- a/drivers/gpu/drm/i915/display/intel_pps.c
> +++ b/drivers/gpu/drm/i915/display/intel_pps.c
> @@ -310,10 +310,27 @@ vlv_initial_power_sequencer_setup(struct intel_dp
> *intel_dp)
>  		    pipe_name(intel_dp->pps.pps_pipe));
>  }
> 
> +static int intel_num_pps(struct drm_i915_private *i915) {
> +	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> +		return 2;
> +
> +	if (IS_GEMINILAKE(i915) || IS_BROXTON(i915))
> +		return 2;
> +
> +	if (INTEL_PCH_TYPE(i915) >= PCH_DG1)
> +		return 1;
> +
> +	if (INTEL_PCH_TYPE(i915) >= PCH_ICP)
> +		return 2;
> +
> +	return 1;
> +}
> +
>  static int
>  bxt_initial_pps_idx(struct drm_i915_private *i915, pps_check check)  {
> -	int pps_idx, pps_num = 2;
> +	int pps_idx, pps_num = intel_num_pps(i915);
> 
>  	for (pps_idx = 0; pps_idx < pps_num; pps_idx++) {
>  		if (check(i915, pps_idx))
> @@ -337,12 +354,13 @@ pps_initial_setup(struct intel_dp *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))
> +	if (intel_num_pps(i915) > 1)
> +		intel_dp->pps.pps_idx = connector-
> >panel.vbt.backlight.controller;
> +	else
> +		intel_dp->pps.pps_idx = 0;
> +
> +	if (drm_WARN_ON(&i915->drm, intel_dp->pps.pps_idx >=
> +intel_num_pps(i915)))
>  		intel_dp->pps.pps_idx = -1;
> 
>  	/* VBT wasn't parsed yet? pick one where the panel is on */ @@ -
> 416,7 +434,7 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
>  				    struct pps_registers *regs)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> -	int pps_idx = 0;
> +	int pps_idx;
> 
>  	memset(regs, 0, sizeof(*regs));
> 
> @@ -424,6 +442,8 @@ static void intel_pps_get_registers(struct intel_dp
> *intel_dp,
>  		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);
> +	else
> +		pps_idx = intel_dp->pps.pps_idx;
> 
>  	regs->pp_ctrl = PP_CONTROL(pps_idx);
>  	regs->pp_stat = PP_STATUS(pps_idx);
> @@ -1508,7 +1528,10 @@ static void pps_init_late(struct intel_dp *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))
> +	if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915))
> +		return;
> +
> +	if (intel_num_pps(i915) < 2)
>  		return;
> 
>  	drm_WARN(&i915->drm, connector->panel.vbt.backlight.controller
> >= 0 && @@ -1551,10 +1574,7 @@ void intel_pps_unlock_regs_wa(struct
> drm_i915_private *dev_priv)
>  	 * This w/a is needed at least on CPT/PPT, but to be sure apply it
>  	 * everywhere where registers can be write protected.
>  	 */
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> -		pps_num = 2;
> -	else
> -		pps_num = 1;
> +	pps_num = intel_num_pps(dev_priv);
> 
>  	for (pps_idx = 0; pps_idx < pps_num; pps_idx++) {
>  		u32 val = intel_de_read(dev_priv, PP_CONTROL(pps_idx));
> --
> 2.37.4