[PATCH v2 0/2] drm: rcar-du: Repair vblank event handling

[PATCH v2 0/2] drm: rcar-du: Repair vblank event handling

[Laurent Pinchart]

Hello,

The recent changes to the rcar-du driver to fix a page flip handling race
condition changed the order of which vblanks and page flips are handled,
resulting in incorrect timestamps being reported in the vblan events.

Correct this by handling vblank events in the same completion handler as page
flips. This removes the need for the IRQ handler on DU instances which are
sourced by a VSP.

Compared to v1,

- Patch 1/3 replaces patch 1/2 to use the VBK interrupt instead of the FRM
  interrupt when not using the VSP
- The new patch 2/3 simplifies plane to CRTC assignment when using the VSP to
  prepare for patch 3/3
- Patch 3/3 doesn't enable the VBK interrupt when using the VSP

Kieran Bingham (1):
  drm: rcar-du: Repair vblank for DRM page flips using the VSP

Laurent Pinchart (2):
  drm: rcar-du: Use the VBK interrupt for vblank events
  drm: rcar-du: Fix planes to CRTC assignment when using the VSP

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c   | 58 +++++++++++++++++++-------------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h   |  2 ++
 drivers/gpu/drm/rcar-du/rcar_du_group.c  | 12 +++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c    | 28 +++++++++------
 drivers/gpu/drm/rcar-du/rcar_du_plane.c  | 10 +-----
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c    | 17 ++++------
 drivers/media/platform/vsp1/vsp1_drm.c   |  5 +--
 drivers/media/platform/vsp1/vsp1_drm.h   |  2 +-
 drivers/media/platform/vsp1/vsp1_pipe.c  | 20 +++++------
 drivers/media/platform/vsp1/vsp1_pipe.h  |  2 +-
 drivers/media/platform/vsp1/vsp1_video.c |  6 +++-
 include/media/vsp1.h                     |  2 +-
 12 files changed, 94 insertions(+), 70 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/3] drm: rcar-du: Use the VBK interrupt for vblank events

[Laurent Pinchart]

When implementing support for interlaced modes, the driver switched from
reporting vblank events on the vertical blanking (VBK) interrupt to the
frame end interrupt (FRM). This incorrectly divided the reported refresh
rate by two. Fix it by moving back to the VBK interrupt.

Fixes: 906eff7fcada ("drm: rcar-du: Implement support for interlaced modes")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 98cf446391dc..17fd1cd5212c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -698,7 +698,7 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 	status = rcar_du_crtc_read(rcrtc, DSSR);
 	rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
 
-	if (status & DSSR_FRM) {
+	if (status & DSSR_VBK) {
 		drm_crtc_handle_vblank(&rcrtc->crtc);
 
 		if (rcdu->info->gen < 3)
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

[Laurent Pinchart]

The DU can compose the output of a VSP with other planes on Gen2
hardware, and of two VSPs on Gen3 hardware. Neither of these features
are supported by the driver, and the current implementation always
assigns planes to CRTCs the same way.

Simplify the implementation by configuring plane assignment when setting
up DU groups, instead of recomputing it for every atomic plane update.
This allows skipping the wait for vertical blanking when stopping a
CRTC, as there's no need to reconfigure plane assignment at that point.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ++++++++++++++++---------------
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 ++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +++++++++++++++++-----------
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +---------
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 ---------
 5 files changed, 46 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 17fd1cd5212c..413ab032afed 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -315,6 +315,10 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
 	unsigned int i;
 	u32 dspr = 0;
 
+	/* Plane assignment is fixed when using the VSP. */
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
+		return;
+
 	for (i = 0; i < rcrtc->group->num_planes; ++i) {
 		struct rcar_du_plane *plane = &rcrtc->group->planes[i];
 		unsigned int j;
@@ -351,17 +355,6 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
 		}
 	}
 
-	/* If VSP+DU integration is enabled the plane assignment is fixed. */
-	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
-		if (rcdu->info->gen < 3) {
-			dspr = (rcrtc->index % 2) + 1;
-			hwplanes = 1 << (rcrtc->index % 2);
-		} else {
-			dspr = (rcrtc->index % 2) ? 3 : 1;
-			hwplanes = 1 << ((rcrtc->index % 2) ? 2 : 0);
-		}
-	}
-
 	/*
 	 * Update the planes to display timing and dot clock generator
 	 * associations.
@@ -462,8 +455,13 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 	rcar_du_crtc_set_display_timing(rcrtc);
 	rcar_du_group_set_routing(rcrtc->group);
 
-	/* Start with all planes disabled. */
-	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
+	/*
+	 * Start with all planes disabled, except when using the VSP in which
+	 * case the fixed plane assignment must not be modified.
+	 */
+	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+		rcar_du_group_write(rcrtc->group,
+				    rcrtc->index % 2 ? DS2PR : DS1PR, 0);
 
 	/* Enable the VSP compositor. */
 	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
@@ -505,8 +503,11 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
 	 * are stopped in one operation as we now wait for one vblank per CRTC.
 	 * Whether this can be improved needs to be researched.
 	 */
-	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
-	drm_crtc_wait_one_vblank(crtc);
+	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+		rcar_du_group_write(rcrtc->group,
+				    rcrtc->index % 2 ? DS2PR : DS1PR, 0);
+		drm_crtc_wait_one_vblank(crtc);
+	}
 
 	/*
 	 * Disable vertical blanking interrupt reporting. We first need to wait
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 00d5f470d377..d26b647207b8 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 	if (rcdu->info->gen >= 3)
 		rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
 
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+		/*
+		 * The CRTCs can compose the output of a VSP with other planes
+		 * on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
+		 * of these features are supported by the driver, so we hardcode
+		 * plane assignment to CRTCs when setting the group up to avoid
+		 * the need to restart then group when setting planes up.
+		 */
+		rcar_du_group_write(rgrp, DS1PR, 1);
+		rcar_du_group_write(rgrp, DS2PR, rcdu->info->gen >= 3 ? 3 : 2);
+	}
+
 	/*
 	 * Use DS1PR and DS2PR to configure planes priorities and connects the
 	 * superposition 0 to DU0 pins. DU1 pins will be configured dynamically.
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 0e4e839afc97..13186a5684f1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -562,17 +562,23 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 		rgrp->index = i;
 		rgrp->num_crtcs = min(rcdu->num_crtcs - 2 * i, 2U);
 
-		/*
-		 * If we have more than one CRTCs in this group pre-associate
-		 * the low-order planes with CRTC 0 and the high-order planes
-		 * with CRTC 1 to minimize flicker occurring when the
-		 * association is changed.
-		 */
-		rgrp->dptsr_planes = rgrp->num_crtcs > 1
-				   ? (rcdu->info->gen >= 3 ? 0x04 : 0xf0)
-				   : 0;
-
-		if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+		if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+			/*
+			 * When using the VSP plane assignment to CRTCs is
+			 * fixed. The first VSP is connected to plane 1, and the
+			 * second VSP to plane 2 on Gen2 hardware and to plane 3
+			 * on Gen3 hardware.
+			 */
+			rgrp->dptsr_planes = rgrp->num_crtcs > 1
+					   ? (rcdu->info->gen >= 3 ? 4 : 2)
+					   : 0;
+		} else {
+			/*
+			 * Pre-associate the planes with the CRTCs if we have
+			 * more than one CRTC in this group to minimize flicker
+			 * when plane association is changed.
+			 */
+			rgrp->dptsr_planes = rgrp->num_crtcs > 1 ? 0xf0 : 0x00;
 			ret = rcar_du_planes_init(rgrp);
 			if (ret < 0)
 				return ret;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index b0040478a3db..787f036b18fb 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -548,17 +548,9 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
 		rcar_du_plane_setup_format(rgrp, (state->hwindex + 1) % 8,
 					   state);
 
+	/* On Gen3 planes have no scanout data. */
 	if (rcdu->info->gen < 3)
 		rcar_du_plane_setup_scanout(rgrp, state);
-
-	if (state->source == RCAR_DU_PLANE_VSPD1) {
-		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
-
-		if (rcdu->vspd1_sink != vspd1_sink) {
-			rcdu->vspd1_sink = vspd1_sink;
-			rcar_du_set_dpad0_vsp1_routing(rcdu);
-		}
-	}
 }
 
 static int rcar_du_plane_atomic_check(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index e43b065e141a..dba150a20f3d 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -74,15 +74,6 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 
 	__rcar_du_plane_setup(crtc->group, &state);
 
-	/*
-	 * Ensure that the plane source configuration takes effect by requesting
-	 * a restart of the group. See rcar_du_plane_atomic_update() for a more
-	 * detailed explanation.
-	 *
-	 * TODO: Check whether this is still needed on Gen3.
-	 */
-	crtc->group->need_restart = true;
-
 	vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
 }
 
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/3] drm: rcar-du: Repair vblank for DRM page flips using the VSP

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

The driver recently switched from handling page flip completion in the
DU vertical blanking handler to the VSP frame end handler to fix a race
condition. This unfortunately resulted in incorrect timestamps in the
vertical blanking events sent to userspace as vertical blanking is now
handled after sending the event.

To fix this we must reverse the order of the two operations. The easiest
way is to handle vertical blanking in the VSP frame end handler before
sending the event. The VSP frame end interrupt occurs approximately 50µs
earlier than the DU frame end interrupt, but this should not cause any
undue harm.

As we need to handle vertical blanking even when page flip completion is
delayed, the VSP driver now needs to call the frame end completion
callback unconditionally, with a new argument to report whether page
flip has completed.

With this new scheme the DU vertical blanking interrupt isn't needed
anymore, so we can stop enabling it.

Fixes: d503a43ac06a ("drm: rcar-du: Register a completion callback with VSP1")
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c   | 25 ++++++++++++++++++-------
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h   |  2 ++
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c    |  8 ++++++--
 drivers/media/platform/vsp1/vsp1_drm.c   |  5 +++--
 drivers/media/platform/vsp1/vsp1_drm.h   |  2 +-
 drivers/media/platform/vsp1/vsp1_pipe.c  | 20 ++++++++++----------
 drivers/media/platform/vsp1/vsp1_pipe.h  |  2 +-
 drivers/media/platform/vsp1/vsp1_video.c |  6 +++++-
 include/media/vsp1.h                     |  2 +-
 9 files changed, 47 insertions(+), 25 deletions(-)

Changes compared to v1:

- Don't enable the VBK interrupt when using the VSP

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 413ab032afed..db08c893bdc6 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -661,8 +661,19 @@ static int rcar_du_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
-	rcar_du_crtc_write(rcrtc, DSRCR, DSRCR_VBCL);
-	rcar_du_crtc_set(rcrtc, DIER, DIER_VBE);
+	/*
+	 * When using the VSP vertical blanking is reported from the VSP frame
+	 * completion handler right before sending the vblank event that signals
+	 * page flip completion, to avoid incorrect vblank event timestamps.
+	 * There is thus no need to enable the vertical blanking interrupt in
+	 * that case.
+	 */
+	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
+		rcar_du_crtc_write(rcrtc, DSRCR, DSRCR_VBCL);
+		rcar_du_crtc_set(rcrtc, DIER, DIER_VBE);
+	}
+
+	rcrtc->vblank_enable = true;
 
 	return 0;
 }
@@ -671,7 +682,10 @@ static void rcar_du_crtc_disable_vblank(struct drm_crtc *crtc)
 {
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 
-	rcar_du_crtc_clr(rcrtc, DIER, DIER_VBE);
+	rcrtc->vblank_enable = false;
+
+	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
+		rcar_du_crtc_clr(rcrtc, DIER, DIER_VBE);
 }
 
 static const struct drm_crtc_funcs crtc_funcs = {
@@ -692,7 +706,6 @@ static const struct drm_crtc_funcs crtc_funcs = {
 static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 {
 	struct rcar_du_crtc *rcrtc = arg;
-	struct rcar_du_device *rcdu = rcrtc->group->dev;
 	irqreturn_t ret = IRQ_NONE;
 	u32 status;
 
@@ -701,9 +714,7 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 
 	if (status & DSSR_VBK) {
 		drm_crtc_handle_vblank(&rcrtc->crtc);
-
-		if (rcdu->info->gen < 3)
-			rcar_du_crtc_finish_page_flip(rcrtc);
+		rcar_du_crtc_finish_page_flip(rcrtc);
 
 		ret = IRQ_HANDLED;
 	}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 3cc376826592..6cc4d13b5458 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -31,6 +31,7 @@ struct rcar_du_vsp;
  * @mmio_offset: offset of the CRTC registers in the DU MMIO block
  * @index: CRTC software and hardware index
  * @initialized: whether the CRTC has been initialized and clocks enabled
+ * @vblank_enable: whether vblank events are enabled on this CRTC
  * @event: event to post when the pending page flip completes
  * @flip_wait: wait queue used to signal page flip completion
  * @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
@@ -47,6 +48,7 @@ struct rcar_du_crtc {
 	unsigned int index;
 	bool initialized;
 
+	bool vblank_enable;
 	struct drm_pending_vblank_event *event;
 	wait_queue_head_t flip_wait;
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index dba150a20f3d..db19cc685e78 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -31,11 +31,15 @@
 #include "rcar_du_kms.h"
 #include "rcar_du_vsp.h"
 
-static void rcar_du_vsp_complete(void *private)
+static void rcar_du_vsp_complete(void *private, bool completed)
 {
 	struct rcar_du_crtc *crtc = private;
 
-	rcar_du_crtc_finish_page_flip(crtc);
+	if (crtc->vblank_enable)
+		drm_crtc_handle_vblank(&crtc->crtc);
+
+	if (completed)
+		rcar_du_crtc_finish_page_flip(crtc);
 }
 
 void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 7791d7b5a743..4dfbeac8f42c 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -32,12 +32,13 @@
  * Interrupt Handling
  */
 
-static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
+static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe,
+				       bool completed)
 {
 	struct vsp1_drm_pipeline *drm_pipe = to_vsp1_drm_pipeline(pipe);
 
 	if (drm_pipe->du_complete)
-		drm_pipe->du_complete(drm_pipe->du_private);
+		drm_pipe->du_complete(drm_pipe->du_private, completed);
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
index fca553ddd184..1cd9db785bf7 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -29,7 +29,7 @@ struct vsp1_drm_pipeline {
 	bool enabled;
 
 	/* Frame synchronisation */
-	void (*du_complete)(void *);
+	void (*du_complete)(void *, bool);
 	void *du_private;
 };
 
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 9bb961298af2..4f4b732df84b 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -335,16 +335,12 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
 	if (pipe == NULL)
 		return;
 
+	/*
+	 * If the DL commit raced with the frame end interrupt, the commit ends
+	 * up being postponed by one frame. @completed represents whether the
+	 * active frame was finished or postponed.
+	 */
 	completed = vsp1_dlm_irq_frame_end(pipe->output->dlm);
-	if (!completed) {
-		/*
-		 * If the DL commit raced with the frame end interrupt, the
-		 * commit ends up being postponed by one frame. Return
-		 * immediately without calling the pipeline's frame end handler
-		 * or incrementing the sequence number.
-		 */
-		return;
-	}
 
 	if (pipe->hgo)
 		vsp1_hgo_frame_end(pipe->hgo);
@@ -352,8 +348,12 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
 	if (pipe->hgt)
 		vsp1_hgt_frame_end(pipe->hgt);
 
+	/*
+	 * Regardless of frame completion we still need to notify the pipe
+	 * frame_end to account for vblank events.
+	 */
 	if (pipe->frame_end)
-		pipe->frame_end(pipe);
+		pipe->frame_end(pipe, completed);
 
 	pipe->sequence++;
 }
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index 91a784a13422..c5d01a365370 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -91,7 +91,7 @@ struct vsp1_pipeline {
 	enum vsp1_pipeline_state state;
 	wait_queue_head_t wq;
 
-	void (*frame_end)(struct vsp1_pipeline *pipe);
+	void (*frame_end)(struct vsp1_pipeline *pipe, bool completed);
 
 	struct mutex lock;
 	struct kref kref;
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 84139affb871..e9f5dcb8fae5 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -440,13 +440,17 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 	vsp1_pipeline_run(pipe);
 }
 
-static void vsp1_video_pipeline_frame_end(struct vsp1_pipeline *pipe)
+static void vsp1_video_pipeline_frame_end(struct vsp1_pipeline *pipe,
+					  bool completed)
 {
 	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
 	enum vsp1_pipeline_state state;
 	unsigned long flags;
 	unsigned int i;
 
+	/* M2M Pipelines should never call here with an incomplete frame. */
+	WARN_ON_ONCE(!completed);
+
 	spin_lock_irqsave(&pipe->irqlock, flags);
 
 	/* Complete buffers on all video nodes. */
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index c8fc868fb0f2..68a8abe4fac5 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -34,7 +34,7 @@ struct vsp1_du_lif_config {
 	unsigned int width;
 	unsigned int height;
 
-	void (*callback)(void *);
+	void (*callback)(void *, bool);
 	void *callback_data;
 };
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

[Kieran Bingham]

Hi Laurent,

Thanks for the patch

Only a minor nit on one comment, but aside from that,

On 11/07/17 23:29, Laurent Pinchart wrote:
> The DU can compose the output of a VSP with other planes on Gen2
> hardware, and of two VSPs on Gen3 hardware. Neither of these features
> are supported by the driver, and the current implementation always
> assigns planes to CRTCs the same way.
> 
> Simplify the implementation by configuring plane assignment when setting
> up DU groups, instead of recomputing it for every atomic plane update.
> This allows skipping the wait for vertical blanking when stopping a
> CRTC, as there's no need to reconfigure plane assignment at that point.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ++++++++++++++++---------------
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 ++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +++++++++++++++++-----------
>  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +---------
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 ---------
>  5 files changed, 46 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 17fd1cd5212c..413ab032afed 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -315,6 +315,10 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
>  	unsigned int i;
>  	u32 dspr = 0;
>  
> +	/* Plane assignment is fixed when using the VSP. */
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
> +		return;
> +
>  	for (i = 0; i < rcrtc->group->num_planes; ++i) {
>  		struct rcar_du_plane *plane = &rcrtc->group->planes[i];
>  		unsigned int j;
> @@ -351,17 +355,6 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
>  		}
>  	}
>  
> -	/* If VSP+DU integration is enabled the plane assignment is fixed. */
> -	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> -		if (rcdu->info->gen < 3) {
> -			dspr = (rcrtc->index % 2) + 1;
> -			hwplanes = 1 << (rcrtc->index % 2);
> -		} else {
> -			dspr = (rcrtc->index % 2) ? 3 : 1;
> -			hwplanes = 1 << ((rcrtc->index % 2) ? 2 : 0);
> -		}
> -	}
> -
>  	/*
>  	 * Update the planes to display timing and dot clock generator
>  	 * associations.
> @@ -462,8 +455,13 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  	rcar_du_crtc_set_display_timing(rcrtc);
>  	rcar_du_group_set_routing(rcrtc->group);
>  
> -	/* Start with all planes disabled. */
> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> +	/*
> +	 * Start with all planes disabled, except when using the VSP in which
> +	 * case the fixed plane assignment must not be modified.
> +	 */
> +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> +		rcar_du_group_write(rcrtc->group,
> +				    rcrtc->index % 2 ? DS2PR : DS1PR, 0);
>  
>  	/* Enable the VSP compositor. */
>  	if (rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
> @@ -505,8 +503,11 @@ static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc)
>  	 * are stopped in one operation as we now wait for one vblank per CRTC.
>  	 * Whether this can be improved needs to be researched.
>  	 */
> -	rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> -	drm_crtc_wait_one_vblank(crtc);
> +	if (!rcar_du_has(rcrtc->group->dev, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> +		rcar_du_group_write(rcrtc->group,
> +				    rcrtc->index % 2 ? DS2PR : DS1PR, 0);
> +		drm_crtc_wait_one_vblank(crtc);
> +	}
>  
>  	/*
>  	 * Disable vertical blanking interrupt reporting. We first need to wait
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 00d5f470d377..d26b647207b8 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  	if (rcdu->info->gen >= 3)
>  		rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | DEFR10_DEFE10);
>  
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> +		/*
> +		 * The CRTCs can compose the output of a VSP with other planes
> +		 * on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
> +		 * of these features are supported by the driver, so we hardcode
> +		 * plane assignment to CRTCs when setting the group up to avoid
> +		 * the need to restart then group when setting planes up.

Minor nits in comment:

  /restart then group/restart the group/

I would also possibly swap the final 'planes up' as 'up planes' if you update
here anyway:

* so we hardcode plane assignment to CRTCs when setting the group up to avoid
* the need to restart the group when setting up planes.

Up to you of course :)


> +		 */
> +		rcar_du_group_write(rgrp, DS1PR, 1);
> +		rcar_du_group_write(rgrp, DS2PR, rcdu->info->gen >= 3 ? 3 : 2);

whew ... that DS2PR indexing change from g2 to g3 looks annoying ... I had to
write out the logic tables on paper to verify the change here from the previous
code.

> +	}
> +
>  	/*
>  	 * Use DS1PR and DS2PR to configure planes priorities and connects the
>  	 * superposition 0 to DU0 pins. DU1 pins will be configured dynamically.
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index 0e4e839afc97..13186a5684f1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -562,17 +562,23 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  		rgrp->index = i;
>  		rgrp->num_crtcs = min(rcdu->num_crtcs - 2 * i, 2U);
>  
> -		/*
> -		 * If we have more than one CRTCs in this group pre-associate
> -		 * the low-order planes with CRTC 0 and the high-order planes
> -		 * with CRTC 1 to minimize flicker occurring when the
> -		 * association is changed.
> -		 */
> -		rgrp->dptsr_planes = rgrp->num_crtcs > 1
> -				   ? (rcdu->info->gen >= 3 ? 0x04 : 0xf0)
> -				   : 0;
> -
> -		if (!rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> +		if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> +			/*
> +			 * When using the VSP plane assignment to CRTCs is
> +			 * fixed. The first VSP is connected to plane 1, and the
> +			 * second VSP to plane 2 on Gen2 hardware and to plane 3
> +			 * on Gen3 hardware.
> +			 */
> +			rgrp->dptsr_planes = rgrp->num_crtcs > 1
> +					   ? (rcdu->info->gen >= 3 ? 4 : 2)
> +					   : 0;
> +		} else {
> +			/*
> +			 * Pre-associate the planes with the CRTCs if we have
> +			 * more than one CRTC in this group to minimize flicker
> +			 * when plane association is changed.
> +			 */
> +			rgrp->dptsr_planes = rgrp->num_crtcs > 1 ? 0xf0 : 0x00;
>  			ret = rcar_du_planes_init(rgrp);
>  			if (ret < 0)
>  				return ret;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> index b0040478a3db..787f036b18fb 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
> @@ -548,17 +548,9 @@ void __rcar_du_plane_setup(struct rcar_du_group *rgrp,
>  		rcar_du_plane_setup_format(rgrp, (state->hwindex + 1) % 8,
>  					   state);
>  
> +	/* On Gen3 planes have no scanout data. */
>  	if (rcdu->info->gen < 3)
>  		rcar_du_plane_setup_scanout(rgrp, state);
> -
> -	if (state->source == RCAR_DU_PLANE_VSPD1) {
> -		unsigned int vspd1_sink = rgrp->index ? 2 : 0;
> -
> -		if (rcdu->vspd1_sink != vspd1_sink) {
> -			rcdu->vspd1_sink = vspd1_sink;
> -			rcar_du_set_dpad0_vsp1_routing(rcdu);
> -		}
> -	}
>  }
>  
>  static int rcar_du_plane_atomic_check(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index e43b065e141a..dba150a20f3d 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -74,15 +74,6 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  
>  	__rcar_du_plane_setup(crtc->group, &state);
>  
> -	/*
> -	 * Ensure that the plane source configuration takes effect by requesting
> -	 * a restart of the group. See rcar_du_plane_atomic_update() for a more
> -	 * detailed explanation.
> -	 *
> -	 * TODO: Check whether this is still needed on Gen3.
> -	 */
> -	crtc->group->need_restart = true;
> -
>  	vsp1_du_setup_lif(crtc->vsp->vsp, crtc->vsp_pipe, &cfg);
>  }
>  
> 

Re: [PATCH v2 1/3] drm: rcar-du: Use the VBK interrupt for vblank events

[Kieran Bingham]

Hi Laurent,

On 11/07/17 23:29, Laurent Pinchart wrote:
> When implementing support for interlaced modes, the driver switched from
> reporting vblank events on the vertical blanking (VBK) interrupt to the
> frame end interrupt (FRM). This incorrectly divided the reported refresh
> rate by two. Fix it by moving back to the VBK interrupt.
> 
> Fixes: 906eff7fcada ("drm: rcar-du: Implement support for interlaced modes")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Of course, this looks much more correct than the patch I submitted :-)

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 98cf446391dc..17fd1cd5212c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -698,7 +698,7 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>  	status = rcar_du_crtc_read(rcrtc, DSSR);
>  	rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
>  
> -	if (status & DSSR_FRM) {
> +	if (status & DSSR_VBK) {
>  		drm_crtc_handle_vblank(&rcrtc->crtc);
>  
>  		if (rcdu->info->gen < 3)
> 

Re: [PATCH v2 2/3] drm: rcar-du: Fix planes to CRTC assignment when using the VSP

[Laurent Pinchart]

Hi Kieran,

On Wednesday 12 Jul 2017 11:30:19 Kieran Bingham wrote:
> On 11/07/17 23:29, Laurent Pinchart wrote:
> > The DU can compose the output of a VSP with other planes on Gen2
> > hardware, and of two VSPs on Gen3 hardware. Neither of these features
> > are supported by the driver, and the current implementation always
> > assigns planes to CRTCs the same way.
> > 
> > Simplify the implementation by configuring plane assignment when setting
> > up DU groups, instead of recomputing it for every atomic plane update.
> > This allows skipping the wait for vertical blanking when stopping a
> > CRTC, as there's no need to reconfigure plane assignment at that point.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 31 ++++++++++++++-------------
> >  drivers/gpu/drm/rcar-du/rcar_du_group.c | 12 ++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 28 +++++++++++++++++-----------
> >  drivers/gpu/drm/rcar-du/rcar_du_plane.c | 10 +---------
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 ---------
> >  5 files changed, 46 insertions(+), 44 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_group.c index
> > 00d5f470d377..d26b647207b8 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> > @@ -126,6 +126,18 @@ static void rcar_du_group_setup(struct rcar_du_group
> > *rgrp)> 
> >  	if (rcdu->info->gen >= 3)
> >  		rcar_du_group_write(rgrp, DEFR10, DEFR10_CODE | 
DEFR10_DEFE10);
> > 
> > +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE)) {
> > +		/*
> > +		 * The CRTCs can compose the output of a VSP with other planes
> > +		 * on Gen2 hardware, and of two VSPs on Gen3 hardware. Neither
> > +		 * of these features are supported by the driver, so we 
hardcode
> > +		 * plane assignment to CRTCs when setting the group up to 
avoid
> > +		 * the need to restart then group when setting planes up.
> 
> Minor nits in comment:
> 
>   /restart then group/restart the group/
> 
> I would also possibly swap the final 'planes up' as 'up planes' if you
> update here anyway:
> 
> * so we hardcode plane assignment to CRTCs when setting the group up to
> avoid
> * the need to restart the group when setting up planes.
> 
> Up to you of course :)

Thanks, I've fixed both, and also replaced "setting the group up" with 
"setting up the group".

> > +		 */
> > +		rcar_du_group_write(rgrp, DS1PR, 1);
> > +		rcar_du_group_write(rgrp, DS2PR, rcdu->info->gen >= 3 ? 3 : 
2);
> 
> whew ... that DS2PR indexing change from g2 to g3 looks annoying ... I had
> to write out the logic tables on paper to verify the change here from the
> previous code.

That's also how I wrote the code :-)

> > +	}
> > +
> > 
> >  	/*
> >  	
> >  	 * Use DS1PR and DS2PR to configure planes priorities and connects the
> >  	 * superposition 0 to DU0 pins. DU1 pins will be configured 
dynamically.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] drm: rcar-du: Repair vblank for DRM page flips using the VSP

[Mauro Carvalho Chehab]

Em Wed, 12 Jul 2017 01:29:42 +0300
Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> escreveu:

> From: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> The driver recently switched from handling page flip completion in the
> DU vertical blanking handler to the VSP frame end handler to fix a race
> condition. This unfortunately resulted in incorrect timestamps in the
> vertical blanking events sent to userspace as vertical blanking is now
> handled after sending the event.
> 
> To fix this we must reverse the order of the two operations. The easiest
> way is to handle vertical blanking in the VSP frame end handler before
> sending the event. The VSP frame end interrupt occurs approximately 50µs
> earlier than the DU frame end interrupt, but this should not cause any
> undue harm.
> 
> As we need to handle vertical blanking even when page flip completion is
> delayed, the VSP driver now needs to call the frame end completion
> callback unconditionally, with a new argument to report whether page
> flip has completed.
> 
> With this new scheme the DU vertical blanking interrupt isn't needed
> anymore, so we can stop enabling it.
> 
> Fixes: d503a43ac06a ("drm: rcar-du: Register a completion callback with VSP1")
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Thanks,
Mauro