[PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh

[PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Use the drm_mode_vrefresh helper where we need refresh rate in case
vrefresh is empty.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 6 +++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c            | 2 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c     | 4 ++--
 4 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 941ac25d2a023..dd71cb6ba4f5c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector,
 
 	list_for_each_entry(cur_mode, &connector->modes, head) {
 		if (cur_mode->vdisplay == adj_mode->vdisplay &&
-			cur_mode->hdisplay == adj_mode->hdisplay &&
-			cur_mode->vrefresh == adj_mode->vrefresh) {
+		    cur_mode->hdisplay == adj_mode->hdisplay &&
+		    drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) {
 			adj_mode->private = cur_mode->private;
 			adj_mode->private_flags |= cur_mode->private_flags;
 		}
@@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 
 	atomic_set(&dpu_enc->frame_done_timeout,
 			DPU_FRAME_DONE_TIMEOUT * 1000 /
-			drm_enc->crtc->state->adjusted_mode.vrefresh);
+			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
 	mod_timer(&dpu_enc->frame_done_timer, jiffies +
 		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 99ab5ca9bed3b..f21163313d635 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
 		return;
 	}
 
-	tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh);
+	tc_cfg.vsync_count = vsync_hz /
+				(mode->vtotal * drm_mode_vrefresh(mode));
 
 	/* enable external TE after kickoff to avoid premature autorefresh */
 	tc_cfg.hw_vsync_mode = 0;
@@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
 	DPU_DEBUG_CMDENC(cmd_enc,
 		"tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
 		phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz,
-		mode->vtotal, mode->vrefresh);
+		mode->vtotal, drm_mode_vrefresh(mode));
 	DPU_DEBUG_CMDENC(cmd_enc,
 		"tc %d enable %u start_pos %u rd_ptr_irq %u\n",
 		phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index b01183b309b9e..da1f727d74957 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
 	ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect);
 	ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect);
 	ot_params.is_wfd = !pdpu->is_rt_pipe;
-	ot_params.frame_rate = crtc->mode.vrefresh;
+	ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode);
 	ot_params.vbif_idx = VBIF_RT;
 	ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl;
 	ot_params.rd = true;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
index c1962f29ec7d6..6341ac010f7de 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
@@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct drm_encoder *encoder,
 		return -EINVAL;
 	}
 
-	total_lines_x100 = mode->vtotal * mode->vrefresh;
+	total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode);
 	if (!total_lines_x100) {
 		DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n",
-				__func__, mode->vtotal, mode->vrefresh);
+			      __func__, mode->vtotal, drm_mode_vrefresh(mode));
 		return -EINVAL;
 	}
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[PATCH 2/4] drm/msm: dpu: Simplify frame_done watchdog timeout calculation

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

Instead of setting the timeout and then immediately reading it back
(along with the hand-rolled msecs_to_jiffies calculation), just
calculate it once and set it in both places at the same time.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index dd71cb6ba4f5c..83a4c47dbed2d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1791,6 +1791,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
+	unsigned long timeout_ms;
 	ktime_t wakeup_time;
 	unsigned int i;
 
@@ -1803,11 +1804,12 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 
 	trace_dpu_enc_kickoff(DRMID(drm_enc));
 
-	atomic_set(&dpu_enc->frame_done_timeout,
-			DPU_FRAME_DONE_TIMEOUT * 1000 /
-			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
-	mod_timer(&dpu_enc->frame_done_timer, jiffies +
-		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
+	timeout_ms = DPU_FRAME_DONE_TIMEOUT * 1000 /
+		drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode);
+
+	atomic_set(&dpu_enc->frame_done_timeout, timeout_ms);
+	mod_timer(&dpu_enc->frame_done_timer,
+		  jiffies + msecs_to_jiffies(timeout_ms));
 
 	/* All phys encs are ready to go, trigger the kickoff */
 	_dpu_encoder_kickoff_phys(dpu_enc, async);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

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

[Sean Paul]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/msm: dpu: Don't queue the frame_done watchdog for cursor

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

In the case of an async/cursor update, we don't wait for the frame_done
event, which means handle_frame_done is never called, and the frame_done
watchdog isn't canceled. Currently, this results in a frame_done timeout
every time the cursor moves without a synchronous frame following it up
before the timeout expires. Since we don't wait for frame_done, and
don't handle it, we shouldn't modify the watchdog.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 51e46b206c73e..05145cf9fb408 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1794,7 +1794,6 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
-	unsigned long timeout_ms;
 	ktime_t wakeup_time;
 	unsigned int i;
 
@@ -1807,12 +1806,20 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 
 	trace_dpu_enc_kickoff(DRMID(drm_enc));
 
-	timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 /
-		drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode);
+	/*
+	 * Asynchronous frames don't handle FRAME_DONE events. As such, they
+	 * shouldn't enable the frame_done watchdog since it will always time
+	 * out.
+	 */
+	if (!async) {
+		unsigned long timeout_ms;
+		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_ms, timeout_ms);
-	mod_timer(&dpu_enc->frame_done_timer,
-		  jiffies + msecs_to_jiffies(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));
+	}
 
 	/* All phys encs are ready to go, trigger the kickoff */
 	_dpu_encoder_kickoff_phys(dpu_enc, async);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh

[Abhinav Kumar]

On 2019-01-28 12:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Use the drm_mode_vrefresh helper where we need refresh rate in case
> vrefresh is empty.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 6 +++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c            | 2 +-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c     | 4 ++--
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 941ac25d2a023..dd71cb6ba4f5c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct
> drm_connector *connector,
> 
>  	list_for_each_entry(cur_mode, &connector->modes, head) {
>  		if (cur_mode->vdisplay == adj_mode->vdisplay &&
> -			cur_mode->hdisplay == adj_mode->hdisplay &&
> -			cur_mode->vrefresh == adj_mode->vrefresh) {
> +		    cur_mode->hdisplay == adj_mode->hdisplay &&
> +		    drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) {
>  			adj_mode->private = cur_mode->private;
>  			adj_mode->private_flags |= cur_mode->private_flags;
>  		}
> @@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc, bool async)
> 
>  	atomic_set(&dpu_enc->frame_done_timeout,
>  			DPU_FRAME_DONE_TIMEOUT * 1000 /
> -			drm_enc->crtc->state->adjusted_mode.vrefresh);
> +			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
>  	mod_timer(&dpu_enc->frame_done_timer, jiffies +
>  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 99ab5ca9bed3b..f21163313d635 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  		return;
>  	}
> 
> -	tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh);
> +	tc_cfg.vsync_count = vsync_hz /
> +				(mode->vtotal * drm_mode_vrefresh(mode));
> 
>  	/* enable external TE after kickoff to avoid premature autorefresh */
>  	tc_cfg.hw_vsync_mode = 0;
> @@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  	DPU_DEBUG_CMDENC(cmd_enc,
>  		"tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
>  		phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz,
> -		mode->vtotal, mode->vrefresh);
> +		mode->vtotal, drm_mode_vrefresh(mode));
>  	DPU_DEBUG_CMDENC(cmd_enc,
>  		"tc %d enable %u start_pos %u rd_ptr_irq %u\n",
>  		phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index b01183b309b9e..da1f727d74957 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct 
> drm_plane *plane,
>  	ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect);
>  	ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect);
>  	ot_params.is_wfd = !pdpu->is_rt_pipe;
> -	ot_params.frame_rate = crtc->mode.vrefresh;
> +	ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode);
>  	ot_params.vbif_idx = VBIF_RT;
>  	ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl;
>  	ot_params.rd = true;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> index c1962f29ec7d6..6341ac010f7de 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> @@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct
> drm_encoder *encoder,
>  		return -EINVAL;
>  	}
> 
> -	total_lines_x100 = mode->vtotal * mode->vrefresh;
> +	total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode);
>  	if (!total_lines_x100) {
>  		DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n",
> -				__func__, mode->vtotal, mode->vrefresh);
> +			      __func__, mode->vtotal, drm_mode_vrefresh(mode));
>  		return -EINVAL;
>  	}
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/msm: dpu: Simplify frame_done watchdog timeout calculation

[Abhinav Kumar]

On 2019-01-28 12:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Instead of setting the timeout and then immediately reading it back
> (along with the hand-rolled msecs_to_jiffies calculation), just
> calculate it once and set it in both places at the same time.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
Reviewed-by: Abhinav Kumar <abhinavk@codeaurora.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dd71cb6ba4f5c..83a4c47dbed2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1791,6 +1791,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc, bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> +	unsigned long timeout_ms;
>  	ktime_t wakeup_time;
>  	unsigned int i;
> 
> @@ -1803,11 +1804,12 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc, bool async)
> 
>  	trace_dpu_enc_kickoff(DRMID(drm_enc));
> 
> -	atomic_set(&dpu_enc->frame_done_timeout,
> -			DPU_FRAME_DONE_TIMEOUT * 1000 /
> -			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
> -	mod_timer(&dpu_enc->frame_done_timer, jiffies +
> -		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
> +	timeout_ms = DPU_FRAME_DONE_TIMEOUT * 1000 /
> +		drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode);
> +
> +	atomic_set(&dpu_enc->frame_done_timeout, timeout_ms);
> +	mod_timer(&dpu_enc->frame_done_timer,
> +		  jiffies + msecs_to_jiffies(timeout_ms));
> 
>  	/* All phys encs are ready to go, trigger the kickoff */
>  	_dpu_encoder_kickoff_phys(dpu_enc, async);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh

[Daniel Vetter]

On Mon, Jan 28, 2019 at 03:42:48PM -0500, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Use the drm_mode_vrefresh helper where we need refresh rate in case
> vrefresh is empty.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

I think adding a todo to remove these fields and switch everone over to
the helpers would be useful. Recomputing is not going to hurt us I think
in modeset code (the one case where we do care is vblank constants, and
those are somewhere else), and would avoid all these bugs ...
-Daniel
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 6 +++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c            | 2 +-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c     | 4 ++--
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 941ac25d2a023..dd71cb6ba4f5c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector,
>  
>  	list_for_each_entry(cur_mode, &connector->modes, head) {
>  		if (cur_mode->vdisplay == adj_mode->vdisplay &&
> -			cur_mode->hdisplay == adj_mode->hdisplay &&
> -			cur_mode->vrefresh == adj_mode->vrefresh) {
> +		    cur_mode->hdisplay == adj_mode->hdisplay &&
> +		    drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) {
>  			adj_mode->private = cur_mode->private;
>  			adj_mode->private_flags |= cur_mode->private_flags;
>  		}
> @@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>  
>  	atomic_set(&dpu_enc->frame_done_timeout,
>  			DPU_FRAME_DONE_TIMEOUT * 1000 /
> -			drm_enc->crtc->state->adjusted_mode.vrefresh);
> +			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
>  	mod_timer(&dpu_enc->frame_done_timer, jiffies +
>  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 99ab5ca9bed3b..f21163313d635 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  		return;
>  	}
>  
> -	tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh);
> +	tc_cfg.vsync_count = vsync_hz /
> +				(mode->vtotal * drm_mode_vrefresh(mode));
>  
>  	/* enable external TE after kickoff to avoid premature autorefresh */
>  	tc_cfg.hw_vsync_mode = 0;
> @@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  	DPU_DEBUG_CMDENC(cmd_enc,
>  		"tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
>  		phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz,
> -		mode->vtotal, mode->vrefresh);
> +		mode->vtotal, drm_mode_vrefresh(mode));
>  	DPU_DEBUG_CMDENC(cmd_enc,
>  		"tc %d enable %u start_pos %u rd_ptr_irq %u\n",
>  		phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index b01183b309b9e..da1f727d74957 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
>  	ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect);
>  	ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect);
>  	ot_params.is_wfd = !pdpu->is_rt_pipe;
> -	ot_params.frame_rate = crtc->mode.vrefresh;
> +	ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode);
>  	ot_params.vbif_idx = VBIF_RT;
>  	ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl;
>  	ot_params.rd = true;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> index c1962f29ec7d6..6341ac010f7de 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> @@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct drm_encoder *encoder,
>  		return -EINVAL;
>  	}
>  
> -	total_lines_x100 = mode->vtotal * mode->vrefresh;
> +	total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode);
>  	if (!total_lines_x100) {
>  		DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n",
> -				__func__, mode->vtotal, mode->vrefresh);
> +			      __func__, mode->vtotal, drm_mode_vrefresh(mode));
>  		return -EINVAL;
>  	}
>  
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh

[Sean Paul]

On Tue, Jan 29, 2019 at 09:59:40AM +0100, Daniel Vetter wrote:
> On Mon, Jan 28, 2019 at 03:42:48PM -0500, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > Use the drm_mode_vrefresh helper where we need refresh rate in case
> > vrefresh is empty.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> I think adding a todo to remove these fields and switch everone over to
> the helpers would be useful. Recomputing is not going to hurt us I think
> in modeset code (the one case where we do care is vblank constants, and
> those are somewhere else), and would avoid all these bugs ...

Yeah, and to boot if vrefresh is populated, the function just returns it, so
it's quite safe (unless someone is relying on vrefresh == 0 as valid data, which
would be nice to weed out anyways).

I'll spin up a TODO patch, good suggestion.

Sean

> -Daniel
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 6 +++---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++--
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c            | 2 +-
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c     | 4 ++--
> >  4 files changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 941ac25d2a023..dd71cb6ba4f5c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct drm_connector *connector,
> >  
> >  	list_for_each_entry(cur_mode, &connector->modes, head) {
> >  		if (cur_mode->vdisplay == adj_mode->vdisplay &&
> > -			cur_mode->hdisplay == adj_mode->hdisplay &&
> > -			cur_mode->vrefresh == adj_mode->vrefresh) {
> > +		    cur_mode->hdisplay == adj_mode->hdisplay &&
> > +		    drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) {
> >  			adj_mode->private = cur_mode->private;
> >  			adj_mode->private_flags |= cur_mode->private_flags;
> >  		}
> > @@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
> >  
> >  	atomic_set(&dpu_enc->frame_done_timeout,
> >  			DPU_FRAME_DONE_TIMEOUT * 1000 /
> > -			drm_enc->crtc->state->adjusted_mode.vrefresh);
> > +			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
> >  	mod_timer(&dpu_enc->frame_done_timer, jiffies +
> >  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
> >  
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > index 99ab5ca9bed3b..f21163313d635 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> > @@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> >  		return;
> >  	}
> >  
> > -	tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh);
> > +	tc_cfg.vsync_count = vsync_hz /
> > +				(mode->vtotal * drm_mode_vrefresh(mode));
> >  
> >  	/* enable external TE after kickoff to avoid premature autorefresh */
> >  	tc_cfg.hw_vsync_mode = 0;
> > @@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> >  	DPU_DEBUG_CMDENC(cmd_enc,
> >  		"tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
> >  		phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz,
> > -		mode->vtotal, mode->vrefresh);
> > +		mode->vtotal, drm_mode_vrefresh(mode));
> >  	DPU_DEBUG_CMDENC(cmd_enc,
> >  		"tc %d enable %u start_pos %u rd_ptr_irq %u\n",
> >  		phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos,
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > index b01183b309b9e..da1f727d74957 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> > @@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct drm_plane *plane,
> >  	ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect);
> >  	ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect);
> >  	ot_params.is_wfd = !pdpu->is_rt_pipe;
> > -	ot_params.frame_rate = crtc->mode.vrefresh;
> > +	ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode);
> >  	ot_params.vbif_idx = VBIF_RT;
> >  	ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl;
> >  	ot_params.rd = true;
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> > index c1962f29ec7d6..6341ac010f7de 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> > @@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct drm_encoder *encoder,
> >  		return -EINVAL;
> >  	}
> >  
> > -	total_lines_x100 = mode->vtotal * mode->vrefresh;
> > +	total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode);
> >  	if (!total_lines_x100) {
> >  		DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n",
> > -				__func__, mode->vtotal, mode->vrefresh);
> > +			      __func__, mode->vtotal, drm_mode_vrefresh(mode));
> >  		return -EINVAL;
> >  	}
> >  
> > -- 
> > Sean Paul, Software Engineer, Google / Chromium OS
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [PATCH 4/4] drm/msm: dpu: Don't queue the frame_done watchdog for cursor

[Fritz Koenig]

On Mon, Jan 28, 2019 at 12:43 PM Sean Paul <sean@poorly.run> wrote:
>
> From: Sean Paul <seanpaul@chromium.org>
>
> In the case of an async/cursor update, we don't wait for the frame_done
> event, which means handle_frame_done is never called, and the frame_done
> watchdog isn't canceled. Currently, this results in a frame_done timeout
> every time the cursor moves without a synchronous frame following it up
> before the timeout expires. Since we don't wait for frame_done, and
> don't handle it, we shouldn't modify the watchdog.
>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 51e46b206c73e..05145cf9fb408 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1794,7 +1794,6 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>  {
>         struct dpu_encoder_virt *dpu_enc;
>         struct dpu_encoder_phys *phys;
> -       unsigned long timeout_ms;
>         ktime_t wakeup_time;
>         unsigned int i;
>
> @@ -1807,12 +1806,20 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>
>         trace_dpu_enc_kickoff(DRMID(drm_enc));
>
> -       timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 /
> -               drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode);
> +       /*
> +        * Asynchronous frames don't handle FRAME_DONE events. As such, they
> +        * shouldn't enable the frame_done watchdog since it will always time
> +        * out.
> +        */
> +       if (!async) {
> +               unsigned long timeout_ms;
> +               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_ms, timeout_ms);
> -       mod_timer(&dpu_enc->frame_done_timer,
> -                 jiffies + msecs_to_jiffies(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));
> +       }
>
>         /* All phys encs are ready to go, trigger the kickoff */
>         _dpu_encoder_kickoff_phys(dpu_enc, async);
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>

Reviewed-by: Fritz Koenig <frkoenig@google.com>

> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Fritz Koenig]

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

Re: [PATCH 1/4] drm/msm: Use drm_mode_vrefresh instead of mode->vrefresh

[Jeykumar Sankaran]

On 2019-01-28 12:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Use the drm_mode_vrefresh helper where we need refresh rate in case
> vrefresh is empty.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

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

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c          | 6 +++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 5 +++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c            | 2 +-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c     | 4 ++--
>  4 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 941ac25d2a023..dd71cb6ba4f5c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -522,8 +522,8 @@ static void _dpu_encoder_adjust_mode(struct
> drm_connector *connector,
> 
>  	list_for_each_entry(cur_mode, &connector->modes, head) {
>  		if (cur_mode->vdisplay == adj_mode->vdisplay &&
> -			cur_mode->hdisplay == adj_mode->hdisplay &&
> -			cur_mode->vrefresh == adj_mode->vrefresh) {
> +		    cur_mode->hdisplay == adj_mode->hdisplay &&
> +		    drm_mode_vrefresh(cur_mode) == drm_mode_vrefresh(adj_mode)) {
>  			adj_mode->private = cur_mode->private;
>  			adj_mode->private_flags |= cur_mode->private_flags;
>  		}
> @@ -1805,7 +1805,7 @@ void dpu_encoder_kickoff(struct drm_encoder 
> *drm_enc,
> bool async)
> 
>  	atomic_set(&dpu_enc->frame_done_timeout,
>  			DPU_FRAME_DONE_TIMEOUT * 1000 /
> -			drm_enc->crtc->state->adjusted_mode.vrefresh);
> +			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
>  	mod_timer(&dpu_enc->frame_done_timer, jiffies +
>  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> index 99ab5ca9bed3b..f21163313d635 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
> @@ -404,7 +404,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  		return;
>  	}
> 
> -	tc_cfg.vsync_count = vsync_hz / (mode->vtotal * mode->vrefresh);
> +	tc_cfg.vsync_count = vsync_hz /
> +				(mode->vtotal * drm_mode_vrefresh(mode));
> 
>  	/* enable external TE after kickoff to avoid premature autorefresh */
>  	tc_cfg.hw_vsync_mode = 0;
> @@ -424,7 +425,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
>  	DPU_DEBUG_CMDENC(cmd_enc,
>  		"tc %d vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
>  		phys_enc->hw_pp->idx - PINGPONG_0, vsync_hz,
> -		mode->vtotal, mode->vrefresh);
> +		mode->vtotal, drm_mode_vrefresh(mode));
>  	DPU_DEBUG_CMDENC(cmd_enc,
>  		"tc %d enable %u start_pos %u rd_ptr_irq %u\n",
>  		phys_enc->hw_pp->idx - PINGPONG_0, tc_enable, tc_cfg.start_pos,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> index b01183b309b9e..da1f727d74957 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
> @@ -387,7 +387,7 @@ static void _dpu_plane_set_ot_limit(struct 
> drm_plane
> *plane,
>  	ot_params.width = drm_rect_width(&pdpu->pipe_cfg.src_rect);
>  	ot_params.height = drm_rect_height(&pdpu->pipe_cfg.src_rect);
>  	ot_params.is_wfd = !pdpu->is_rt_pipe;
> -	ot_params.frame_rate = crtc->mode.vrefresh;
> +	ot_params.frame_rate = drm_mode_vrefresh(&crtc->mode);
>  	ot_params.vbif_idx = VBIF_RT;
>  	ot_params.clk_ctrl = pdpu->pipe_hw->cap->clk_ctrl;
>  	ot_params.rd = true;
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> index c1962f29ec7d6..6341ac010f7de 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_cmd_encoder.c
> @@ -59,10 +59,10 @@ static int pingpong_tearcheck_setup(struct 
> drm_encoder
> *encoder,
>  		return -EINVAL;
>  	}
> 
> -	total_lines_x100 = mode->vtotal * mode->vrefresh;
> +	total_lines_x100 = mode->vtotal * drm_mode_vrefresh(mode);
>  	if (!total_lines_x100) {
>  		DRM_DEV_ERROR(dev, "%s: vtotal(%d) or vrefresh(%d) is 0\n",
> -				__func__, mode->vtotal, mode->vrefresh);
> +			      __func__, mode->vtotal, drm_mode_vrefresh(mode));
>  		return -EINVAL;
>  	}

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

Re: [PATCH 2/4] drm/msm: dpu: Simplify frame_done watchdog timeout calculation

[Jeykumar Sankaran]

On 2019-01-28 12:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> Instead of setting the timeout and then immediately reading it back
> (along with the hand-rolled msecs_to_jiffies calculation), just
> calculate it once and set it in both places at the same time.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index dd71cb6ba4f5c..83a4c47dbed2d 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1791,6 +1791,7 @@ void dpu_encoder_kickoff(struct drm_encoder 
> *drm_enc,
> bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> +	unsigned long timeout_ms;
>  	ktime_t wakeup_time;
>  	unsigned int i;
> 
> @@ -1803,11 +1804,12 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc, bool async)
> 
>  	trace_dpu_enc_kickoff(DRMID(drm_enc));
> 
> -	atomic_set(&dpu_enc->frame_done_timeout,
> -			DPU_FRAME_DONE_TIMEOUT * 1000 /
> -			drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode));
> -	mod_timer(&dpu_enc->frame_done_timer, jiffies +
> -		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
> +	timeout_ms = DPU_FRAME_DONE_TIMEOUT * 1000 /
> +		drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode);

For future cleanup: Is drm_enc->crtc a valid usage here?

> +
> +	atomic_set(&dpu_enc->frame_done_timeout, timeout_ms);
> +	mod_timer(&dpu_enc->frame_done_timer,
> +		  jiffies + msecs_to_jiffies(timeout_ms));
> 
>  	/* All phys encs are ready to go, trigger the kickoff */
>  	_dpu_encoder_kickoff_phys(dpu_enc, async);

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>
-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Jeykumar Sankaran]

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

Re: [PATCH 4/4] drm/msm: dpu: Don't queue the frame_done watchdog for cursor

[Jeykumar Sankaran]

On 2019-01-28 12:42, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> In the case of an async/cursor update, we don't wait for the frame_done
> event, which means handle_frame_done is never called, and the 
> frame_done
> watchdog isn't canceled. Currently, this results in a frame_done 
> timeout
> every time the cursor moves without a synchronous frame following it up
> before the timeout expires. Since we don't wait for frame_done, and
> don't handle it, we shouldn't modify the watchdog.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---

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

>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 51e46b206c73e..05145cf9fb408 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1794,7 +1794,6 @@ void dpu_encoder_kickoff(struct drm_encoder 
> *drm_enc,
> bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> -	unsigned long timeout_ms;
>  	ktime_t wakeup_time;
>  	unsigned int i;
> 
> @@ -1807,12 +1806,20 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc, bool async)
> 
>  	trace_dpu_enc_kickoff(DRMID(drm_enc));
> 
> -	timeout_ms = DPU_ENCODER_FRAME_DONE_TIMEOUT_FRAMES * 1000 /
> -		drm_mode_vrefresh(&drm_enc->crtc->state->adjusted_mode);
> +	/*
> +	 * Asynchronous frames don't handle FRAME_DONE events. As such, they
> +	 * shouldn't enable the frame_done watchdog since it will always time
> +	 * out.
> +	 */
> +	if (!async) {
> +		unsigned long timeout_ms;
> +		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_ms, timeout_ms);
> -	mod_timer(&dpu_enc->frame_done_timer,
> -		  jiffies + msecs_to_jiffies(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));
> +	}
> 
>  	/* All phys encs are ready to go, trigger the kickoff */
>  	_dpu_encoder_kickoff_phys(dpu_enc, async);

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