On Mon, Jan 28, 2019 at 12:43 PM Sean Paul <sean@xxxxxxxxxx> wrote:
>
> From: Sean Paul <seanpaul@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> ---
> 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
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
Nice cleanup! Units make it much easier to read.
Reviewed-by: Fritz Koenig <frkoenig@xxxxxxxxxx>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
