| Message ID | 20250210160711.24010-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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; 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);
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). > > 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);
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. 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);
On 2/12/2025 6:39 PM, 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. > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > 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);
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.
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) + 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);