Re: [PATCH 2/2] drm/msm: dpu: Make legacy cursor updates asynchronous

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2018-09-19 11:56, Sean Paul wrote:
> From: Sean Paul <seanpaul@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 <seanpaul@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, &params);
> +		dpu_encoder_prepare_for_kickoff(encoder, &params, 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?

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno