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

[PATCH v3 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. This presents the opportunity for some race conditions to
affect the quality of the display output.

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] handles potential race conditions in vsp1_dlm_irq_frame_end() and
            prevents signalling the frame end in this event.
[PATCH 2/3] extends the VSP1 to allow a callback to be registered giving the
            VSP1 the ability to notify completion events.
[PATCH 3/3] utilises the callback extension to send page flips at the end of
            VSP processing for Gen3 platforms.

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

Extensive testing around the race window has been performed by dynamically
adapting the artificial delay between 10, and 17 seconds in 100uS increments
for periods of 5 seconds on each delay test. These tests have successfully run
for 3 hours.

Manual start/stop testing has also been performed.

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

 drivers/gpu/drm/rcar-du/rcar_du_crtc.c  |  8 ++++++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h  |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   |  9 +++++++++
 drivers/media/platform/vsp1/vsp1_dl.c   | 19 +++++++++++++++++--
 drivers/media/platform/vsp1/vsp1_dl.h   |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c  | 17 +++++++++++++++++
 drivers/media/platform/vsp1/vsp1_drm.h  | 11 +++++++++++
 drivers/media/platform/vsp1/vsp1_pipe.c | 13 ++++++++++++-
 include/media/vsp1.h                    | 13 +++++++++++++
 9 files changed, 87 insertions(+), 6 deletions(-)

base-commit: cdb5795cbc4ddbe5082c25c52ebc1d811ac3849e
-- 
git-series 0.9.1

[PATCH v3 1/3] v4l: vsp1: Postpone frame end handling 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 skip the pipeline completion callback handler so that
the pipeline will not mistakenly report frame completion to the user.

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

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c b/drivers/media/platform/vsp1/vsp1_dl.c
index b9e5027778ff..f449ca689554 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -562,9 +562,19 @@ 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)
+/**
+ * vsp1_dlm_irq_frame_end - Display list handler for the frame end interrupt
+ * @dlm: the display list manager
+ *
+ * Return true if the previous display list has completed at frame end, or false
+ * if it has been delayed by one frame because the display list commit raced
+ * with the frame end interrupt. The function always returns true in header mode
+ * as display list processing is then not continuous and races never occur.
+ */
+bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 {
 	struct vsp1_device *vsp1 = dlm->vsp1;
+	bool completed = false;
 
 	spin_lock(&dlm->lock);
 
@@ -576,8 +586,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 	 * perform any operation as there can't be any new display list queued
 	 * in that case.
 	 */
-	if (dlm->mode == VSP1_DL_MODE_HEADER)
+	if (dlm->mode == VSP1_DL_MODE_HEADER) {
+		completed = true;
 		goto done;
+	}
 
 	/*
 	 * The UPD bit set indicates that the commit operation raced with the
@@ -597,6 +609,7 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 	if (dlm->queued) {
 		dlm->active = dlm->queued;
 		dlm->queued = NULL;
+		completed = true;
 	}
 
 	/*
@@ -619,6 +632,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
 
 done:
 	spin_unlock(&dlm->lock);
+
+	return completed;
 }
 
 /* Hardware Setup */
diff --git a/drivers/media/platform/vsp1/vsp1_dl.h b/drivers/media/platform/vsp1/vsp1_dl.h
index 7131aa3c5978..6ec1380a10af 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);
+bool 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_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 35364f594e19..d15327701ad8 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -304,10 +304,21 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
 
 void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
 {
+	bool completed;
+
 	if (pipe == NULL)
 		return;
 
-	vsp1_dlm_irq_frame_end(pipe->output->dlm);
+	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->frame_end)
 		pipe->frame_end(pipe);
-- 
git-series 0.9.1

Re: [PATCH v3 1/3] v4l: vsp1: Postpone frame end handling in event of display list race

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Sunday 05 Mar 2017 16:00:02 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 skip the pipeline completion callback handler so that
> the pipeline will not mistakenly report frame completion to the user.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c   | 19 +++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_dl.h   |  2 +-
>  drivers/media/platform/vsp1/vsp1_pipe.c | 13 ++++++++++++-
>  3 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index b9e5027778ff..f449ca689554
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -562,9 +562,19 @@ 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)
> +/**
> + * vsp1_dlm_irq_frame_end - Display list handler for the frame end
> interrupt + * @dlm: the display list manager
> + *
> + * Return true if the previous display list has completed at frame end, or
> false + * if it has been delayed by one frame because the display list
> commit raced + * with the frame end interrupt. The function always returns
> true in header mode + * as display list processing is then not continuous
> and races never occur. + */
> +bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>  {
>  	struct vsp1_device *vsp1 = dlm->vsp1;
> +	bool completed = false;
> 
>  	spin_lock(&dlm->lock);
> 
> @@ -576,8 +586,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager
> *dlm) * perform any operation as there can't be any new display list queued
> * in that case.
>  	 */
> -	if (dlm->mode == VSP1_DL_MODE_HEADER)
> +	if (dlm->mode == VSP1_DL_MODE_HEADER) {
> +		completed = true;
>  		goto done;
> +	}
> 
>  	/*
>  	 * The UPD bit set indicates that the commit operation raced with the
> @@ -597,6 +609,7 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> if (dlm->queued) {
>  		dlm->active = dlm->queued;
>  		dlm->queued = NULL;
> +		completed = true;
>  	}
> 
>  	/*
> @@ -619,6 +632,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> 
>  done:
>  	spin_unlock(&dlm->lock);
> +
> +	return completed;
>  }
> 
>  /* Hardware Setup */
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 7131aa3c5978..6ec1380a10af
> 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);
> +bool 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_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 35364f594e19..d15327701ad8
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -304,10 +304,21 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
> 
>  void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
>  {
> +	bool completed;
> +
>  	if (pipe == NULL)
>  		return;
> 
> -	vsp1_dlm_irq_frame_end(pipe->output->dlm);
> +	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->frame_end)
>  		pipe->frame_end(pipe);

-- 
Regards,

Laurent Pinchart

[PATCH v3 2/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>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 17 +++++++++++++++++
 drivers/media/platform/vsp1/vsp1_drm.h | 11 +++++++++++
 include/media/vsp1.h                   | 13 +++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 4ee437c7ff0c..d93bf7d3a39e 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -37,6 +37,14 @@ 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_complete(drm->du_private);
+}
+
 /* -----------------------------------------------------------------------------
  * DU Driver API
  */
@@ -96,6 +104,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);
@@ -200,6 +209,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) {
@@ -607,6 +623,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 c8d2f88fc483..3de2095cb0ce 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -23,6 +23,8 @@
  * @num_inputs: number of active pipeline inputs at the beginning of an update
  * @inputs: source crop rectangle, destination compose rectangle and z-order
  *	position for every input
+ * @du_complete: frame completion callback for the DU driver (optional)
+ * @du_private: data to be passed to the du_complete callback
  */
 struct vsp1_drm {
 	struct vsp1_pipeline pipe;
@@ -33,8 +35,17 @@ struct vsp1_drm {
 		struct v4l2_rect compose;
 		unsigned int zpos;
 	} inputs[VSP1_MAX_RPF];
+
+	/* Frame syncronisation */
+	void (*du_complete)(void *);
+	void *du_private;
 };
 
+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..d59d0adf560d 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -20,9 +20,22 @@ struct device;
 
 int vsp1_du_init(struct device *dev);
 
+/**
+ * struct vsp1_du_lif_config - VSP LIF configuration
+ * @width: output frame width
+ * @height: output frame height
+ * @callback: frame completion callback function (optional)
+ * @callback_data: data to be passed to the frame completion callback
+ *
+ * When the optional callback is provided to the VSP1, the VSP1 must guarantee
+ * that one completion callback is performed after every vsp1_du_atomic_flush()
+ */
 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

Re: [PATCH v3 2/3] v4l: vsp1: Extend VSP1 module API to allow DRM callbacks

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Sunday 05 Mar 2017 16:00:03 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>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 17 +++++++++++++++++
>  drivers/media/platform/vsp1/vsp1_drm.h | 11 +++++++++++
>  include/media/vsp1.h                   | 13 +++++++++++++
>  3 files changed, 41 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 4ee437c7ff0c..d93bf7d3a39e
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -37,6 +37,14 @@ 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_complete(drm->du_private);
> +}
> +
>  /* ------------------------------------------------------------------------
>   * DU Driver API
>   */
> @@ -96,6 +104,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);
> @@ -200,6 +209,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

s/framework/driver/

> +	 * 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) {
> @@ -607,6 +623,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 c8d2f88fc483..3de2095cb0ce
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -23,6 +23,8 @@
>   * @num_inputs: number of active pipeline inputs at the beginning of an
> update
>   * @inputs: source crop rectangle, destination compose rectangle and
> z-order
>   *	position for every input
> + * @du_complete: frame completion callback for the DU driver (optional)
> + * @du_private: data to be passed to the du_complete callback
>   */
>  struct vsp1_drm {
>  	struct vsp1_pipeline pipe;
> @@ -33,8 +35,17 @@ struct vsp1_drm {
>  		struct v4l2_rect compose;
>  		unsigned int zpos;
>  	} inputs[VSP1_MAX_RPF];
> +
> +	/* Frame syncronisation */

s/syncronisation/synchronisation/

> +	void (*du_complete)(void *);
> +	void *du_private;
>  };
> 
> +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..d59d0adf560d 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -20,9 +20,22 @@ struct device;
> 
>  int vsp1_du_init(struct device *dev);
> 
> +/**
> + * struct vsp1_du_lif_config - VSP LIF configuration
> + * @width: output frame width
> + * @height: output frame height
> + * @callback: frame completion callback function (optional)
> + * @callback_data: data to be passed to the frame completion callback
> + *
> + * When the optional callback is provided to the VSP1, the VSP1 must
> guarantee
> + * that one completion callback is performed after every
> vsp1_du_atomic_flush()

This paragraph should be part of the @callback documentation. I would phrase 
it as

 * @callback: frame completion callback function (optional). When a callback
 *	      is provided, the VSP driver guarantees that it will be called
 *	      once and only once for each vsp1_du_atomic_flush() call.

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If you're fine with the above changes there's no need to resubmit, I'll fix 
when applying the patch.

> + */
>  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);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 2/3] v4l: vsp1: Extend VSP1 module API to allow DRM callbacks

[Kieran Bingham]

Hi Laurent,

On 05/03/17 21:58, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Sunday 05 Mar 2017 16:00:03 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>
>> ---
>>  drivers/media/platform/vsp1/vsp1_drm.c | 17 +++++++++++++++++
>>  drivers/media/platform/vsp1/vsp1_drm.h | 11 +++++++++++
>>  include/media/vsp1.h                   | 13 +++++++++++++
>>  3 files changed, 41 insertions(+)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>> b/drivers/media/platform/vsp1/vsp1_drm.c index 4ee437c7ff0c..d93bf7d3a39e
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>> @@ -37,6 +37,14 @@ 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_complete(drm->du_private);
>> +}
>> +
>>  /* ------------------------------------------------------------------------
>>   * DU Driver API
>>   */
>> @@ -96,6 +104,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);
>> @@ -200,6 +209,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
> 
> s/framework/driver/
> 
>> +	 * 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) {
>> @@ -607,6 +623,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 c8d2f88fc483..3de2095cb0ce
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.h
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
>> @@ -23,6 +23,8 @@
>>   * @num_inputs: number of active pipeline inputs at the beginning of an
>> update
>>   * @inputs: source crop rectangle, destination compose rectangle and
>> z-order
>>   *	position for every input
>> + * @du_complete: frame completion callback for the DU driver (optional)
>> + * @du_private: data to be passed to the du_complete callback
>>   */
>>  struct vsp1_drm {
>>  	struct vsp1_pipeline pipe;
>> @@ -33,8 +35,17 @@ struct vsp1_drm {
>>  		struct v4l2_rect compose;
>>  		unsigned int zpos;
>>  	} inputs[VSP1_MAX_RPF];
>> +
>> +	/* Frame syncronisation */
> 
> s/syncronisation/synchronisation/
> 
>> +	void (*du_complete)(void *);
>> +	void *du_private;
>>  };
>>
>> +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..d59d0adf560d 100644
>> --- a/include/media/vsp1.h
>> +++ b/include/media/vsp1.h
>> @@ -20,9 +20,22 @@ struct device;
>>
>>  int vsp1_du_init(struct device *dev);
>>
>> +/**
>> + * struct vsp1_du_lif_config - VSP LIF configuration
>> + * @width: output frame width
>> + * @height: output frame height
>> + * @callback: frame completion callback function (optional)
>> + * @callback_data: data to be passed to the frame completion callback
>> + *
>> + * When the optional callback is provided to the VSP1, the VSP1 must
>> guarantee
>> + * that one completion callback is performed after every
>> vsp1_du_atomic_flush()
> 
> This paragraph should be part of the @callback documentation. I would phrase 
> it as
> 
>  * @callback: frame completion callback function (optional). When a callback
>  *	      is provided, the VSP driver guarantees that it will be called
>  *	      once and only once for each vsp1_du_atomic_flush() call.
> 
> With this fixed,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> If you're fine with the above changes there's no need to resubmit, I'll fix 
> when applying the patch.

Thanks - that would be great.
--
Regards
Kieran

> 
>> + */
>>  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);
> 

[PATCH v3 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>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 ++++++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  9 +++++++++
 3 files changed, 16 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 2aceb84fc15d..caaaa1812e20 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;
@@ -571,6 +571,7 @@ 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;
 
@@ -579,7 +580,10 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 
 	if (status & DSSR_FRM) {
 		drm_crtc_handle_vblank(&rcrtc->crtc);
-		rcar_du_crtc_finish_page_flip(rcrtc);
+
+		if (rcdu->info->gen < 3)
+			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..ebdbff9d8e59 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -71,5 +71,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..cbb6f54c99ef 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -28,6 +28,13 @@
 #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;
+
+	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 +42,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 = {
-- 
git-series 0.9.1

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

[Sergei Shtylyov]

Hello!

On 03/05/2017 07:00 PM, 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>
[...]
>  #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..cbb6f54c99ef 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,13 @@
>  #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;

    No need for explicit cast.

> +
> +	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;
[...]

MBR, Sergei

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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Sunday 05 Mar 2017 16:00:04 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>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 ++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  9 +++++++++
>  3 files changed, 16 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 2aceb84fc15d..caaaa1812e20
> 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;
> @@ -571,6 +571,7 @@ 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;
> 
> @@ -579,7 +580,10 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
> 
>  	if (status & DSSR_FRM) {
>  		drm_crtc_handle_vblank(&rcrtc->crtc);
> -		rcar_du_crtc_finish_page_flip(rcrtc);
> +
> +		if (rcdu->info->gen < 3)
> +			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..ebdbff9d8e59
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -71,5 +71,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..cbb6f54c99ef
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,13 @@
>  #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;

I'll remove the cast when applying, as pointed out by Sergei.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	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 +42,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 = {

-- 
Regards,

Laurent Pinchart