On Thu, Nov 08, 2018 at 02:00:51PM -0800, Jeykumar Sankaran wrote:
On 2018-10-30 09:00, Sean Paul wrote:
> From: Sean Paul <seanpaul@xxxxxxxxxxxx>
>
> 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.
>
> Changes in v2:
> - None
>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> 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 | 22 +++++++----
> 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, 49 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 ed84cf44a222..1e3e57817b72 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -702,7 +702,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 = crtc->dev;
> @@ -731,27 +731,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);
>
> @@ -759,11 +762,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 4822602402f9..ec633ce3ee6c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -277,8 +277,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 82c55efb500f..a8ba10ceaacf 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1375,7 +1375,8 @@ static void dpu_encoder_off_work(struct
> kthread_work
> *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)
> + struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
> + bool async)
> {
> struct dpu_hw_ctl *ctl;
> int pending_kickoff_cnt;
> @@ -1398,7 +1399,10 @@ static 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);
> @@ -1511,7 +1515,8 @@ 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)
> +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt
*dpu_enc,
> + bool async)
> {
> struct dpu_hw_ctl *ctl;
> uint32_t i, pending_flush;
> @@ -1542,7 +1547,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);
> }
> @@ -1552,7 +1558,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);
> @@ -1736,7 +1742,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;
> @@ -1775,7 +1781,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;
> @@ -1798,7 +1804,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 985c855796ae..57ad83868766 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 2088a20eb270..f5b1256e32b6 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -83,7 +83,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)
> + msm_atomic_wait_for_commit_done(dev, state);
Was this change tested with MDP5? They have a different path
to support async updates.
I just realized I didn't respond to this. I haven't tested with MDP5, I
don't
have the proper hw. That said, cursor on MDP5 won't work without this
change
since it'll ratelimit cursor updates to once-pre-vsync.
Sean
