Re: [PATCH 17/18] drm/i915/dsb: Add support for triggering VRR push with DSB

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 12/11/2024 2:40 AM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> We have at least two options for how to do the
> TRANS_PUSH_SEND + commit completion signalling
> with the DSB:
>
> Option A)
>   1. trigger TRANS_PUSH_SEND
>   2. wait for "safe window"
>   3. signal the interrupt
>
> In this cases step 2 should not do anything if we were already
> between vmin and vmax decision boundaries. Otherwise we'll wait
> until the next start of the vblank period.
>
> Option B)
>   1. wait for "safe window"
>   2. trigger TRANS_PUSH_SEND
>   3. signal the interrupt
>
> This option is perhaps a bit less racy, but if we do somehow
> screw up and the wait is a nop but the push gets deferred
> until the next frame then we'll end up completing the commit
> a frame too early.
>
> So for now I'm leaning towards option A since losing the race
> won't have any drastic consequences. To deal with the race we
> can give the DSB a bit more time to start step 2 before the
> hardware has started the vblank termination properly. Often
> times it seems to be fast enough to make it in time even without
> any extra vblank delay (the push is issued somewhere within a
> scanline and it latches on the next scanline).

Makes sense. LGTM.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>


>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_display.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 22b5eacda0f0..4b3199dcbe7a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2630,6 +2630,14 @@ static int intel_crtc_vblank_delay(const struct intel_crtc_state *crtc_state)
>   	if (intel_crtc_needs_wa_14015401596(crtc_state))
>   		vblank_delay = max(vblank_delay, 1);
>   
> +	/*
> +	 * Add a minimal vblank delay to make sure the push
> +	 * doesn't doesn't race with the "wait for safe
> +	 * window" we use for frame completion with DSB.
> +	 */
> +	if (HAS_VRR(display))
> +		vblank_delay = max(vblank_delay, 1);
> +
>   	return vblank_delay;
>   }
>   
> @@ -7725,6 +7733,8 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state,
>   		intel_crtc_planes_update_arm(new_crtc_state->dsb_commit,
>   					     state, crtc);
>   
> +		intel_vrr_send_push(new_crtc_state->dsb_commit, new_crtc_state);
> +
>   		if (!new_crtc_state->dsb_color_vblank) {
>   			intel_dsb_wait_vblanks(new_crtc_state->dsb_commit, 1);
>   			intel_dsb_wait_vblank_delay(state, new_crtc_state->dsb_commit);