On Fri, 25 Feb 2022, Jouni Högander <jouni.hogan...@intel.com> 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 ADL_P 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 ADL_P. Bit 31 has different meaning in ADL_P.
>
> Bspec: 49274
>
> v2: Fix Mihai Harpau email address
> v3: Modify commit message and remove unnecessary comment
>
> Fixes: 7f6002e58025 ("drm/i915/display: Enable PSR2 selective fetch by
> default")
> Reported-by: Lyude Paul <ly...@redhat.com>
> Cc: Mihai Harpau <mhar...@gmail.com>
> Cc: José Roberto de Souza <jose.so...@intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Bugzilla: https://gitlab.freedesktop.org/drm/intel/-/issues/5077
> Signed-off-by: Jouni Högander <jouni.hogan...@intel.com>
> Reviewed-by: José Roberto de Souza <jose.so...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_psr.c | 16 ++++++++++++++--
> drivers/gpu/drm/i915/i915_reg.h | 1 +
> 2 files changed, 15 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..b6b7bb5be5ae 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)
Generally, please don't use inline in .c files, just let the compiler do
its thing. We won't get warnings if inlines become unused.
Please name struct drm_i915_private * pointers i915 instead of dev_priv
in new code where possible. (We've still got some implicit assumptions
on dev_priv in some register macros unfortunately.)
> +{
> + 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,13 @@ 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;
> +
> + 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);
Not sure if the separate function for this is warranted if you're anyway
including an if (!IS_ALDERLAKE_P(dev_priv)) condition here.
BR,
Jani.
>
> if (full_update) {
> /*
> @@ -1563,7 +1576,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 a2f36ef91cec..b347a8921178 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)
--
Jani Nikula, Intel Open Source Graphics Center
