[PATCH v2 0/3] RCAR-DU, VSP1: Prevent pre-emptive frame flips on VSP1-DRM pipelines

[PATCH v2 0/3] RCAR-DU, VSP1: Prevent pre-emptive frame flips on VSP1-DRM pipelines

[Kieran Bingham]

The RCAR-DU utilises a running VSPD pipeline to perform processing
for the display pipeline.

Changes to this pipeline are performed with an atomic flush operation which
updates the state in the VSPD. Due to the way the running pipeline is
operated, any flush operation has an implicit latency of one frame interval.

This comes about as the display list is committed, but not updated until the
next VSP1 interrupt. At this point the frame is being processed, but is not
complete until the following VSP1 frame end interrupt.

To prevent reporting page flips early, we must track this timing through the
VSP1, and only allow the rcar-du object to report the page-flip completion
event after the VSP1 has processed.

This series ensures that tearing and flicker is prevented, without introducing the
performance impact mentioned in the previous series.

[PATCH 1/3] extends the VSP1 to allow a callback to be registered giving the
            VSP1 the ability to notify completion events
[PATCH 2/3] checks for race conditions in the commits of the display list, and
            in such event postpones the sending of the completion event
[PATCH 3/3] Utilises the callback extension to send page flips at the end of
            VSP processing.

These patches have been tested by introducing artificial delays in the commit
code paths and verifying that no visual tearing or flickering occurs.

Manual start/stop testing has also been performed

Kieran Bingham (3):
  v4l: vsp1: extend VSP1 module API to allow DRM callbacks
  v4l: vsp1: Postpone page flip in event of display list race
  drm: rcar-du: Register a completion callback with VSP1

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  | 10 +++++++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  2 ++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 29 ++++++++++++++++++++++++++-
 drivers/media/platform/vsp1/vsp1_dl.c   |  9 ++++++--
 drivers/media/platform/vsp1/vsp1_dl.h   |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c  | 22 ++++++++++++++++++++-
 drivers/media/platform/vsp1/vsp1_drm.h  | 10 +++++++++-
 drivers/media/platform/vsp1/vsp1_pipe.c |  6 ++++-
 drivers/media/platform/vsp1/vsp1_pipe.h |  2 ++-
 include/media/vsp1.h                    |  3 +++-
 10 files changed, 89 insertions(+), 6 deletions(-)

base-commit: 55e78dfc82988a79773ccca67e121f9a88df81c2
-- 
git-series 0.9.1

[PATCH v2 1/3] v4l: vsp1: extend VSP1 module API to allow DRM callbacks

[Kieran Bingham]

To be able to perform page flips in DRM without flicker we need to be
able to notify the rcar-du module when the VSP has completed its
processing.

We must not have bidirectional dependencies on the two components to
maintain support for loadable modules, thus we extend the API to allow
a callback to be registered within the VSP DRM interface.

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

---
v2:
 - vsp1_du_setup_lif() uses config structure to set callbacks
 - vsp1_du_pipeline_frame_end() moved to interrupt section
 - vsp1_du_pipeline_frame_end registered in vsp1_drm_init()
   meaning of any NULL values
 - removed unnecessary 'private data' variables

 drivers/media/platform/vsp1/vsp1_drm.c | 20 ++++++++++++++++++++
 drivers/media/platform/vsp1/vsp1_drm.h | 10 ++++++++++
 include/media/vsp1.h                   |  3 +++
 3 files changed, 33 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 7dce55043379..85e5ebca82a5 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -36,6 +36,16 @@ void vsp1_drm_display_start(struct vsp1_device *vsp1)
 	vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
 }
 
+static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
+{
+	struct vsp1_drm *drm = to_vsp1_drm(pipe);
+
+	if (drm->du_complete && drm->du_pending) {
+		drm->du_complete(drm->du_private);
+		drm->du_pending = false;
+	}
+}
+
 /* -----------------------------------------------------------------------------
  * DU Driver API
  */
@@ -95,6 +105,7 @@ int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg)
 		}
 
 		pipe->num_inputs = 0;
+		vsp1->drm->du_complete = NULL;
 
 		vsp1_dlm_reset(pipe->output->dlm);
 		vsp1_device_put(vsp1);
@@ -196,6 +207,13 @@ int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * Register a callback to allow us to notify the DRM framework of frame
+	 * completion events.
+	 */
+	vsp1->drm->du_complete = cfg->callback;
+	vsp1->drm->du_private = cfg->callback_data;
+
 	ret = media_pipeline_start(&pipe->output->entity.subdev.entity,
 					  &pipe->pipe);
 	if (ret < 0) {
@@ -504,6 +522,7 @@ void vsp1_du_atomic_flush(struct device *dev)
 
 	vsp1_dl_list_commit(pipe->dl);
 	pipe->dl = NULL;
+	vsp1->drm->du_pending = true;
 
 	/* Start or stop the pipeline if needed. */
 	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
@@ -597,6 +616,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
 	pipe->lif = &vsp1->lif->entity;
 	pipe->output = vsp1->wpf[0];
 	pipe->output->pipe = pipe;
+	pipe->frame_end = vsp1_du_pipeline_frame_end;
 
 	return 0;
 }
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
index 9e28ab9254ba..3a53e9a60c73 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -33,8 +33,18 @@ struct vsp1_drm {
 		struct v4l2_rect compose;
 		unsigned int zpos;
 	} inputs[VSP1_MAX_RPF];
+
+	/* Frame syncronisation */
+	void (*du_complete)(void *);
+	void *du_private;
+	bool du_pending;
 };
 
+static inline struct vsp1_drm *to_vsp1_drm(struct vsp1_pipeline *pipe)
+{
+	return container_of(pipe, struct vsp1_drm, pipe);
+}
+
 int vsp1_drm_init(struct vsp1_device *vsp1);
 void vsp1_drm_cleanup(struct vsp1_device *vsp1);
 int vsp1_drm_create_links(struct vsp1_device *vsp1);
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index bfc701f04f3f..f6629f19f209 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -23,6 +23,9 @@ int vsp1_du_init(struct device *dev);
 struct vsp1_du_lif_config {
 	unsigned int width;
 	unsigned int height;
+
+	void (*callback)(void *);
+	void *callback_data;
 };
 
 int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config *cfg);
-- 
git-series 0.9.1

[PATCH v2 2/3] v4l: vsp1: Postpone page flip in event of display list race

[Kieran Bingham]

If we try to commit the display list while an update is pending, we have
missed our opportunity. The display list manager will hold the commit
until the next interrupt.

In this event, we inform the vsp1 completion callback handler so that
the du will not perform a page flip out of turn.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_dl.c   |  9 +++++++--
 drivers/media/platform/vsp1/vsp1_dl.h   |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c  |  4 +++-
 drivers/media/platform/vsp1/vsp1_pipe.c |  6 +++++-
 drivers/media/platform/vsp1/vsp1_pipe.h |  2 ++
 5 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index ad545aff4e35..f8e8c90f22bc 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -557,9 +557,10 @@ void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm)
 	spin_unlock(&dlm->lock);
 }
 
-void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
+int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 {
 	struct vsp1_device *vsp1 = dlm->vsp1;
+	int ret = 0;
 
 	spin_lock(&dlm->lock);
 
@@ -578,8 +579,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 	 * before interrupt processing. The hardware hasn't taken the update
 	 * into account yet, we'll thus skip one frame and retry.
 	 */
-	if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD)
+	if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD) {
+		ret = -EBUSY;
 		goto done;
+	}
 
 	/* The device starts processing the queued display list right after the
 	 * frame end interrupt. The display list thus becomes active.
@@ -606,6 +609,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 
 done:
 	spin_unlock(&dlm->lock);
+
+	return ret;
 }
 
 /* Hardware Setup */
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
index 7131aa3c5978..c772a1d92513 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.h
+++ b/drivers/media/platform/vsp1/vsp1_dl.h
@@ -28,7 +28,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
 void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
 void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
 void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
-void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
+int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
 
 struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
 void vsp1_dl_list_put(struct vsp1_dl_list *dl);
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 85e5ebca82a5..6f2dd42ca01b 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -40,10 +40,12 @@ static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
 {
 	struct vsp1_drm *drm = to_vsp1_drm(pipe);
 
-	if (drm->du_complete && drm->du_pending) {
+	if (drm->du_complete && drm->du_pending && !pipe->dl_postponed) {
 		drm->du_complete(drm->du_private);
 		drm->du_pending = false;
 	}
+
+	pipe->dl_postponed = false;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 280ba0804699..3c5aae8767dd 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -297,10 +297,14 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
 
 void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
 {
+	int ret;
+
 	if (pipe == NULL)
 		return;
 
-	vsp1_dlm_irq_frame_end(pipe->output->dlm);
+	ret = vsp1_dlm_irq_frame_end(pipe->output->dlm);
+	if (ret)
+		pipe->dl_postponed = true;
 
 	if (pipe->frame_end)
 		pipe->frame_end(pipe);
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index ac4ad2655551..65cc8fb76662 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -77,6 +77,7 @@ enum vsp1_pipeline_state {
  * @uds_input: entity at the input of the UDS, if the UDS is present
  * @entities: list of entities in the pipeline
  * @dl: display list associated with the pipeline
+ * @dl_postponed: identifies if the dl commit was caught by a race condition
  * @div_size: The maximum allowed partition size for the pipeline
  * @partitions: The number of partitions used to process one frame
  * @current_partition: The partition number currently being configured
@@ -107,6 +108,7 @@ struct vsp1_pipeline {
 	struct list_head entities;
 
 	struct vsp1_dl_list *dl;
+	bool dl_postponed;
 
 	unsigned int div_size;
 	unsigned int partitions;
-- 
git-series 0.9.1

[PATCH v2 3/3] drm: rcar-du: Register a completion callback with VSP1

[Kieran Bingham]

Currently we process page flip events on every display interrupt,
however this does not take into consideration the processing time needed
by the VSP1 utilised in the pipeline.

Register a callback with the VSP driver to obtain completion events, and
track them so that we only perform page flips when the full display
pipeline has completed for the frame.

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

---
v2:
 - Commit message completely re-worded for patch re-work.
 - drm_crtc_handle_vblank() re-instated in event of rcrtc->pending
 - removed passing of unnecessary 'data' through callbacks
 - perform page flips from the VSP completion handler
 - add locking around pending flags

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 +++++++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 29 +++++++++++++++++++++++++++-
 3 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 7391dd95c733..b7ff00bb45de 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -299,7 +299,7 @@ static void rcar_du_crtc_update_planes(struct rcar_du_crtc *rcrtc)
  * Page Flip
  */
 
-static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
+void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
 {
 	struct drm_pending_vblank_event *event;
 	struct drm_device *dev = rcrtc->crtc.dev;
@@ -328,7 +328,7 @@ static bool rcar_du_crtc_page_flip_pending(struct rcar_du_crtc *rcrtc)
 	bool pending;
 
 	spin_lock_irqsave(&dev->event_lock, flags);
-	pending = rcrtc->event != NULL;
+	pending = (rcrtc->event != NULL) || (rcrtc->pending);
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
 	return pending;
@@ -579,6 +579,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 
 	if (status & DSSR_FRM) {
 		drm_crtc_handle_vblank(&rcrtc->crtc);
+
+		if (rcrtc->pending) {
+			trace_printk("VBlank loss due to VSP Overrun\n");
+			return IRQ_HANDLED;
+		}
+
 		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 a7194812997e..b73ec6de7af4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -47,6 +47,7 @@ struct rcar_du_crtc {
 
 	struct drm_pending_vblank_event *event;
 	wait_queue_head_t flip_wait;
+	bool pending;
 
 	unsigned int outputs;
 
@@ -71,5 +72,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
 
 void rcar_du_crtc_route_output(struct drm_crtc *crtc,
 			       enum rcar_du_output output);
+void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
 
 #endif /* __RCAR_DU_CRTC_H__ */
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index b0ff304ce3dc..1fcd311badb1 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -28,6 +28,22 @@
 #include "rcar_du_kms.h"
 #include "rcar_du_vsp.h"
 
+static void rcar_du_vsp_complete(void *private)
+{
+	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
+	struct drm_device *dev = crtc->crtc.dev;
+	unsigned long flags;
+	bool pending;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	pending = crtc->pending;
+	crtc->pending = false;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	if (pending)
+		rcar_du_crtc_finish_page_flip(crtc);
+}
+
 void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 {
 	const struct drm_display_mode *mode = &crtc->crtc.state->adjusted_mode;
@@ -35,6 +51,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 	struct vsp1_du_lif_config cfg = {
 		.width = mode->hdisplay,
 		.height = mode->vdisplay,
+		.callback = rcar_du_vsp_complete,
+		.callback_data = crtc,
 	};
 	struct rcar_du_plane_state state = {
 		.state = {
@@ -85,6 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
 
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
 {
+	struct drm_device *dev = crtc->crtc.dev;
+	unsigned long flags;
+	bool pending;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	pending = crtc->pending;
+	crtc->pending = true;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	WARN_ON(pending);
+
 	vsp1_du_atomic_flush(crtc->vsp->vsp);
 }
 
-- 
git-series 0.9.1

Re: [PATCH v2 3/3] drm: rcar-du: Register a completion callback with VSP1

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:19 Kieran Bingham wrote:
> Currently we process page flip events on every display interrupt,
> however this does not take into consideration the processing time needed
> by the VSP1 utilised in the pipeline.
> 
> Register a callback with the VSP driver to obtain completion events, and
> track them so that we only perform page flips when the full display
> pipeline has completed for the frame.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> v2:
>  - Commit message completely re-worded for patch re-work.
>  - drm_crtc_handle_vblank() re-instated in event of rcrtc->pending
>  - removed passing of unnecessary 'data' through callbacks
>  - perform page flips from the VSP completion handler
>  - add locking around pending flags
> 
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 +++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++-
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 29 +++++++++++++++++++++++++++-
>  3 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 7391dd95c733..b7ff00bb45de
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -299,7 +299,7 @@ static void rcar_du_crtc_update_planes(struct
> rcar_du_crtc *rcrtc) * Page Flip
>   */
> 
> -static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
>  {
>  	struct drm_pending_vblank_event *event;
>  	struct drm_device *dev = rcrtc->crtc.dev;
> @@ -328,7 +328,7 @@ static bool rcar_du_crtc_page_flip_pending(struct
> rcar_du_crtc *rcrtc) bool pending;
> 
>  	spin_lock_irqsave(&dev->event_lock, flags);
> -	pending = rcrtc->event != NULL;
> +	pending = (rcrtc->event != NULL) || (rcrtc->pending);

No need for parenthesis.

>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> 
>  	return pending;
> @@ -579,6 +579,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
> 
>  	if (status & DSSR_FRM) {
>  		drm_crtc_handle_vblank(&rcrtc->crtc);
> +
> +		if (rcrtc->pending) {
> +			trace_printk("VBlank loss due to VSP Overrun\n");
> +			return IRQ_HANDLED;
> +		}
> +

More than that, now that the VSP completion handler finishes the page flip, 
you should skip the rcar_du_crtc_finish_page_flip() call here unconditionally 
on Gen3.

Something like

	struct rcar_du_crtc *rcrtc = arg;
	struct rcar_du_device *rcdu = rcrtc->group->dev;
	...

	if (status & DSSR_FRM) {
		drm_crtc_handle_vblank(&rcrtc->crtc);

		if (rcdu->info->gen < 3)
			rcar_du_crtc_finish_page_flip(rcrtc);

		ret = IRQ_HANDLED;
	}

>  		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 a7194812997e..b73ec6de7af4
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -47,6 +47,7 @@ struct rcar_du_crtc {
> 
>  	struct drm_pending_vblank_event *event;
>  	wait_queue_head_t flip_wait;
> +	bool pending;
> 
>  	unsigned int outputs;
> 
> @@ -71,5 +72,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
> 
>  void rcar_du_crtc_route_output(struct drm_crtc *crtc,
>  			       enum rcar_du_output output);
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
> 
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1fcd311badb1
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,22 @@
>  #include "rcar_du_kms.h"
>  #include "rcar_du_vsp.h"
> 
> +static void rcar_du_vsp_complete(void *private)
> +{
> +	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
> +	struct drm_device *dev = crtc->crtc.dev;
> +	unsigned long flags;
> +	bool pending;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = crtc->pending;
> +	crtc->pending = false;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	if (pending)
> +		rcar_du_crtc_finish_page_flip(crtc);

This seems to duplicate the synchronization mechanism based on events in 
rcar_du_crtc_atomic_begin(). I need to check that in more details.

> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>  	const struct drm_display_mode *mode = &crtc->crtc.state-
>adjusted_mode;
> @@ -35,6 +51,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  	struct vsp1_du_lif_config cfg = {
>  		.width = mode->hdisplay,
>  		.height = mode->vdisplay,
> +		.callback = rcar_du_vsp_complete,
> +		.callback_data = crtc,
>  	};
>  	struct rcar_du_plane_state state = {
>  		.state = {
> @@ -85,6 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> 
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
> +	struct drm_device *dev = crtc->crtc.dev;
> +	unsigned long flags;
> +	bool pending;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	pending = crtc->pending;
> +	crtc->pending = true;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	WARN_ON(pending);
> +
>  	vsp1_du_atomic_flush(crtc->vsp->vsp);
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] drm: rcar-du: Register a completion callback with VSP1

[Laurent Pinchart]

Hi Kieran,

On Saturday 04 Mar 2017 15:07:09 Laurent Pinchart wrote:
> On Saturday 04 Mar 2017 02:01:19 Kieran Bingham wrote:
> > Currently we process page flip events on every display interrupt,
> > however this does not take into consideration the processing time needed
> > by the VSP1 utilised in the pipeline.
> > 
> > Register a callback with the VSP driver to obtain completion events, and
> > track them so that we only perform page flips when the full display
> > pipeline has completed for the frame.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > 
> > ---
> > 
> > v2:
> >  - Commit message completely re-worded for patch re-work.
> >  - drm_crtc_handle_vblank() re-instated in event of rcrtc->pending
> >  - removed passing of unnecessary 'data' through callbacks
> >  - perform page flips from the VSP completion handler
> >  - add locking around pending flags
> >  
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 +++++++--
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++-
> >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 29 +++++++++++++++++++++++++++-
> >  3 files changed, 39 insertions(+), 2 deletions(-)

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1fcd311badb1
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c

[snip]

> > @@ -28,6 +28,22 @@
> >  #include "rcar_du_kms.h"
> >  #include "rcar_du_vsp.h"
> > 
> > +static void rcar_du_vsp_complete(void *private)
> > +{
> > +	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
> > +	struct drm_device *dev = crtc->crtc.dev;
> > +	unsigned long flags;
> > +	bool pending;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, flags);
> > +	pending = crtc->pending;
> > +	crtc->pending = false;
> > +	spin_unlock_irqrestore(&dev->event_lock, flags);
> > +
> > +	if (pending)
> > +		rcar_du_crtc_finish_page_flip(crtc);
> 
> This seems to duplicate the synchronization mechanism based on events in
> rcar_du_crtc_atomic_begin(). I need to check that in more details.

Indeed it does, and I don't think that's needed. You might be able to shorten 
the race window on the DU side, but you won't be able to close it completely 
as detecting the race requires information that is only available to the VSP 
driver. Fixing the race on the VSP side will make this dead code, so you can 
remove the addition of the pending flag from this patch.

> > +}

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l: vsp1: Postpone page flip in event of display list race

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:18 Kieran Bingham wrote:
> If we try to commit the display list while an update is pending, we have
> missed our opportunity. The display list manager will hold the commit
> until the next interrupt.
> 
> In this event, we inform the vsp1 completion callback handler so that
> the du will not perform a page flip out of turn.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_dl.c   |  9 +++++++--
>  drivers/media/platform/vsp1/vsp1_dl.h   |  2 +-
>  drivers/media/platform/vsp1/vsp1_drm.c  |  4 +++-
>  drivers/media/platform/vsp1/vsp1_pipe.c |  6 +++++-
>  drivers/media/platform/vsp1/vsp1_pipe.h |  2 ++
>  5 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index ad545aff4e35..f8e8c90f22bc
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -557,9 +557,10 @@ void vsp1_dlm_irq_display_start(struct vsp1_dl_manager
> *dlm)
>  	spin_unlock(&dlm->lock);
>  }
> 
> -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> +int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
>  	struct vsp1_device *vsp1 = dlm->vsp1;
> +	int ret = 0;
> 
>  	spin_lock(&dlm->lock);
> 
> @@ -578,8 +579,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager
> *dlm) * before interrupt processing. The hardware hasn't taken the update *
> into account yet, we'll thus skip one frame and retry.
>  	 */
> -	if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD)
> +	if (vsp1_read(vsp1, VI6_DL_BODY_SIZE) & VI6_DL_BODY_SIZE_UPD) {
> +		ret = -EBUSY;

Getting there, but not quite :-)

What we need to protect against is the display list not being committed early 
enough, resulting in one frame skip in the hardware. The good news is that the 
driver already detects the opposite condition just below. 

>  		goto done;
> +	}
> 
>  	/* The device starts processing the queued display list right after 
the
>  	 * frame end interrupt. The display list thus becomes active.

This is what we want to report. You can simply return a bool that will tell 
whether the previous display list has completed at frame end. You should 
return true when the condition right below this comment is true, as well as 
when using header mode (to avoid breaking mem-to-mem pipelines), and false in 
all other cases.

> @@ -606,6 +609,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> 
>  done:
>  	spin_unlock(&dlm->lock);
> +
> +	return ret;
>  }
> 
>  /* Hardware Setup */
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 7131aa3c5978..c772a1d92513
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -28,7 +28,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device
> *vsp1, void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
>  void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
> -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> +int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> 
>  struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
>  void vsp1_dl_list_put(struct vsp1_dl_list *dl);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 85e5ebca82a5..6f2dd42ca01b
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -40,10 +40,12 @@ static void vsp1_du_pipeline_frame_end(struct
> vsp1_pipeline *pipe) {
>  	struct vsp1_drm *drm = to_vsp1_drm(pipe);
> 
> -	if (drm->du_complete && drm->du_pending) {
> +	if (drm->du_complete && drm->du_pending && !pipe->dl_postponed) {
>  		drm->du_complete(drm->du_private);
>  		drm->du_pending = false;
>  	}
> +
> +	pipe->dl_postponed = false;
>  }
> 
>  /* ------------------------------------------------------------------------
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 280ba0804699..3c5aae8767dd
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -297,10 +297,14 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
> 
>  void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
>  {
> +	int ret;
> +
>  	if (pipe == NULL)
>  		return;
> 
> -	vsp1_dlm_irq_frame_end(pipe->output->dlm);
> +	ret = vsp1_dlm_irq_frame_end(pipe->output->dlm);
> +	if (ret)
> +		pipe->dl_postponed = true;

This can be simplified greatly. If vsp1_dlm_irq_frame_end() returns false, 
return immediately without calling the pipe->frame_end() handler or 
incrementing the sequence number, as the frame has not completed. You can then 
remove the dl_postponed field from the vsp1_pipeline structure and call the 
.du_complete() handler unconditionally in vsp1_du_pipeline_frame_end() 
(provided it's not NULL of course). As the vsp1_dlm_irq_frame_end() function 
can't return true if a commit hasn't been queued in the first place, there's 
no need for a dl_pending flag either.

> 
>  	if (pipe->frame_end)
>  		pipe->frame_end(pipe);
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index ac4ad2655551..65cc8fb76662
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -77,6 +77,7 @@ enum vsp1_pipeline_state {
>   * @uds_input: entity at the input of the UDS, if the UDS is present
>   * @entities: list of entities in the pipeline
>   * @dl: display list associated with the pipeline
> + * @dl_postponed: identifies if the dl commit was caught by a race
> condition * @div_size: The maximum allowed partition size for the pipeline
> * @partitions: The number of partitions used to process one frame *
> @current_partition: The partition number currently being configured @@
> -107,6 +108,7 @@ struct vsp1_pipeline {
>  	struct list_head entities;
> 
>  	struct vsp1_dl_list *dl;
> +	bool dl_postponed;
> 
>  	unsigned int div_size;
>  	unsigned int partitions;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] v4l: vsp1: extend VSP1 module API to allow DRM callbacks

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Saturday 04 Mar 2017 02:01:17 Kieran Bingham wrote:
> To be able to perform page flips in DRM without flicker we need to be
> able to notify the rcar-du module when the VSP has completed its
> processing.
> 
> We must not have bidirectional dependencies on the two components to
> maintain support for loadable modules, thus we extend the API to allow
> a callback to be registered within the VSP DRM interface.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> v2:
>  - vsp1_du_setup_lif() uses config structure to set callbacks
>  - vsp1_du_pipeline_frame_end() moved to interrupt section
>  - vsp1_du_pipeline_frame_end registered in vsp1_drm_init()
>    meaning of any NULL values
>  - removed unnecessary 'private data' variables
> 
>  drivers/media/platform/vsp1/vsp1_drm.c | 20 ++++++++++++++++++++
>  drivers/media/platform/vsp1/vsp1_drm.h | 10 ++++++++++
>  include/media/vsp1.h                   |  3 +++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 7dce55043379..85e5ebca82a5
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -36,6 +36,16 @@ void vsp1_drm_display_start(struct vsp1_device *vsp1)
>  	vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
>  }
> 
> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
> +{
> +	struct vsp1_drm *drm = to_vsp1_drm(pipe);
> +
> +	if (drm->du_complete && drm->du_pending) {
> +		drm->du_complete(drm->du_private);
> +		drm->du_pending = false;
> +	}
> +}
> +
>  /* ------------------------------------------------------------------------
>   * DU Driver API
>   */
> @@ -95,6 +105,7 @@ int vsp1_du_setup_lif(struct device *dev, const struct
> vsp1_du_lif_config *cfg)
> 		}
> 
>  		pipe->num_inputs = 0;
> +		vsp1->drm->du_complete = NULL;
> 
>  		vsp1_dlm_reset(pipe->output->dlm);
>  		vsp1_device_put(vsp1);
> @@ -196,6 +207,13 @@ int vsp1_du_setup_lif(struct device *dev, const struct
> vsp1_du_lif_config *cfg) if (ret < 0)
>  		return ret;
> 
> +	/*
> +	 * Register a callback to allow us to notify the DRM framework of 
frame
> +	 * completion events.
> +	 */
> +	vsp1->drm->du_complete = cfg->callback;
> +	vsp1->drm->du_private = cfg->callback_data;
> +
>  	ret = media_pipeline_start(&pipe->output->entity.subdev.entity,
>  					  &pipe->pipe);
>  	if (ret < 0) {
> @@ -504,6 +522,7 @@ void vsp1_du_atomic_flush(struct device *dev)
> 
>  	vsp1_dl_list_commit(pipe->dl);
>  	pipe->dl = NULL;
> +	vsp1->drm->du_pending = true;
> 
>  	/* Start or stop the pipeline if needed. */
>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> @@ -597,6 +616,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>  	pipe->lif = &vsp1->lif->entity;
>  	pipe->output = vsp1->wpf[0];
>  	pipe->output->pipe = pipe;
> +	pipe->frame_end = vsp1_du_pipeline_frame_end;
> 
>  	return 0;
>  }
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..3a53e9a60c73
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -33,8 +33,18 @@ struct vsp1_drm {
>  		struct v4l2_rect compose;
>  		unsigned int zpos;
>  	} inputs[VSP1_MAX_RPF];
> +
> +	/* Frame syncronisation */
> +	void (*du_complete)(void *);
> +	void *du_private;

These fields need to be documented.

> +	bool du_pending;

You can remove the du_pending flag, see my comments in patch 2/3.

>  };
> 
> +static inline struct vsp1_drm *to_vsp1_drm(struct vsp1_pipeline *pipe)
> +{
> +	return container_of(pipe, struct vsp1_drm, pipe);
> +}
> +
>  int vsp1_drm_init(struct vsp1_device *vsp1);
>  void vsp1_drm_cleanup(struct vsp1_device *vsp1);
>  int vsp1_drm_create_links(struct vsp1_device *vsp1);
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index bfc701f04f3f..f6629f19f209 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -23,6 +23,9 @@ int vsp1_du_init(struct device *dev);
>  struct vsp1_du_lif_config {
>  	unsigned int width;
>  	unsigned int height;
> +
> +	void (*callback)(void *);
> +	void *callback_data;

These fields need to be documented too.

>  };
> 
>  int vsp1_du_setup_lif(struct device *dev, const struct vsp1_du_lif_config
> *cfg);

-- 
Regards,

Laurent Pinchart