On Wed, Sep 26, 2018 at 11:56:47AM -0700, Jeykumar Sankaran wrote:
> On 2018-09-19 11:56, Sean Paul wrote:
> > From: Sean Paul <seanp...@chromium.org>
> >
> > This patch sprinkles a few async/legacy_cursor_update checks
> > through commit to ensure that cursor updates aren't blocked on vsync.
> > There are 2 main components to this, the first is that we don't want to
> > wait_for_commit_done in msm_atomic before returning from
> > atomic_complete.
> > The second is that in dpu we don't want to wait for frame_done events
> > when
> > updating the cursor.
> >
> > Signed-off-by: Sean Paul <seanp...@chromium.org>
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 44 +++++++++++----------
> > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 3 +-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 21 ++++++----
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 6 ++-
> > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 5 ++-
> > drivers/gpu/drm/msm/msm_atomic.c | 3 +-
> > 6 files changed, 48 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index a8f2dd7a37c7..b23f00a2554b 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -816,7 +816,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
> > drm_crtc *crtc)
> > return rc;
> > }
> >
> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
> > {
> > struct drm_encoder *encoder;
> > struct drm_device *dev;
> > @@ -862,27 +862,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> > * Encoder will flush/start now, unless it has a tx
> > pending.
> > * If so, it may delay and flush at an irq event (e.g.
> > ppdone)
> > */
> > - dpu_encoder_prepare_for_kickoff(encoder, ¶ms);
> > + dpu_encoder_prepare_for_kickoff(encoder, ¶ms, async);
> > }
> >
> > - /* wait for 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 (!async) {
> > + /* wait for 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);
> >
> > - dpu_crtc->play_count++;
> > + dpu_crtc->play_count++;
> > + }
> >
> > dpu_vbif_clear_errors(dpu_kms);
> >
> > @@ -890,11 +893,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> > if (encoder->crtc != crtc)
> > continue;
> >
> > - dpu_encoder_kickoff(encoder);
> > + dpu_encoder_kickoff(encoder, async);
> > }
> >
> > end:
> > - reinit_completion(&dpu_crtc->frame_done_comp);
> > + if (!async)
> > + 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 3723b4830335..67c9f59997cf 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -285,8 +285,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> > /**
> > * 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);
> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
> >
> > /**
> > * 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 c2e8985b4c54..fc729fc8dd8c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -1410,7 +1410,7 @@ static void dpu_encoder_off_work(struct
> > kthread_work
> > *work)
> > * extra_flush_bits: Additional bit mask to include in flush trigger
> > */
> > static inline void _dpu_encoder_trigger_flush(struct drm_encoder
> > *drm_enc,
> > - struct dpu_encoder_phys *phys, uint32_t extra_flush_bits)
> > + struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
> > bool async)
> > {
> > struct dpu_hw_ctl *ctl;
> > int pending_kickoff_cnt;
> > @@ -1433,7 +1433,10 @@ static inline void
> > _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
> > return;
> > }
> >
> > - pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> > + if (!async)
> > + pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> > + else
> > + pending_kickoff_cnt =
> > atomic_read(&phys->pending_kickoff_cnt);
> >
> > if (extra_flush_bits && ctl->ops.update_pending_flush)
> > ctl->ops.update_pending_flush(ctl, extra_flush_bits);
> > @@ -1545,7 +1548,8 @@ 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)
> > +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
> > + bool async)
> > {
> > struct dpu_hw_ctl *ctl;
> > uint32_t i, pending_flush;
> > @@ -1576,7 +1580,8 @@ static void _dpu_encoder_kickoff_phys(struct
> > dpu_encoder_virt *dpu_enc)
> > 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);
> > + _dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> > 0x0,
> > + async);
> > else if (ctl->ops.get_pending_flush)
> > pending_flush |= ctl->ops.get_pending_flush(ctl);
> > }
> > @@ -1586,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);
> > + pending_flush, async);
> > }
> >
> > _dpu_encoder_trigger_start(dpu_enc->cur_master);
> > @@ -1770,7 +1775,7 @@ static void
> > dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
> > }
> >
> > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
> > - struct dpu_encoder_kickoff_params *params)
> > + struct dpu_encoder_kickoff_params *params, bool async)
> > {
> > struct dpu_encoder_virt *dpu_enc;
> > struct dpu_encoder_phys *phys;
> > @@ -1811,7 +1816,7 @@ void dpu_encoder_prepare_for_kickoff(struct
> > drm_encoder *drm_enc,
> > }
> > }
> >
> > -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
> > +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
> > {
> > struct dpu_encoder_virt *dpu_enc;
> > struct dpu_encoder_phys *phys;
> > @@ -1834,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> > *drm_enc)
> > ((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
> > 1000));
> >
> > /* All phys encs are ready to go, trigger the kickoff */
> > - _dpu_encoder_kickoff_phys(dpu_enc);
> > + _dpu_encoder_kickoff_phys(dpu_enc, async);
> >
> > /* 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 9dbf38f446d9..c2044122d609 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -81,9 +81,10 @@ void dpu_encoder_register_frame_event_callback(struct
> > drm_encoder *encoder,
> > * Delayed: Block until next trigger can be issued.
> > * @encoder: encoder pointer
> > * @params: kickoff time parameters
> > + * @async: true if this is an asynchronous commit
> > */
> > void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
> > - struct dpu_encoder_kickoff_params *params);
> > + struct dpu_encoder_kickoff_params *params, bool async);
> >
> > /**
> > * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
> > previous
> > @@ -96,8 +97,9 @@ 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);
> > +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
> >
> > /**
> > * 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 0a683e65a9f3..1cba21edd862 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
> > *encoder)
> >
> > if (crtc && crtc->state->active) {
> > trace_dpu_kms_enc_enable(DRMID(crtc));
> > - dpu_crtc_commit_kickoff(crtc);
> > + dpu_crtc_commit_kickoff(crtc, false);
> > }
> > }
> >
> > @@ -369,7 +369,8 @@ 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);
> > + dpu_crtc_commit_kickoff(crtc,
> > +
> > state->legacy_cursor_update);
> > }
> > }
> > }
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index c1f1779c980f..7912130ce5ce 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -76,7 +76,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
> > *state)
> > kms->funcs->commit(kms, state);
> > }
> >
> > - msm_atomic_wait_for_commit_done(dev, state);
> > + if (!state->legacy_cursor_update)
> I see state->async_update is updated after validation checks on the async
> capabilities. Shouldn't we use
> that var instead of state->legacy_cursor_update?
> > + msm_atomic_wait_for_commit_done(dev, state);
> >
> > kms->funcs->complete_commit(kms, state);
>
> Do we need to introduce plane helpers atomic_async_update and
> atomic_async_check in DPU before supporting
> these wait skips? or are they irrelevant for this legacy async path?
I was trying to limit the scope of this to just cursor updates. I think once/if
support is added for generic async it makes sense to change over to that
verbage.
Sean
>
> --
> Jeykumar S
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
