[PATCH v2 1/3] drm/msm: dpu: Mask inactive pending flushes

[PATCH v2 1/3] drm/msm: dpu: Mask inactive pending flushes

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

This patch masks any pending flushes which have not been latched for a
commit. This will catch the case where an asynchronous update is
nullified by a disable in the same frame.

Changes in v2:
- Added to the set

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
index 8fa601a9abbf..d7a7fedc09f7 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
@@ -28,6 +28,7 @@
 #define   CTL_TOP                       0x014
 #define   CTL_FLUSH                     0x018
 #define   CTL_START                     0x01C
+#define   CTL_FLUSH_MASK                0x090
 #define   CTL_PREPARE                   0x0d0
 #define   CTL_SW_RESET                  0x030
 #define   CTL_LAYER_EXTN_OFFSET         0x40
@@ -121,6 +122,12 @@ static inline void dpu_hw_ctl_trigger_flush(struct dpu_hw_ctl *ctx)
 {
 	trace_dpu_hw_ctl_trigger_pending_flush(ctx->pending_flush_mask,
 				     dpu_hw_ctl_get_flush_register(ctx));
+
+	/*
+	 * Async updates could have changed CTL_FLUSH since it was last latched.
+	 * Mask anything not involved in this latest commit.
+	 */
+	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH_MASK, ~ctx->pending_flush_mask);
 	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
 }
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

[PATCH v2 2/3] drm/msm: dpu: Only check flush register against pending flushes

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

There exists a case where a flush of a plane/dma may have been triggered
& started from an async commit. If that plane/dma is subsequently disabled
by the next commit, the flush register will continue to hold the flush
bit for the disabled plane. Since the bit remains active,
pending_kickoff_cnt will never decrement and we'll miss frame_done
events.

This patch limits the check of flush_register to include only those bits
which have been updated with the latest commit.

Changes in v2:
- None

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
 1 file changed, 1 insertion(+), 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 b3c68c4fcc8e..667f304c92ea 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
@@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)
 	if (hw_ctl && hw_ctl->ops.get_flush_register)
 		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
 
-	if (flush_register == 0)
+	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
 		new_cnt = atomic_add_unless(&phys_enc->pending_kickoff_cnt,
 				-1, 0);
 	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH v2 2/3] drm/msm: dpu: Only check flush register against pending flushes

[Jeykumar Sankaran]

On 2018-10-30 09:00, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> There exists a case where a flush of a plane/dma may have been 
> triggered
> & started from an async commit. If that plane/dma is subsequently 
> disabled
> by the next commit, the flush register will continue to hold the flush
> bit for the disabled plane. Since the bit remains active,
> pending_kickoff_cnt will never decrement and we'll miss frame_done
> events.
> 
> This patch limits the check of flush_register to include only those 
> bits
> which have been updated with the latest commit.
> 
> Changes in v2:
> - None
> 

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

> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>  1 file changed, 1 insertion(+), 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 b3c68c4fcc8e..667f304c92ea 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
> @@ -331,7 +331,7 @@ static void dpu_encoder_phys_vid_vblank_irq(void 
> *arg,
> int irq_idx)
>  	if (hw_ctl && hw_ctl->ops.get_flush_register)
>  		flush_register = hw_ctl->ops.get_flush_register(hw_ctl);
> 
> -	if (flush_register == 0)
> +	if (!(flush_register & hw_ctl->ops.get_pending_flush(hw_ctl)))
>  		new_cnt =
> atomic_add_unless(&phys_enc->pending_kickoff_cnt,
>  				-1, 0);
>  	spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);

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

[PATCH v2 3/3] drm/msm: dpu: Make legacy cursor updates asynchronous

[Sean Paul]

From: Sean Paul <seanpaul@chromium.org>

This patch sprinkles a few async/legacy_cursor_update checks
through commit to ensure that cursor updates aren't blocked on vsync.
There are 2 main components to this, the first is that we don't want to
wait_for_commit_done in msm_atomic  before returning from atomic_complete.
The second is that in dpu we don't want to wait for frame_done events when
updating the cursor.

Changes in v2:
- None

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44 +++++++++++----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
 drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
 6 files changed, 49 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index ed84cf44a222..1e3e57817b72 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -702,7 +702,7 @@ static int _dpu_crtc_wait_for_frame_done(struct drm_crtc *crtc)
 	return rc;
 }
 
-void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
+void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
 {
 	struct drm_encoder *encoder;
 	struct drm_device *dev = crtc->dev;
@@ -731,27 +731,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 		 * Encoder will flush/start now, unless it has a tx pending.
 		 * If so, it may delay and flush at an irq event (e.g. ppdone)
 		 */
-		dpu_encoder_prepare_for_kickoff(encoder, &params);
+		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
 	}
 
-	/* wait for 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 (!async) {
+		/* wait for 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);
 
-	dpu_crtc->play_count++;
+		dpu_crtc->play_count++;
+	}
 
 	dpu_vbif_clear_errors(dpu_kms);
 
@@ -759,11 +762,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
 		if (encoder->crtc != crtc)
 			continue;
 
-		dpu_encoder_kickoff(encoder);
+		dpu_encoder_kickoff(encoder, async);
 	}
 
 end:
-	reinit_completion(&dpu_crtc->frame_done_comp);
+	if (!async)
+		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 4822602402f9..ec633ce3ee6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
@@ -277,8 +277,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
 /**
  * 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);
+void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
 
 /**
  * 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 82c55efb500f..a8ba10ceaacf 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1375,7 +1375,8 @@ static void dpu_encoder_off_work(struct kthread_work *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)
+		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
+		bool async)
 {
 	struct dpu_hw_ctl *ctl;
 	int pending_kickoff_cnt;
@@ -1398,7 +1399,10 @@ static void _dpu_encoder_trigger_flush(struct drm_encoder *drm_enc,
 		return;
 	}
 
-	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
+	if (!async)
+		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
+	else
+		pending_kickoff_cnt = atomic_read(&phys->pending_kickoff_cnt);
 
 	if (extra_flush_bits && ctl->ops.update_pending_flush)
 		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
@@ -1511,7 +1515,8 @@ 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)
+static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
+				      bool async)
 {
 	struct dpu_hw_ctl *ctl;
 	uint32_t i, pending_flush;
@@ -1542,7 +1547,8 @@ static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc)
 			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);
+			_dpu_encoder_trigger_flush(&dpu_enc->base, phys, 0x0,
+						   async);
 		else if (ctl->ops.get_pending_flush)
 			pending_flush |= ctl->ops.get_pending_flush(ctl);
 	}
@@ -1552,7 +1558,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);
+				pending_flush, async);
 	}
 
 	_dpu_encoder_trigger_start(dpu_enc->cur_master);
@@ -1736,7 +1742,7 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
 }
 
 void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
-		struct dpu_encoder_kickoff_params *params)
+		struct dpu_encoder_kickoff_params *params, bool async)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
@@ -1775,7 +1781,7 @@ void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
 	}
 }
 
-void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
+void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
 {
 	struct dpu_encoder_virt *dpu_enc;
 	struct dpu_encoder_phys *phys;
@@ -1798,7 +1804,7 @@ void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
 		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) / 1000));
 
 	/* All phys encs are ready to go, trigger the kickoff */
-	_dpu_encoder_kickoff_phys(dpu_enc);
+	_dpu_encoder_kickoff_phys(dpu_enc, async);
 
 	/* 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 9dbf38f446d9..c2044122d609 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -81,9 +81,10 @@ void dpu_encoder_register_frame_event_callback(struct drm_encoder *encoder,
  *	Delayed: Block until next trigger can be issued.
  * @encoder:	encoder pointer
  * @params:	kickoff time parameters
+ * @async:	true if this is an asynchronous commit
  */
 void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
-		struct dpu_encoder_kickoff_params *params);
+		struct dpu_encoder_kickoff_params *params, bool async);
 
 /**
  * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from previous
@@ -96,8 +97,9 @@ 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);
+void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
 
 /**
  * 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 985c855796ae..57ad83868766 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
 
 	if (crtc && crtc->state->active) {
 		trace_dpu_kms_enc_enable(DRMID(crtc));
-		dpu_crtc_commit_kickoff(crtc);
+		dpu_crtc_commit_kickoff(crtc, false);
 	}
 }
 
@@ -369,7 +369,8 @@ 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);
+			dpu_crtc_commit_kickoff(crtc,
+						state->legacy_cursor_update);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 2088a20eb270..f5b1256e32b6 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -83,7 +83,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
 		kms->funcs->commit(kms, state);
 	}
 
-	msm_atomic_wait_for_commit_done(dev, state);
+	if (!state->legacy_cursor_update)
+		msm_atomic_wait_for_commit_done(dev, state);
 
 	kms->funcs->complete_commit(kms, state);
 
-- 
Sean Paul, Software Engineer, Google / Chromium OS

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

Re: [PATCH v2 3/3] drm/msm: dpu: Make legacy cursor updates asynchronous

[Jeykumar Sankaran]

On 2018-10-30 09:00, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch sprinkles a few async/legacy_cursor_update checks
> through commit to ensure that cursor updates aren't blocked on vsync.
> There are 2 main components to this, the first is that we don't want to
> wait_for_commit_done in msm_atomic  before returning from 
> atomic_complete.
> The second is that in dpu we don't want to wait for frame_done events 
> when
> updating the cursor.
> 
> Changes in v2:
> - None
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44 +++++++++++----------
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
>  drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
>  6 files changed, 49 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index ed84cf44a222..1e3e57817b72 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -702,7 +702,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
> drm_crtc *crtc)
>  	return rc;
>  }
> 
> -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
>  {
>  	struct drm_encoder *encoder;
>  	struct drm_device *dev = crtc->dev;
> @@ -731,27 +731,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
> *crtc)
>  		 * Encoder will flush/start now, unless it has a tx
> pending.
>  		 * If so, it may delay and flush at an irq event (e.g.
> ppdone)
>  		 */
> -		dpu_encoder_prepare_for_kickoff(encoder, &params);
> +		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
>  	}
> 
> -	/* wait for 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 (!async) {
> +		/* wait for 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);
> 
> -	dpu_crtc->play_count++;
> +		dpu_crtc->play_count++;
> +	}
> 
>  	dpu_vbif_clear_errors(dpu_kms);
> 
> @@ -759,11 +762,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc 
> *crtc)
>  		if (encoder->crtc != crtc)
>  			continue;
> 
> -		dpu_encoder_kickoff(encoder);
> +		dpu_encoder_kickoff(encoder, async);
>  	}
> 
>  end:
> -	reinit_completion(&dpu_crtc->frame_done_comp);
> +	if (!async)
> +		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 4822602402f9..ec633ce3ee6c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> @@ -277,8 +277,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool 
> en);
>  /**
>   * 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);
> +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
> 
>  /**
>   * 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 82c55efb500f..a8ba10ceaacf 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1375,7 +1375,8 @@ static void dpu_encoder_off_work(struct 
> kthread_work
> *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)
> +		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
> +		bool async)
>  {
>  	struct dpu_hw_ctl *ctl;
>  	int pending_kickoff_cnt;
> @@ -1398,7 +1399,10 @@ static void _dpu_encoder_trigger_flush(struct
> drm_encoder *drm_enc,
>  		return;
>  	}
> 
> -	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> +	if (!async)
> +		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> +	else
> +		pending_kickoff_cnt =
> atomic_read(&phys->pending_kickoff_cnt);
> 
>  	if (extra_flush_bits && ctl->ops.update_pending_flush)
>  		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
> @@ -1511,7 +1515,8 @@ 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)
> +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt 
> *dpu_enc,
> +				      bool async)
>  {
>  	struct dpu_hw_ctl *ctl;
>  	uint32_t i, pending_flush;
> @@ -1542,7 +1547,8 @@ static void _dpu_encoder_kickoff_phys(struct
> dpu_encoder_virt *dpu_enc)
>  			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);
> +			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> 0x0,
> +						   async);
>  		else if (ctl->ops.get_pending_flush)
>  			pending_flush |= ctl->ops.get_pending_flush(ctl);
>  	}
> @@ -1552,7 +1558,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);
> +				pending_flush, async);
>  	}
> 
>  	_dpu_encoder_trigger_start(dpu_enc->cur_master);
> @@ -1736,7 +1742,7 @@ static void
> dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
>  }
> 
>  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
> -		struct dpu_encoder_kickoff_params *params)
> +		struct dpu_encoder_kickoff_params *params, bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> @@ -1775,7 +1781,7 @@ void dpu_encoder_prepare_for_kickoff(struct
> drm_encoder *drm_enc,
>  	}
>  }
> 
> -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
> +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>  {
>  	struct dpu_encoder_virt *dpu_enc;
>  	struct dpu_encoder_phys *phys;
> @@ -1798,7 +1804,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> *drm_enc)
>  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
> 1000));
> 
>  	/* All phys encs are ready to go, trigger the kickoff */
> -	_dpu_encoder_kickoff_phys(dpu_enc);
> +	_dpu_encoder_kickoff_phys(dpu_enc, async);
> 
>  	/* 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 9dbf38f446d9..c2044122d609 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -81,9 +81,10 @@ void 
> dpu_encoder_register_frame_event_callback(struct
> drm_encoder *encoder,
>   *	Delayed: Block until next trigger can be issued.
>   * @encoder:	encoder pointer
>   * @params:	kickoff time parameters
> + * @async:	true if this is an asynchronous commit
>   */
>  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
> -		struct dpu_encoder_kickoff_params *params);
> +		struct dpu_encoder_kickoff_params *params, bool async);
> 
>  /**
>   * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
> previous
> @@ -96,8 +97,9 @@ 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);
> +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
> 
>  /**
>   * 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 985c855796ae..57ad83868766 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
> *encoder)
> 
>  	if (crtc && crtc->state->active) {
>  		trace_dpu_kms_enc_enable(DRMID(crtc));
> -		dpu_crtc_commit_kickoff(crtc);
> +		dpu_crtc_commit_kickoff(crtc, false);
>  	}
>  }
> 
> @@ -369,7 +369,8 @@ 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);
> +			dpu_crtc_commit_kickoff(crtc,
> +
> state->legacy_cursor_update);
>  		}
>  	}
>  }
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> b/drivers/gpu/drm/msm/msm_atomic.c
> index 2088a20eb270..f5b1256e32b6 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -83,7 +83,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
> *state)
>  		kms->funcs->commit(kms, state);
>  	}
> 
> -	msm_atomic_wait_for_commit_done(dev, state);
> +	if (!state->legacy_cursor_update)
> +		msm_atomic_wait_for_commit_done(dev, state);
Was this change tested with MDP5? They have a different path
to support async updates.

Thanks,
Jeykumar S.
> 
>  	kms->funcs->complete_commit(kms, state);

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

Re: [PATCH v2 3/3] drm/msm: dpu: Make legacy cursor updates asynchronous

[Sean Paul]

On Thu, Nov 08, 2018 at 02:00:51PM -0800, Jeykumar Sankaran wrote:
> On 2018-10-30 09:00, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch sprinkles a few async/legacy_cursor_update checks
> > through commit to ensure that cursor updates aren't blocked on vsync.
> > There are 2 main components to this, the first is that we don't want to
> > wait_for_commit_done in msm_atomic  before returning from
> > atomic_complete.
> > The second is that in dpu we don't want to wait for frame_done events
> > when
> > updating the cursor.
> > 
> > Changes in v2:
> > - None
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44 +++++++++++----------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
> >  drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
> >  6 files changed, 49 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index ed84cf44a222..1e3e57817b72 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -702,7 +702,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
> > drm_crtc *crtc)
> >  	return rc;
> >  }
> > 
> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
> >  {
> >  	struct drm_encoder *encoder;
> >  	struct drm_device *dev = crtc->dev;
> > @@ -731,27 +731,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> >  		 * Encoder will flush/start now, unless it has a tx
> > pending.
> >  		 * If so, it may delay and flush at an irq event (e.g.
> > ppdone)
> >  		 */
> > -		dpu_encoder_prepare_for_kickoff(encoder, &params);
> > +		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
> >  	}
> > 
> > -	/* wait for 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 (!async) {
> > +		/* wait for 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);
> > 
> > -	dpu_crtc->play_count++;
> > +		dpu_crtc->play_count++;
> > +	}
> > 
> >  	dpu_vbif_clear_errors(dpu_kms);
> > 
> > @@ -759,11 +762,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
> > *crtc)
> >  		if (encoder->crtc != crtc)
> >  			continue;
> > 
> > -		dpu_encoder_kickoff(encoder);
> > +		dpu_encoder_kickoff(encoder, async);
> >  	}
> > 
> >  end:
> > -	reinit_completion(&dpu_crtc->frame_done_comp);
> > +	if (!async)
> > +		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 4822602402f9..ec633ce3ee6c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
> > @@ -277,8 +277,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool en);
> >  /**
> >   * 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);
> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
> > 
> >  /**
> >   * 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 82c55efb500f..a8ba10ceaacf 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -1375,7 +1375,8 @@ static void dpu_encoder_off_work(struct
> > kthread_work
> > *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)
> > +		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
> > +		bool async)
> >  {
> >  	struct dpu_hw_ctl *ctl;
> >  	int pending_kickoff_cnt;
> > @@ -1398,7 +1399,10 @@ static void _dpu_encoder_trigger_flush(struct
> > drm_encoder *drm_enc,
> >  		return;
> >  	}
> > 
> > -	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> > +	if (!async)
> > +		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
> > +	else
> > +		pending_kickoff_cnt =
> > atomic_read(&phys->pending_kickoff_cnt);
> > 
> >  	if (extra_flush_bits && ctl->ops.update_pending_flush)
> >  		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
> > @@ -1511,7 +1515,8 @@ 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)
> > +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt *dpu_enc,
> > +				      bool async)
> >  {
> >  	struct dpu_hw_ctl *ctl;
> >  	uint32_t i, pending_flush;
> > @@ -1542,7 +1547,8 @@ static void _dpu_encoder_kickoff_phys(struct
> > dpu_encoder_virt *dpu_enc)
> >  			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);
> > +			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
> > 0x0,
> > +						   async);
> >  		else if (ctl->ops.get_pending_flush)
> >  			pending_flush |= ctl->ops.get_pending_flush(ctl);
> >  	}
> > @@ -1552,7 +1558,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);
> > +				pending_flush, async);
> >  	}
> > 
> >  	_dpu_encoder_trigger_start(dpu_enc->cur_master);
> > @@ -1736,7 +1742,7 @@ static void
> > dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
> >  }
> > 
> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
> > -		struct dpu_encoder_kickoff_params *params)
> > +		struct dpu_encoder_kickoff_params *params, bool async)
> >  {
> >  	struct dpu_encoder_virt *dpu_enc;
> >  	struct dpu_encoder_phys *phys;
> > @@ -1775,7 +1781,7 @@ void dpu_encoder_prepare_for_kickoff(struct
> > drm_encoder *drm_enc,
> >  	}
> >  }
> > 
> > -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
> > +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
> >  {
> >  	struct dpu_encoder_virt *dpu_enc;
> >  	struct dpu_encoder_phys *phys;
> > @@ -1798,7 +1804,7 @@ void dpu_encoder_kickoff(struct drm_encoder
> > *drm_enc)
> >  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
> > 1000));
> > 
> >  	/* All phys encs are ready to go, trigger the kickoff */
> > -	_dpu_encoder_kickoff_phys(dpu_enc);
> > +	_dpu_encoder_kickoff_phys(dpu_enc, async);
> > 
> >  	/* 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 9dbf38f446d9..c2044122d609 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -81,9 +81,10 @@ void dpu_encoder_register_frame_event_callback(struct
> > drm_encoder *encoder,
> >   *	Delayed: Block until next trigger can be issued.
> >   * @encoder:	encoder pointer
> >   * @params:	kickoff time parameters
> > + * @async:	true if this is an asynchronous commit
> >   */
> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
> > -		struct dpu_encoder_kickoff_params *params);
> > +		struct dpu_encoder_kickoff_params *params, bool async);
> > 
> >  /**
> >   * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
> > previous
> > @@ -96,8 +97,9 @@ 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);
> > +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
> > 
> >  /**
> >   * 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 985c855796ae..57ad83868766 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
> > *encoder)
> > 
> >  	if (crtc && crtc->state->active) {
> >  		trace_dpu_kms_enc_enable(DRMID(crtc));
> > -		dpu_crtc_commit_kickoff(crtc);
> > +		dpu_crtc_commit_kickoff(crtc, false);
> >  	}
> >  }
> > 
> > @@ -369,7 +369,8 @@ 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);
> > +			dpu_crtc_commit_kickoff(crtc,
> > +
> > state->legacy_cursor_update);
> >  		}
> >  	}
> >  }
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
> > b/drivers/gpu/drm/msm/msm_atomic.c
> > index 2088a20eb270..f5b1256e32b6 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -83,7 +83,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
> > *state)
> >  		kms->funcs->commit(kms, state);
> >  	}
> > 
> > -	msm_atomic_wait_for_commit_done(dev, state);
> > +	if (!state->legacy_cursor_update)
> > +		msm_atomic_wait_for_commit_done(dev, state);
> Was this change tested with MDP5? They have a different path
> to support async updates.
> 

I just realized I didn't respond to this. I haven't tested with MDP5, I don't
have the proper hw. That said, cursor on MDP5 won't work without this change
since it'll ratelimit cursor updates to once-pre-vsync.

Sean

> Thanks,
> Jeykumar S.
> > 
> >  	kms->funcs->complete_commit(kms, state);
> 
> -- 
> Jeykumar S

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

Re: [PATCH v2 3/3] drm/msm: dpu: Make legacy cursor updates asynchronous

[Jeykumar Sankaran]

On 2018-11-16 12:02, Sean Paul wrote:
> On Thu, Nov 08, 2018 at 02:00:51PM -0800, Jeykumar Sankaran wrote:
>> On 2018-10-30 09:00, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > This patch sprinkles a few async/legacy_cursor_update checks
>> > through commit to ensure that cursor updates aren't blocked on vsync.
>> > There are 2 main components to this, the first is that we don't want
> to
>> > wait_for_commit_done in msm_atomic  before returning from
>> > atomic_complete.
>> > The second is that in dpu we don't want to wait for frame_done events
>> > when
>> > updating the cursor.
>> >
>> > Changes in v2:
>> > - None
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 44
> +++++++++++----------
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h    |  3 +-
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++----
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  6 ++-
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     |  5 ++-
>> >  drivers/gpu/drm/msm/msm_atomic.c            |  3 +-
>> >  6 files changed, 49 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > index ed84cf44a222..1e3e57817b72 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> > @@ -702,7 +702,7 @@ static int _dpu_crtc_wait_for_frame_done(struct
>> > drm_crtc *crtc)
>> >  	return rc;
>> >  }
>> >
>> > -void dpu_crtc_commit_kickoff(struct drm_crtc *crtc)
>> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async)
>> >  {
>> >  	struct drm_encoder *encoder;
>> >  	struct drm_device *dev = crtc->dev;
>> > @@ -731,27 +731,30 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > *crtc)
>> >  		 * Encoder will flush/start now, unless it has a tx
>> > pending.
>> >  		 * If so, it may delay and flush at an irq event (e.g.
>> > ppdone)
>> >  		 */
>> > -		dpu_encoder_prepare_for_kickoff(encoder, &params);
>> > +		dpu_encoder_prepare_for_kickoff(encoder, &params, async);
>> >  	}
>> >
>> > -	/* wait for 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 (!async) {
>> > +		/* wait for 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);
>> >
>> > -	dpu_crtc->play_count++;
>> > +		dpu_crtc->play_count++;
>> > +	}
>> >
>> >  	dpu_vbif_clear_errors(dpu_kms);
>> >
>> > @@ -759,11 +762,12 @@ void dpu_crtc_commit_kickoff(struct drm_crtc
>> > *crtc)
>> >  		if (encoder->crtc != crtc)
>> >  			continue;
>> >
>> > -		dpu_encoder_kickoff(encoder);
>> > +		dpu_encoder_kickoff(encoder, async);
>> >  	}
>> >
>> >  end:
>> > -	reinit_completion(&dpu_crtc->frame_done_comp);
>> > +	if (!async)
>> > +		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 4822602402f9..ec633ce3ee6c 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> > @@ -277,8 +277,9 @@ int dpu_crtc_vblank(struct drm_crtc *crtc, bool
> en);
>> >  /**
>> >   * 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);
>> > +void dpu_crtc_commit_kickoff(struct drm_crtc *crtc, bool async);
>> >
>> >  /**
>> >   * 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 82c55efb500f..a8ba10ceaacf 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > @@ -1375,7 +1375,8 @@ static void dpu_encoder_off_work(struct
>> > kthread_work
>> > *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)
>> > +		struct dpu_encoder_phys *phys, uint32_t extra_flush_bits,
>> > +		bool async)
>> >  {
>> >  	struct dpu_hw_ctl *ctl;
>> >  	int pending_kickoff_cnt;
>> > @@ -1398,7 +1399,10 @@ static void _dpu_encoder_trigger_flush(struct
>> > drm_encoder *drm_enc,
>> >  		return;
>> >  	}
>> >
>> > -	pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
>> > +	if (!async)
>> > +		pending_kickoff_cnt = dpu_encoder_phys_inc_pending(phys);
>> > +	else
>> > +		pending_kickoff_cnt =
>> > atomic_read(&phys->pending_kickoff_cnt);
>> >
>> >  	if (extra_flush_bits && ctl->ops.update_pending_flush)
>> >  		ctl->ops.update_pending_flush(ctl, extra_flush_bits);
>> > @@ -1511,7 +1515,8 @@ 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)
>> > +static void _dpu_encoder_kickoff_phys(struct dpu_encoder_virt
> *dpu_enc,
>> > +				      bool async)
>> >  {
>> >  	struct dpu_hw_ctl *ctl;
>> >  	uint32_t i, pending_flush;
>> > @@ -1542,7 +1547,8 @@ static void _dpu_encoder_kickoff_phys(struct
>> > dpu_encoder_virt *dpu_enc)
>> >  			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);
>> > +			_dpu_encoder_trigger_flush(&dpu_enc->base, phys,
>> > 0x0,
>> > +						   async);
>> >  		else if (ctl->ops.get_pending_flush)
>> >  			pending_flush |= ctl->ops.get_pending_flush(ctl);
>> >  	}
>> > @@ -1552,7 +1558,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);
>> > +				pending_flush, async);
>> >  	}
>> >
>> >  	_dpu_encoder_trigger_start(dpu_enc->cur_master);
>> > @@ -1736,7 +1742,7 @@ static void
>> > dpu_encoder_vsync_event_work_handler(struct kthread_work *work)
>> >  }
>> >
>> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *drm_enc,
>> > -		struct dpu_encoder_kickoff_params *params)
>> > +		struct dpu_encoder_kickoff_params *params, bool async)
>> >  {
>> >  	struct dpu_encoder_virt *dpu_enc;
>> >  	struct dpu_encoder_phys *phys;
>> > @@ -1775,7 +1781,7 @@ void dpu_encoder_prepare_for_kickoff(struct
>> > drm_encoder *drm_enc,
>> >  	}
>> >  }
>> >
>> > -void dpu_encoder_kickoff(struct drm_encoder *drm_enc)
>> > +void dpu_encoder_kickoff(struct drm_encoder *drm_enc, bool async)
>> >  {
>> >  	struct dpu_encoder_virt *dpu_enc;
>> >  	struct dpu_encoder_phys *phys;
>> > @@ -1798,7 +1804,7 @@ void dpu_encoder_kickoff(struct drm_encoder
>> > *drm_enc)
>> >  		((atomic_read(&dpu_enc->frame_done_timeout) * HZ) /
>> > 1000));
>> >
>> >  	/* All phys encs are ready to go, trigger the kickoff */
>> > -	_dpu_encoder_kickoff_phys(dpu_enc);
>> > +	_dpu_encoder_kickoff_phys(dpu_enc, async);
>> >
>> >  	/* 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 9dbf38f446d9..c2044122d609 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> > @@ -81,9 +81,10 @@ void
> dpu_encoder_register_frame_event_callback(struct
>> > drm_encoder *encoder,
>> >   *	Delayed: Block until next trigger can be issued.
>> >   * @encoder:	encoder pointer
>> >   * @params:	kickoff time parameters
>> > + * @async:	true if this is an asynchronous commit
>> >   */
>> >  void dpu_encoder_prepare_for_kickoff(struct drm_encoder *encoder,
>> > -		struct dpu_encoder_kickoff_params *params);
>> > +		struct dpu_encoder_kickoff_params *params, bool async);
>> >
>> >  /**
>> >   * dpu_encoder_trigger_kickoff_pending - Clear the flush bits from
>> > previous
>> > @@ -96,8 +97,9 @@ 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);
>> > +void dpu_encoder_kickoff(struct drm_encoder *encoder, bool async);
>> >
>> >  /**
>> >   * 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 985c855796ae..57ad83868766 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> > @@ -352,7 +352,7 @@ void dpu_kms_encoder_enable(struct drm_encoder
>> > *encoder)
>> >
>> >  	if (crtc && crtc->state->active) {
>> >  		trace_dpu_kms_enc_enable(DRMID(crtc));
>> > -		dpu_crtc_commit_kickoff(crtc);
>> > +		dpu_crtc_commit_kickoff(crtc, false);
>> >  	}
>> >  }
>> >
>> > @@ -369,7 +369,8 @@ 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);
>> > +			dpu_crtc_commit_kickoff(crtc,
>> > +
>> > state->legacy_cursor_update);
>> >  		}
>> >  	}
>> >  }
>> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c
>> > b/drivers/gpu/drm/msm/msm_atomic.c
>> > index 2088a20eb270..f5b1256e32b6 100644
>> > --- a/drivers/gpu/drm/msm/msm_atomic.c
>> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> > @@ -83,7 +83,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state
>> > *state)
>> >  		kms->funcs->commit(kms, state);
>> >  	}
>> >
>> > -	msm_atomic_wait_for_commit_done(dev, state);
>> > +	if (!state->legacy_cursor_update)
>> > +		msm_atomic_wait_for_commit_done(dev, state);
>> Was this change tested with MDP5? They have a different path
>> to support async updates.
>> 
> 
> I just realized I didn't respond to this. I haven't tested with MDP5, I
> don't
> have the proper hw. That said, cursor on MDP5 won't work without this
> change
> since it'll ratelimit cursor updates to once-pre-vsync.
> 
> Sean

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

> 
>> Thanks,
>> Jeykumar S.
>> >
>> >  	kms->funcs->complete_commit(kms, state);
>> 
>> --
>> Jeykumar S

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

Re: [PATCH v2 1/3] drm/msm: dpu: Mask inactive pending flushes

[Jeykumar Sankaran]

On 2018-10-30 09:00, Sean Paul wrote:
> From: Sean Paul <seanpaul@chromium.org>
> 
> This patch masks any pending flushes which have not been latched for a
> commit. This will catch the case where an asynchronous update is
> nullified by a disable in the same frame.
> 
> Changes in v2:
> - Added to the set
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> index 8fa601a9abbf..d7a7fedc09f7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> @@ -28,6 +28,7 @@
>  #define   CTL_TOP                       0x014
>  #define   CTL_FLUSH                     0x018
>  #define   CTL_START                     0x01C
> +#define   CTL_FLUSH_MASK                0x090
>  #define   CTL_PREPARE                   0x0d0
>  #define   CTL_SW_RESET                  0x030
>  #define   CTL_LAYER_EXTN_OFFSET         0x40
> @@ -121,6 +122,12 @@ static inline void dpu_hw_ctl_trigger_flush(struct
> dpu_hw_ctl *ctx)
>  {
>  	trace_dpu_hw_ctl_trigger_pending_flush(ctx->pending_flush_mask,
>  				     dpu_hw_ctl_get_flush_register(ctx));
> +
> +	/*
> +	 * Async updates could have changed CTL_FLUSH since it was last
> latched.
> +	 * Mask anything not involved in this latest commit.
> +	 */
> +	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH_MASK, ~ctx->pending_flush_mask);
Do we need this change for adding the current async cursor support?
We are not masking any bit by default. So there is no need for updating 
it here.

The usage of flush_mask is not completely explored yet. Maybe we can add
this register support when we revisit this async logic as we discussed.

Thanks and Regards,
Jeykumar S.
>  	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
>  }

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

Re: [PATCH v2 1/3] drm/msm: dpu: Mask inactive pending flushes

[Sean Paul]

On Thu, Nov 08, 2018 at 01:03:03PM -0800, Jeykumar Sankaran wrote:
> On 2018-10-30 09:00, Sean Paul wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> > 
> > This patch masks any pending flushes which have not been latched for a
> > commit. This will catch the case where an asynchronous update is
> > nullified by a disable in the same frame.
> > 
> > Changes in v2:
> > - Added to the set
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > index 8fa601a9abbf..d7a7fedc09f7 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
> > @@ -28,6 +28,7 @@
> >  #define   CTL_TOP                       0x014
> >  #define   CTL_FLUSH                     0x018
> >  #define   CTL_START                     0x01C
> > +#define   CTL_FLUSH_MASK                0x090
> >  #define   CTL_PREPARE                   0x0d0
> >  #define   CTL_SW_RESET                  0x030
> >  #define   CTL_LAYER_EXTN_OFFSET         0x40
> > @@ -121,6 +122,12 @@ static inline void dpu_hw_ctl_trigger_flush(struct
> > dpu_hw_ctl *ctx)
> >  {
> >  	trace_dpu_hw_ctl_trigger_pending_flush(ctx->pending_flush_mask,
> >  				     dpu_hw_ctl_get_flush_register(ctx));
> > +
> > +	/*
> > +	 * Async updates could have changed CTL_FLUSH since it was last
> > latched.
> > +	 * Mask anything not involved in this latest commit.
> > +	 */
> > +	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH_MASK, ~ctx->pending_flush_mask);
> Do we need this change for adding the current async cursor support?

Hmm, I think you asked me to implement this at the weekly meeting a little
while ago. Apparently HW team requested that we mask off the bits for
planes which have been disabled-but-not-flushed?

Sean

> We are not masking any bit by default. So there is no need for updating it
> here.
> 
> The usage of flush_mask is not completely explored yet. Maybe we can add
> this register support when we revisit this async logic as we discussed.
> 
> Thanks and Regards,
> Jeykumar S.
> >  	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
> >  }
> 
> -- 
> Jeykumar S

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

Re: [PATCH v2 1/3] drm/msm: dpu: Mask inactive pending flushes

[Jeykumar Sankaran]

On 2018-11-08 13:40, Sean Paul wrote:
> On Thu, Nov 08, 2018 at 01:03:03PM -0800, Jeykumar Sankaran wrote:
>> On 2018-10-30 09:00, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > This patch masks any pending flushes which have not been latched for a
>> > commit. This will catch the case where an asynchronous update is
>> > nullified by a disable in the same frame.
>> >
>> > Changes in v2:
>> > - Added to the set
>> >
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> > index 8fa601a9abbf..d7a7fedc09f7 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_ctl.c
>> > @@ -28,6 +28,7 @@
>> >  #define   CTL_TOP                       0x014
>> >  #define   CTL_FLUSH                     0x018
>> >  #define   CTL_START                     0x01C
>> > +#define   CTL_FLUSH_MASK                0x090
>> >  #define   CTL_PREPARE                   0x0d0
>> >  #define   CTL_SW_RESET                  0x030
>> >  #define   CTL_LAYER_EXTN_OFFSET         0x40
>> > @@ -121,6 +122,12 @@ static inline void
> dpu_hw_ctl_trigger_flush(struct
>> > dpu_hw_ctl *ctx)
>> >  {
>> >  	trace_dpu_hw_ctl_trigger_pending_flush(ctx->pending_flush_mask,
>> >  				     dpu_hw_ctl_get_flush_register(ctx));
>> > +
>> > +	/*
>> > +	 * Async updates could have changed CTL_FLUSH since it was last
>> > latched.
>> > +	 * Mask anything not involved in this latest commit.
>> > +	 */
>> > +	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH_MASK, ~ctx->pending_flush_mask);
>> Do we need this change for adding the current async cursor support?
> 
> Hmm, I think you asked me to implement this at the weekly meeting a 
> little
> while ago. Apparently HW team requested that we mask off the bits for
> planes which have been disabled-but-not-flushed?
> 
OK. If you want to implement the HW team recommendation, you should 
block the
FLUSH writes until both FLUSH and FLUSH_MASK writes goes through. We can 
do
that by writing 0xFFFFFFFF to the FLUSH_MASK indicating "hardware is not 
ready"
at the beginnging of the new vsync window. Since async updates dont wait
for commit_done (vsync), we can do that only for sync commits. Once we 
are
done programming all the registers and the final flush bits are ready, 
the
order of writing has to be reversed by writing FLUSH first and then 
FLUSH_MASK
to the inverse of FLUSH to unblock the hardware programming on vsync.

Still, there is a small window of error where vsync can happen between 
FLUSH
and FLUSH_MASK writes where we will end up missing the vsync but no 
partial
frame registers will be programmed.

I believe we have decided to try out this approach with a fresh set of 
patches
and let the current cursor support get in as such. In that case, we can 
drop
this patch from this series.

Thanks,
Jeykumar S.

> Sean
> 
>> We are not masking any bit by default. So there is no need for 
>> updating
> it
>> here.
>> 
>> The usage of flush_mask is not completely explored yet. Maybe we can 
>> add
>> this register support when we revisit this async logic as we 
>> discussed.
>> 
>> Thanks and Regards,
>> Jeykumar S.
>> >  	DPU_REG_WRITE(&ctx->hw, CTL_FLUSH, ctx->pending_flush_mask);
>> >  }
>> 
>> --
>> Jeykumar S

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