Re: [Intel-gfx] [PATCH 1/3] drm/i915: Move vrr push after the frame counter sampling again

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Wed, Nov 17, 2021 at 08:31:01PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Moving the vrr push to happen before sampling the frame counter
> was wrong. If we are already in vblank when the push is sent
> the vblank exit will start immediately which causes the sampled
> frame counter to correspond to the next frame instead of the current
> frame.
> 
> So put things back into the original order (except we should
> keep the vrr push within the irq disable section to avoid
> pointless irq related delays here).
> 
> We'll just have to accept the tiny race that exists between
> sampling the frame counter vs. vrr push. And let's at least
> document said race properly in a comment.
> 
> I suppose we could try to minimize the race by sampling the frame
> counter just before sending the push, but that would require
> changing drm_crtc_arm_vblank_event() to accept a caller provided
> vblank counter value, so leave it be for now. Another thing we
> could do is change the vblank evasion to account for the case
> where a push was already sent. That would anyway be required
> for mailbox style updates. Currently mailbox updates are only
> used by the legacy cursor, but we don't do a vrr push for those.
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Fixes: 6f9976bd1310 ("drm/i915: Do vrr push before sampling the frame counter")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

The original order was to send push after enabling IRQs but I think it makes
sense to send push just before enabling IRQs so avoid the vblank
termination getting delayed due to IRQs

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> ---
>  drivers/gpu/drm/i915/display/intel_crtc.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index f09df2cf052b..cf403be7736c 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> @@ -610,9 +610,6 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  
>  	trace_intel_pipe_update_end(crtc, end_vbl_count, scanline_end);
>  
> -	/* Send VRR Push to terminate Vblank */
> -	intel_vrr_send_push(new_crtc_state);
> -
>  	/*
>  	 * Incase of mipi dsi command mode, we need to set frame update
>  	 * request for every commit.
> @@ -641,6 +638,22 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
>  		new_crtc_state->uapi.event = NULL;
>  	}
>  
> +	/*
> +	 * Send VRR Push to terminate Vblank. If we are already in vblank
> +	 * this has to be done _after_ sampling the frame counter, as
> +	 * otherwise the push would immediately terminate the vblank and
> +	 * the sampled frame counter would correspond to the next frame
> +	 * instead of the current frame.
> +	 *
> +	 * There is a tiny race here (iff vblank evasion failed us) where
> +	 * we might sample the frame counter just before vmax vblank start
> +	 * but the push would be sent just after it. That would cause the
> +	 * push to affect the next frame instead of the current frame,
> +	 * which would cause the next frame to terminate already at vmin
> +	 * vblank start instead of vmax vblank start.
> +	 */
> +	intel_vrr_send_push(new_crtc_state);
> +
>  	local_irq_enable();
>  
>  	if (intel_vgpu_active(dev_priv))
> -- 
> 2.32.0
>