Re: [PATCH 3/4] drm/msm: dpu: Untangle frame_done timeout units

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

On 2019-01-28 12:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> There exists a bunch of confusion as to what the actual units of
> frame_done is:
> 
> - The definition states it's in # of frames
> - CRTC treats it like it's ms
> - frame_done_timeout comment thinks it's Hz, but it stores ms
> - frame_done timer is setup such that it _should_ be in frames, but the
>   timeout is super long
> 
> So this patch tries to interpret what the driver really wants. I've
> de-centralized the #define since the consumers are expecting different
> units.
> 
> For crtc, we just use 60ms since that's what it was doing before.
> Perhaps we could get fancy and scale with vrefresh, but that's for
> another time.
> 
> For encoder, fix the comments and rename frame_done_timeout so it's
> obvious what the units are. In practice, frame_done_timeout is really
> just checked against 0 || !0, which I guess is why the units being 
> wrong
> didn't matter. I've also dropped the timeout from the previous 60 
> frames
> to 5. That seems like more than enough time to give up on a frame, and
> my guess is that no one intended for the timeout to _actually_ be 60
> frames.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  5 ++++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 +++++++++++--------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h     |  3 ---
>  3 files changed, 15 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 4e4b64821c9e8..b0b394af2a7ef 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -46,6 +46,9 @@
>  #define LEFT_MIXER 0
>  #define RIGHT_MIXER 1
> 
> +/* timeout in ms waiting for frame done */
> +#define DPU_CRTC_FRAME_DONE_TIMEOUT_MS	60
> +
>  static struct dpu_kms *_dpu_crtc_get_kms(struct drm_crtc *crtc)
>  {
>  	struct msm_drm_private *priv = crtc->dev->dev_private;
> @@ -683,7 +686,7 @@ static int _dpu_crtc_wait_for_frame_done(struct 
> drm_crtc
> *crtc)
> 
>  	DPU_ATRACE_BEGIN("frame done completion wait");
>  	ret = wait_for_completion_timeout(&dpu_crtc->frame_done_comp,
> -			msecs_to_jiffies(DPU_FRAME_DONE_TIMEOUT));
> +			msecs_to_jiffies(DPU_CRTC_FRAME_DONE_TIMEOUT_MS));
>  	if (!ret) {
>  		DRM_ERROR("frame done wait timed out, ret:%d\n", ret);
>  		rc = -ETIMEDOUT;
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 83a4c47dbed2d..51e46b206c73e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -69,6 +69,9 @@
> 
>  #define MAX_VDISPLAY_SPLIT 1080
> 
> +/* timeout in frames waiting for frame done */
> +#define DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES 5
> +
>  /**
>   * enum dpu_enc_rc_events - events for resource control state machine
>   * @DPU_ENC_RC_EVENT_KICKOFF:
> @@ -158,7 +161,7 @@ enum dpu_enc_rc_states {
>   *				Bit0 = phys_encs[0] etc.
>   * @crtc_frame_event_cb:	callback handler for frame event
>   * @crtc_frame_event_cb_data:	callback handler private data
> - * @frame_done_timeout:		frame done timeout in Hz
> + * @frame_done_timeout_ms:	frame done timeout in ms
>   * @frame_done_timer:		watchdog timer for frame done event
>   * @vsync_event_timer:		vsync timer
>   * @disp_info:			local copy of msm_display_info struct
> @@ -196,7 +199,7 @@ struct dpu_encoder_virt {
>  	void (*crtc_frame_event_cb)(void *, u32 event);
>  	void *crtc_frame_event_cb_data;
> 
> -	atomic_t frame_done_timeout;
> +	atomic_t frame_done_timeout_ms;
>  	struct timer_list frame_done_timer;
>  	struct timer_list vsync_event_timer;
> 
> @@ -1184,7 +1187,7 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc)
>  	}
> 
>  	/* after phys waits for frame-done, should be no more frames pending 
> */
> -	if (atomic_xchg(&dpu_enc->frame_done_timeout, 0)) {
> +	if (atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
>  		DPU_ERROR("enc%d timeout pending\n", drm_enc->base.id);
>  		del_timer_sync(&dpu_enc->frame_done_timer);
>  	}
> @@ -1341,7 +1344,7 @@ static void dpu_encoder_frame_done_callback(
>  		}
> 
>  		if (!dpu_enc->frame_busy_mask[0]) {
> -			atomic_set(&dpu_enc->frame_done_timeout, 0);
> +			atomic_set(&dpu_enc->frame_done_timeout_ms, 0);
>  			del_timer(&dpu_enc->frame_done_timer);
> 
>  			dpu_encoder_resource_control(drm_enc,
> @@ -1804,10 +1807,10 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc, bool async)
> 
>  	trace_dpu_enc_kickoff(DRMID(drm_enc));
> 
> -	timeout_ms = DPU_FRAME_DONE_TIMEOUT * 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, 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));
> 
> @@ -2129,7 +2132,7 @@ static void dpu_encoder_frame_done_timeout(struct
> timer_list *t)
>  		DRM_DEBUG_KMS("id:%u invalid timeout frame_busy_mask=%lu\n",
>  			      DRMID(drm_enc), dpu_enc->frame_busy_mask[0]);
>  		return;
> -	} else if (!atomic_xchg(&dpu_enc->frame_done_timeout, 0)) {
> +	} else if (!atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
>  		DRM_DEBUG_KMS("id:%u invalid timeout\n", DRMID(drm_enc));
>  		return;
>  	}
> @@ -2175,7 +2178,7 @@ int dpu_encoder_setup(struct drm_device *dev, 
> struct
> drm_encoder *enc,
> 
>  	spin_lock_init(&dpu_enc->enc_spinlock);
> 
> -	atomic_set(&dpu_enc->frame_done_timeout, 0);
> +	atomic_set(&dpu_enc->frame_done_timeout_ms, 0);
>  	timer_setup(&dpu_enc->frame_done_timer,
>  			dpu_encoder_frame_done_timeout, 0);
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> index ac75cfc267f40..31e9ef96ca5dc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
> @@ -73,9 +73,6 @@
> 
>  #define DPU_NAME_SIZE  12
> 
> -/* timeout in frames waiting for frame done */
> -#define DPU_FRAME_DONE_TIMEOUT	60
> -
>  /*
>   * struct dpu_irq_callback - IRQ callback handlers
>   * @list: list to callback

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