linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/9] drm/msm: async commit support
@ 2019-08-27 21:33 Rob Clark
  2019-08-27 21:33 ` [PATCH 1/9] drm/msm/dpu: unwind async commit handling Rob Clark
                   ` (8 more replies)
  0 siblings, 9 replies; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Abhinav Kumar, Alexios Zavras, Allison Randal,
	Boris Brezillon, Bruce Wang, Daniel Vetter, Enrico Weigelt,
	open list:DRM DRIVER FOR MSM ADRENO GPU, Fritz Koenig,
	Georgi Djakov, Greg Kroah-Hartman, Jeykumar Sankaran,
	Jonathan Marek, Jordan Crouse,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list, Mamta Shukla,
	Sean Paul, Sravanthi Kollukuduru, Thomas Gleixner

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.

Rob Clark (9):
  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

 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              | 210 +++++++++++++++---
 drivers/gpu/drm/msm/msm_drv.c                 |   1 +
 drivers/gpu/drm/msm/msm_drv.h                 |   4 +
 drivers/gpu/drm/msm/msm_kms.h                 | 108 ++++++++-
 12 files changed, 466 insertions(+), 199 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/9] drm/msm/dpu: unwind async commit handling
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-29 13:04   ` Sean Paul
  2019-08-27 21:33 ` [PATCH 2/9] drm/msm/dpu: add real wait_for_commit_done() Rob Clark
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Bruce Wang, Jordan Crouse,
	Sravanthi Kollukuduru, Thomas Gleixner, Abhinav Kumar,
	Fritz Koenig, Alexios Zavras, Greg Kroah-Hartman,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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>
---
 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/9] drm/msm/dpu: add real wait_for_commit_done()
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
  2019-08-27 21:33 ` [PATCH 1/9] drm/msm/dpu: unwind async commit handling Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-29 13:04   ` Sean Paul
  2019-08-27 21:33 ` [PATCH 3/9] drm/msm/dpu: handle_frame_done() from vblank irq Rob Clark
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Allison Randal, Jordan Crouse,
	Thomas Gleixner, open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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>
---
 .../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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/9] drm/msm/dpu: handle_frame_done() from vblank irq
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
  2019-08-27 21:33 ` [PATCH 1/9] drm/msm/dpu: unwind async commit handling Rob Clark
  2019-08-27 21:33 ` [PATCH 2/9] drm/msm/dpu: add real wait_for_commit_done() Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-29 13:05   ` Sean Paul
  2019-08-27 21:33 ` [PATCH 4/9] drm/msm: add kms->wait_flush() Rob Clark
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Bruce Wang, Jordan Crouse,
	Sravanthi Kollukuduru, Thomas Gleixner,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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>
---
 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 4/9] drm/msm: add kms->wait_flush()
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
                   ` (2 preceding siblings ...)
  2019-08-27 21:33 ` [PATCH 3/9] drm/msm/dpu: handle_frame_done() from vblank irq Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-29 13:04   ` Sean Paul
  2019-08-27 21:33 ` [PATCH 5/9] drm/msm: convert kms->complete_commit() to crtc_mask Rob Clark
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Jordan Crouse, Greg Kroah-Hartman,
	Sravanthi Kollukuduru, Bruce Wang, Jonathan Marek,
	Thomas Gleixner, Allison Randal, Mamta Shukla, Enrico Weigelt,
	Georgi Djakov, open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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()

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..639cc88c31a1 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 |= BIT(crtc->index);
+
+	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..8562bbfd5dca 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) \
+	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head) \
+		for_each_if (BIT((crtc)->index) & (crtc_mask))
+
 #endif /* __MSM_KMS_H__ */
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 5/9] drm/msm: convert kms->complete_commit() to crtc_mask
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
                   ` (3 preceding siblings ...)
  2019-08-27 21:33 ` [PATCH 4/9] drm/msm: add kms->wait_flush() Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-27 21:33 ` [PATCH 6/9] drm/msm: add kms->flush_commit() Rob Clark
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Bruce Wang, Jordan Crouse,
	Sravanthi Kollukuduru, Thomas Gleixner, Abhinav Kumar,
	Jonathan Marek, Mamta Shukla, Greg Kroah-Hartman, Allison Randal,
	Alexios Zavras, Boris Brezillon, Georgi Djakov,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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 639cc88c31a1..bcb6d6144d4d 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 8562bbfd5dca..c56c54b698ec 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 6/9] drm/msm: add kms->flush_commit()
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
                   ` (4 preceding siblings ...)
  2019-08-27 21:33 ` [PATCH 5/9] drm/msm: convert kms->complete_commit() to crtc_mask Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-27 21:33 ` [PATCH 7/9] drm/msm: split power control from prepare/complete_commit Rob Clark
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Jordan Crouse, Abhinav Kumar,
	Sravanthi Kollukuduru, Bruce Wang, Jonathan Marek,
	Enrico Weigelt, Mamta Shukla, Thomas Gleixner,
	Greg Kroah-Hartman, Alexios Zavras, Boris Brezillon,
	Georgi Djakov, open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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 bcb6d6144d4d..27369b020bee 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 c56c54b698ec..55234f661382 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 7/9] drm/msm: split power control from prepare/complete_commit
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
                   ` (5 preceding siblings ...)
  2019-08-27 21:33 ` [PATCH 6/9] drm/msm: add kms->flush_commit() Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-27 21:33 ` [PATCH 8/9] drm/msm: async commit support Rob Clark
  2019-08-27 21:33 ` [PATCH 9/9] drm/msm/dpu: " Rob Clark
  8 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Jordan Crouse, Abhinav Kumar,
	Sravanthi Kollukuduru, Bruce Wang, Jonathan Marek, Mamta Shukla,
	Enrico Weigelt, Thomas Gleixner, Georgi Djakov, Boris Brezillon,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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 27369b020bee..3d0424205349 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 55234f661382..f9847eb39143 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 8/9] drm/msm: async commit support
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
                   ` (6 preceding siblings ...)
  2019-08-27 21:33 ` [PATCH 7/9] drm/msm: split power control from prepare/complete_commit Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  2019-08-27 21:33 ` [PATCH 9/9] drm/msm/dpu: " Rob Clark
  8 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_atomic.c | 153 ++++++++++++++++++++++++++++++-
 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, 207 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 3d0424205349..de767a19e193 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -29,6 +29,94 @@ 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))
+		goto out;
+
+	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);
+
+	/*
+	 * Wait for flush to complete:
+	 */
+	kms->funcs->wait_flush(kms, crtc_mask);
+
+	kms->funcs->complete_commit(kms, crtc_mask);
+	kms->funcs->disable_commit(kms);
+
+out:
+	mutex_unlock(&kms->commit_lock);
+
+}
+
+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 +138,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 +166,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[async_crtc->index];
+
+		/* async updates are limited to single-crtc updates: */
+		WARN_ON(crtc_mask != BIT(async_crtc->index));
+
+		/*
+		 * 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:
 	 */
@@ -71,11 +218,15 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 	kms->funcs->flush_commit(kms, crtc_mask);
 
+	/*
+	 * Wait for flush to complete:
+	 */
 	kms->funcs->wait_flush(kms, crtc_mask);
+
 	kms->funcs->complete_commit(kms, crtc_mask);
 	kms->funcs->disable_commit(kms);
+	mutex_unlock(&kms->commit_lock);
 
 	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 f9847eb39143..9a0236eb6487 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 9/9] drm/msm/dpu: async commit support
  2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
                   ` (7 preceding siblings ...)
  2019-08-27 21:33 ` [PATCH 8/9] drm/msm: async commit support Rob Clark
@ 2019-08-27 21:33 ` Rob Clark
  8 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2019-08-27 21:33 UTC (permalink / raw)
  To: dri-devel
  Cc: Rob Clark, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Bruce Wang, Jordan Crouse,
	Sravanthi Kollukuduru, Abhinav Kumar, Allison Randal,
	Alexios Zavras, Thomas Gleixner, Greg Kroah-Hartman,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

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 de767a19e193..de167f0ddca5 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -212,10 +212,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);
 
 	/*
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 9a0236eb6487..c8cdf5763dd6 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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 4/9] drm/msm: add kms->wait_flush()
  2019-08-27 21:33 ` [PATCH 4/9] drm/msm: add kms->wait_flush() Rob Clark
@ 2019-08-29 13:04   ` Sean Paul
  0 siblings, 0 replies; 14+ messages in thread
From: Sean Paul @ 2019-08-29 13:04 UTC (permalink / raw)
  To: Rob Clark
  Cc: dri-devel, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Jordan Crouse, Greg Kroah-Hartman,
	Sravanthi Kollukuduru, Bruce Wang, Jonathan Marek,
	Thomas Gleixner, Allison Randal, Mamta Shukla, Enrico Weigelt,
	Georgi Djakov, open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

On Tue, Aug 27, 2019 at 02:33:34PM -0700, Rob Clark wrote:
> 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()
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Hi Rob,
A few nits below, but lgtm overall


> ---
>  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(-)
> 

/snip

> index e5aae1645933..639cc88c31a1 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 |= BIT(crtc->index);

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..8562bbfd5dca 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) \
> +	list_for_each_entry(crtc, &(dev)->mode_config.crtc_list, head) \

drm_for_each_crtc

> +		for_each_if (BIT((crtc)->index) & (crtc_mask))
> +
>  #endif /* __MSM_KMS_H__ */
> -- 
> 2.21.0
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/9] drm/msm/dpu: unwind async commit handling
  2019-08-27 21:33 ` [PATCH 1/9] drm/msm/dpu: unwind async commit handling Rob Clark
@ 2019-08-29 13:04   ` Sean Paul
  0 siblings, 0 replies; 14+ messages in thread
From: Sean Paul @ 2019-08-29 13:04 UTC (permalink / raw)
  To: Rob Clark
  Cc: dri-devel, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Bruce Wang, Jordan Crouse,
	Sravanthi Kollukuduru, Thomas Gleixner, Abhinav Kumar,
	Fritz Koenig, Alexios Zavras, Greg Kroah-Hartman,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

On Tue, Aug 27, 2019 at 02:33:31PM -0700, Rob Clark wrote:
> 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
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/9] drm/msm/dpu: add real wait_for_commit_done()
  2019-08-27 21:33 ` [PATCH 2/9] drm/msm/dpu: add real wait_for_commit_done() Rob Clark
@ 2019-08-29 13:04   ` Sean Paul
  0 siblings, 0 replies; 14+ messages in thread
From: Sean Paul @ 2019-08-29 13:04 UTC (permalink / raw)
  To: Rob Clark
  Cc: dri-devel, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Allison Randal, Jordan Crouse,
	Thomas Gleixner, open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

On Tue, Aug 27, 2019 at 02:33:32PM -0700, Rob Clark wrote:
> 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
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 3/9] drm/msm/dpu: handle_frame_done() from vblank irq
  2019-08-27 21:33 ` [PATCH 3/9] drm/msm/dpu: handle_frame_done() from vblank irq Rob Clark
@ 2019-08-29 13:05   ` Sean Paul
  0 siblings, 0 replies; 14+ messages in thread
From: Sean Paul @ 2019-08-29 13:05 UTC (permalink / raw)
  To: Rob Clark
  Cc: dri-devel, Rob Clark, Sean Paul, David Airlie, Daniel Vetter,
	Jeykumar Sankaran, Bruce Wang, Jordan Crouse,
	Sravanthi Kollukuduru, Thomas Gleixner,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:DRM DRIVER FOR MSM ADRENO GPU, open list

On Tue, Aug 27, 2019 at 02:33:33PM -0700, Rob Clark wrote:
> 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
> 

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-08-29 13:05 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-27 21:33 [PATCH 0/9] drm/msm: async commit support Rob Clark
2019-08-27 21:33 ` [PATCH 1/9] drm/msm/dpu: unwind async commit handling Rob Clark
2019-08-29 13:04   ` Sean Paul
2019-08-27 21:33 ` [PATCH 2/9] drm/msm/dpu: add real wait_for_commit_done() Rob Clark
2019-08-29 13:04   ` Sean Paul
2019-08-27 21:33 ` [PATCH 3/9] drm/msm/dpu: handle_frame_done() from vblank irq Rob Clark
2019-08-29 13:05   ` Sean Paul
2019-08-27 21:33 ` [PATCH 4/9] drm/msm: add kms->wait_flush() Rob Clark
2019-08-29 13:04   ` Sean Paul
2019-08-27 21:33 ` [PATCH 5/9] drm/msm: convert kms->complete_commit() to crtc_mask Rob Clark
2019-08-27 21:33 ` [PATCH 6/9] drm/msm: add kms->flush_commit() Rob Clark
2019-08-27 21:33 ` [PATCH 7/9] drm/msm: split power control from prepare/complete_commit Rob Clark
2019-08-27 21:33 ` [PATCH 8/9] drm/msm: async commit support Rob Clark
2019-08-27 21:33 ` [PATCH 9/9] drm/msm/dpu: " Rob Clark

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).