diff mbox

[2/7] drm/vc4: Fix vblank handling

Message ID 1496392332-8722-3-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON June 2, 2017, 8:32 a.m. UTC
There are two problems related to VBLANK handling in the current CRTC
driver:

* VBLANK events are missed when the CRTC is being disabled because the
  driver does not wait till the end of the frame before stopping the
  HVS and PV blocks. In this case, we should explicitly issue a VBLANK
  event if there's one waiting
* when we are enabling a CRTC, drm_crtc_vblank_get() is called before
  drm_crtc_vblank_on(), which is not supposed to happen (hence the
  WARN_ON() in the code). To solve the problem, we delay the 'update
  display list' operation after the CRTC is actually enabled.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c | 80 ++++++++++++++++++++++++++++++------------
 1 file changed, 57 insertions(+), 23 deletions(-)

Comments

Eric Anholt June 15, 2017, 11:30 p.m. UTC | #1
Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> There are two problems related to VBLANK handling in the current CRTC
> driver:
>
> * VBLANK events are missed when the CRTC is being disabled because the
>   driver does not wait till the end of the frame before stopping the
>   HVS and PV blocks. In this case, we should explicitly issue a VBLANK
>   event if there's one waiting
> * when we are enabling a CRTC, drm_crtc_vblank_get() is called before
>   drm_crtc_vblank_on(), which is not supposed to happen (hence the
>   WARN_ON() in the code). To solve the problem, we delay the 'update
>   display list' operation after the CRTC is actually enabled.

Does drm_crtc_vblank_get() actually fail when drm_crtc_vblank_on()
hasn't been called?  The code is a bit twisty, but it looked like we
would track the get refcount and then turn on the interrupt once
drm_crtc_vblank_on() was called.

The first change of the two seems straightforward and good, though (and
possible to separate, right?).
Boris BREZILLON June 16, 2017, 7:47 a.m. UTC | #2
On Thu, 15 Jun 2017 16:30:58 -0700
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > There are two problems related to VBLANK handling in the current CRTC
> > driver:
> >
> > * VBLANK events are missed when the CRTC is being disabled because the
> >   driver does not wait till the end of the frame before stopping the
> >   HVS and PV blocks. In this case, we should explicitly issue a VBLANK
> >   event if there's one waiting
> > * when we are enabling a CRTC, drm_crtc_vblank_get() is called before
> >   drm_crtc_vblank_on(), which is not supposed to happen (hence the
> >   WARN_ON() in the code). To solve the problem, we delay the 'update
> >   display list' operation after the CRTC is actually enabled.  
> 
> Does drm_crtc_vblank_get() actually fail when drm_crtc_vblank_on()
> hasn't been called?  The code is a bit twisty, but it looked like we
> would track the get refcount and then turn on the interrupt once
> drm_crtc_vblank_on() was called.

I thought I had faced the WARN_ON() backtrace while developing the TXP
block, but I can't reproduce it (even after reverting these changes),
so it was probably caused by something else.

> 
> The first change of the two seems straightforward and good, though (and
> possible to separate, right?).

I'll prepare a new patch containing only the first change.
diff mbox

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index d86c8cce3182..df1d81533076 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -528,6 +528,47 @@  static void vc4_crtc_disable(struct drm_crtc *crtc)
 	WARN_ON_ONCE((HVS_READ(SCALER_DISPSTATX(chan)) &
 		      (SCALER_DISPSTATX_FULL | SCALER_DISPSTATX_EMPTY)) !=
 		     SCALER_DISPSTATX_EMPTY);
+
+	/*
+	 * Make sure we issue a vblank event after disabling the CRTC if
+	 * someone was waiting it.
+	 */
+	if (crtc->state->event) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&dev->event_lock, flags);
+		drm_crtc_send_vblank_event(crtc, crtc->state->event);
+		crtc->state->event = NULL;
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+	}
+}
+
+static void vc4_crtc_update_dlist(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
+
+	if (crtc->state->event) {
+		unsigned long flags;
+
+		crtc->state->event->pipe = drm_crtc_index(crtc);
+
+		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
+
+		spin_lock_irqsave(&dev->event_lock, flags);
+		vc4_crtc->event = crtc->state->event;
+		crtc->state->event = NULL;
+
+		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
+			  vc4_state->mm.start);
+
+		spin_unlock_irqrestore(&dev->event_lock, flags);
+	} else {
+		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
+			  vc4_state->mm.start);
+	}
 }
 
 static void vc4_crtc_enable(struct drm_crtc *crtc)
@@ -540,6 +581,12 @@  static void vc4_crtc_enable(struct drm_crtc *crtc)
 
 	require_hvs_enabled(dev);
 
+	/* Enable vblank irq handling before crtc is started otherwise
+	 * drm_crtc_get_vblank() fails in vc4_crtc_update_dlist().
+	 */
+	drm_crtc_vblank_on(crtc);
+	vc4_crtc_update_dlist(crtc);
+
 	/* Turn on the scaler, which will wait for vstart to start
 	 * compositing.
 	 */
@@ -551,9 +598,6 @@  static void vc4_crtc_enable(struct drm_crtc *crtc)
 	/* Turn on the pixel valve, which will emit the vstart signal. */
 	CRTC_WRITE(PV_V_CONTROL,
 		   CRTC_READ(PV_V_CONTROL) | PV_VCONTROL_VIDEN);
-
-	/* Enable vblank irq handling after crtc is started. */
-	drm_crtc_vblank_on(crtc);
 }
 
 static bool vc4_crtc_mode_fixup(struct drm_crtc *crtc,
@@ -608,7 +652,6 @@  static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
 {
 	struct drm_device *dev = crtc->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
 	struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
 	struct drm_plane *plane;
 	bool debug_dump_regs = false;
@@ -630,25 +673,16 @@  static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
 
 	WARN_ON_ONCE(dlist_next - dlist_start != vc4_state->mm.size);
 
-	if (crtc->state->event) {
-		unsigned long flags;
-
-		crtc->state->event->pipe = drm_crtc_index(crtc);
-
-		WARN_ON(drm_crtc_vblank_get(crtc) != 0);
-
-		spin_lock_irqsave(&dev->event_lock, flags);
-		vc4_crtc->event = crtc->state->event;
-		crtc->state->event = NULL;
-
-		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
-			  vc4_state->mm.start);
-
-		spin_unlock_irqrestore(&dev->event_lock, flags);
-	} else {
-		HVS_WRITE(SCALER_DISPLISTX(vc4_crtc->channel),
-			  vc4_state->mm.start);
-	}
+	/*
+	 * Only update DISPLIST if the CRTC was already running and is not
+	 * being disabled.
+	 * vc4_crtc_enable() takes care of updating the dlist just after
+	 * re-enabling VBLANK interrupts and before enabling the engine.
+	 * If the CRTC is being disabled, there's no point in updating this
+	 * information.
+	 */
+	if (crtc->state->active && old_state->active)
+		vc4_crtc_update_dlist(crtc);
 
 	if (debug_dump_regs) {
 		DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc));