| Message ID | 20190827213421.21917-2-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: async commit support | expand |
On Tue, Aug 27, 2019 at 02:33:31PM -0700, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > It attempted to avoid fps drops in the presence of cursor updates. But > it is racing, and can result in hw updates after flush before vblank, > which leads to underruns. > > Signed-off-by: Rob Clark <robdclark@chromium.org> Reviewed-by: Sean Paul <sean@poorly.run> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 41 ++++++++++----------- > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 39 +++++++------------- > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 3 +- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 +-- > drivers/gpu/drm/msm/msm_atomic.c | 3 +- > 6 files changed, 37 insertions(+), 57 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > index a52439e029c9..c3f7154017c4 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c > @@ -610,7 +610,7 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc) > return rc; > } > > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async) > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) > { > struct drm_encoder *encoder; > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); > @@ -636,35 +636,32 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async) > crtc->state->encoder_mask) > dpu_encoder_prepare_for_kickoff(encoder); > > - if (!async) { > - /* wait for previous frame_event_done completion */ > - DPU_ATRACE_BEGIN("wait_for_frame_done_event"); > - ret = _dpu_crtc_wait_for_frame_done(crtc); > - DPU_ATRACE_END("wait_for_frame_done_event"); > - if (ret) { > - DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n", > - crtc->base.id, > - atomic_read(&dpu_crtc->frame_pending)); > - goto end; > - } > + /* wait for previous frame_event_done completion */ > + DPU_ATRACE_BEGIN("wait_for_frame_done_event"); > + ret = _dpu_crtc_wait_for_frame_done(crtc); > + DPU_ATRACE_END("wait_for_frame_done_event"); > + if (ret) { > + DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n", > + crtc->base.id, > + atomic_read(&dpu_crtc->frame_pending)); > + goto end; > + } > > - if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) { > - /* acquire bandwidth and other resources */ > - DPU_DEBUG("crtc%d first commit\n", crtc->base.id); > - } else > - DPU_DEBUG("crtc%d commit\n", crtc->base.id); > + if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) { > + /* acquire bandwidth and other resources */ > + DPU_DEBUG("crtc%d first commit\n", crtc->base.id); > + } else > + DPU_DEBUG("crtc%d commit\n", crtc->base.id); > > - dpu_crtc->play_count++; > - } > + dpu_crtc->play_count++; > > dpu_vbif_clear_errors(dpu_kms); > > drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) > - dpu_encoder_kickoff(encoder, async); > + dpu_encoder_kickoff(encoder); > > end: > - if (!async) > - reinit_completion(&dpu_crtc->frame_done_comp); > + reinit_completion(&dpu_crtc->frame_done_comp); > DPU_ATRACE_END("crtc_commit"); > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > index 5181f079a6a1..10f78459f6c2 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h > @@ -238,9 +238,8 @@ void dpu_crtc_vblank_callback(struct drm_crtc *crtc); > /** > * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this crtc > * @crtc: Pointer to drm crtc object > - * @async: true if the commit is asynchronous, false otherwise > */ > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async); > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc); > > /** > * dpu_crtc_complete_commit - callback signalling completion of current commit > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > index 627c57594221..ac2d534bf59e 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > @@ -1421,8 +1421,7 @@ static void dpu_encoder_off_work(struct work_struct *work) > * extra_flush_bits: Additional bit mask to include in flush trigger > */ > static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc, > - struct dpu_encoder_phys *phys, uint32_t extra_flush_bits, > - bool async) > + struct dpu_encoder_phys *phys, uint32_t extra_flush_bits) > { > struct dpu_hw_ctl *ctl; > int pending_kickoff_cnt; > @@ -1439,10 +1438,7 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc, > return; > } > > - if (!async) > - pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys); > - else > - pending_kickoff_cnt = atomic_read(&phys->pending_kickoff_cnt); > + pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys); > > if (extra_flush_bits && ctl->ops.update_pending_flush) > ctl->ops.update_pending_flush(ctl, extra_flush_bits); > @@ -1553,8 +1549,7 @@ static void dpu_encoder_helper_hw_reset(struct dpu_encoder_phys *phys_enc) > * a time. > * dpu_enc: Pointer to virtual encoder structure > */ > -static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc, > - bool async) > +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc) > { > struct dpu_hw_ctl *ctl; > uint32_t i, pending_flush; > @@ -1581,13 +1576,12 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc, > * for async commits. So don't set this for async, since it'll > * roll over to the next commit. > */ > - if (!async && phys->split_role != ENC_ROLE_SLAVE) > + if (phys->split_role != ENC_ROLE_SLAVE) > set_bit(i, dpu_enc->frame_busy_mask); > > if (!phys->ops.needs_single_flush || > !phys->ops.needs_single_flush(phys)) > - _dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0, > - async); > + _dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0); > else if (ctl->ops.get_pending_flush) > pending_flush |= ctl->ops.get_pending_flush(ctl); > } > @@ -1597,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc, > _dpu_encoder_trigger_flush( > &dpu_enc->base, > dpu_enc->cur_master, > - pending_flush, async); > + pending_flush); > } > > _dpu_encoder_trigger_start(dpu_enc->cur_master); > @@ -1815,11 +1809,12 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) > } > } > > -void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) > +void dpu_encoder_kickoff(struct drm_encoder *drm_enc) > { > struct dpu_encoder_virt *dpu_enc; > struct dpu_encoder_phys *phys; > ktime_t wakeup_time; > + unsigned long timeout_ms; > unsigned int i; > > DPU_ATRACE_BEGIN("encoder_kickoff"); > @@ -1827,23 +1822,15 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) > > trace_dpu_enc_kickoff(DRMID(drm_enc)); > > - /* > - * Asynchronous frames don't handle FRAME_DONE events. As such, they > - * shouldn't enable the frame_done watchdog since it will always time > - * out. > - */ > - if (!async) { > - unsigned long timeout_ms; > - timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / > + timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / > drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); > > - atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms); > - mod_timer(&dpu_enc->frame_done_timer, > - jiffies + msecs_to_jiffies(timeout_ms)); > - } > + atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms); > + mod_timer(&dpu_enc->frame_done_timer, > + jiffies + msecs_to_jiffies(timeout_ms)); > > /* All phys encs are ready to go, trigger the kickoff */ > - _dpu_encoder_kickoff_phys(dpu_enc, async); > + _dpu_encoder_kickoff_phys(dpu_enc); > > /* allow phys encs to handle any post-kickoff business */ > for (i = 0; i < dpu_enc->num_phys_encs; i++) { > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > index 997d131c2440..8465b37adf3b 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > @@ -82,9 +82,8 @@ void dpu_encoder_trigger_kickoff_pending(struct drm_encoder *encoder); > * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path > * (i.e. ctl flush and start) immediately. > * @encoder: encoder pointer > - * @async: true if this is an asynchronous commit > */ > -void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async); > +void dpu_encoder_kickoff(struct drm_encoder *encoder); > > /** > * dpu_encoder_wait_for_event - Waits for encoder events > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > index b8d264bd15df..31454cc5d8c5 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c > @@ -298,7 +298,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder) > continue; > > trace_dpu_kms_enc_enable(DRMID(crtc)); > - dpu_crtc_commit_kickoff(crtc, false); > + dpu_crtc_commit_kickoff(crtc); > } > } > > @@ -315,8 +315,7 @@ static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state) > > if (crtc->state->active) { > trace_dpu_kms_commit(DRMID(crtc)); > - dpu_crtc_commit_kickoff(crtc, > - state->legacy_cursor_update); > + dpu_crtc_commit_kickoff(crtc); > } > } > } > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c > index dd16babdd8c0..e5aae1645933 100644 > --- a/drivers/gpu/drm/msm/msm_atomic.c > +++ b/drivers/gpu/drm/msm/msm_atomic.c > @@ -70,8 +70,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state) > kms->funcs->commit(kms, state); > } > > - if (!state->legacy_cursor_update) > - msm_atomic_wait_for_commit_done(dev, state); > + msm_atomic_wait_for_commit_done(dev, state); > > kms->funcs->complete_commit(kms, state); > > -- > 2.21.0 >
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index a52439e029c9..c3f7154017c4 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -610,7 +610,7 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc) return rc; } -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async) +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc) { struct drm_encoder *encoder; struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc); @@ -636,35 +636,32 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async) crtc->state->encoder_mask) dpu_encoder_prepare_for_kickoff(encoder); - if (!async) { - /* wait for previous frame_event_done completion */ - DPU_ATRACE_BEGIN("wait_for_frame_done_event"); - ret = _dpu_crtc_wait_for_frame_done(crtc); - DPU_ATRACE_END("wait_for_frame_done_event"); - if (ret) { - DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n", - crtc->base.id, - atomic_read(&dpu_crtc->frame_pending)); - goto end; - } + /* wait for previous frame_event_done completion */ + DPU_ATRACE_BEGIN("wait_for_frame_done_event"); + ret = _dpu_crtc_wait_for_frame_done(crtc); + DPU_ATRACE_END("wait_for_frame_done_event"); + if (ret) { + DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n", + crtc->base.id, + atomic_read(&dpu_crtc->frame_pending)); + goto end; + } - if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) { - /* acquire bandwidth and other resources */ - DPU_DEBUG("crtc%d first commit\n", crtc->base.id); - } else - DPU_DEBUG("crtc%d commit\n", crtc->base.id); + if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) { + /* acquire bandwidth and other resources */ + DPU_DEBUG("crtc%d first commit\n", crtc->base.id); + } else + DPU_DEBUG("crtc%d commit\n", crtc->base.id); - dpu_crtc->play_count++; - } + dpu_crtc->play_count++; dpu_vbif_clear_errors(dpu_kms); drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) - dpu_encoder_kickoff(encoder, async); + dpu_encoder_kickoff(encoder); end: - if (!async) - reinit_completion(&dpu_crtc->frame_done_comp); + reinit_completion(&dpu_crtc->frame_done_comp); DPU_ATRACE_END("crtc_commit"); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h index 5181f079a6a1..10f78459f6c2 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h @@ -238,9 +238,8 @@ void dpu_crtc_vblank_callback(struct drm_crtc *crtc); /** * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this crtc * @crtc: Pointer to drm crtc object - * @async: true if the commit is asynchronous, false otherwise */ -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async); +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc); /** * dpu_crtc_complete_commit - callback signalling completion of current commit diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 627c57594221..ac2d534bf59e 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1421,8 +1421,7 @@ static void dpu_encoder_off_work(struct work_struct *work) * extra_flush_bits: Additional bit mask to include in flush trigger */ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc, - struct dpu_encoder_phys *phys, uint32_t extra_flush_bits, - bool async) + struct dpu_encoder_phys *phys, uint32_t extra_flush_bits) { struct dpu_hw_ctl *ctl; int pending_kickoff_cnt; @@ -1439,10 +1438,7 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc, return; } - if (!async) - pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys); - else - pending_kickoff_cnt = atomic_read(&phys->pending_kickoff_cnt); + pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys); if (extra_flush_bits && ctl->ops.update_pending_flush) ctl->ops.update_pending_flush(ctl, extra_flush_bits); @@ -1553,8 +1549,7 @@ static void dpu_encoder_helper_hw_reset(struct dpu_encoder_phys *phys_enc) * a time. * dpu_enc: Pointer to virtual encoder structure */ -static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc, - bool async) +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc) { struct dpu_hw_ctl *ctl; uint32_t i, pending_flush; @@ -1581,13 +1576,12 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc, * for async commits. So don't set this for async, since it'll * roll over to the next commit. */ - if (!async && phys->split_role != ENC_ROLE_SLAVE) + if (phys->split_role != ENC_ROLE_SLAVE) set_bit(i, dpu_enc->frame_busy_mask); if (!phys->ops.needs_single_flush || !phys->ops.needs_single_flush(phys)) - _dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0, - async); + _dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0); else if (ctl->ops.get_pending_flush) pending_flush |= ctl->ops.get_pending_flush(ctl); } @@ -1597,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc, _dpu_encoder_trigger_flush( &dpu_enc->base, dpu_enc->cur_master, - pending_flush, async); + pending_flush); } _dpu_encoder_trigger_start(dpu_enc->cur_master); @@ -1815,11 +1809,12 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc) } } -void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) +void dpu_encoder_kickoff(struct drm_encoder *drm_enc) { struct dpu_encoder_virt *dpu_enc; struct dpu_encoder_phys *phys; ktime_t wakeup_time; + unsigned long timeout_ms; unsigned int i; DPU_ATRACE_BEGIN("encoder_kickoff"); @@ -1827,23 +1822,15 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async) trace_dpu_enc_kickoff(DRMID(drm_enc)); - /* - * Asynchronous frames don't handle FRAME_DONE events. As such, they - * shouldn't enable the frame_done watchdog since it will always time - * out. - */ - if (!async) { - unsigned long timeout_ms; - timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / + timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 / drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode); - atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms); - mod_timer(&dpu_enc->frame_done_timer, - jiffies + msecs_to_jiffies(timeout_ms)); - } + atomic_set(&dpu_enc->frame_done_timeout_ms, timeout_ms); + mod_timer(&dpu_enc->frame_done_timer, + jiffies + msecs_to_jiffies(timeout_ms)); /* All phys encs are ready to go, trigger the kickoff */ - _dpu_encoder_kickoff_phys(dpu_enc, async); + _dpu_encoder_kickoff_phys(dpu_enc); /* allow phys encs to handle any post-kickoff business */ for (i = 0; i < dpu_enc->num_phys_encs; i++) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 997d131c2440..8465b37adf3b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -82,9 +82,8 @@ void dpu_encoder_trigger_kickoff_pending(struct drm_encoder *encoder); * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path * (i.e. ctl flush and start) immediately. * @encoder: encoder pointer - * @async: true if this is an asynchronous commit */ -void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async); +void dpu_encoder_kickoff(struct drm_encoder *encoder); /** * dpu_encoder_wait_for_event - Waits for encoder events diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index b8d264bd15df..31454cc5d8c5 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -298,7 +298,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder) continue; trace_dpu_kms_enc_enable(DRMID(crtc)); - dpu_crtc_commit_kickoff(crtc, false); + dpu_crtc_commit_kickoff(crtc); } } @@ -315,8 +315,7 @@ static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state) if (crtc->state->active) { trace_dpu_kms_commit(DRMID(crtc)); - dpu_crtc_commit_kickoff(crtc, - state->legacy_cursor_update); + dpu_crtc_commit_kickoff(crtc); } } } diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index dd16babdd8c0..e5aae1645933 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -70,8 +70,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state) kms->funcs->commit(kms, state); } - if (!state->legacy_cursor_update) - msm_atomic_wait_for_commit_done(dev, state); + msm_atomic_wait_for_commit_done(dev, state); kms->funcs->complete_commit(kms, state);