Re: [Intel-gfx] [PATCH v2] drm/i915/psr: Set "SF Partial Frame Enable" also on full update

["Souza, Jose" <jose.souza@intel.com>]

On Wed, 2022-02-23 at 14:48 +0200, Jouni Högander wrote:
> Currently we are observing occasional screen flickering when
> PSR2 selective fetch is enabled. More specifically glitch seems
> to happen on full frame update when cursor moves to coords
> x = -1 or y = -1.
> 
> According to Bspec SF Single full frame should not be set if
> SF Partial Frame Enable is not set. This happened to be true for
> ADLP as PSR2_MAN_TRK_CTL_ENABLE is always set and for ADLP it's
> actually "SF Partial Frame Enable" (Bit 31).
> 
> Setting "SF Partial Frame Enable" bit also on full update seems to
> fix screen flickering.
> 
> Also make code more clear by setting PSR2_MAN_TRK_CTL_ENABLE
> only if not on ADLP as this bit doesn't exist in ADLP.

Bit exist but has another name.

> 
> Bspec: 49274
> 
> v2: Fix Mihai Harpau email address
> 
> Reported-by: Lyude Paul <lyude@redhat.com>
> Cc: Mihai Harpau <mharpau@gmail.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c | 20 ++++++++++++++++++--
>  drivers/gpu/drm/i915/i915_reg.h          |  1 +
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 2e0b092f4b6b..90aca75e05e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1439,6 +1439,13 @@ static inline u32 man_trk_ctl_single_full_frame_bit_get(struct drm_i915_private
>  	       PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME;
>  }
>  
> +static inline u32 man_trk_ctl_partial_frame_bit_get(struct drm_i915_private *dev_priv)
> +{
> +	return IS_ALDERLAKE_P(dev_priv) ?
> +	       ADLP_PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE :
> +	       PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
> +}
> +
>  static void psr_force_hw_tracking_exit(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> @@ -1543,7 +1550,17 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	u32 val = PSR2_MAN_TRK_CTL_ENABLE;
> +	u32 val = 0;
> +
> +	/*
> +	 * ADL_P doesn't have HW tracking nor manual tracking enable
> +	 * bit
> +	 */

Nit: Unnecessary comment.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +	if (!IS_ALDERLAKE_P(dev_priv))
> +		val = PSR2_MAN_TRK_CTL_ENABLE;
> +
> +	/* SF partial frame enable has to be set even on full update */
> +	val |= man_trk_ctl_partial_frame_bit_get(dev_priv);
>  
>  	if (full_update) {
>  		/*
> @@ -1563,7 +1580,6 @@ static void psr2_man_trk_ctl_calc(struct intel_crtc_state *crtc_state,
>  	} else {
>  		drm_WARN_ON(crtc_state->uapi.crtc->dev, clip->y1 % 4 || clip->y2 % 4);
>  
> -		val |= PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE;
>  		val |= PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(clip->y1 / 4 + 1);
>  		val |= PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(clip->y2 / 4 + 1);
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2b8a3086ed35..89bbb64e520d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2316,6 +2316,7 @@
>  #define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR(val)	REG_FIELD_PREP(ADLP_PSR2_MAN_TRK_CTL_SU_REGION_START_ADDR_MASK, val)
>  #define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK		REG_GENMASK(12, 0)
>  #define  ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR(val)		REG_FIELD_PREP(ADLP_PSR2_MAN_TRK_CTL_SU_REGION_END_ADDR_MASK, val)
> +#define  ADLP_PSR2_MAN_TRK_CTL_SF_PARTIAL_FRAME_UPDATE		REG_BIT(31)
>  #define  ADLP_PSR2_MAN_TRK_CTL_SF_SINGLE_FULL_FRAME		REG_BIT(14)
>  #define  ADLP_PSR2_MAN_TRK_CTL_SF_CONTINUOS_FULL_FRAME		REG_BIT(13)
>