On Wed, Feb 12, 2025 at 06:39:56PM +0530, Nautiyal, Ankit K wrote: > > On 2/11/2025 11:08 PM, Ville Syrjälä wrote: > > On Tue, Feb 11, 2025 at 12:38:47PM +0530, Nautiyal, Ankit K wrote: > >> On 2/10/2025 9:37 PM, Ville Syrjala wrote: > >>> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>> > >>> Since we don't do mailbox updates the push send bit > >>> should alwyas clear by the time the delay vblank fires > >>> and the flip completes. Check for that to make sure we > >>> haven't screwed up the sequencing/vblank evasion/etc. > >>> > >>> On the DSB path we should be able to guarantee this > >>> since we don't have to deal with any scheduler latencies > >>> and whatnot. I suppose unexpected DMA/memory latencies > >>> might be the only thing that might trip us up here. > >>> > >>> For the MMIO path we do always have a non-zero chance > >>> that vblank evasion fails (since we can't really guarantee > >>> anything about the scheduling behaviour). That could trip > >>> up this check, but that seems fine since we already print > >>> errors for other types of vblank evasion failures. > >>> > >>> Should the CPU vblank evasion actually fail, then the push > >>> send bit can still be set when the next commit happens. But > >>> both the DSB and MMIO paths should handle that situation > >>> gracefully. > >>> > >>> v2: Only check once instead of polling for two scanlines > >>> since we should now be guaranteed to be past the > >>> delayed vblank. > >>> Also check in the MMIO path for good measure > >>> v3: Skip the push send check when VRR is enabled. > >>> With joiner the secondary pipe's DSBs doen't have access > >>> to the transcoder registers, and so doing this check > >>> there triggers a reponse timeout error on the DSB. VRR > >>> is not currently allowed when using joiner, so this will > >>> prevent the bogus register access. > >>> > >>> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx> #v1 > >>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>> --- > >>> drivers/gpu/drm/i915/display/intel_color.c | 1 + > >>> drivers/gpu/drm/i915/display/intel_display.c | 4 +++ > >>> drivers/gpu/drm/i915/display/intel_vrr.c | 34 ++++++++++++++++++++ > >>> drivers/gpu/drm/i915/display/intel_vrr.h | 2 ++ > >>> 4 files changed, 41 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c > >>> index 4d8f6509cac4..cfe14162231d 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_color.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_color.c > >>> @@ -1991,6 +1991,7 @@ void intel_color_prepare_commit(struct intel_atomic_state *state, > >>> if (crtc_state->use_dsb) { > >>> intel_vrr_send_push(crtc_state->dsb_color_vblank, crtc_state); > >>> intel_dsb_wait_vblank_delay(state, crtc_state->dsb_color_vblank); > >>> + intel_vrr_check_push_sent(crtc_state->dsb_color_vblank, crtc_state); > >>> intel_dsb_interrupt(crtc_state->dsb_color_vblank); > >>> } > >>> > >>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >>> index 0790b2a4583e..34434071a415 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_display.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_display.c > >>> @@ -7737,6 +7737,7 @@ static void intel_atomic_dsb_finish(struct intel_atomic_state *state, > >>> > >>> intel_vrr_send_push(new_crtc_state->dsb_commit, new_crtc_state); > >>> intel_dsb_wait_vblank_delay(state, new_crtc_state->dsb_commit); > >>> + intel_vrr_check_push_sent(new_crtc_state->dsb_commit, new_crtc_state); > >>> intel_dsb_interrupt(new_crtc_state->dsb_commit); > >>> } > >>> } > >>> @@ -7886,6 +7887,9 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) > >>> intel_crtc_disable_flip_done(state, crtc); > >>> > >>> intel_atomic_dsb_wait_commit(new_crtc_state); > >>> + > >>> + if (!state->base.legacy_cursor_update && !new_crtc_state->use_dsb) > >>> + intel_vrr_check_push_sent(NULL, new_crtc_state); > >>> } > >>> > >>> /* > >>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c > >>> index adb51609d0a3..cac49319026d 100644 > >>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c > >>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c > >>> @@ -416,6 +416,40 @@ void intel_vrr_send_push(struct intel_dsb *dsb, > >>> intel_dsb_nonpost_end(dsb); > >>> } > >>> > >>> +void intel_vrr_check_push_sent(struct intel_dsb *dsb, > >>> + const struct intel_crtc_state *crtc_state) > >>> +{ > >>> + struct intel_display *display = to_intel_display(crtc_state); > >>> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > >>> + enum transcoder cpu_transcoder = crtc_state->cpu_transcoder; > >>> + > >>> + if (!crtc_state->vrr.enable) > >> I think you mean: > >> > >> if (crtc_state->vrr.enable) return; > > No. We want to do the check when VRR is enabled (when we are > > actually sending pushes), and skip it otherwise (when we don't > > send pushes anyway). > Oh ok, I got confused with the change log: > > v3: Skip the push send check when VRR is enabled. My bad. I'll fix up the commit msg when pushing. Thanks for the review. -- Ville Syrjälä Intel