Re: [PATCH v3 7/8] drm/i915/vrr: Check that the push send bit is clear after delayed vblank

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

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@linux.intel.com>
> >>>
> >>> 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@intel.com> #v1
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>    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