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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux