On Wed, Nov 17, 2021 at 08:31:01PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> Fixes: 6f9976bd1310 ("drm/i915: Do vrr push before sampling the frame counter")
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
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@xxxxxxxxx>
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
>
