Re: [PATCH v6 08/12] drm/i915/psr: Remove DSB_SKIP_WAITS_EN chicken bit

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 27, 2025 at 12:28:42PM +0200, Jouni Högander wrote:
> We have different approach on how flip is considered being complete. We are
> waiting for vblank on DSB and generate interrupt when it happens and this
> interrupt is considered as indication of completion -> we definitely do not
> want to skip vblank wait.
> 
> Also not skipping scanline wait shouldn't cause any problems if we are in
> DEEP_SLEEP PIPEDSL register is returning 0 -> evasion does nothing and if
> we are not in DEEP_SLEEP evasion works same way as without PSR.
> 
> v2: add comment explaining why we are not setting DSB_SKIP_WAITS_EN
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 2f2812c239725..30782ab0b9082 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -164,17 +164,26 @@ static int dsb_scanline_to_hw(struct intel_atomic_state *state,
>  	return (scanline + vtotal - intel_crtc_scanline_offset(crtc_state)) % vtotal;
>  }
>  
> +/*
> + * Bspec suggests that we should always set DSB_SKIP_WAITS_EN. We have approach
> + * different from what is explained in Bspec on how flip is considered being
> + * complete. We are waiting for vblank in DSB and generate interrupt when it
> + * happens and this interrupt is considered as indication of completion -> we
> + * definitely do not want to skip vblank wait. We also have concern what comes
> + * to skipping vblank evasion. I.e. arming registers are latched before we have
> + * managed writing them. Due to these reasons we are not setting
> + * DSB_SKIP_WAITS_EN.
> + */
>  static u32 dsb_chicken(struct intel_atomic_state *state,
>  		       struct intel_crtc *crtc)
>  {
>  	if (pre_commit_is_vrr_active(state, crtc))
> -		return DSB_SKIP_WAITS_EN |
> -			DSB_CTRL_WAIT_SAFE_WINDOW |
> +		return DSB_CTRL_WAIT_SAFE_WINDOW |
>  			DSB_CTRL_NO_WAIT_VBLANK |
>  			DSB_INST_WAIT_SAFE_WINDOW |
>  			DSB_INST_NO_WAIT_VBLANK;
>  	else
> -		return DSB_SKIP_WAITS_EN;
> +		return 0;
>  }
>  
>  static bool assert_dsb_has_room(struct intel_dsb *dsb)
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel