[PATCH 00/10] drm/msm: async commit support (v2)

[PATCH 00/10] drm/msm: async commit support (v2)

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Currently the dpu backend attempts to handle async commits.  But it
is racey and could result in flushing multiple times in a frame, or
modifying hw state (such as scanout address or cursor position) after
the previous flush, but before vblank, causing underflows (which
manifest as brief black flashes).

This patchset removes the previous dpu async commit handling, and
reworks the internal kms backend API to decouple flushing.  And in
the end introduces an hrtimer to flush async updates.  The overall
approach is:

 1) Move flushing various hw state out of encoder/crtc atomic commit
    (which is anyways an improvement over the current state, where
    we either flush from crtc or encoder, depending on whether it is
    a full modeset)

 2) Switch to crtc_mask for anything that completes after atomic
    _commit_tail(), so we do not need to keep the atomic state
    around.  Ie. from drm core's perspective, an async commit
    completes immediately.

    This avoids fighting with drm core about the lifecycle of an
    atomic state object.

 3) Track a bitmask of crtcs w/ pending async flush, and setup
    an hrtimer with expiration 1ms before vblank, to trigger
    the flush.  For async commits, we push the state down to
    the double buffered hw registers immediately, and only
    defer writing the flush registers.

Current patchset only includes the dpu backend support for async
commits.. mdp4 and mdp5 should be relatively trivial (less layers
of indirection involved).  But I won't have access to any mdp4 hw
for a few more weeks, so at least that part I might punt on for
now.

v2: couple small cosmetic updates, re-work locking to avoid stalls,
    add some tracepoints

Rob Clark (10):
  drm/msm/dpu: unwind async commit handling
  drm/msm/dpu: add real wait_for_commit_done()
  drm/msm/dpu: handle_frame_done() from vblank irq
  drm/msm: add kms->wait_flush()
  drm/msm: convert kms->complete_commit() to crtc_mask
  drm/msm: add kms->flush_commit()
  drm/msm: split power control from prepare/complete_commit
  drm/msm: async commit support
  drm/msm/dpu: async commit support
  drm/msm: add atomic traces

 drivers/gpu/drm/msm/Makefile                  |   1 +
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  48 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h      |   7 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  46 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h   |   8 +-
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  |  39 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c       |  99 +++++---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c      |  48 ++--
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c      |  47 ++--
 drivers/gpu/drm/msm/msm_atomic.c              | 233 +++++++++++++++---
 drivers/gpu/drm/msm/msm_atomic_trace.h        | 110 +++++++++
 drivers/gpu/drm/msm/msm_atomic_tracepoints.c  |   3 +
 drivers/gpu/drm/msm/msm_drv.c                 |   1 +
 drivers/gpu/drm/msm/msm_drv.h                 |   4 +
 drivers/gpu/drm/msm/msm_gpu_trace.h           |   2 +-
 drivers/gpu/drm/msm/msm_kms.h                 | 108 +++++++-
 16 files changed, 603 insertions(+), 201 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/msm_atomic_trace.h
 create mode 100644 drivers/gpu/drm/msm/msm_atomic_tracepoints.c

-- 
2.21.0

[PATCH 01/10] drm/msm/dpu: unwind async commit handling

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

It attempted to avoid fps drops in the presence of cursor updates.  But
it is racing, and can result in hw updates after flush before vblank,
which leads to underruns.

Signed-off-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 41 ++++++++++-----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 39 +++++++-------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 +--
 drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
 6 files changed, 37 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index a52439e029c9..c3f7154017c4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -610,7 +610,7 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
 	return rc;
 }
 
-void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
+void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 {
 	struct drm_encoder *encoder;
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
@@ -636,35 +636,32 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
 				  crtc->state->encoder_mask)
 		dpu_encoder_prepare_for_kickoff(encoder);
 
-	if (!async) {
-		/* wait for previous frame_event_done completion */
-		DPU_ATRACE_BEGIN("wait_for_frame_done_event");
-		ret = _dpu_crtc_wait_for_frame_done(crtc);
-		DPU_ATRACE_END("wait_for_frame_done_event");
-		if (ret) {
-			DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
-					crtc->base.id,
-					atomic_read(&dpu_crtc->frame_pending));
-			goto end;
-		}
+	/* wait for previous frame_event_done completion */
+	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
+	ret = _dpu_crtc_wait_for_frame_done(crtc);
+	DPU_ATRACE_END("wait_for_frame_done_event");
+	if (ret) {
+		DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
+				crtc->base.id,
+				atomic_read(&dpu_crtc->frame_pending));
+		goto end;
+	}
 
-		if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
-			/* acquire bandwidth and other resources */
-			DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
-		} else
-			DPU_DEBUG("crtc%d commit\n", crtc->base.id);
+	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
+		/* acquire bandwidth and other resources */
+		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
+	} else
+		DPU_DEBUG("crtc%d commit\n", crtc->base.id);
 
-		dpu_crtc->play_count++;
-	}
+	dpu_crtc->play_count++;
 
 	dpu_vbif_clear_errors(dpu_kms);
 
 	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
-		dpu_encoder_kickoff(encoder, async);
+		dpu_encoder_kickoff(encoder);
 
 end:
-	if (!async)
-		reinit_completion(&dpu_crtc->frame_done_comp);
+	reinit_completion(&dpu_crtc->frame_done_comp);
 	DPU_ATRACE_END("crtc_commit");
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 5181f079a6a1..10f78459f6c2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -238,9 +238,8 @@ void dpu_crtc_vblank_callback(struct drm_crtc *crtc);
 /**
  * dpu_crtc_commit_kickoff - trigger kickoff of the commit for this crtc
  * @crtc: Pointer to drm crtc object
- * @async: true if the commit is asynchronous, false otherwise
  */
-void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
+void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
 
 /**
  * dpu_crtc_complete_commit - callback signalling completion of current commit
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 627c57594221..ac2d534bf59e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1421,8 +1421,7 @@ static void dpu_encoder_off_work(struct work_struct *work)
  * extra_flush_bits: Additional bit mask to include in flush trigger
  */
 static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
-		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
-		bool async)
+		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits)
 {
 	struct dpu_hw_ctl *ctl;
 	int pending_kickoff_cnt;
@@ -1439,10 +1438,7 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
 		return;
 	}
 
-	if (!async)
-		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
-	else
-		pending_kickoff_cnt = atomic_read(&phys->pending_kickoff_cnt);
+	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
 
 	if (extra_flush_bits && ctl->ops.update_pending_flush)
 		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
@@ -1553,8 +1549,7 @@ static void dpu_encoder_helper_hw_reset(struct dpu_encoder_phys *phys_enc)
  *	a time.
  * dpu_enc: Pointer to virtual encoder structure
  */
-static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
-				      bool async)
+static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc)
 {
 	struct dpu_hw_ctl *ctl;
 	uint32_t i, pending_flush;
@@ -1581,13 +1576,12 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
 		 * for async commits. So don't set this for async, since it'll
 		 * roll over to the next commit.
 		 */
-		if (!async && phys->split_role != ENC_ROLE_SLAVE)
+		if (phys->split_role != ENC_ROLE_SLAVE)
 			set_bit(i, dpu_enc->frame_busy_mask);
 
 		if (!phys->ops.needs_single_flush ||
 				!phys->ops.needs_single_flush(phys))
-			_dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0,
-						   async);
+			_dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0);
 		else if (ctl->ops.get_pending_flush)
 			pending_flush |= ctl->ops.get_pending_flush(ctl);
 	}
@@ -1597,7 +1591,7 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
 		_dpu_encoder_trigger_flush(
 				&dpu_enc->base,
 				dpu_enc->cur_master,
-				pending_flush, async);
+				pending_flush);
 	}
 
 	_dpu_encoder_trigger_start(dpu_enc->cur_master);
@@ -1815,11 +1809,12 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc)
 	}
 }
 
-void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
+void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
 	ktime_t wakeup_time;
+	unsigned long timeout_ms;
 	unsigned int i;
 
 	DPU_ATRACE_BEGIN("encoder_kickoff");
@@ -1827,23 +1822,15 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 
 	trace_dpu_enc_kickoff(DRMID(drm_enc));
 
-	/*
-	 * 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 /
+	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);
+	_dpu_encoder_kickoff_phys(dpu_enc);
 
 	/* allow phys encs to handle any post-kickoff business */
 	for (i = 0; i < dpu_enc->num_phys_encs; i++) {
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 997d131c2440..8465b37adf3b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -82,9 +82,8 @@ void dpu_encoder_trigger_kickoff_pending(struct drm_encoder *encoder);
  * dpu_encoder_kickoff - trigger a double buffer flip of the ctl path
  *	(i.e. ctl flush and start) immediately.
  * @encoder:	encoder pointer
- * @async:	true if this is an asynchronous commit
  */
-void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
+void dpu_encoder_kickoff(struct drm_encoder *encoder);
 
 /**
  * dpu_encoder_wait_for_event - Waits for encoder events
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index b8d264bd15df..31454cc5d8c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -298,7 +298,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 			continue;
 
 		trace_dpu_kms_enc_enable(DRMID(crtc));
-		dpu_crtc_commit_kickoff(crtc, false);
+		dpu_crtc_commit_kickoff(crtc);
 	}
 }
 
@@ -315,8 +315,7 @@ static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 
 		if (crtc->state->active) {
 			trace_dpu_kms_commit(DRMID(crtc));
-			dpu_crtc_commit_kickoff(crtc,
-						state->legacy_cursor_update);
+			dpu_crtc_commit_kickoff(crtc);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index dd16babdd8c0..e5aae1645933 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -70,8 +70,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		kms->funcs->commit(kms, state);
 	}
 
-	if (!state->legacy_cursor_update)
-		msm_atomic_wait_for_commit_done(dev, state);
+	msm_atomic_wait_for_commit_done(dev, state);
 
 	kms->funcs->complete_commit(kms, state);
 
-- 
2.21.0

[PATCH 02/10] drm/msm/dpu: add real wait_for_commit_done()

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Just waiting for next vblank isn't ideal.. we should really be looking
at the hw FLUSH register value to know if there is still an in-progress
flush without stalling unnecessarily when there is no pending flush.

Signed-off-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Sean Paul <sean@poorly.run>
---
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 737fe963a490..7c73b09894f0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -526,6 +526,26 @@ static int dpu_encoder_phys_vid_wait_for_vblank(
 	return _dpu_encoder_phys_vid_wait_for_vblank(phys_enc, true);
 }
 
+static int dpu_encoder_phys_vid_wait_for_commit_done(
+		struct dpu_encoder_phys *phys_enc)
+{
+	struct dpu_hw_ctl *hw_ctl = phys_enc->hw_ctl;
+	int ret;
+
+	if (!hw_ctl)
+		return 0;
+
+	ret = wait_event_timeout(phys_enc->pending_kickoff_wq,
+		(hw_ctl->ops.get_flush_register(hw_ctl) == 0),
+		msecs_to_jiffies(50));
+	if (ret <= 0) {
+		DPU_ERROR("vblank timeout\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static void dpu_encoder_phys_vid_prepare_for_kickoff(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -676,7 +696,7 @@ static void dpu_encoder_phys_vid_init_ops(struct dpu_encoder_phys_ops *ops)
 	ops->destroy = dpu_encoder_phys_vid_destroy;
 	ops->get_hw_resources = dpu_encoder_phys_vid_get_hw_resources;
 	ops->control_vblank_irq = dpu_encoder_phys_vid_control_vblank_irq;
-	ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_vblank;
+	ops->wait_for_commit_done = dpu_encoder_phys_vid_wait_for_commit_done;
 	ops->wait_for_vblank = dpu_encoder_phys_vid_wait_for_vblank;
 	ops->wait_for_tx_complete = dpu_encoder_phys_vid_wait_for_vblank;
 	ops->irq_control = dpu_encoder_phys_vid_irq_control;
-- 
2.21.0

[PATCH 03/10] drm/msm/dpu: handle_frame_done() from vblank irq

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Previously the callback was called from whoever called wait_for_vblank(),
but that isn't a great plan when wait_for_vblank() stops getting called,
and results in frame_done_timer expiring.

Signed-off-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Sean Paul <sean@poorly.run>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c      |  7 +-----
 .../drm/msm/disp/dpu1/dpu_encoder_phys_vid.c  | 25 ++++++-------------
 2 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index c3f7154017c4..e7354aef9805 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -311,12 +311,7 @@ static void dpu_crtc_frame_event_work(struct kthread_work *work)
 				| DPU_ENCODER_FRAME_EVENT_PANEL_DEAD)) {
 
 		if (atomic_read(&dpu_crtc->frame_pending) < 1) {
-			/* this should not happen */
-			DRM_ERROR("crtc%d ev:%u ts:%lld frame_pending:%d\n",
-					crtc->base.id,
-					fevent->event,
-					ktime_to_ns(fevent->ts),
-					atomic_read(&dpu_crtc->frame_pending));
+			/* ignore vblank when not pending */
 		} else if (atomic_dec_return(&dpu_crtc->frame_pending) == 0) {
 			/* release bandwidth and other resources */
 			trace_dpu_crtc_frame_event_done(DRMID(crtc),
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 7c73b09894f0..b9c84fb4d4a1 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -324,6 +324,10 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
 
 	/* Signal any waiting atomic commit thread */
 	wake_up_all(&phys_enc->pending_kickoff_wq);
+
+	phys_enc->parent_ops->handle_frame_done(phys_enc->parent, phys_enc,
+			DPU_ENCODER_FRAME_EVENT_DONE);
+
 	DPU_ATRACE_END("vblank_irq");
 }
 
@@ -483,8 +487,8 @@ static void dpu_encoder_phys_vid_get_hw_resources(
 	hw_res->intfs[phys_enc->intf_idx - INTF_0] = INTF_MODE_VIDEO;
 }
 
-static int _dpu_encoder_phys_vid_wait_for_vblank(
-		struct dpu_encoder_phys *phys_enc, bool notify)
+static int dpu_encoder_phys_vid_wait_for_vblank(
+		struct dpu_encoder_phys *phys_enc)
 {
 	struct dpu_encoder_wait_info wait_info;
 	int ret;
@@ -499,10 +503,6 @@ static int _dpu_encoder_phys_vid_wait_for_vblank(
 	wait_info.timeout_ms = KICKOFF_TIMEOUT_MS;
 
 	if (!dpu_encoder_phys_vid_is_master(phys_enc)) {
-		if (notify && phys_enc->parent_ops->handle_frame_done)
-			phys_enc->parent_ops->handle_frame_done(
-					phys_enc->parent, phys_enc,
-					DPU_ENCODER_FRAME_EVENT_DONE);
 		return 0;
 	}
 
@@ -512,20 +512,11 @@ static int _dpu_encoder_phys_vid_wait_for_vblank(
 
 	if (ret == -ETIMEDOUT) {
 		dpu_encoder_helper_report_irq_timeout(phys_enc, INTR_IDX_VSYNC);
-	} else if (!ret && notify && phys_enc->parent_ops->handle_frame_done)
-		phys_enc->parent_ops->handle_frame_done(
-				phys_enc->parent, phys_enc,
-				DPU_ENCODER_FRAME_EVENT_DONE);
+	}
 
 	return ret;
 }
 
-static int dpu_encoder_phys_vid_wait_for_vblank(
-		struct dpu_encoder_phys *phys_enc)
-{
-	return _dpu_encoder_phys_vid_wait_for_vblank(phys_enc, true);
-}
-
 static int dpu_encoder_phys_vid_wait_for_commit_done(
 		struct dpu_encoder_phys *phys_enc)
 {
@@ -615,7 +606,7 @@ static void dpu_encoder_phys_vid_disable(struct dpu_encoder_phys *phys_enc)
 	 * scanout buffer) don't latch properly..
 	 */
 	if (dpu_encoder_phys_vid_is_master(phys_enc)) {
-		ret = _dpu_encoder_phys_vid_wait_for_vblank(phys_enc, false);
+		ret = dpu_encoder_phys_vid_wait_for_vblank(phys_enc);
 		if (ret) {
 			atomic_set(&phys_enc->pending_kickoff_cnt, 0);
 			DRM_ERROR("wait disable failed: id:%u intf:%d ret:%d\n",
-- 
2.21.0

[PATCH 04/10] drm/msm: add kms->wait_flush()

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

First step in re-working the atomic related internal API to prepare for
async updates pending.. ->wait_flush() is intended to block until there
is no in-progress flush.

A crtc_mask is used, rather than an atomic state object, as this will
later be used for async flush after the atomic state is destroyed.

This replaces ->wait_for_crtc_commit_done()

v2: update for review comments

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 11 ++++++-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 17 ++++++----
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 17 ++++++----
 drivers/gpu/drm/msm/msm_atomic.c         | 42 ++++++++++--------------
 drivers/gpu/drm/msm/msm_kms.h            |  9 +++--
 5 files changed, 54 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 31454cc5d8c5..df421b986bc3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -388,6 +388,15 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
 	}
 }
 
+static void dpu_kms_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
+{
+	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+	struct drm_crtc *crtc;
+
+	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask)
+		dpu_kms_wait_for_commit_done(kms, crtc);
+}
+
 static int _dpu_kms_initialize_dsi(struct drm_device *dev,
 				    struct msm_drm_private *priv,
 				    struct dpu_kms *dpu_kms)
@@ -682,8 +691,8 @@ static const struct msm_kms_funcs kms_funcs = {
 	.irq             = dpu_irq,
 	.prepare_commit  = dpu_kms_prepare_commit,
 	.commit          = dpu_kms_commit,
+	.wait_flush      = dpu_kms_wait_flush,
 	.complete_commit = dpu_kms_complete_commit,
-	.wait_for_crtc_commit_done = dpu_kms_wait_for_commit_done,
 	.enable_vblank   = dpu_kms_enable_vblank,
 	.disable_vblank  = dpu_kms_disable_vblank,
 	.check_modified_format = dpu_format_check_modified_format,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 7a9ab55b4608..32dcb1d7860c 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -107,6 +107,15 @@ static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
 		drm_crtc_vblank_get(crtc);
 }
 
+static void mdp4_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
+{
+	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
+	struct drm_crtc *crtc;
+
+	for_each_crtc_mask(mdp4_kms->dev, crtc, crtc_mask)
+		mdp4_crtc_wait_for_commit_done(crtc);
+}
+
 static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
@@ -123,12 +132,6 @@ static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s
 	mdp4_disable(mdp4_kms);
 }
 
-static void mdp4_wait_for_crtc_commit_done(struct msm_kms *kms,
-						struct drm_crtc *crtc)
-{
-	mdp4_crtc_wait_for_commit_done(crtc);
-}
-
 static long mdp4_round_pixclk(struct msm_kms *kms, unsigned long rate,
 		struct drm_encoder *encoder)
 {
@@ -179,8 +182,8 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.enable_vblank   = mdp4_enable_vblank,
 		.disable_vblank  = mdp4_disable_vblank,
 		.prepare_commit  = mdp4_prepare_commit,
+		.wait_flush      = mdp4_wait_flush,
 		.complete_commit = mdp4_complete_commit,
-		.wait_for_crtc_commit_done = mdp4_wait_for_crtc_commit_done,
 		.get_format      = mdp_get_format,
 		.round_pixclk    = mdp4_round_pixclk,
 		.destroy         = mdp4_destroy,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 8bf5f3264dc9..440e000c8c3d 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -154,6 +154,15 @@ static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
 		mdp5_smp_prepare_commit(mdp5_kms->smp, &global_state->smp);
 }
 
+static void mdp5_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
+{
+	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
+	struct drm_crtc *crtc;
+
+	for_each_crtc_mask(mdp5_kms->dev, crtc, crtc_mask)
+		mdp5_crtc_wait_for_commit_done(crtc);
+}
+
 static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
@@ -170,12 +179,6 @@ static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s
 	pm_runtime_put_sync(dev);
 }
 
-static void mdp5_wait_for_crtc_commit_done(struct msm_kms *kms,
-						struct drm_crtc *crtc)
-{
-	mdp5_crtc_wait_for_commit_done(crtc);
-}
-
 static long mdp5_round_pixclk(struct msm_kms *kms, unsigned long rate,
 		struct drm_encoder *encoder)
 {
@@ -272,8 +275,8 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.enable_vblank   = mdp5_enable_vblank,
 		.disable_vblank  = mdp5_disable_vblank,
 		.prepare_commit  = mdp5_prepare_commit,
+		.wait_flush      = mdp5_wait_flush,
 		.complete_commit = mdp5_complete_commit,
-		.wait_for_crtc_commit_done = mdp5_wait_for_crtc_commit_done,
 		.get_format      = mdp_get_format,
 		.round_pixclk    = mdp5_round_pixclk,
 		.set_split_display = mdp5_set_split_display,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index e5aae1645933..4ca4b654c221 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -10,28 +10,6 @@
 #include "msm_gem.h"
 #include "msm_kms.h"
 
-static void msm_atomic_wait_for_commit_done(struct drm_device *dev,
-		struct drm_atomic_state *old_state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *new_crtc_state;
-	struct msm_drm_private *priv = old_state->dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-	int i;
-
-	for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) {
-		if (!new_crtc_state->active)
-			continue;
-
-		if (drm_crtc_vblank_get(crtc))
-			continue;
-
-		kms->funcs->wait_for_crtc_commit_done(kms, crtc);
-
-		drm_crtc_vblank_put(crtc);
-	}
-}
-
 int msm_atomic_prepare_fb(struct drm_plane *plane,
 			  struct drm_plane_state *new_state)
 {
@@ -51,11 +29,28 @@ int msm_atomic_prepare_fb(struct drm_plane *plane,
 	return msm_framebuffer_prepare(new_state->fb, kms->aspace);
 }
 
+/* Get bitmask of crtcs that will need to be flushed.  The bitmask
+ * can be used with for_each_crtc_mask() iterator, to iterate
+ * effected crtcs without needing to preserve the atomic state.
+ */
+static unsigned get_crtc_mask(struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned i, mask = 0;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+		mask |= drm_crtc_mask(crtc);
+
+	return mask;
+}
+
 void msm_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
+	unsigned crtc_mask = get_crtc_mask(state);
 
 	kms->funcs->prepare_commit(kms, state);
 
@@ -70,8 +65,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		kms->funcs->commit(kms, state);
 	}
 
-	msm_atomic_wait_for_commit_done(dev, state);
-
+	kms->funcs->wait_flush(kms, crtc_mask);
 	kms->funcs->complete_commit(kms, state);
 
 	drm_atomic_helper_commit_hw_done(state);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index c7588a42635e..a112dfb36301 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -34,9 +34,8 @@ struct msm_kms_funcs {
 	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
 	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
 	void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
-	/* functions to wait for atomic commit completed on each CRTC */
-	void (*wait_for_crtc_commit_done)(struct msm_kms *kms,
-					struct drm_crtc *crtc);
+	void (*wait_flush)(struct msm_kms *kms, unsigned crtc_mask);
+
 	/* get msm_format w/ optional format modifiers from drm_mode_fb_cmd2 */
 	const struct msm_format *(*get_format)(struct msm_kms *kms,
 					const uint32_t format,
@@ -98,4 +97,8 @@ struct msm_mdss {
 int mdp5_mdss_init(struct drm_device *dev);
 int dpu_mdss_init(struct drm_device *dev);
 
+#define for_each_crtc_mask(dev, crtc, crtc_mask) \
+	drm_for_each_crtc(crtc, dev) \
+		for_each_if (drm_crtc_mask(crtc) & (crtc_mask))
+
 #endif /* __MSM_KMS_H__ */
-- 
2.21.0

[PATCH 05/10] drm/msm: convert kms->complete_commit() to crtc_mask

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Prep work for async commits, in which case this will be called after we
no longer have the atomic state object.

This drops some wait_for_vblanks(), but those should be unnecessary, as
we call this after waiting for flush to complete.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c |  7 +------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h |  4 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 20 ++++----------------
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  8 ++------
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  4 +---
 drivers/gpu/drm/msm/msm_atomic.c         |  2 +-
 drivers/gpu/drm/msm/msm_kms.h            |  2 +-
 7 files changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e7354aef9805..31debd31ab8c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -389,13 +389,8 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
 	kthread_queue_work(&priv->event_thread[crtc_id].worker, &fevent->work);
 }
 
-void dpu_crtc_complete_commit(struct drm_crtc *crtc,
-		struct drm_crtc_state *old_state)
+void dpu_crtc_complete_commit(struct drm_crtc *crtc)
 {
-	if (!crtc || !crtc->state) {
-		DPU_ERROR("invalid crtc\n");
-		return;
-	}
 	trace_dpu_crtc_complete_commit(DRMID(crtc));
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
index 10f78459f6c2..5174e86124cc 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -244,10 +244,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
 /**
  * dpu_crtc_complete_commit - callback signalling completion of current commit
  * @crtc: Pointer to drm crtc object
- * @old_state: Pointer to drm crtc old state object
  */
-void dpu_crtc_complete_commit(struct drm_crtc *crtc,
-		struct drm_crtc_state *old_state);
+void dpu_crtc_complete_commit(struct drm_crtc *crtc);
 
 /**
  * dpu_crtc_init - create a new crtc object
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index df421b986bc3..606815e50625 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -320,27 +320,15 @@ static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 	}
 }
 
-static void dpu_kms_complete_commit(struct msm_kms *kms,
-		struct drm_atomic_state *old_state)
+static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
 {
-	struct dpu_kms *dpu_kms;
-	struct msm_drm_private *priv;
+	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
-	int i;
-
-	if (!kms || !old_state)
-		return;
-	dpu_kms = to_dpu_kms(kms);
-
-	if (!dpu_kms->dev || !dpu_kms->dev->dev_private)
-		return;
-	priv = dpu_kms->dev->dev_private;
 
 	DPU_ATRACE_BEGIN("kms_complete_commit");
 
-	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i)
-		dpu_crtc_complete_commit(crtc, old_crtc_state);
+	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask)
+		dpu_crtc_complete_commit(crtc);
 
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 32dcb1d7860c..a6a056df5878 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -116,17 +116,13 @@ static void mdp4_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 		mdp4_crtc_wait_for_commit_done(crtc);
 }
 
-static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state)
+static void mdp4_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
-	int i;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-
-	drm_atomic_helper_wait_for_vblanks(mdp4_kms->dev, state);
 
 	/* see 119ecb7fd */
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
+	for_each_crtc_mask(mdp4_kms->dev, crtc, crtc_mask)
 		drm_crtc_vblank_put(crtc);
 
 	mdp4_disable(mdp4_kms);
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 440e000c8c3d..7a19526eef50 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -163,14 +163,12 @@ static void mdp5_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 		mdp5_crtc_wait_for_commit_done(crtc);
 }
 
-static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state)
+static void mdp5_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
 	struct device *dev = &mdp5_kms->pdev->dev;
 	struct mdp5_global_state *global_state;
 
-	drm_atomic_helper_wait_for_vblanks(mdp5_kms->dev, state);
-
 	global_state = mdp5_get_existing_global_state(mdp5_kms);
 
 	if (mdp5_kms->smp)
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 4ca4b654c221..bdcc92fbacb3 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -66,7 +66,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	kms->funcs->wait_flush(kms, crtc_mask);
-	kms->funcs->complete_commit(kms, state);
+	kms->funcs->complete_commit(kms, crtc_mask);
 
 	drm_atomic_helper_commit_hw_done(state);
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index a112dfb36301..10dd171b43f8 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -33,7 +33,7 @@ struct msm_kms_funcs {
 	/* modeset, bracketing atomic_commit(): */
 	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
 	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
-	void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
+	void (*complete_commit)(struct msm_kms *kms, unsigned crtc_mask);
 	void (*wait_flush)(struct msm_kms *kms, unsigned crtc_mask);
 
 	/* get msm_format w/ optional format modifiers from drm_mode_fb_cmd2 */
-- 
2.21.0

Re: [PATCH 05/10] drm/msm: convert kms->complete_commit() to crtc_mask

[Sean Paul]

On Thu, Aug 29, 2019 at 09:45:13AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Prep work for async commits, in which case this will be called after we
> no longer have the atomic state object.
> 
> This drops some wait_for_vblanks(), but those should be unnecessary, as
> we call this after waiting for flush to complete.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c |  7 +------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h |  4 +---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 20 ++++----------------
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  8 ++------
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  4 +---
>  drivers/gpu/drm/msm/msm_atomic.c         |  2 +-
>  drivers/gpu/drm/msm/msm_kms.h            |  2 +-
>  7 files changed, 11 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index e7354aef9805..31debd31ab8c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -389,13 +389,8 @@ static void dpu_crtc_frame_event_cb(void *data, u32 event)
>  	kthread_queue_work(&priv->event_thread[crtc_id].worker, &fevent->work);
>  }
>  
> -void dpu_crtc_complete_commit(struct drm_crtc *crtc,
> -		struct drm_crtc_state *old_state)
> +void dpu_crtc_complete_commit(struct drm_crtc *crtc)
>  {
> -	if (!crtc || !crtc->state) {
> -		DPU_ERROR("invalid crtc\n");
> -		return;
> -	}
>  	trace_dpu_crtc_complete_commit(DRMID(crtc));
>  }
>  
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> index 10f78459f6c2..5174e86124cc 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -244,10 +244,8 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc);
>  /**
>   * dpu_crtc_complete_commit - callback signalling completion of current commit
>   * @crtc: Pointer to drm crtc object
> - * @old_state: Pointer to drm crtc old state object
>   */
> -void dpu_crtc_complete_commit(struct drm_crtc *crtc,
> -		struct drm_crtc_state *old_state);
> +void dpu_crtc_complete_commit(struct drm_crtc *crtc);
>  
>  /**
>   * dpu_crtc_init - create a new crtc object
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index df421b986bc3..606815e50625 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -320,27 +320,15 @@ static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state)
>  	}
>  }
>  
> -static void dpu_kms_complete_commit(struct msm_kms *kms,
> -		struct drm_atomic_state *old_state)
> +static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
>  {
> -	struct dpu_kms *dpu_kms;
> -	struct msm_drm_private *priv;
> +	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *old_crtc_state;
> -	int i;
> -
> -	if (!kms || !old_state)
> -		return;
> -	dpu_kms = to_dpu_kms(kms);
> -
> -	if (!dpu_kms->dev || !dpu_kms->dev->dev_private)
> -		return;
> -	priv = dpu_kms->dev->dev_private;
>  
>  	DPU_ATRACE_BEGIN("kms_complete_commit");
>  
> -	for_each_old_crtc_in_state(old_state, crtc, old_crtc_state, i)
> -		dpu_crtc_complete_commit(crtc, old_crtc_state);
> +	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask)
> +		dpu_crtc_complete_commit(crtc);
>  
>  	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>  
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 32dcb1d7860c..a6a056df5878 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -116,17 +116,13 @@ static void mdp4_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
>  		mdp4_crtc_wait_for_commit_done(crtc);
>  }
>  
> -static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> +static void mdp4_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
>  {
>  	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
> -	int i;
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -
> -	drm_atomic_helper_wait_for_vblanks(mdp4_kms->dev, state);
>  
>  	/* see 119ecb7fd */
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> +	for_each_crtc_mask(mdp4_kms->dev, crtc, crtc_mask)
>  		drm_crtc_vblank_put(crtc);
>  
>  	mdp4_disable(mdp4_kms);
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index 440e000c8c3d..7a19526eef50 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -163,14 +163,12 @@ static void mdp5_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
>  		mdp5_crtc_wait_for_commit_done(crtc);
>  }
>  
> -static void mdp5_complete_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> +static void mdp5_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
>  {
>  	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
>  	struct device *dev = &mdp5_kms->pdev->dev;
>  	struct mdp5_global_state *global_state;
>  
> -	drm_atomic_helper_wait_for_vblanks(mdp5_kms->dev, state);
> -
>  	global_state = mdp5_get_existing_global_state(mdp5_kms);
>  
>  	if (mdp5_kms->smp)
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 4ca4b654c221..bdcc92fbacb3 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -66,7 +66,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	}
>  
>  	kms->funcs->wait_flush(kms, crtc_mask);
> -	kms->funcs->complete_commit(kms, state);
> +	kms->funcs->complete_commit(kms, crtc_mask);
>  
>  	drm_atomic_helper_commit_hw_done(state);
>  
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index a112dfb36301..10dd171b43f8 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -33,7 +33,7 @@ struct msm_kms_funcs {
>  	/* modeset, bracketing atomic_commit(): */
>  	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
>  	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
> -	void (*complete_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
> +	void (*complete_commit)(struct msm_kms *kms, unsigned crtc_mask);
>  	void (*wait_flush)(struct msm_kms *kms, unsigned crtc_mask);
>  
>  	/* get msm_format w/ optional format modifiers from drm_mode_fb_cmd2 */
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 06/10] drm/msm: add kms->flush_commit()

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add ->flush_commit(crtc_mask).  Currently a no-op, but kms backends
should migrate writing flush registers to this hook, so we can decouple
pushing updates to hardware, and flushing the updates.

Once we add async commit support, the hw updates will be pushed down to
the hw synchronously, but flushing the updates will be deferred until as
close to vblank as possible, so that multiple updates can be combined in
a single frame.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  6 ++++
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  6 ++++
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++
 drivers/gpu/drm/msm/msm_atomic.c         |  9 ++++--
 drivers/gpu/drm/msm/msm_kms.h            | 40 ++++++++++++++++++++++--
 5 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 606815e50625..efbf8fd343de 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -278,6 +278,11 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
 	}
 }
 
+static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
+{
+	/* TODO */
+}
+
 /*
  * Override the encoder enable since we need to setup the inline rotator and do
  * some crtc magic before enabling any bridge that might be present.
@@ -678,6 +683,7 @@ static const struct msm_kms_funcs kms_funcs = {
 	.irq_uninstall   = dpu_irq_uninstall,
 	.irq             = dpu_irq,
 	.prepare_commit  = dpu_kms_prepare_commit,
+	.flush_commit    = dpu_kms_flush_commit,
 	.commit          = dpu_kms_commit,
 	.wait_flush      = dpu_kms_wait_flush,
 	.complete_commit = dpu_kms_complete_commit,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index a6a056df5878..78ce2c8a9a38 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -107,6 +107,11 @@ static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
 		drm_crtc_vblank_get(crtc);
 }
 
+static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
+{
+	/* TODO */
+}
+
 static void mdp4_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
@@ -178,6 +183,7 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.enable_vblank   = mdp4_enable_vblank,
 		.disable_vblank  = mdp4_disable_vblank,
 		.prepare_commit  = mdp4_prepare_commit,
+		.flush_commit    = mdp4_flush_commit,
 		.wait_flush      = mdp4_wait_flush,
 		.complete_commit = mdp4_complete_commit,
 		.get_format      = mdp_get_format,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 7a19526eef50..eff1b000258e 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -154,6 +154,11 @@ static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st
 		mdp5_smp_prepare_commit(mdp5_kms->smp, &global_state->smp);
 }
 
+static void mdp5_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
+{
+	/* TODO */
+}
+
 static void mdp5_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
@@ -272,6 +277,7 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.irq             = mdp5_irq,
 		.enable_vblank   = mdp5_enable_vblank,
 		.disable_vblank  = mdp5_disable_vblank,
+		.flush_commit    = mdp5_flush_commit,
 		.prepare_commit  = mdp5_prepare_commit,
 		.wait_flush      = mdp5_wait_flush,
 		.complete_commit = mdp5_complete_commit,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index bdcc92fbacb3..e3537df848fa 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -54,16 +54,21 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 
 	kms->funcs->prepare_commit(kms, state);
 
+	/*
+	 * Push atomic updates down to hardware:
+	 */
 	drm_atomic_helper_commit_modeset_disables(dev, state);
-
 	drm_atomic_helper_commit_planes(dev, state, 0);
-
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
+	/*
+	 * Flush hardware updates:
+	 */
 	if (kms->funcs->commit) {
 		DRM_DEBUG_ATOMIC("triggering commit\n");
 		kms->funcs->commit(kms, state);
 	}
+	kms->funcs->flush_commit(kms, crtc_mask);
 
 	kms->funcs->wait_flush(kms, crtc_mask);
 	kms->funcs->complete_commit(kms, crtc_mask);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 10dd171b43f8..bb70c1758c72 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -30,12 +30,47 @@ struct msm_kms_funcs {
 	irqreturn_t (*irq)(struct msm_kms *kms);
 	int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
 	void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
-	/* modeset, bracketing atomic_commit(): */
+
+	/*
+	 * Atomic commit handling:
+	 */
+
+	/**
+	 * Prepare for atomic commit.  This is called after any previous
+	 * (async or otherwise) commit has completed.
+	 */
 	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
+
+	/**
+	 * Flush an atomic commit.  This is called after the hardware
+	 * updates have already been pushed down to effected planes/
+	 * crtcs/encoders/connectors.
+	 */
+	void (*flush_commit)(struct msm_kms *kms, unsigned crtc_mask);
+
+	/* TODO remove ->commit(), use ->flush_commit() instead: */
 	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
-	void (*complete_commit)(struct msm_kms *kms, unsigned crtc_mask);
+
+	/**
+	 * Wait for any in-progress flush to complete on the specified
+	 * crtcs.  This should not block if there is no in-progress
+	 * commit (ie. don't just wait for a vblank), as it will also
+	 * be called before ->prepare_commit() to ensure any potential
+	 * "async" commit has completed.
+	 */
 	void (*wait_flush)(struct msm_kms *kms, unsigned crtc_mask);
 
+	/**
+	 * Clean up are commit is completed.  This is called after
+	 * ->wait_flush(), to give the backend a chance to do any
+	 * post-commit cleanup.
+	 */
+	void (*complete_commit)(struct msm_kms *kms, unsigned crtc_mask);
+
+	/*
+	 * Format handling:
+	 */
+
 	/* get msm_format w/ optional format modifiers from drm_mode_fb_cmd2 */
 	const struct msm_format *(*get_format)(struct msm_kms *kms,
 					const uint32_t format,
@@ -45,6 +80,7 @@ struct msm_kms_funcs {
 			const struct msm_format *msm_fmt,
 			const struct drm_mode_fb_cmd2 *cmd,
 			struct drm_gem_object **bos);
+
 	/* misc: */
 	long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
 			struct drm_encoder *encoder);
-- 
2.21.0

Re: [PATCH 06/10] drm/msm: add kms->flush_commit()

[Sean Paul]

On Thu, Aug 29, 2019 at 09:45:14AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Add ->flush_commit(crtc_mask).  Currently a no-op, but kms backends
> should migrate writing flush registers to this hook, so we can decouple
> pushing updates to hardware, and flushing the updates.
> 
> Once we add async commit support, the hw updates will be pushed down to
> the hw synchronously, but flushing the updates will be deferred until as
> close to vblank as possible, so that multiple updates can be combined in
> a single frame.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  6 ++++
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  6 ++++
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c |  6 ++++
>  drivers/gpu/drm/msm/msm_atomic.c         |  9 ++++--
>  drivers/gpu/drm/msm/msm_kms.h            | 40 ++++++++++++++++++++++--
>  5 files changed, 63 insertions(+), 4 deletions(-)
> 

/snip

> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index 10dd171b43f8..bb70c1758c72 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -30,12 +30,47 @@ struct msm_kms_funcs {
>  	irqreturn_t (*irq)(struct msm_kms *kms);
>  	int (*enable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
>  	void (*disable_vblank)(struct msm_kms *kms, struct drm_crtc *crtc);
> -	/* modeset, bracketing atomic_commit(): */
> +
> +	/*
> +	 * Atomic commit handling:
> +	 */
> +
> +	/**
> +	 * Prepare for atomic commit.  This is called after any previous
> +	 * (async or otherwise) commit has completed.
> +	 */
>  	void (*prepare_commit)(struct msm_kms *kms, struct drm_atomic_state *state);
> +
> +	/**
> +	 * Flush an atomic commit.  This is called after the hardware
> +	 * updates have already been pushed down to effected planes/
> +	 * crtcs/encoders/connectors.
> +	 */
> +	void (*flush_commit)(struct msm_kms *kms, unsigned crtc_mask);
> +
> +	/* TODO remove ->commit(), use ->flush_commit() instead: */
>  	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
> -	void (*complete_commit)(struct msm_kms *kms, unsigned crtc_mask);
> +
> +	/**
> +	 * Wait for any in-progress flush to complete on the specified
> +	 * crtcs.  This should not block if there is no in-progress
> +	 * commit (ie. don't just wait for a vblank), as it will also
> +	 * be called before ->prepare_commit() to ensure any potential
> +	 * "async" commit has completed.
> +	 */
>  	void (*wait_flush)(struct msm_kms *kms, unsigned crtc_mask);
>  
> +	/**
> +	 * Clean up are commit is completed.  This is called after

s/are/our/?

With that fixed,

Reviewed-by: Sean Paul <sean@poorly.run>



> +	 * ->wait_flush(), to give the backend a chance to do any
> +	 * post-commit cleanup.
> +	 */
> +	void (*complete_commit)(struct msm_kms *kms, unsigned crtc_mask);
> +
> +	/*
> +	 * Format handling:
> +	 */
> +
>  	/* get msm_format w/ optional format modifiers from drm_mode_fb_cmd2 */
>  	const struct msm_format *(*get_format)(struct msm_kms *kms,
>  					const uint32_t format,
> @@ -45,6 +80,7 @@ struct msm_kms_funcs {
>  			const struct msm_format *msm_fmt,
>  			const struct drm_mode_fb_cmd2 *cmd,
>  			struct drm_gem_object **bos);
> +
>  	/* misc: */
>  	long (*round_pixclk)(struct msm_kms *kms, unsigned long rate,
>  			struct drm_encoder *encoder);
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 07/10] drm/msm: split power control from prepare/complete_commit

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

With atomic commit, ->prepare_commit() and ->complete_commit() may not
be evenly balanced (although ->complete_commit() will complete each
crtc that had been previously prepared).  So these will no longer be
a good place to enable/disable clocks needed for hw access.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 17 ++++++++++++++---
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 19 ++++++++++++++-----
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 20 ++++++++++++++------
 drivers/gpu/drm/msm/msm_atomic.c         |  2 ++
 drivers/gpu/drm/msm/msm_kms.h            | 10 ++++++++++
 5 files changed, 54 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index efbf8fd343de..d54741f3ad9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -248,6 +248,18 @@ static void dpu_kms_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
 	dpu_crtc_vblank(crtc, false);
 }
 
+static void dpu_kms_enable_commit(struct msm_kms *kms)
+{
+	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+	pm_runtime_get_sync(&dpu_kms->pdev->dev);
+}
+
+static void dpu_kms_disable_commit(struct msm_kms *kms)
+{
+	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+	pm_runtime_put_sync(&dpu_kms->pdev->dev);
+}
+
 static void dpu_kms_prepare_commit(struct msm_kms *kms,
 		struct drm_atomic_state *state)
 {
@@ -267,7 +279,6 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
 	if (!dev || !dev->dev_private)
 		return;
 	priv = dev->dev_private;
-	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
 	/* Call prepare_commit for all affected encoders */
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
@@ -335,8 +346,6 @@ static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
 	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask)
 		dpu_crtc_complete_commit(crtc);
 
-	pm_runtime_put_sync(&dpu_kms->pdev->dev);
-
 	DPU_ATRACE_END("kms_complete_commit");
 }
 
@@ -682,6 +691,8 @@ static const struct msm_kms_funcs kms_funcs = {
 	.irq_preinstall  = dpu_irq_preinstall,
 	.irq_uninstall   = dpu_irq_uninstall,
 	.irq             = dpu_irq,
+	.enable_commit   = dpu_kms_enable_commit,
+	.disable_commit  = dpu_kms_disable_commit,
 	.prepare_commit  = dpu_kms_prepare_commit,
 	.flush_commit    = dpu_kms_flush_commit,
 	.commit          = dpu_kms_commit,
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index 78ce2c8a9a38..500e5b08c11f 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -93,15 +93,24 @@ static int mdp4_hw_init(struct msm_kms *kms)
 	return ret;
 }
 
-static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
+static void mdp4_enable_commit(struct msm_kms *kms)
+{
+	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
+	mdp4_enable(mdp4_kms);
+}
+
+static void mdp4_disable_commit(struct msm_kms *kms)
 {
 	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
+	mdp4_disable(mdp4_kms);
+}
+
+static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
+{
 	int i;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 
-	mdp4_enable(mdp4_kms);
-
 	/* see 119ecb7fd */
 	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
 		drm_crtc_vblank_get(crtc);
@@ -129,8 +138,6 @@ static void mdp4_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
 	/* see 119ecb7fd */
 	for_each_crtc_mask(mdp4_kms->dev, crtc, crtc_mask)
 		drm_crtc_vblank_put(crtc);
-
-	mdp4_disable(mdp4_kms);
 }
 
 static long mdp4_round_pixclk(struct msm_kms *kms, unsigned long rate,
@@ -182,6 +189,8 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.irq             = mdp4_irq,
 		.enable_vblank   = mdp4_enable_vblank,
 		.disable_vblank  = mdp4_disable_vblank,
+		.enable_commit   = mdp4_enable_commit,
+		.disable_commit  = mdp4_disable_commit,
 		.prepare_commit  = mdp4_prepare_commit,
 		.flush_commit    = mdp4_flush_commit,
 		.wait_flush      = mdp4_wait_flush,
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index eff1b000258e..ba67bde1dbef 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -140,16 +140,25 @@ static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms)
 	return 0;
 }
 
+static void mdp5_enable_commit(struct msm_kms *kms)
+{
+	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
+	pm_runtime_get_sync(&mdp5_kms->pdev->dev);
+}
+
+static void mdp5_disable_commit(struct msm_kms *kms)
+{
+	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
+	pm_runtime_put_sync(&mdp5_kms->pdev->dev);
+}
+
 static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
-	struct device *dev = &mdp5_kms->pdev->dev;
 	struct mdp5_global_state *global_state;
 
 	global_state = mdp5_get_existing_global_state(mdp5_kms);
 
-	pm_runtime_get_sync(dev);
-
 	if (mdp5_kms->smp)
 		mdp5_smp_prepare_commit(mdp5_kms->smp, &global_state->smp);
 }
@@ -171,15 +180,12 @@ static void mdp5_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
 static void mdp5_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
 {
 	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
-	struct device *dev = &mdp5_kms->pdev->dev;
 	struct mdp5_global_state *global_state;
 
 	global_state = mdp5_get_existing_global_state(mdp5_kms);
 
 	if (mdp5_kms->smp)
 		mdp5_smp_complete_commit(mdp5_kms->smp, &global_state->smp);
-
-	pm_runtime_put_sync(dev);
 }
 
 static long mdp5_round_pixclk(struct msm_kms *kms, unsigned long rate,
@@ -278,6 +284,8 @@ static const struct mdp_kms_funcs kms_funcs = {
 		.enable_vblank   = mdp5_enable_vblank,
 		.disable_vblank  = mdp5_disable_vblank,
 		.flush_commit    = mdp5_flush_commit,
+		.enable_commit   = mdp5_enable_commit,
+		.disable_commit  = mdp5_disable_commit,
 		.prepare_commit  = mdp5_prepare_commit,
 		.wait_flush      = mdp5_wait_flush,
 		.complete_commit = mdp5_complete_commit,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index e3537df848fa..614fb9c5bb58 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -52,6 +52,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	struct msm_kms *kms = priv->kms;
 	unsigned crtc_mask = get_crtc_mask(state);
 
+	kms->funcs->enable_commit(kms);
 	kms->funcs->prepare_commit(kms, state);
 
 	/*
@@ -72,6 +73,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 
 	kms->funcs->wait_flush(kms, crtc_mask);
 	kms->funcs->complete_commit(kms, crtc_mask);
+	kms->funcs->disable_commit(kms);
 
 	drm_atomic_helper_commit_hw_done(state);
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index bb70c1758c72..811f5e2c2405 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -35,6 +35,16 @@ struct msm_kms_funcs {
 	 * Atomic commit handling:
 	 */
 
+	/**
+	 * Enable/disable power/clks needed for hw access done in other
+	 * commit related methods.
+	 *
+	 * If mdp4 is migrated to runpm, we could probably drop these
+	 * and use runpm directly.
+	 */
+	void (*enable_commit)(struct msm_kms *kms);
+	void (*disable_commit)(struct msm_kms *kms);
+
 	/**
 	 * Prepare for atomic commit.  This is called after any previous
 	 * (async or otherwise) commit has completed.
-- 
2.21.0

Re: [PATCH 07/10] drm/msm: split power control from prepare/complete_commit

[Sean Paul]

On Thu, Aug 29, 2019 at 09:45:15AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> With atomic commit, ->prepare_commit() and ->complete_commit() may not
> be evenly balanced (although ->complete_commit() will complete each
> crtc that had been previously prepared).  So these will no longer be
> a good place to enable/disable clocks needed for hw access.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 17 ++++++++++++++---
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 19 ++++++++++++++-----
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 20 ++++++++++++++------
>  drivers/gpu/drm/msm/msm_atomic.c         |  2 ++
>  drivers/gpu/drm/msm/msm_kms.h            | 10 ++++++++++
>  5 files changed, 54 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index efbf8fd343de..d54741f3ad9f 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -248,6 +248,18 @@ static void dpu_kms_disable_vblank(struct msm_kms *kms, struct drm_crtc *crtc)
>  	dpu_crtc_vblank(crtc, false);
>  }
>  
> +static void dpu_kms_enable_commit(struct msm_kms *kms)
> +{
> +	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> +	pm_runtime_get_sync(&dpu_kms->pdev->dev);
> +}
> +
> +static void dpu_kms_disable_commit(struct msm_kms *kms)
> +{
> +	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> +	pm_runtime_put_sync(&dpu_kms->pdev->dev);
> +}
> +
>  static void dpu_kms_prepare_commit(struct msm_kms *kms,
>  		struct drm_atomic_state *state)
>  {
> @@ -267,7 +279,6 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
>  	if (!dev || !dev->dev_private)
>  		return;
>  	priv = dev->dev_private;
> -	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>  
>  	/* Call prepare_commit for all affected encoders */
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> @@ -335,8 +346,6 @@ static void dpu_kms_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
>  	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask)
>  		dpu_crtc_complete_commit(crtc);
>  
> -	pm_runtime_put_sync(&dpu_kms->pdev->dev);
> -
>  	DPU_ATRACE_END("kms_complete_commit");
>  }
>  
> @@ -682,6 +691,8 @@ static const struct msm_kms_funcs kms_funcs = {
>  	.irq_preinstall  = dpu_irq_preinstall,
>  	.irq_uninstall   = dpu_irq_uninstall,
>  	.irq             = dpu_irq,
> +	.enable_commit   = dpu_kms_enable_commit,
> +	.disable_commit  = dpu_kms_disable_commit,
>  	.prepare_commit  = dpu_kms_prepare_commit,
>  	.flush_commit    = dpu_kms_flush_commit,
>  	.commit          = dpu_kms_commit,
> diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> index 78ce2c8a9a38..500e5b08c11f 100644
> --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
> @@ -93,15 +93,24 @@ static int mdp4_hw_init(struct msm_kms *kms)
>  	return ret;
>  }
>  
> -static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> +static void mdp4_enable_commit(struct msm_kms *kms)
> +{
> +	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
> +	mdp4_enable(mdp4_kms);
> +}
> +
> +static void mdp4_disable_commit(struct msm_kms *kms)
>  {
>  	struct mdp4_kms *mdp4_kms = to_mdp4_kms(to_mdp_kms(kms));
> +	mdp4_disable(mdp4_kms);
> +}
> +
> +static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> +{
>  	int i;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  
> -	mdp4_enable(mdp4_kms);
> -
>  	/* see 119ecb7fd */
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i)
>  		drm_crtc_vblank_get(crtc);
> @@ -129,8 +138,6 @@ static void mdp4_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
>  	/* see 119ecb7fd */
>  	for_each_crtc_mask(mdp4_kms->dev, crtc, crtc_mask)
>  		drm_crtc_vblank_put(crtc);
> -
> -	mdp4_disable(mdp4_kms);
>  }
>  
>  static long mdp4_round_pixclk(struct msm_kms *kms, unsigned long rate,
> @@ -182,6 +189,8 @@ static const struct mdp_kms_funcs kms_funcs = {
>  		.irq             = mdp4_irq,
>  		.enable_vblank   = mdp4_enable_vblank,
>  		.disable_vblank  = mdp4_disable_vblank,
> +		.enable_commit   = mdp4_enable_commit,
> +		.disable_commit  = mdp4_disable_commit,
>  		.prepare_commit  = mdp4_prepare_commit,
>  		.flush_commit    = mdp4_flush_commit,
>  		.wait_flush      = mdp4_wait_flush,
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> index eff1b000258e..ba67bde1dbef 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> @@ -140,16 +140,25 @@ static int mdp5_global_obj_init(struct mdp5_kms *mdp5_kms)
>  	return 0;
>  }
>  
> +static void mdp5_enable_commit(struct msm_kms *kms)
> +{
> +	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> +	pm_runtime_get_sync(&mdp5_kms->pdev->dev);
> +}
> +
> +static void mdp5_disable_commit(struct msm_kms *kms)
> +{
> +	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> +	pm_runtime_put_sync(&mdp5_kms->pdev->dev);
> +}
> +
>  static void mdp5_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state)
>  {
>  	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> -	struct device *dev = &mdp5_kms->pdev->dev;
>  	struct mdp5_global_state *global_state;
>  
>  	global_state = mdp5_get_existing_global_state(mdp5_kms);
>  
> -	pm_runtime_get_sync(dev);
> -
>  	if (mdp5_kms->smp)
>  		mdp5_smp_prepare_commit(mdp5_kms->smp, &global_state->smp);
>  }
> @@ -171,15 +180,12 @@ static void mdp5_wait_flush(struct msm_kms *kms, unsigned crtc_mask)
>  static void mdp5_complete_commit(struct msm_kms *kms, unsigned crtc_mask)
>  {
>  	struct mdp5_kms *mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
> -	struct device *dev = &mdp5_kms->pdev->dev;
>  	struct mdp5_global_state *global_state;
>  
>  	global_state = mdp5_get_existing_global_state(mdp5_kms);
>  
>  	if (mdp5_kms->smp)
>  		mdp5_smp_complete_commit(mdp5_kms->smp, &global_state->smp);
> -
> -	pm_runtime_put_sync(dev);
>  }
>  
>  static long mdp5_round_pixclk(struct msm_kms *kms, unsigned long rate,
> @@ -278,6 +284,8 @@ static const struct mdp_kms_funcs kms_funcs = {
>  		.enable_vblank   = mdp5_enable_vblank,
>  		.disable_vblank  = mdp5_disable_vblank,
>  		.flush_commit    = mdp5_flush_commit,
> +		.enable_commit   = mdp5_enable_commit,
> +		.disable_commit  = mdp5_disable_commit,
>  		.prepare_commit  = mdp5_prepare_commit,
>  		.wait_flush      = mdp5_wait_flush,
>  		.complete_commit = mdp5_complete_commit,
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index e3537df848fa..614fb9c5bb58 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -52,6 +52,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct msm_kms *kms = priv->kms;
>  	unsigned crtc_mask = get_crtc_mask(state);
>  
> +	kms->funcs->enable_commit(kms);
>  	kms->funcs->prepare_commit(kms, state);
>  
>  	/*
> @@ -72,6 +73,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	kms->funcs->wait_flush(kms, crtc_mask);
>  	kms->funcs->complete_commit(kms, crtc_mask);
> +	kms->funcs->disable_commit(kms);
>  
>  	drm_atomic_helper_commit_hw_done(state);
>  
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index bb70c1758c72..811f5e2c2405 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -35,6 +35,16 @@ struct msm_kms_funcs {
>  	 * Atomic commit handling:
>  	 */
>  
> +	/**
> +	 * Enable/disable power/clks needed for hw access done in other
> +	 * commit related methods.
> +	 *
> +	 * If mdp4 is migrated to runpm, we could probably drop these
> +	 * and use runpm directly.
> +	 */
> +	void (*enable_commit)(struct msm_kms *kms);
> +	void (*disable_commit)(struct msm_kms *kms);
> +
>  	/**
>  	 * Prepare for atomic commit.  This is called after any previous
>  	 * (async or otherwise) commit has completed.
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 08/10] drm/msm: async commit support

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Now that flush/wait/complete is decoupled from the "synchronous" part of
atomic commit_tail(), add support to defer flush to a timer that expires
shortly before vblank for async commits.  In this way, multiple atomic
commits (for example, cursor updates) can be coalesced into a single
flush at the end of the frame.

v2: don't hold lock over ->wait_flush(), to avoid locking interaction
    that was causing fps drop when combining page flips or non-async
    atomic commits and lots of legacy cursor updates

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_atomic.c | 156 ++++++++++++++++++++++++++++++-
 drivers/gpu/drm/msm/msm_drv.c    |   1 +
 drivers/gpu/drm/msm/msm_drv.h    |   4 +
 drivers/gpu/drm/msm/msm_kms.h    |  50 ++++++++++
 4 files changed, 210 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 614fb9c5bb58..8f8f74337cb4 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -29,6 +29,95 @@ int msm_atomic_prepare_fb(struct drm_plane *plane,
 	return msm_framebuffer_prepare(new_state->fb, kms->aspace);
 }
 
+static void msm_atomic_async_commit(struct msm_kms *kms, int crtc_idx)
+{
+	unsigned crtc_mask = BIT(crtc_idx);
+
+	mutex_lock(&kms->commit_lock);
+
+	if (!(kms->pending_crtc_mask & crtc_mask)) {
+		mutex_unlock(&kms->commit_lock);
+		return;
+	}
+
+	kms->pending_crtc_mask &= ~crtc_mask;
+
+	kms->funcs->enable_commit(kms);
+
+	/*
+	 * Flush hardware updates:
+	 */
+	DRM_DEBUG_ATOMIC("triggering async commit\n");
+	kms->funcs->flush_commit(kms, crtc_mask);
+	mutex_unlock(&kms->commit_lock);
+
+	/*
+	 * Wait for flush to complete:
+	 */
+	kms->funcs->wait_flush(kms, crtc_mask);
+
+	mutex_lock(&kms->commit_lock);
+	kms->funcs->complete_commit(kms, crtc_mask);
+	mutex_unlock(&kms->commit_lock);
+	kms->funcs->disable_commit(kms);
+}
+
+static enum hrtimer_restart msm_atomic_pending_timer(struct hrtimer *t)
+{
+	struct msm_pending_timer *timer = container_of(t,
+			struct msm_pending_timer, timer);
+	struct msm_drm_private *priv = timer->kms->dev->dev_private;
+
+	queue_work(priv->wq, &timer->work);
+
+	return HRTIMER_NORESTART;
+}
+
+static void msm_atomic_pending_work(struct work_struct *work)
+{
+	struct msm_pending_timer *timer = container_of(work,
+			struct msm_pending_timer, work);
+
+	msm_atomic_async_commit(timer->kms, timer->crtc_idx);
+}
+
+void msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
+		struct msm_kms *kms, int crtc_idx)
+{
+	timer->kms = kms;
+	timer->crtc_idx = crtc_idx;
+	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
+	timer->timer.function = msm_atomic_pending_timer;
+	INIT_WORK(&timer->work, msm_atomic_pending_work);
+}
+
+static bool can_do_async(struct drm_atomic_state *state,
+		struct drm_crtc **async_crtc)
+{
+	struct drm_connector_state *connector_state;
+	struct drm_connector *connector;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	int i, num_crtcs = 0;
+
+	if (!(state->legacy_cursor_update || state->async_update))
+		return false;
+
+	/* any connector change, means slow path: */
+	for_each_new_connector_in_state(state, connector, connector_state, i)
+		return false;
+
+	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
+		if (drm_atomic_crtc_needs_modeset(crtc_state))
+			return false;
+		if (++num_crtcs > 1)
+			return false;
+		*async_crtc = crtc;
+	}
+
+	return true;
+}
+
 /* Get bitmask of crtcs that will need to be flushed.  The bitmask
  * can be used with for_each_crtc_mask() iterator, to iterate
  * effected crtcs without needing to preserve the atomic state.
@@ -50,9 +139,25 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	struct drm_device *dev = state->dev;
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
+	struct drm_crtc *async_crtc = NULL;
 	unsigned crtc_mask = get_crtc_mask(state);
+	bool async = kms->funcs->vsync_time &&
+			can_do_async(state, &async_crtc);
 
 	kms->funcs->enable_commit(kms);
+
+	/*
+	 * Ensure any previous (potentially async) commit has
+	 * completed:
+	 */
+	kms->funcs->wait_flush(kms, crtc_mask);
+
+	mutex_lock(&kms->commit_lock);
+
+	/*
+	 * Now that there is no in-progress flush is complete,
+	 * prepare the current update:
+	 */
 	kms->funcs->prepare_commit(kms, state);
 
 	/*
@@ -62,6 +167,49 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	drm_atomic_helper_commit_planes(dev, state, 0);
 	drm_atomic_helper_commit_modeset_enables(dev, state);
 
+	if (async) {
+		struct msm_pending_timer *timer =
+			&kms->pending_timers[drm_crtc_index(async_crtc)];
+
+		/* async updates are limited to single-crtc updates: */
+		WARN_ON(crtc_mask != drm_crtc_mask(async_crtc));
+
+		/*
+		 * Start timer if we don't already have an update pending
+		 * on this crtc:
+		 */
+		if (!(kms->pending_crtc_mask & crtc_mask)) {
+			ktime_t vsync_time, wakeup_time;
+
+			kms->pending_crtc_mask |= crtc_mask;
+
+			vsync_time = kms->funcs->vsync_time(kms, async_crtc);
+			wakeup_time = ktime_sub(vsync_time, ms_to_ktime(1));
+
+			hrtimer_start(&timer->timer, wakeup_time,
+					HRTIMER_MODE_ABS);
+		}
+
+		kms->funcs->disable_commit(kms);
+		mutex_unlock(&kms->commit_lock);
+
+		/*
+		 * At this point, from drm core's perspective, we
+		 * are done with the atomic update, so we can just
+		 * go ahead and signal that it is done:
+		 */
+		drm_atomic_helper_commit_hw_done(state);
+		drm_atomic_helper_cleanup_planes(dev, state);
+
+		return;
+	}
+
+	/*
+	 * If there is any async flush pending on updated crtcs, fold
+	 * them into the current flush.
+	 */
+	kms->pending_crtc_mask &= ~crtc_mask;
+
 	/*
 	 * Flush hardware updates:
 	 */
@@ -70,12 +218,18 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		kms->funcs->commit(kms, state);
 	}
 	kms->funcs->flush_commit(kms, crtc_mask);
+	mutex_unlock(&kms->commit_lock);
 
+	/*
+	 * Wait for flush to complete:
+	 */
 	kms->funcs->wait_flush(kms, crtc_mask);
+
+	mutex_lock(&kms->commit_lock);
 	kms->funcs->complete_commit(kms, crtc_mask);
+	mutex_unlock(&kms->commit_lock);
 	kms->funcs->disable_commit(kms);
 
 	drm_atomic_helper_commit_hw_done(state);
-
 	drm_atomic_helper_cleanup_planes(dev, state);
 }
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 336a6d0a4cd3..65262a993440 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -532,6 +532,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv)
 	ddev->mode_config.normalize_zpos = true;
 
 	if (kms) {
+		kms->dev = ddev;
 		ret = kms->funcs->hw_init(kms);
 		if (ret) {
 			DRM_DEV_ERROR(dev, "kms hw init failed: %d\n", ret);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 79d480a7d97d..7d164d5c18b4 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -222,8 +222,12 @@ struct msm_format {
 	uint32_t pixel_format;
 };
 
+struct msm_pending_timer;
+
 int msm_atomic_prepare_fb(struct drm_plane *plane,
 			  struct drm_plane_state *new_state);
+void msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
+		struct msm_kms *kms, int crtc_idx);
 void msm_atomic_commit_tail(struct drm_atomic_state *state);
 struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev);
 void msm_atomic_state_clear(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 811f5e2c2405..5eafc9686d29 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -33,6 +33,20 @@ struct msm_kms_funcs {
 
 	/*
 	 * Atomic commit handling:
+	 *
+	 * Note that in the case of async commits, the funcs which take
+	 * a crtc_mask (ie. ->flush_commit(), and ->complete_commit())
+	 * might not be evenly balanced with ->prepare_commit(), however
+	 * each crtc that effected by a ->perpare_commit() (potentially
+	 * multiple times) will eventually (at end of vsync period) be
+	 * flushed and completed.
+	 *
+	 * This has some implications about tracking of cleanup state,
+	 * for example SMP blocks to release after commit completes.  Ie.
+	 * cleanup state should be also duplicated in the various
+	 * duplicate_state() methods, as the current cleanup state at
+	 * ->complete_commit() time may have accumulated cleanup work
+	 * from multiple commits.
 	 */
 
 	/**
@@ -45,6 +59,14 @@ struct msm_kms_funcs {
 	void (*enable_commit)(struct msm_kms *kms);
 	void (*disable_commit)(struct msm_kms *kms);
 
+	/**
+	 * If the kms backend supports async commit, it should implement
+	 * this method to return the time of the next vsync.  This is
+	 * used to determine a time slightly before vsync, for the async
+	 * commit timer to run and complete an async commit.
+	 */
+	ktime_t (*vsync_time)(struct msm_kms *kms, struct drm_crtc *crtc);
+
 	/**
 	 * Prepare for atomic commit.  This is called after any previous
 	 * (async or otherwise) commit has completed.
@@ -109,20 +131,48 @@ struct msm_kms_funcs {
 #endif
 };
 
+struct msm_kms;
+
+/*
+ * A per-crtc timer for pending async atomic flushes.  Scheduled to expire
+ * shortly before vblank to flush pending async updates.
+ */
+struct msm_pending_timer {
+	struct hrtimer timer;
+	struct work_struct work;
+	struct msm_kms *kms;
+	unsigned crtc_idx;
+};
+
 struct msm_kms {
 	const struct msm_kms_funcs *funcs;
+	struct drm_device *dev;
 
 	/* irq number to be passed on to drm_irq_install */
 	int irq;
 
 	/* mapper-id used to request GEM buffer mapped for scanout: */
 	struct msm_gem_address_space *aspace;
+
+	/*
+	 * For async commit, where ->flush_commit() and later happens
+	 * from the crtc's pending_timer close to end of the frame:
+	 */
+	struct mutex commit_lock;
+	unsigned pending_crtc_mask;
+	struct msm_pending_timer pending_timers[MAX_CRTCS];
 };
 
 static inline void msm_kms_init(struct msm_kms *kms,
 		const struct msm_kms_funcs *funcs)
 {
+	unsigned i;
+
+	mutex_init(&kms->commit_lock);
 	kms->funcs = funcs;
+
+	for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++)
+		msm_atomic_init_pending_timer(&kms->pending_timers[i], kms, i);
 }
 
 struct msm_kms *mdp4_kms_init(struct drm_device *dev);
-- 
2.21.0

Re: [PATCH 08/10] drm/msm: async commit support

[Sean Paul]

On Thu, Aug 29, 2019 at 09:45:16AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> Now that flush/wait/complete is decoupled from the "synchronous" part of
> atomic commit_tail(), add support to defer flush to a timer that expires
> shortly before vblank for async commits.  In this way, multiple atomic
> commits (for example, cursor updates) can be coalesced into a single
> flush at the end of the frame.
> 
> v2: don't hold lock over ->wait_flush(), to avoid locking interaction
>     that was causing fps drop when combining page flips or non-async
>     atomic commits and lots of legacy cursor updates
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Some nits, with them addressed,

Reviewed-by: Sean Paul <sean@poorly.run>


> ---
>  drivers/gpu/drm/msm/msm_atomic.c | 156 ++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/msm/msm_drv.c    |   1 +
>  drivers/gpu/drm/msm/msm_drv.h    |   4 +
>  drivers/gpu/drm/msm/msm_kms.h    |  50 ++++++++++
>  4 files changed, 210 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 614fb9c5bb58..8f8f74337cb4 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -29,6 +29,95 @@ int msm_atomic_prepare_fb(struct drm_plane *plane,
>  	return msm_framebuffer_prepare(new_state->fb, kms->aspace);
>  }
>  
> +static void msm_atomic_async_commit(struct msm_kms *kms, int crtc_idx)
> +{
> +	unsigned crtc_mask = BIT(crtc_idx);
> +
> +	mutex_lock(&kms->commit_lock);
> +
> +	if (!(kms->pending_crtc_mask & crtc_mask)) {
> +		mutex_unlock(&kms->commit_lock);
> +		return;
> +	}
> +
> +	kms->pending_crtc_mask &= ~crtc_mask;
> +
> +	kms->funcs->enable_commit(kms);
> +
> +	/*
> +	 * Flush hardware updates:
> +	 */
> +	DRM_DEBUG_ATOMIC("triggering async commit\n");
> +	kms->funcs->flush_commit(kms, crtc_mask);
> +	mutex_unlock(&kms->commit_lock);
> +
> +	/*
> +	 * Wait for flush to complete:
> +	 */
> +	kms->funcs->wait_flush(kms, crtc_mask);
> +
> +	mutex_lock(&kms->commit_lock);
> +	kms->funcs->complete_commit(kms, crtc_mask);
> +	mutex_unlock(&kms->commit_lock);
> +	kms->funcs->disable_commit(kms);
> +}
> +
> +static enum hrtimer_restart msm_atomic_pending_timer(struct hrtimer *t)
> +{
> +	struct msm_pending_timer *timer = container_of(t,
> +			struct msm_pending_timer, timer);
> +	struct msm_drm_private *priv = timer->kms->dev->dev_private;
> +

As discussed on IRC, a TODO here explaining that we'd like to remove this worker eventually but there are mutexes further down that need to be removed.

> +	queue_work(priv->wq, &timer->work);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
> +static void msm_atomic_pending_work(struct work_struct *work)
> +{
> +	struct msm_pending_timer *timer = container_of(work,
> +			struct msm_pending_timer, work);
> +
> +	msm_atomic_async_commit(timer->kms, timer->crtc_idx);
> +}
> +
> +void msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
> +		struct msm_kms *kms, int crtc_idx)
> +{
> +	timer->kms = kms;
> +	timer->crtc_idx = crtc_idx;
> +	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> +	timer->timer.function = msm_atomic_pending_timer;
> +	INIT_WORK(&timer->work, msm_atomic_pending_work);
> +}
> +
> +static bool can_do_async(struct drm_atomic_state *state,
> +		struct drm_crtc **async_crtc)
> +{
> +	struct drm_connector_state *connector_state;
> +	struct drm_connector *connector;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	int i, num_crtcs = 0;
> +
> +	if (!(state->legacy_cursor_update || state->async_update))
> +		return false;
> +
> +	/* any connector change, means slow path: */
> +	for_each_new_connector_in_state(state, connector, connector_state, i)
> +		return false;
> +
> +	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (drm_atomic_crtc_needs_modeset(crtc_state))
> +			return false;
> +		if (++num_crtcs > 1)
> +			return false;
> +		*async_crtc = crtc;
> +	}
> +
> +	return true;
> +}
> +
>  /* Get bitmask of crtcs that will need to be flushed.  The bitmask
>   * can be used with for_each_crtc_mask() iterator, to iterate
>   * effected crtcs without needing to preserve the atomic state.
> @@ -50,9 +139,25 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	struct drm_device *dev = state->dev;
>  	struct msm_drm_private *priv = dev->dev_private;
>  	struct msm_kms *kms = priv->kms;
> +	struct drm_crtc *async_crtc = NULL;
>  	unsigned crtc_mask = get_crtc_mask(state);
> +	bool async = kms->funcs->vsync_time &&
> +			can_do_async(state, &async_crtc);
>  
>  	kms->funcs->enable_commit(kms);
> +
> +	/*
> +	 * Ensure any previous (potentially async) commit has
> +	 * completed:
> +	 */
> +	kms->funcs->wait_flush(kms, crtc_mask);
> +
> +	mutex_lock(&kms->commit_lock);
> +
> +	/*
> +	 * Now that there is no in-progress flush is complete,

s/is complete//

> +	 * prepare the current update:
> +	 */
>  	kms->funcs->prepare_commit(kms, state);
>  
>  	/*
> @@ -62,6 +167,49 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	drm_atomic_helper_commit_planes(dev, state, 0);
>  	drm_atomic_helper_commit_modeset_enables(dev, state);
>  
> +	if (async) {
> +		struct msm_pending_timer *timer =
> +			&kms->pending_timers[drm_crtc_index(async_crtc)];
> +
> +		/* async updates are limited to single-crtc updates: */
> +		WARN_ON(crtc_mask != drm_crtc_mask(async_crtc));

Put this in can_do_async()?

> +
> +		/*
> +		 * Start timer if we don't already have an update pending
> +		 * on this crtc:
> +		 */
> +		if (!(kms->pending_crtc_mask & crtc_mask)) {
> +			ktime_t vsync_time, wakeup_time;
> +
> +			kms->pending_crtc_mask |= crtc_mask;
> +
> +			vsync_time = kms->funcs->vsync_time(kms, async_crtc);
> +			wakeup_time = ktime_sub(vsync_time, ms_to_ktime(1));
> +
> +			hrtimer_start(&timer->timer, wakeup_time,
> +					HRTIMER_MODE_ABS);
> +		}
> +
> +		kms->funcs->disable_commit(kms);

enable and disable are pretty loaded. How about begin/end instead?

> +		mutex_unlock(&kms->commit_lock);
> +
> +		/*
> +		 * At this point, from drm core's perspective, we
> +		 * are done with the atomic update, so we can just
> +		 * go ahead and signal that it is done:
> +		 */
> +		drm_atomic_helper_commit_hw_done(state);
> +		drm_atomic_helper_cleanup_planes(dev, state);
> +
> +		return;
> +	}
> +
> +	/*
> +	 * If there is any async flush pending on updated crtcs, fold
> +	 * them into the current flush.
> +	 */
> +	kms->pending_crtc_mask &= ~crtc_mask;
> +
>  	/*
>  	 * Flush hardware updates:
>  	 */
> @@ -70,12 +218,18 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  		kms->funcs->commit(kms, state);
>  	}
>  	kms->funcs->flush_commit(kms, crtc_mask);
> +	mutex_unlock(&kms->commit_lock);
>  
> +	/*
> +	 * Wait for flush to complete:
> +	 */
>  	kms->funcs->wait_flush(kms, crtc_mask);
> +
> +	mutex_lock(&kms->commit_lock);
>  	kms->funcs->complete_commit(kms, crtc_mask);
> +	mutex_unlock(&kms->commit_lock);
>  	kms->funcs->disable_commit(kms);
>  
>  	drm_atomic_helper_commit_hw_done(state);
> -
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 336a6d0a4cd3..65262a993440 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -532,6 +532,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv)
>  	ddev->mode_config.normalize_zpos = true;
>  
>  	if (kms) {
> +		kms->dev = ddev;
>  		ret = kms->funcs->hw_init(kms);
>  		if (ret) {
>  			DRM_DEV_ERROR(dev, "kms hw init failed: %d\n", ret);
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index 79d480a7d97d..7d164d5c18b4 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -222,8 +222,12 @@ struct msm_format {
>  	uint32_t pixel_format;
>  };
>  
> +struct msm_pending_timer;
> +
>  int msm_atomic_prepare_fb(struct drm_plane *plane,
>  			  struct drm_plane_state *new_state);
> +void msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
> +		struct msm_kms *kms, int crtc_idx);
>  void msm_atomic_commit_tail(struct drm_atomic_state *state);
>  struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev);
>  void msm_atomic_state_clear(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index 811f5e2c2405..5eafc9686d29 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -33,6 +33,20 @@ struct msm_kms_funcs {
>  
>  	/*
>  	 * Atomic commit handling:
> +	 *
> +	 * Note that in the case of async commits, the funcs which take
> +	 * a crtc_mask (ie. ->flush_commit(), and ->complete_commit())
> +	 * might not be evenly balanced with ->prepare_commit(), however
> +	 * each crtc that effected by a ->perpare_commit() (potentially

s/perpare/prepare/

> +	 * multiple times) will eventually (at end of vsync period) be
> +	 * flushed and completed.
> +	 *
> +	 * This has some implications about tracking of cleanup state,
> +	 * for example SMP blocks to release after commit completes.  Ie.
> +	 * cleanup state should be also duplicated in the various
> +	 * duplicate_state() methods, as the current cleanup state at
> +	 * ->complete_commit() time may have accumulated cleanup work
> +	 * from multiple commits.
>  	 */
>  
>  	/**
> @@ -45,6 +59,14 @@ struct msm_kms_funcs {
>  	void (*enable_commit)(struct msm_kms *kms);
>  	void (*disable_commit)(struct msm_kms *kms);
>  
> +	/**
> +	 * If the kms backend supports async commit, it should implement
> +	 * this method to return the time of the next vsync.  This is
> +	 * used to determine a time slightly before vsync, for the async
> +	 * commit timer to run and complete an async commit.
> +	 */
> +	ktime_t (*vsync_time)(struct msm_kms *kms, struct drm_crtc *crtc);
> +
>  	/**
>  	 * Prepare for atomic commit.  This is called after any previous
>  	 * (async or otherwise) commit has completed.
> @@ -109,20 +131,48 @@ struct msm_kms_funcs {
>  #endif
>  };
>  
> +struct msm_kms;
> +
> +/*
> + * A per-crtc timer for pending async atomic flushes.  Scheduled to expire
> + * shortly before vblank to flush pending async updates.
> + */
> +struct msm_pending_timer {
> +	struct hrtimer timer;
> +	struct work_struct work;
> +	struct msm_kms *kms;
> +	unsigned crtc_idx;
> +};
> +
>  struct msm_kms {
>  	const struct msm_kms_funcs *funcs;
> +	struct drm_device *dev;
>  
>  	/* irq number to be passed on to drm_irq_install */
>  	int irq;
>  
>  	/* mapper-id used to request GEM buffer mapped for scanout: */
>  	struct msm_gem_address_space *aspace;
> +
> +	/*
> +	 * For async commit, where ->flush_commit() and later happens
> +	 * from the crtc's pending_timer close to end of the frame:
> +	 */
> +	struct mutex commit_lock;
> +	unsigned pending_crtc_mask;
> +	struct msm_pending_timer pending_timers[MAX_CRTCS];
>  };
>  
>  static inline void msm_kms_init(struct msm_kms *kms,
>  		const struct msm_kms_funcs *funcs)
>  {
> +	unsigned i;
> +
> +	mutex_init(&kms->commit_lock);
>  	kms->funcs = funcs;
> +
> +	for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++)
> +		msm_atomic_init_pending_timer(&kms->pending_timers[i], kms, i);
>  }
>  
>  struct msm_kms *mdp4_kms_init(struct drm_device *dev);
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 09/10] drm/msm/dpu: async commit support

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

In addition, moving to kms->flush_commit() lets us drop the only user
of kms->commit().

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 13 ------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  7 ++--
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  5 +++
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 46 +++++++++++----------
 drivers/gpu/drm/msm/msm_atomic.c            |  5 +--
 drivers/gpu/drm/msm/msm_kms.h               |  3 --
 6 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 31debd31ab8c..f38a7d27a1c0 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -606,7 +606,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
 	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
 	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
-	int ret;
 
 	/*
 	 * If no mixers has been allocated in dpu_crtc_atomic_check(),
@@ -626,17 +625,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 				  crtc->state->encoder_mask)
 		dpu_encoder_prepare_for_kickoff(encoder);
 
-	/* wait for previous frame_event_done completion */
-	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
-	ret = _dpu_crtc_wait_for_frame_done(crtc);
-	DPU_ATRACE_END("wait_for_frame_done_event");
-	if (ret) {
-		DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
-				crtc->base.id,
-				atomic_read(&dpu_crtc->frame_pending));
-		goto end;
-	}
-
 	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
 		/* acquire bandwidth and other resources */
 		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
@@ -650,7 +638,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
 		dpu_encoder_kickoff(encoder);
 
-end:
 	reinit_completion(&dpu_crtc->frame_done_comp);
 	DPU_ATRACE_END("crtc_commit");
 }
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index ac2d534bf59e..3a69b93d8fb6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1678,8 +1678,7 @@ static u32 _dpu_encoder_calculate_linetime(struct dpu_encoder_virt *dpu_enc,
 	return line_time;
 }
 
-static int _dpu_encoder_wakeup_time(struct drm_encoder *drm_enc,
-		ktime_t *wakeup_time)
+int dpu_encoder_vsync_time(struct drm_encoder *drm_enc, ktime_t *wakeup_time)
 {
 	struct drm_display_mode *mode;
 	struct dpu_encoder_virt *dpu_enc;
@@ -1766,7 +1765,7 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
 		return;
 	}
 
-	if (_dpu_encoder_wakeup_time(&dpu_enc->base, &wakeup_time))
+	if (dpu_encoder_vsync_time(&dpu_enc->base, &wakeup_time))
 		return;
 
 	trace_dpu_enc_vsync_event_work(DRMID(&dpu_enc->base), wakeup_time);
@@ -1840,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
 	}
 
 	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
-			!_dpu_encoder_wakeup_time(drm_enc, &wakeup_time)) {
+			!dpu_encoder_vsync_time(drm_enc, &wakeup_time)) {
 		trace_dpu_enc_early_kickoff(DRMID(drm_enc),
 					    ktime_to_ms(wakeup_time));
 		mod_timer(&dpu_enc->vsync_event_timer,
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 8465b37adf3b..b4913465e602 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -85,6 +85,11 @@ void dpu_encoder_trigger_kickoff_pending(struct drm_encoder *encoder);
  */
 void dpu_encoder_kickoff(struct drm_encoder *encoder);
 
+/**
+ * dpu_encoder_wakeup_time - get the time of the next vsync
+ */
+int dpu_encoder_vsync_time(struct drm_encoder *drm_enc, ktime_t *wakeup_time);
+
 /**
  * dpu_encoder_wait_for_event - Waits for encoder events
  * @encoder:	encoder pointer
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index d54741f3ad9f..af41af1731c2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -260,6 +260,20 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 
+static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
+{
+	struct drm_encoder *encoder;
+
+	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) {
+		ktime_t vsync_time;
+
+		if (dpu_encoder_vsync_time(encoder, &vsync_time) == 0)
+			return vsync_time;
+	}
+
+	return ktime_get();
+}
+
 static void dpu_kms_prepare_commit(struct msm_kms *kms,
 		struct drm_atomic_state *state)
 {
@@ -291,7 +305,16 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
 
 static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
 {
-	/* TODO */
+	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
+	struct drm_crtc *crtc;
+
+	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask) {
+		if (!crtc->state->active)
+			continue;
+
+		trace_dpu_kms_commit(DRMID(crtc));
+		dpu_crtc_commit_kickoff(crtc);
+	}
 }
 
 /*
@@ -314,25 +337,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 			continue;
 
 		trace_dpu_kms_enc_enable(DRMID(crtc));
-		dpu_crtc_commit_kickoff(crtc);
-	}
-}
-
-static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state)
-{
-	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int i;
-
-	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
-		/* If modeset is required, kickoff is run in encoder_enable */
-		if (drm_atomic_crtc_needs_modeset(crtc_state))
-			continue;
-
-		if (crtc->state->active) {
-			trace_dpu_kms_commit(DRMID(crtc));
-			dpu_crtc_commit_kickoff(crtc);
-		}
 	}
 }
 
@@ -693,9 +697,9 @@ static const struct msm_kms_funcs kms_funcs = {
 	.irq             = dpu_irq,
 	.enable_commit   = dpu_kms_enable_commit,
 	.disable_commit  = dpu_kms_disable_commit,
+	.vsync_time      = dpu_kms_vsync_time,
 	.prepare_commit  = dpu_kms_prepare_commit,
 	.flush_commit    = dpu_kms_flush_commit,
-	.commit          = dpu_kms_commit,
 	.wait_flush      = dpu_kms_wait_flush,
 	.complete_commit = dpu_kms_complete_commit,
 	.enable_vblank   = dpu_kms_enable_vblank,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 8f8f74337cb4..80536538967b 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -213,10 +213,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	/*
 	 * Flush hardware updates:
 	 */
-	if (kms->funcs->commit) {
-		DRM_DEBUG_ATOMIC("triggering commit\n");
-		kms->funcs->commit(kms, state);
-	}
+	DRM_DEBUG_ATOMIC("triggering commit\n");
 	kms->funcs->flush_commit(kms, crtc_mask);
 	mutex_unlock(&kms->commit_lock);
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 5eafc9686d29..32ff2e070ea2 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -80,9 +80,6 @@ struct msm_kms_funcs {
 	 */
 	void (*flush_commit)(struct msm_kms *kms, unsigned crtc_mask);
 
-	/* TODO remove ->commit(), use ->flush_commit() instead: */
-	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
-
 	/**
 	 * Wait for any in-progress flush to complete on the specified
 	 * crtcs.  This should not block if there is no in-progress
-- 
2.21.0

Re: [PATCH 09/10] drm/msm/dpu: async commit support

[Sean Paul]

On Thu, Aug 29, 2019 at 09:45:17AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> In addition, moving to kms->flush_commit() lets us drop the only user
> of kms->commit().
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Sean Paul <sean@poorly.run>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 13 ------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  7 ++--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  5 +++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 46 +++++++++++----------
>  drivers/gpu/drm/msm/msm_atomic.c            |  5 +--
>  drivers/gpu/drm/msm/msm_kms.h               |  3 --
>  6 files changed, 34 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 31debd31ab8c..f38a7d27a1c0 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -606,7 +606,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  	struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
>  	struct dpu_kms *dpu_kms = _dpu_crtc_get_kms(crtc);
>  	struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> -	int ret;
>  
>  	/*
>  	 * If no mixers has been allocated in dpu_crtc_atomic_check(),
> @@ -626,17 +625,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  				  crtc->state->encoder_mask)
>  		dpu_encoder_prepare_for_kickoff(encoder);
>  
> -	/* wait for previous frame_event_done completion */
> -	DPU_ATRACE_BEGIN("wait_for_frame_done_event");
> -	ret = _dpu_crtc_wait_for_frame_done(crtc);
> -	DPU_ATRACE_END("wait_for_frame_done_event");
> -	if (ret) {
> -		DPU_ERROR("crtc%d wait for frame done failed;frame_pending%d\n",
> -				crtc->base.id,
> -				atomic_read(&dpu_crtc->frame_pending));
> -		goto end;
> -	}
> -
>  	if (atomic_inc_return(&dpu_crtc->frame_pending) == 1) {
>  		/* acquire bandwidth and other resources */
>  		DPU_DEBUG("crtc%d first commit\n", crtc->base.id);
> @@ -650,7 +638,6 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>  	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>  		dpu_encoder_kickoff(encoder);
>  
> -end:
>  	reinit_completion(&dpu_crtc->frame_done_comp);
>  	DPU_ATRACE_END("crtc_commit");
>  }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index ac2d534bf59e..3a69b93d8fb6 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1678,8 +1678,7 @@ static u32 _dpu_encoder_calculate_linetime(struct dpu_encoder_virt *dpu_enc,
>  	return line_time;
>  }
>  
> -static int _dpu_encoder_wakeup_time(struct drm_encoder *drm_enc,
> -		ktime_t *wakeup_time)
> +int dpu_encoder_vsync_time(struct drm_encoder *drm_enc, ktime_t *wakeup_time)
>  {
>  	struct drm_display_mode *mode;
>  	struct dpu_encoder_virt *dpu_enc;
> @@ -1766,7 +1765,7 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
>  		return;
>  	}
>  
> -	if (_dpu_encoder_wakeup_time(&dpu_enc->base, &wakeup_time))
> +	if (dpu_encoder_vsync_time(&dpu_enc->base, &wakeup_time))
>  		return;
>  
>  	trace_dpu_enc_vsync_event_work(DRMID(&dpu_enc->base), wakeup_time);
> @@ -1840,7 +1839,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
>  	}
>  
>  	if (dpu_enc->disp_info.intf_type == DRM_MODE_ENCODER_DSI &&
> -			!_dpu_encoder_wakeup_time(drm_enc, &wakeup_time)) {
> +			!dpu_encoder_vsync_time(drm_enc, &wakeup_time)) {
>  		trace_dpu_enc_early_kickoff(DRMID(drm_enc),
>  					    ktime_to_ms(wakeup_time));
>  		mod_timer(&dpu_enc->vsync_event_timer,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 8465b37adf3b..b4913465e602 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -85,6 +85,11 @@ void dpu_encoder_trigger_kickoff_pending(struct drm_encoder *encoder);
>   */
>  void dpu_encoder_kickoff(struct drm_encoder *encoder);
>  
> +/**
> + * dpu_encoder_wakeup_time - get the time of the next vsync
> + */
> +int dpu_encoder_vsync_time(struct drm_encoder *drm_enc, ktime_t *wakeup_time);
> +
>  /**
>   * dpu_encoder_wait_for_event - Waits for encoder events
>   * @encoder:	encoder pointer
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index d54741f3ad9f..af41af1731c2 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -260,6 +260,20 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
>  	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>  }
>  
> +static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc)
> +{
> +	struct drm_encoder *encoder;
> +
> +	drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) {
> +		ktime_t vsync_time;
> +
> +		if (dpu_encoder_vsync_time(encoder, &vsync_time) == 0)
> +			return vsync_time;
> +	}
> +
> +	return ktime_get();
> +}
> +
>  static void dpu_kms_prepare_commit(struct msm_kms *kms,
>  		struct drm_atomic_state *state)
>  {
> @@ -291,7 +305,16 @@ static void dpu_kms_prepare_commit(struct msm_kms *kms,
>  
>  static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
>  {
> -	/* TODO */
> +	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
> +	struct drm_crtc *crtc;
> +
> +	for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask) {
> +		if (!crtc->state->active)
> +			continue;
> +
> +		trace_dpu_kms_commit(DRMID(crtc));
> +		dpu_crtc_commit_kickoff(crtc);
> +	}
>  }
>  
>  /*
> @@ -314,25 +337,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
>  			continue;
>  
>  		trace_dpu_kms_enc_enable(DRMID(crtc));
> -		dpu_crtc_commit_kickoff(crtc);
> -	}
> -}
> -
> -static void dpu_kms_commit(struct msm_kms *kms, struct drm_atomic_state *state)
> -{
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	int i;
> -
> -	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
> -		/* If modeset is required, kickoff is run in encoder_enable */
> -		if (drm_atomic_crtc_needs_modeset(crtc_state))
> -			continue;
> -
> -		if (crtc->state->active) {
> -			trace_dpu_kms_commit(DRMID(crtc));
> -			dpu_crtc_commit_kickoff(crtc);
> -		}
>  	}
>  }
>  
> @@ -693,9 +697,9 @@ static const struct msm_kms_funcs kms_funcs = {
>  	.irq             = dpu_irq,
>  	.enable_commit   = dpu_kms_enable_commit,
>  	.disable_commit  = dpu_kms_disable_commit,
> +	.vsync_time      = dpu_kms_vsync_time,
>  	.prepare_commit  = dpu_kms_prepare_commit,
>  	.flush_commit    = dpu_kms_flush_commit,
> -	.commit          = dpu_kms_commit,
>  	.wait_flush      = dpu_kms_wait_flush,
>  	.complete_commit = dpu_kms_complete_commit,
>  	.enable_vblank   = dpu_kms_enable_vblank,
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 8f8f74337cb4..80536538967b 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -213,10 +213,7 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	/*
>  	 * Flush hardware updates:
>  	 */
> -	if (kms->funcs->commit) {
> -		DRM_DEBUG_ATOMIC("triggering commit\n");
> -		kms->funcs->commit(kms, state);
> -	}
> +	DRM_DEBUG_ATOMIC("triggering commit\n");
>  	kms->funcs->flush_commit(kms, crtc_mask);
>  	mutex_unlock(&kms->commit_lock);
>  
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index 5eafc9686d29..32ff2e070ea2 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -80,9 +80,6 @@ struct msm_kms_funcs {
>  	 */
>  	void (*flush_commit)(struct msm_kms *kms, unsigned crtc_mask);
>  
> -	/* TODO remove ->commit(), use ->flush_commit() instead: */
> -	void (*commit)(struct msm_kms *kms, struct drm_atomic_state *state);
> -
>  	/**
>  	 * Wait for any in-progress flush to complete on the specified
>  	 * crtcs.  This should not block if there is no in-progress
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS

[PATCH 10/10] drm/msm: add atomic traces

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

This was useful for debugging fps drops.  I suspect it will be useful
again.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/Makefile                 |   1 +
 drivers/gpu/drm/msm/msm_atomic.c             |  24 +++-
 drivers/gpu/drm/msm/msm_atomic_trace.h       | 110 +++++++++++++++++++
 drivers/gpu/drm/msm/msm_atomic_tracepoints.c |   3 +
 drivers/gpu/drm/msm/msm_gpu_trace.h          |   2 +-
 5 files changed, 136 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/msm/msm_atomic_trace.h
 create mode 100644 drivers/gpu/drm/msm/msm_atomic_tracepoints.c

diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
index 7a05cbf2f820..1579cf0d828f 100644
--- a/drivers/gpu/drm/msm/Makefile
+++ b/drivers/gpu/drm/msm/Makefile
@@ -75,6 +75,7 @@ msm-y := \
 	disp/dpu1/dpu_rm.o \
 	disp/dpu1/dpu_vbif.o \
 	msm_atomic.o \
+	msm_atomic_tracepoints.o \
 	msm_debugfs.o \
 	msm_drv.o \
 	msm_fb.o \
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 80536538967b..fb247aa1081e 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -6,6 +6,7 @@
 
 #include <drm/drm_atomic_uapi.h>
 
+#include "msm_atomic_trace.h"
 #include "msm_drv.h"
 #include "msm_gem.h"
 #include "msm_kms.h"
@@ -33,11 +34,13 @@ static void msm_atomic_async_commit(struct msm_kms *kms, int crtc_idx)
 {
 	unsigned crtc_mask = BIT(crtc_idx);
 
+	trace_msm_atomic_async_commit_start(crtc_mask);
+
 	mutex_lock(&kms->commit_lock);
 
 	if (!(kms->pending_crtc_mask & crtc_mask)) {
 		mutex_unlock(&kms->commit_lock);
-		return;
+		goto out;
 	}
 
 	kms->pending_crtc_mask &= ~crtc_mask;
@@ -47,19 +50,24 @@ static void msm_atomic_async_commit(struct msm_kms *kms, int crtc_idx)
 	/*
 	 * Flush hardware updates:
 	 */
-	DRM_DEBUG_ATOMIC("triggering async commit\n");
+	trace_msm_atomic_flush_commit(crtc_mask);
 	kms->funcs->flush_commit(kms, crtc_mask);
 	mutex_unlock(&kms->commit_lock);
 
 	/*
 	 * Wait for flush to complete:
 	 */
+	trace_msm_atomic_wait_flush_start(crtc_mask);
 	kms->funcs->wait_flush(kms, crtc_mask);
+	trace_msm_atomic_wait_flush_finish(crtc_mask);
 
 	mutex_lock(&kms->commit_lock);
 	kms->funcs->complete_commit(kms, crtc_mask);
 	mutex_unlock(&kms->commit_lock);
 	kms->funcs->disable_commit(kms);
+
+out:
+	trace_msm_atomic_async_commit_finish(crtc_mask);
 }
 
 static enum hrtimer_restart msm_atomic_pending_timer(struct hrtimer *t)
@@ -144,13 +152,17 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	bool async = kms->funcs->vsync_time &&
 			can_do_async(state, &async_crtc);
 
+	trace_msm_atomic_commit_tail_start(async, crtc_mask);
+
 	kms->funcs->enable_commit(kms);
 
 	/*
 	 * Ensure any previous (potentially async) commit has
 	 * completed:
 	 */
+	trace_msm_atomic_wait_flush_start(crtc_mask);
 	kms->funcs->wait_flush(kms, crtc_mask);
+	trace_msm_atomic_wait_flush_finish(crtc_mask);
 
 	mutex_lock(&kms->commit_lock);
 
@@ -201,6 +213,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		drm_atomic_helper_commit_hw_done(state);
 		drm_atomic_helper_cleanup_planes(dev, state);
 
+		trace_msm_atomic_commit_tail_finish(async, crtc_mask);
+
 		return;
 	}
 
@@ -213,14 +227,16 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	/*
 	 * Flush hardware updates:
 	 */
-	DRM_DEBUG_ATOMIC("triggering commit\n");
+	trace_msm_atomic_flush_commit(crtc_mask);
 	kms->funcs->flush_commit(kms, crtc_mask);
 	mutex_unlock(&kms->commit_lock);
 
 	/*
 	 * Wait for flush to complete:
 	 */
+	trace_msm_atomic_wait_flush_start(crtc_mask);
 	kms->funcs->wait_flush(kms, crtc_mask);
+	trace_msm_atomic_wait_flush_finish(crtc_mask);
 
 	mutex_lock(&kms->commit_lock);
 	kms->funcs->complete_commit(kms, crtc_mask);
@@ -229,4 +245,6 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 
 	drm_atomic_helper_commit_hw_done(state);
 	drm_atomic_helper_cleanup_planes(dev, state);
+
+	trace_msm_atomic_commit_tail_finish(async, crtc_mask);
 }
diff --git a/drivers/gpu/drm/msm/msm_atomic_trace.h b/drivers/gpu/drm/msm/msm_atomic_trace.h
new file mode 100644
index 000000000000..b4ca0ed3b4a3
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_atomic_trace.h
@@ -0,0 +1,110 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#if !defined(_MSM_GPU_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
+#define _MSM_GPU_TRACE_H_
+
+#include <linux/tracepoint.h>
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM drm_msm_atomic
+#define TRACE_INCLUDE_FILE msm_atomic_trace
+
+TRACE_EVENT(msm_atomic_commit_tail_start,
+	    TP_PROTO(bool async, unsigned crtc_mask),
+	    TP_ARGS(async, crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(bool, async)
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->async = async;
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("async=%d crtc_mask=%x",
+		    __entry->async, __entry->crtc_mask)
+);
+
+TRACE_EVENT(msm_atomic_commit_tail_finish,
+	    TP_PROTO(bool async, unsigned crtc_mask),
+	    TP_ARGS(async, crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(bool, async)
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->async = async;
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("async=%d crtc_mask=%x",
+		    __entry->async, __entry->crtc_mask)
+);
+
+TRACE_EVENT(msm_atomic_async_commit_start,
+	    TP_PROTO(unsigned crtc_mask),
+	    TP_ARGS(crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("crtc_mask=%x",
+		    __entry->crtc_mask)
+);
+
+TRACE_EVENT(msm_atomic_async_commit_finish,
+	    TP_PROTO(unsigned crtc_mask),
+	    TP_ARGS(crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("crtc_mask=%x",
+		    __entry->crtc_mask)
+);
+
+TRACE_EVENT(msm_atomic_wait_flush_start,
+	    TP_PROTO(unsigned crtc_mask),
+	    TP_ARGS(crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("crtc_mask=%x",
+		    __entry->crtc_mask)
+);
+
+TRACE_EVENT(msm_atomic_wait_flush_finish,
+	    TP_PROTO(unsigned crtc_mask),
+	    TP_ARGS(crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("crtc_mask=%x",
+		    __entry->crtc_mask)
+);
+
+TRACE_EVENT(msm_atomic_flush_commit,
+	    TP_PROTO(unsigned crtc_mask),
+	    TP_ARGS(crtc_mask),
+	    TP_STRUCT__entry(
+		    __field(u32, crtc_mask)
+		    ),
+	    TP_fast_assign(
+		    __entry->crtc_mask = crtc_mask;
+		    ),
+	    TP_printk("crtc_mask=%x",
+		    __entry->crtc_mask)
+);
+
+#endif
+
+#undef TRACE_INCLUDE_PATH
+#define TRACE_INCLUDE_PATH ../../drivers/gpu/drm/msm
+#include <trace/define_trace.h>
diff --git a/drivers/gpu/drm/msm/msm_atomic_tracepoints.c b/drivers/gpu/drm/msm/msm_atomic_tracepoints.c
new file mode 100644
index 000000000000..011dc881f391
--- /dev/null
+++ b/drivers/gpu/drm/msm/msm_atomic_tracepoints.c
@@ -0,0 +1,3 @@
+// SPDX-License-Identifier: GPL-2.0
+#define CREATE_TRACE_POINTS
+#include "msm_atomic_trace.h"
diff --git a/drivers/gpu/drm/msm/msm_gpu_trace.h b/drivers/gpu/drm/msm/msm_gpu_trace.h
index 1155118a27a1..122b84789238 100644
--- a/drivers/gpu/drm/msm/msm_gpu_trace.h
+++ b/drivers/gpu/drm/msm/msm_gpu_trace.h
@@ -5,7 +5,7 @@
 #include <linux/tracepoint.h>
 
 #undef TRACE_SYSTEM
-#define TRACE_SYSTEM drm_msm
+#define TRACE_SYSTEM drm_msm_gpu
 #define TRACE_INCLUDE_FILE msm_gpu_trace
 
 TRACE_EVENT(msm_gpu_submit,
-- 
2.21.0

Re: [PATCH 10/10] drm/msm: add atomic traces

[Sean Paul]

On Thu, Aug 29, 2019 at 09:45:18AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> This was useful for debugging fps drops.  I suspect it will be useful
> again.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

I'm a simple man, I see tracepoints patches and R-b tracepoints patches :)

Reviewed-by: Sean Paul <sean@poorly.run>


> ---
>  drivers/gpu/drm/msm/Makefile                 |   1 +
>  drivers/gpu/drm/msm/msm_atomic.c             |  24 +++-
>  drivers/gpu/drm/msm/msm_atomic_trace.h       | 110 +++++++++++++++++++
>  drivers/gpu/drm/msm/msm_atomic_tracepoints.c |   3 +
>  drivers/gpu/drm/msm/msm_gpu_trace.h          |   2 +-
>  5 files changed, 136 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/msm_atomic_trace.h
>  create mode 100644 drivers/gpu/drm/msm/msm_atomic_tracepoints.c
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 7a05cbf2f820..1579cf0d828f 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -75,6 +75,7 @@ msm-y := \
>  	disp/dpu1/dpu_rm.o \
>  	disp/dpu1/dpu_vbif.o \
>  	msm_atomic.o \
> +	msm_atomic_tracepoints.o \
>  	msm_debugfs.o \
>  	msm_drv.o \
>  	msm_fb.o \
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index 80536538967b..fb247aa1081e 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -6,6 +6,7 @@
>  
>  #include <drm/drm_atomic_uapi.h>
>  
> +#include "msm_atomic_trace.h"
>  #include "msm_drv.h"
>  #include "msm_gem.h"
>  #include "msm_kms.h"
> @@ -33,11 +34,13 @@ static void msm_atomic_async_commit(struct msm_kms *kms, int crtc_idx)
>  {
>  	unsigned crtc_mask = BIT(crtc_idx);
>  
> +	trace_msm_atomic_async_commit_start(crtc_mask);
> +
>  	mutex_lock(&kms->commit_lock);
>  
>  	if (!(kms->pending_crtc_mask & crtc_mask)) {
>  		mutex_unlock(&kms->commit_lock);
> -		return;
> +		goto out;
>  	}
>  
>  	kms->pending_crtc_mask &= ~crtc_mask;
> @@ -47,19 +50,24 @@ static void msm_atomic_async_commit(struct msm_kms *kms, int crtc_idx)
>  	/*
>  	 * Flush hardware updates:
>  	 */
> -	DRM_DEBUG_ATOMIC("triggering async commit\n");
> +	trace_msm_atomic_flush_commit(crtc_mask);
>  	kms->funcs->flush_commit(kms, crtc_mask);
>  	mutex_unlock(&kms->commit_lock);
>  
>  	/*
>  	 * Wait for flush to complete:
>  	 */
> +	trace_msm_atomic_wait_flush_start(crtc_mask);
>  	kms->funcs->wait_flush(kms, crtc_mask);
> +	trace_msm_atomic_wait_flush_finish(crtc_mask);
>  
>  	mutex_lock(&kms->commit_lock);
>  	kms->funcs->complete_commit(kms, crtc_mask);
>  	mutex_unlock(&kms->commit_lock);
>  	kms->funcs->disable_commit(kms);
> +
> +out:
> +	trace_msm_atomic_async_commit_finish(crtc_mask);
>  }
>  
>  static enum hrtimer_restart msm_atomic_pending_timer(struct hrtimer *t)
> @@ -144,13 +152,17 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	bool async = kms->funcs->vsync_time &&
>  			can_do_async(state, &async_crtc);
>  
> +	trace_msm_atomic_commit_tail_start(async, crtc_mask);
> +
>  	kms->funcs->enable_commit(kms);
>  
>  	/*
>  	 * Ensure any previous (potentially async) commit has
>  	 * completed:
>  	 */
> +	trace_msm_atomic_wait_flush_start(crtc_mask);
>  	kms->funcs->wait_flush(kms, crtc_mask);
> +	trace_msm_atomic_wait_flush_finish(crtc_mask);
>  
>  	mutex_lock(&kms->commit_lock);
>  
> @@ -201,6 +213,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  		drm_atomic_helper_commit_hw_done(state);
>  		drm_atomic_helper_cleanup_planes(dev, state);
>  
> +		trace_msm_atomic_commit_tail_finish(async, crtc_mask);
> +
>  		return;
>  	}
>  
> @@ -213,14 +227,16 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  	/*
>  	 * Flush hardware updates:
>  	 */
> -	DRM_DEBUG_ATOMIC("triggering commit\n");
> +	trace_msm_atomic_flush_commit(crtc_mask);
>  	kms->funcs->flush_commit(kms, crtc_mask);
>  	mutex_unlock(&kms->commit_lock);
>  
>  	/*
>  	 * Wait for flush to complete:
>  	 */
> +	trace_msm_atomic_wait_flush_start(crtc_mask);
>  	kms->funcs->wait_flush(kms, crtc_mask);
> +	trace_msm_atomic_wait_flush_finish(crtc_mask);
>  
>  	mutex_lock(&kms->commit_lock);
>  	kms->funcs->complete_commit(kms, crtc_mask);
> @@ -229,4 +245,6 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
>  
>  	drm_atomic_helper_commit_hw_done(state);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> +
> +	trace_msm_atomic_commit_tail_finish(async, crtc_mask);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_atomic_trace.h b/drivers/gpu/drm/msm/msm_atomic_trace.h
> new file mode 100644
> index 000000000000..b4ca0ed3b4a3
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_atomic_trace.h
> @@ -0,0 +1,110 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#if !defined(_MSM_GPU_TRACE_H_) || defined(TRACE_HEADER_MULTI_READ)
> +#define _MSM_GPU_TRACE_H_
> +
> +#include <linux/tracepoint.h>
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM drm_msm_atomic
> +#define TRACE_INCLUDE_FILE msm_atomic_trace
> +
> +TRACE_EVENT(msm_atomic_commit_tail_start,
> +	    TP_PROTO(bool async, unsigned crtc_mask),
> +	    TP_ARGS(async, crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(bool, async)
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->async = async;
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("async=%d crtc_mask=%x",
> +		    __entry->async, __entry->crtc_mask)
> +);
> +
> +TRACE_EVENT(msm_atomic_commit_tail_finish,
> +	    TP_PROTO(bool async, unsigned crtc_mask),
> +	    TP_ARGS(async, crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(bool, async)
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->async = async;
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("async=%d crtc_mask=%x",
> +		    __entry->async, __entry->crtc_mask)
> +);
> +
> +TRACE_EVENT(msm_atomic_async_commit_start,
> +	    TP_PROTO(unsigned crtc_mask),
> +	    TP_ARGS(crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("crtc_mask=%x",
> +		    __entry->crtc_mask)
> +);
> +
> +TRACE_EVENT(msm_atomic_async_commit_finish,
> +	    TP_PROTO(unsigned crtc_mask),
> +	    TP_ARGS(crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("crtc_mask=%x",
> +		    __entry->crtc_mask)
> +);
> +
> +TRACE_EVENT(msm_atomic_wait_flush_start,
> +	    TP_PROTO(unsigned crtc_mask),
> +	    TP_ARGS(crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("crtc_mask=%x",
> +		    __entry->crtc_mask)
> +);
> +
> +TRACE_EVENT(msm_atomic_wait_flush_finish,
> +	    TP_PROTO(unsigned crtc_mask),
> +	    TP_ARGS(crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("crtc_mask=%x",
> +		    __entry->crtc_mask)
> +);
> +
> +TRACE_EVENT(msm_atomic_flush_commit,
> +	    TP_PROTO(unsigned crtc_mask),
> +	    TP_ARGS(crtc_mask),
> +	    TP_STRUCT__entry(
> +		    __field(u32, crtc_mask)
> +		    ),
> +	    TP_fast_assign(
> +		    __entry->crtc_mask = crtc_mask;
> +		    ),
> +	    TP_printk("crtc_mask=%x",
> +		    __entry->crtc_mask)
> +);
> +
> +#endif
> +
> +#undef TRACE_INCLUDE_PATH
> +#define TRACE_INCLUDE_PATH ../../drivers/gpu/drm/msm
> +#include <trace/define_trace.h>
> diff --git a/drivers/gpu/drm/msm/msm_atomic_tracepoints.c b/drivers/gpu/drm/msm/msm_atomic_tracepoints.c
> new file mode 100644
> index 000000000000..011dc881f391
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_atomic_tracepoints.c
> @@ -0,0 +1,3 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define CREATE_TRACE_POINTS
> +#include "msm_atomic_trace.h"
> diff --git a/drivers/gpu/drm/msm/msm_gpu_trace.h b/drivers/gpu/drm/msm/msm_gpu_trace.h
> index 1155118a27a1..122b84789238 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_trace.h
> +++ b/drivers/gpu/drm/msm/msm_gpu_trace.h
> @@ -5,7 +5,7 @@
>  #include <linux/tracepoint.h>
>  
>  #undef TRACE_SYSTEM
> -#define TRACE_SYSTEM drm_msm
> +#define TRACE_SYSTEM drm_msm_gpu
>  #define TRACE_INCLUDE_FILE msm_gpu_trace
>  
>  TRACE_EVENT(msm_gpu_submit,
> -- 
> 2.21.0
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS