On Thu, Aug 29, 2024 at 01:48:41PM GMT, Jessica Zhang wrote:
> Add a helper that will handle the correct order of the encoder kickoffs
> for concurrent writeback.
>
> For concurrent writeback, the realtime encoder must always kickoff last
> as it will call the trigger flush and start.
>
> This avoids the following scenario where the writeback encoder
> increments the pending kickoff count after the WB_DONE interrupt is
> fired:
>
> If the realtime encoder is kicked off first, the encoder kickoff will
> flush/start the encoder and increment the pending kickoff count. The
> WB_DONE interrupt then fires (before the writeback encoder is kicked
> off). When the writeback encoder enters its kickoff, it will skip the
> flush/start (due to CWB being enabled) and hit a frame done timeout
> as the frame was kicked off (and the WB_DONE interrupt fired) without
> the pending kickoff count being incremented.
>
> In addition, the writeback timer should only start after the realtime
> encoder is kicked off to ensure that we don't get timeouts when the
> system has a heavy load (ex. when debug logs are enabled)
>
> Signed-off-by: Jessica Zhang <quic_jessz...@quicinc.com>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 67
> ++++++++++++++++++++++++++------
> 1 file changed, 55 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index daf5f751f584..d2f91e89eba7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -928,6 +928,42 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc
> *crtc)
> return rc;
> }
>
> +static int dpu_crtc_kickoff_clone_mode(struct drm_crtc *crtc)
> +{
> + struct drm_encoder *encoder;
> + struct drm_encoder *rt_encoder = NULL, *wb_encoder;
> +
> + /* Find encoder for real time display */
> + drm_for_each_encoder_mask(encoder, crtc->dev,
> + crtc->state->encoder_mask) {
> + if (encoder->encoder_type == DRM_MODE_ENCODER_VIRTUAL)
> + wb_encoder = encoder;
> + else
> + rt_encoder = encoder;
> + }
> +
> + if (!rt_encoder || !wb_encoder) {
> + DRM_DEBUG_ATOMIC("real time or wb encoder not found\n");
> + return -EINVAL;
> + }
> +
> + dpu_encoder_prepare_for_kickoff(wb_encoder);
> + dpu_encoder_prepare_for_kickoff(rt_encoder);
> +
> + /*
> + * Kickoff real time encoder last as it's the encoder that
> + * will do the flush
> + */
> + dpu_encoder_kickoff(wb_encoder);
> + dpu_encoder_kickoff(rt_encoder);
> +
> + /* Don't start frame done timers until the kickoffs have finished */
> + dpu_encoder_start_frame_done_timer(wb_encoder);
> + dpu_encoder_start_frame_done_timer(rt_encoder);
> +
> + return 0;
> +}
> +
> void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> {
> struct drm_encoder *encoder;
> @@ -952,13 +988,25 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> goto end;
> }
> }
> - /*
> - * 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)
> - */
> - drm_for_each_encoder_mask(encoder, crtc->dev,
> - crtc->state->encoder_mask)
> - dpu_encoder_prepare_for_kickoff(encoder);
> +
> + if (drm_crtc_in_clone_mode(crtc->state)) {
> + if (dpu_crtc_kickoff_clone_mode(crtc))
> + goto end;
> + } else {
> + /*
> + * 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)
> + */
> + drm_for_each_encoder_mask(encoder, crtc->dev,
> + crtc->state->encoder_mask)
> + dpu_encoder_prepare_for_kickoff(encoder);
> +
> + drm_for_each_encoder_mask(encoder, crtc->dev,
> + crtc->state->encoder_mask) {
> + dpu_encoder_kickoff(encoder);
> + dpu_encoder_start_frame_done_timer(encoder);
> + }
> + }
>
> if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
> /* acquire bandwidth and other resources */
> @@ -970,11 +1018,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>
> dpu_vbif_clear_errors(dpu_kms);
>
> - drm_for_each_encoder_mask(encoder, crtc->dev,
> crtc->state->encoder_mask) {
> - dpu_encoder_kickoff(encoder);
> - dpu_encoder_start_frame_done_timer(encoder);
> - }
> -
I think I understand why you wanted to move the kickoff before the
frame_pending, etc. However at the same time dpu_vbif_clear_errors()
should be called before kickoff.
> reinit_completion(&dpu_crtc->frame_done_comp);
>
> end:
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
