| Message ID | 20190128204306.95076-3-sean@poorly.run (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh | expand |
On Mon, Jan 28, 2019 at 12:43 PM Sean Paul <sean@poorly.run> 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> > --- > 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@google.com> > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
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