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

[Nautiyal, Ankit K]

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


Regards,
Ankit

>
>> Regards,
>>
>> Ankit
>>
>>> +		return;
>>> +
>>> +	/*
>>> +	 * Make sure the push send bit has cleared. This should
>>> +	 * already be the case as long as the caller makes sure
>>> +	 * this is called after the delayed vblank has occurred.
>>> +	 */
>>> +	if (dsb) {
>>> +		int wait_us, count;
>>> +
>>> +		wait_us = 2;
>>> +		count = 1;
>>> +
>>> +		/*
>>> +		 * If the bit hasn't cleared the DSB will
>>> +		 * raise the poll error interrupt.
>>> +		 */
>>> +		intel_dsb_poll(dsb, TRANS_PUSH(display, cpu_transcoder),
>>> +			       TRANS_PUSH_SEND, 0, wait_us, count);
>>> +	} else {
>>> +		if (intel_vrr_is_push_sent(crtc_state))
>>> +			drm_err(display->drm, "[CRTC:%d:%s] VRR push send still pending\n",
>>> +				crtc->base.base.id, crtc->base.name);
>>> +	}
>>> +}
>>> +
>>>    bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state)
>>>    {
>>>    	struct intel_display *display = to_intel_display(crtc_state);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
>>> index 899cbf40f880..514822577e8a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
>>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
>>> @@ -25,6 +25,8 @@ void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state)
>>>    void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>>>    void intel_vrr_send_push(struct intel_dsb *dsb,
>>>    			 const struct intel_crtc_state *crtc_state);
>>> +void intel_vrr_check_push_sent(struct intel_dsb *dsb,
>>> +			       const struct intel_crtc_state *crtc_state);
>>>    bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state);
>>>    void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state);
>>>    void intel_vrr_get_config(struct intel_crtc_state *crtc_state);