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

[RFC PATCH 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.

[PATCH 1/3] fixes the VSP DRM object to register it's pipeline correctly.
[PATCH 2/3] extends the VSP1 to allow a callback to be registered allowing the
            VSP1 to notify completion events, and extend the existing atomic
            flush API to allow private event data to be passed through.
[PATCH 3/3] Utilises this API extension to postpone page flips as required.

In current testing, with kmstest, and kmscube, it can be seen that our refresh
rate has halved. I believe this is due to the one frame latency imposed by the
VSPD and will need further investigation.

Kieran Bingham (3):
  v4l: vsp1: Register pipe with output WPF
  v4l: vsp1: extend VSP1 module API to allow DRM callback registration
  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  | 34 ++++++++++++++++++++-
 drivers/media/platform/vsp1/vsp1_drm.c | 43 +++++++++++++++++++++++++--
 drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
 include/media/vsp1.h                   |  6 +++-
 6 files changed, 99 insertions(+), 5 deletions(-)

base-commit: a194138cd82dff52d4c39895fd89dc6f26eafc97
-- 
git-series 0.9.1

[RFC PATCH 1/3] v4l: vsp1: Register pipe with output WPF

[Kieran Bingham]

The DRM object does not register the pipe with the WPF object. This is
used internally throughout the driver as a means of accessing the pipe.
As such this breaks operations which require access to the pipe from WPF
interrupts.

Register the pipe inside the WPF object after it has been declared as
the output.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index cd209dccff1b..8e2aa3f8e52f 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -596,6 +596,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
 	pipe->bru = &vsp1->bru->entity;
 	pipe->lif = &vsp1->lif->entity;
 	pipe->output = vsp1->wpf[0];
+	pipe->output->pipe = pipe;
 
 	return 0;
 }
-- 
git-series 0.9.1

Re: [RFC PATCH 1/3] v4l: vsp1: Register pipe with output WPF

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 01 Mar 2017 13:12:54 Kieran Bingham wrote:
> The DRM object does not register the pipe with the WPF object. This is
> used internally throughout the driver as a means of accessing the pipe.
> As such this breaks operations which require access to the pipe from WPF
> interrupts.
> 
> Register the pipe inside the WPF object after it has been declared as
> the output.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index cd209dccff1b..8e2aa3f8e52f
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -596,6 +596,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>  	pipe->bru = &vsp1->bru->entity;
>  	pipe->lif = &vsp1->lif->entity;
>  	pipe->output = vsp1->wpf[0];
> +	pipe->output->pipe = pipe;

The vsp1_irq_handler() function calls vsp1_pipeline_frame_end() with wpf-
>pipe, which is currently NULL. With this patch the function will get a non-
NULL pipeline and will thus proceed to calling vsp1_dlm_irq_frame_end():

void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
{
	if (pipe == NULL)
		return;

	vsp1_dlm_irq_frame_end(pipe->output->dlm);

	if (pipe->frame_end)
		pipe->frame_end(pipe);

	pipe->sequence++;
}

pipe->frame_end is NULL, pipe->sequence doesn't matter, but we now end up 
calling vsp1_dlm_irq_frame_end(). This is a major change regarding display 
list processing, yet it seems to have no effect at all.

The following commit is to blame for skipping the call to 
vsp1_dlm_irq_frame_end().

commit ff7e97c94d9f7f370fe3ce2a72e85361ca22a605
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Date:   Tue Jan 19 19:16:36 2016 -0200

    [media] v4l: vsp1: Store pipeline pointer in rwpf

I've added a few debug print statements to vsp1_dlm_irq_frame_end(), and it 
looks like we only hit the if (dlm->queued) test or none of them at all. It 
looks like we've been lucky that nothing broke.

Restoring the previous behaviour should be safe, but it would be worth it 
inspecting the code very carefully to make sure the logic is still correct. 
I'll do it tomorrow if you don't beat me to it.

In any case, how about adding a

Fixes: ff7e97c94d9f ("[media] v4l: vsp1: Store pipeline pointer in rwpf")

line ?

>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/3] v4l: vsp1: Register pipe with output WPF

[Kieran Bingham]

Hi Laurent,

Thanks for the review,

On 03/03/17 01:57, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 01 Mar 2017 13:12:54 Kieran Bingham wrote:
>> The DRM object does not register the pipe with the WPF object. This is
>> used internally throughout the driver as a means of accessing the pipe.
>> As such this breaks operations which require access to the pipe from WPF
>> interrupts.
>>
>> Register the pipe inside the WPF object after it has been declared as
>> the output.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/vsp1/vsp1_drm.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>> b/drivers/media/platform/vsp1/vsp1_drm.c index cd209dccff1b..8e2aa3f8e52f
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>> @@ -596,6 +596,7 @@ int vsp1_drm_init(struct vsp1_device *vsp1)
>>  	pipe->bru = &vsp1->bru->entity;
>>  	pipe->lif = &vsp1->lif->entity;
>>  	pipe->output = vsp1->wpf[0];
>> +	pipe->output->pipe = pipe;
> 
> The vsp1_irq_handler() function calls vsp1_pipeline_frame_end() with wpf-
>> pipe, which is currently NULL. With this patch the function will get a non-
> NULL pipeline and will thus proceed to calling vsp1_dlm_irq_frame_end():
> 
> void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
> {
> 	if (pipe == NULL)
> 		return;
> 
> 	vsp1_dlm_irq_frame_end(pipe->output->dlm);
> 
> 	if (pipe->frame_end)
> 		pipe->frame_end(pipe);
> 
> 	pipe->sequence++;
> }
> 
> pipe->frame_end is NULL, pipe->sequence doesn't matter, but we now end up 
> calling vsp1_dlm_irq_frame_end(). This is a major change regarding display 
> list processing, yet it seems to have no effect at all.

I was a bit surprised at this bit. I had expected vsp1_dlm_irq_frame_end() to be
more critical, but ultimately - it's only job is to handle a race condition on
DL commits which we (presumably) don't hit. At least we don't hit in our testing
more than $(DL_QTY) times, or I believe the display would have hung.

In fact we would have panic'd on a NULL dereference with pipe->dl:
	pipe->dl = vsp1_dl_list_get(pipe->output->dlm);

where I don't see any checks on pipe->dl != NULL in the current code paths.

> The following commit is to blame for skipping the call to 
> vsp1_dlm_irq_frame_end().
> 
> commit ff7e97c94d9f7f370fe3ce2a72e85361ca22a605
> Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Date:   Tue Jan 19 19:16:36 2016 -0200
> 
>     [media] v4l: vsp1: Store pipeline pointer in rwpf
> 
> I've added a few debug print statements to vsp1_dlm_irq_frame_end(), and it 
> looks like we only hit the if (dlm->queued) test or none of them at all. It 
> looks like we've been lucky that nothing broke.
> 
> Restoring the previous behaviour should be safe, but it would be worth it 
> inspecting the code very carefully to make sure the logic is still correct. 
> I'll do it tomorrow if you don't beat me to it.

As far as I can tell it still seems correct. If we had ever hit this race, I
believe we would have leaked the DL, (which is quite a limited resource to us),
which if anything just raises the importance of this fix.

> 
> In any case, how about adding a
> 
> Fixes: ff7e97c94d9f ("[media] v4l: vsp1: Store pipeline pointer in rwpf")

Absolutely.

This seems worthy of a stable backport...? (though not critical)

The breakage was introduced in 4.6...

Should I add a CC: stable@kernel.org #4.6+

> line ?
> 
>>  	return 0;
>>  }
> 

-- 
Regards

Kieran Bingham

[RFC PATCH 2/3] v4l: vsp1: extend VSP1 module API to allow DRM callback registration

[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.

To synchronise the page flip events for userspace, we move the required
event through the VSP to track the data flow. When the frame is
completed, the event can be returned back to the originator through the
registered callback.

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/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
 drivers/media/platform/vsp1/vsp1_drm.c | 42 +++++++++++++++++++++++++--
 drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
 include/media/vsp1.h                   |  6 +++-
 4 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index b5bfbe50bd87..71e70e1e0881 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -81,7 +81,7 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
 
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
 {
-	vsp1_du_atomic_flush(crtc->vsp->vsp);
+	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
 }
 
 /* Keep the two tables in sync. */
diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index 8e2aa3f8e52f..743cbce48d0c 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -52,6 +52,40 @@ int vsp1_du_init(struct device *dev)
 EXPORT_SYMBOL_GPL(vsp1_du_init);
 
 /**
+ * vsp1_du_register_callback - Register VSP completion notifier callback
+ *
+ * Allow the DRM framework to register a callback with us to notify the end of
+ * processing each frame. This allows synchronisation for page flipping.
+ *
+ * @dev: the VSP device
+ * @callback: the callback function to notify the DU module
+ * @private: private structure data to pass with the callback
+ *
+ */
+void vsp1_du_register_callback(struct device *dev,
+			       void (*callback)(void *, void *),
+			       void *private)
+{
+	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+
+	vsp1->drm->du_complete = callback;
+	vsp1->drm->du_private = private;
+}
+EXPORT_SYMBOL_GPL(vsp1_du_register_callback);
+
+static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
+{
+	struct vsp1_drm *drm = to_vsp1_drm(pipe);
+
+	if (drm->du_complete && drm->active_data)
+		drm->du_complete(drm->du_private, drm->active_data);
+
+	/* The pending frame is now active */
+	drm->active_data = drm->pending_data;
+	drm->pending_data = NULL;
+}
+
+/**
  * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
  * @dev: the VSP device
  * @width: output frame width in pixels
@@ -99,7 +133,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 		}
 
 		pipe->num_inputs = 0;
-
+		pipe->frame_end = NULL;
+		vsp1->drm->du_complete = NULL;
 		vsp1_dlm_reset(pipe->output->dlm);
 		vsp1_device_put(vsp1);
 
@@ -196,6 +231,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 	if (ret < 0)
 		return ret;
 
+	pipe->frame_end = vsp1_du_pipeline_frame_end;
+
 	ret = media_entity_pipeline_start(&pipe->output->entity.subdev.entity,
 					  &pipe->pipe);
 	if (ret < 0) {
@@ -420,7 +457,7 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1, struct vsp1_rwpf *rpf)
  * vsp1_du_atomic_flush - Commit an atomic update
  * @dev: the VSP device
  */
-void vsp1_du_atomic_flush(struct device *dev)
+void vsp1_du_atomic_flush(struct device *dev, void *data)
 {
 	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
 	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
@@ -504,6 +541,7 @@ void vsp1_du_atomic_flush(struct device *dev)
 
 	vsp1_dl_list_commit(pipe->dl);
 	pipe->dl = NULL;
+	vsp1->drm->pending_data = data;
 
 	/* Start or stop the pipeline if needed. */
 	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
diff --git a/drivers/media/platform/vsp1/vsp1_drm.h b/drivers/media/platform/vsp1/vsp1_drm.h
index 9e28ab9254ba..fde19e5948a0 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -33,8 +33,20 @@ struct vsp1_drm {
 		struct v4l2_rect compose;
 		unsigned int zpos;
 	} inputs[VSP1_MAX_RPF];
+
+	/* Frame syncronisation */
+	void (*du_complete)(void *, void *);
+	void *du_private;
+	void *pending_data;
+	void *active_data;
 };
 
+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 458b400373d4..f82fbab01f21 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -20,6 +20,10 @@ struct device;
 
 int vsp1_du_init(struct device *dev);
 
+void vsp1_du_register_callback(struct device *dev,
+			       void (*callback)(void *, void *),
+			       void *private);
+
 int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 		      unsigned int height);
 
@@ -36,6 +40,6 @@ struct vsp1_du_atomic_config {
 void vsp1_du_atomic_begin(struct device *dev);
 int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
 			  const struct vsp1_du_atomic_config *cfg);
-void vsp1_du_atomic_flush(struct device *dev);
+void vsp1_du_atomic_flush(struct device *dev, void *data);
 
 #endif /* __MEDIA_VSP1_H__ */
-- 
git-series 0.9.1

Re: [RFC PATCH 2/3] v4l: vsp1: extend VSP1 module API to allow DRM callback registration

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 01 Mar 2017 13:12:55 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.
> 
> To synchronise the page flip events for userspace, we move the required
> event through the VSP to track the data flow. When the frame is
> completed, the event can be returned back to the originator through the
> registered callback.
> 
> 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/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
>  drivers/media/platform/vsp1/vsp1_drm.c | 42 +++++++++++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
>  include/media/vsp1.h                   |  6 +++-
>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b5bfbe50bd87..71e70e1e0881
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -81,7 +81,7 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> 
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
> -	vsp1_du_atomic_flush(crtc->vsp->vsp);
> +	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
>  }
> 
>  /* Keep the two tables in sync. */
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 8e2aa3f8e52f..743cbce48d0c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -52,6 +52,40 @@ int vsp1_du_init(struct device *dev)
>  EXPORT_SYMBOL_GPL(vsp1_du_init);
> 
>  /**
> + * vsp1_du_register_callback - Register VSP completion notifier callback
> + *
> + * Allow the DRM framework to register a callback with us to notify the end
> of + * processing each frame. This allows synchronisation for page
> flipping. + *
> + * @dev: the VSP device
> + * @callback: the callback function to notify the DU module
> + * @private: private structure data to pass with the callback
> + *
> + */
> +void vsp1_du_register_callback(struct device *dev,
> +			       void (*callback)(void *, void *),
> +			       void *private)
> +{
> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> +	vsp1->drm->du_complete = callback;
> +	vsp1->drm->du_private = private;
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_register_callback);

As they're not supposed to change at runtime while the display is running, how 
about passing the callback and private data pointer to the vsp1_du_setup_lif() 
function ? Feel free to create a structure for all the parameters passed to 
the function if you think we'll have too much (which would, as a side effect, 
made updates to the API easier in the future as changes to the two subsystems 
will be easier to decouple).

> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
> +{
> +	struct vsp1_drm *drm = to_vsp1_drm(pipe);
> +
> +	if (drm->du_complete && drm->active_data)
> +		drm->du_complete(drm->du_private, drm->active_data);
> +
> +	/* The pending frame is now active */
> +	drm->active_data = drm->pending_data;
> +	drm->pending_data = NULL;
> +}

I would move this function to the "Interrupt Handling" section.

> +/**
>   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
>   * @dev: the VSP device
>   * @width: output frame width in pixels
> @@ -99,7 +133,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> width, }
> 
>  		pipe->num_inputs = 0;
> -
> +		pipe->frame_end = NULL;

You can drop this if ...

> +		vsp1->drm->du_complete = NULL;
>  		vsp1_dlm_reset(pipe->output->dlm);
>  		vsp1_device_put(vsp1);
> 
> @@ -196,6 +231,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> width, if (ret < 0)
>  		return ret;
> 
> +	pipe->frame_end = vsp1_du_pipeline_frame_end;
> +

... you move this to vsp1_drm_init().

>  	ret = media_entity_pipeline_start(&pipe->output->entity.subdev.entity,
>  					  &pipe->pipe);
>  	if (ret < 0) {
> @@ -420,7 +457,7 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1,
> struct vsp1_rwpf *rpf) * vsp1_du_atomic_flush - Commit an atomic update
>   * @dev: the VSP device
>   */
> -void vsp1_du_atomic_flush(struct device *dev)
> +void vsp1_du_atomic_flush(struct device *dev, void *data)
>  {
>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
> @@ -504,6 +541,7 @@ void vsp1_du_atomic_flush(struct device *dev)
> 
>  	vsp1_dl_list_commit(pipe->dl);
>  	pipe->dl = NULL;
> +	vsp1->drm->pending_data = data;
> 
>  	/* Start or stop the pipeline if needed. */
>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..fde19e5948a0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -33,8 +33,20 @@ struct vsp1_drm {
>  		struct v4l2_rect compose;
>  		unsigned int zpos;
>  	} inputs[VSP1_MAX_RPF];
> +
> +	/* Frame syncronisation */
> +	void (*du_complete)(void *, void *);
> +	void *du_private;
> +	void *pending_data;
> +	void *active_data;
>  };
> 
> +static inline struct vsp1_drm *
> +to_vsp1_drm(struct vsp1_pipeline *pipe)

No need for a line split.

> +{
> +	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 458b400373d4..f82fbab01f21 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -20,6 +20,10 @@ struct device;
> 
>  int vsp1_du_init(struct device *dev);
> 
> +void vsp1_du_register_callback(struct device *dev,
> +			       void (*callback)(void *, void *),
> +			       void *private);
> +
>  int vsp1_du_setup_lif(struct device *dev, unsigned int width,
>  		      unsigned int height);
> 
> @@ -36,6 +40,6 @@ struct vsp1_du_atomic_config {
>  void vsp1_du_atomic_begin(struct device *dev);
>  int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
>  			  const struct vsp1_du_atomic_config *cfg);
> -void vsp1_du_atomic_flush(struct device *dev);
> +void vsp1_du_atomic_flush(struct device *dev, void *data);
> 
>  #endif /* __MEDIA_VSP1_H__ */

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 2/3] v4l: vsp1: extend VSP1 module API to allow DRM callback registration

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 01 Mar 2017 13:12:55 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.
> 
> To synchronise the page flip events for userspace, we move the required
> event through the VSP to track the data flow. When the frame is
> completed, the event can be returned back to the originator through the
> registered callback.
> 
> 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/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
>  drivers/media/platform/vsp1/vsp1_drm.c | 42 +++++++++++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
>  include/media/vsp1.h                   |  6 +++-
>  4 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b5bfbe50bd87..71e70e1e0881
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -81,7 +81,7 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> 
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
> -	vsp1_du_atomic_flush(crtc->vsp->vsp);
> +	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
>  }
> 
>  /* Keep the two tables in sync. */
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index 8e2aa3f8e52f..743cbce48d0c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -52,6 +52,40 @@ int vsp1_du_init(struct device *dev)
>  EXPORT_SYMBOL_GPL(vsp1_du_init);
> 
>  /**
> + * vsp1_du_register_callback - Register VSP completion notifier callback
> + *
> + * Allow the DRM framework to register a callback with us to notify the end
> of + * processing each frame. This allows synchronisation for page
> flipping. + *
> + * @dev: the VSP device
> + * @callback: the callback function to notify the DU module
> + * @private: private structure data to pass with the callback
> + *
> + */
> +void vsp1_du_register_callback(struct device *dev,
> +			       void (*callback)(void *, void *),
> +			       void *private)
> +{
> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
> +
> +	vsp1->drm->du_complete = callback;
> +	vsp1->drm->du_private = private;
> +}
> +EXPORT_SYMBOL_GPL(vsp1_du_register_callback);

As they're not supposed to change at runtime while the display is running, how 
about passing the callback and private data pointer to the vsp1_du_setup_lif() 
function ? Feel free to create a structure for all the parameters passed to 
the function if you think we'll have too much (which would, as a side effect, 
made updates to the API easier in the future as changes to the two subsystems 
will be easier to decouple).

> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
> +{
> +	struct vsp1_drm *drm = to_vsp1_drm(pipe);
> +
> +	if (drm->du_complete && drm->active_data)
> +		drm->du_complete(drm->du_private, drm->active_data);
> +
> +	/* The pending frame is now active */
> +	drm->active_data = drm->pending_data;
> +	drm->pending_data = NULL;
> +}

I would move this function to the "Interrupt Handling" section.

> +/**
>   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
>   * @dev: the VSP device
>   * @width: output frame width in pixels
> @@ -99,7 +133,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> width, }
> 
>  		pipe->num_inputs = 0;
> -
> +		pipe->frame_end = NULL;

You can drop this if ...

> +		vsp1->drm->du_complete = NULL;
>  		vsp1_dlm_reset(pipe->output->dlm);
>  		vsp1_device_put(vsp1);
> 
> @@ -196,6 +231,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> width, if (ret < 0)
>  		return ret;
> 
> +	pipe->frame_end = vsp1_du_pipeline_frame_end;
> +

... you move this to vsp1_drm_init().

>  	ret = media_entity_pipeline_start(&pipe->output->entity.subdev.entity,
>  					  &pipe->pipe);
>  	if (ret < 0) {
> @@ -420,7 +457,7 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1,
> struct vsp1_rwpf *rpf) * vsp1_du_atomic_flush - Commit an atomic update
>   * @dev: the VSP device
>   */
> -void vsp1_du_atomic_flush(struct device *dev)
> +void vsp1_du_atomic_flush(struct device *dev, void *data)
>  {
>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>  	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
> @@ -504,6 +541,7 @@ void vsp1_du_atomic_flush(struct device *dev)
> 
>  	vsp1_dl_list_commit(pipe->dl);
>  	pipe->dl = NULL;
> +	vsp1->drm->pending_data = data;
> 
>  	/* Start or stop the pipeline if needed. */
>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..fde19e5948a0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.h
> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
> @@ -33,8 +33,20 @@ struct vsp1_drm {
>  		struct v4l2_rect compose;
>  		unsigned int zpos;
>  	} inputs[VSP1_MAX_RPF];
> +
> +	/* Frame syncronisation */
> +	void (*du_complete)(void *, void *);
> +	void *du_private;
> +	void *pending_data;
> +	void *active_data;
>  };
> 
> +static inline struct vsp1_drm *
> +to_vsp1_drm(struct vsp1_pipeline *pipe)

No need for a line split.

> +{
> +	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 458b400373d4..f82fbab01f21 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -20,6 +20,10 @@ struct device;
> 
>  int vsp1_du_init(struct device *dev);
> 
> +void vsp1_du_register_callback(struct device *dev,
> +			       void (*callback)(void *, void *),
> +			       void *private);
> +
>  int vsp1_du_setup_lif(struct device *dev, unsigned int width,
>  		      unsigned int height);
> 
> @@ -36,6 +40,6 @@ struct vsp1_du_atomic_config {
>  void vsp1_du_atomic_begin(struct device *dev);
>  int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
>  			  const struct vsp1_du_atomic_config *cfg);
> -void vsp1_du_atomic_flush(struct device *dev);
> +void vsp1_du_atomic_flush(struct device *dev, void *data);
> 
>  #endif /* __MEDIA_VSP1_H__ */

-- 
Regards,

Laurent Pinchart

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

Re: [RFC PATCH 2/3] v4l: vsp1: extend VSP1 module API to allow DRM callback registration

[Kieran Bingham]

Hi Laurent,

On 03/03/17 02:11, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 01 Mar 2017 13:12:55 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.
>>
>> To synchronise the page flip events for userspace, we move the required
>> event through the VSP to track the data flow. When the frame is
>> completed, the event can be returned back to the originator through the
>> registered callback.
>>
>> 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/gpu/drm/rcar-du/rcar_du_vsp.c  |  2 +-
>>  drivers/media/platform/vsp1/vsp1_drm.c | 42 +++++++++++++++++++++++++--
>>  drivers/media/platform/vsp1/vsp1_drm.h | 12 ++++++++-
>>  include/media/vsp1.h                   |  6 +++-
>>  4 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b5bfbe50bd87..71e70e1e0881
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> @@ -81,7 +81,7 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>>
>>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>>  {
>> -	vsp1_du_atomic_flush(crtc->vsp->vsp);
>> +	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
>>  }
>>
>>  /* Keep the two tables in sync. */
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>> b/drivers/media/platform/vsp1/vsp1_drm.c index 8e2aa3f8e52f..743cbce48d0c
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
 >> @@ -52,6 +52,40 @@ int vsp1_du_init(struct device *dev)
>>  EXPORT_SYMBOL_GPL(vsp1_du_init);
>>
>>  /**
>> + * vsp1_du_register_callback - Register VSP completion notifier callback
>> + *
>> + * Allow the DRM framework to register a callback with us to notify the end
>> of + * processing each frame. This allows synchronisation for page
>> flipping. + *
>> + * @dev: the VSP device
>> + * @callback: the callback function to notify the DU module
>> + * @private: private structure data to pass with the callback
>> + *
>> + */
>> +void vsp1_du_register_callback(struct device *dev,
>> +			       void (*callback)(void *, void *),
>> +			       void *private)
>> +{
>> +	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>> +
>> +	vsp1->drm->du_complete = callback;
>> +	vsp1->drm->du_private = private;
>> +}
>> +EXPORT_SYMBOL_GPL(vsp1_du_register_callback);
> 
> As they're not supposed to change at runtime while the display is running, how 
> about passing the callback and private data pointer to the vsp1_du_setup_lif() 
> function ? Feel free to create a structure for all the parameters passed to 
> the function if you think we'll have too much (which would, as a side effect, 
> made updates to the API easier in the future as changes to the two subsystems 
> will be easier to decouple).

Sure, that's fine. I think a config structure makes sense here.

>> +static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
>> +{
>> +	struct vsp1_drm *drm = to_vsp1_drm(pipe);
>> +
>> +	if (drm->du_complete && drm->active_data)
>> +		drm->du_complete(drm->du_private, drm->active_data);
>> +
>> +	/* The pending frame is now active */
>> +	drm->active_data = drm->pending_data;
>> +	drm->pending_data = NULL;
>> +}
> 
> I would move this function to the "Interrupt Handling" section.

Ack.

>> +/**
>>   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
>>   * @dev: the VSP device
>>   * @width: output frame width in pixels
>> @@ -99,7 +133,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
>> width, }
>>
>>  		pipe->num_inputs = 0;
>> -
>> +		pipe->frame_end = NULL;
> 
> You can drop this if ...
> 
>> +		vsp1->drm->du_complete = NULL;
>>  		vsp1_dlm_reset(pipe->output->dlm);
>>  		vsp1_device_put(vsp1);
>>
>> @@ -196,6 +231,8 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
>> width, if (ret < 0)
>>  		return ret;
>>
>> +	pipe->frame_end = vsp1_du_pipeline_frame_end;
>> +

> 
> ... you move this to vsp1_drm_init().

Done.


> 
>>  	ret = media_entity_pipeline_start(&pipe->output->entity.subdev.entity,
>>  					  &pipe->pipe);
>>  	if (ret < 0) {
>> @@ -420,7 +457,7 @@ static unsigned int rpf_zpos(struct vsp1_device *vsp1,
>> struct vsp1_rwpf *rpf) * vsp1_du_atomic_flush - Commit an atomic update
>>   * @dev: the VSP device
>>   */
>> -void vsp1_du_atomic_flush(struct device *dev)
>> +void vsp1_du_atomic_flush(struct device *dev, void *data)
>>  {
>>  	struct vsp1_device *vsp1 = dev_get_drvdata(dev);
>>  	struct vsp1_pipeline *pipe = &vsp1->drm->pipe;
>> @@ -504,6 +541,7 @@ void vsp1_du_atomic_flush(struct device *dev)
>>
>>  	vsp1_dl_list_commit(pipe->dl);
>>  	pipe->dl = NULL;
>> +	vsp1->drm->pending_data = data;
>>
>>  	/* Start or stop the pipeline if needed. */
>>  	if (!vsp1->drm->num_inputs && pipe->num_inputs) {
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.h
>> b/drivers/media/platform/vsp1/vsp1_drm.h index 9e28ab9254ba..fde19e5948a0
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.h
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.h
>> @@ -33,8 +33,20 @@ struct vsp1_drm {
>>  		struct v4l2_rect compose;
>>  		unsigned int zpos;
>>  	} inputs[VSP1_MAX_RPF];
>> +
>> +	/* Frame syncronisation */
>> +	void (*du_complete)(void *, void *);
>> +	void *du_private;
>> +	void *pending_data;
>> +	void *active_data;
>>  };
>>
>> +static inline struct vsp1_drm *
>> +to_vsp1_drm(struct vsp1_pipeline *pipe)
> 
> No need for a line split.

Fixed.

>> +{
>> +	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 458b400373d4..f82fbab01f21 100644
>> --- a/include/media/vsp1.h
>> +++ b/include/media/vsp1.h
>> @@ -20,6 +20,10 @@ struct device;
>>
>>  int vsp1_du_init(struct device *dev);
>>
>> +void vsp1_du_register_callback(struct device *dev,
>> +			       void (*callback)(void *, void *),
>> +			       void *private);
>> +
>>  int vsp1_du_setup_lif(struct device *dev, unsigned int width,
>>  		      unsigned int height);
>>
>> @@ -36,6 +40,6 @@ struct vsp1_du_atomic_config {
>>  void vsp1_du_atomic_begin(struct device *dev);
>>  int vsp1_du_atomic_update(struct device *dev, unsigned int rpf,
>>  			  const struct vsp1_du_atomic_config *cfg);
>> -void vsp1_du_atomic_flush(struct device *dev);
>> +void vsp1_du_atomic_flush(struct device *dev, void *data);
>>
>>  #endif /* __MEDIA_VSP1_H__ */
> 

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

[Kieran Bingham]

Updating the state in a running VSP1 requires two interrupts from the
VSP. Initially, the updated state will be committed - but only after the
VSP1 has completed processing it's current frame will the new state be
taken into account. As such, the committed state will only be 'completed'
after an extra frame completion interrupt.

Track this delay, by passing the frame flip event through the VSP
module; It will be returned only when the frame has completed and can be
returned to the caller.

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  | 34 ++++++++++++++++++++++++++-
 3 files changed, 41 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..0a824633a012 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -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 != NULL);
 	spin_unlock_irqrestore(&dev->event_lock, flags);
 
 	return pending;
@@ -578,6 +578,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
 	rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
 
 	if (status & DSSR_FRM) {
+
+		if (rcrtc->pending) {
+			trace_printk("VBlank loss due to VSP Overrun\n");
+			return IRQ_HANDLED;
+		}
+
 		drm_crtc_handle_vblank(&rcrtc->crtc);
 		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..8374a858446a 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -46,6 +46,7 @@ struct rcar_du_crtc {
 	bool started;
 
 	struct drm_pending_vblank_event *event;
+	struct drm_pending_vblank_event *pending;
 	wait_queue_head_t flip_wait;
 
 	unsigned int outputs;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 71e70e1e0881..408375aff1a0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -28,6 +28,26 @@
 #include "rcar_du_kms.h"
 #include "rcar_du_vsp.h"
 
+static void rcar_du_vsp_complete(void *private, void *data)
+{
+	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
+	struct drm_device *dev = crtc->crtc.dev;
+	struct drm_pending_vblank_event *event;
+	bool match;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	event = crtc->event;
+	crtc->event = data;
+	match = (crtc->event == crtc->pending);
+	crtc->pending = NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	/* Safety checks */
+	WARN(event, "Event lost by VSP completion callback\n");
+	WARN(!match, "Stored pending event, does not match completion\n");
+}
+
 void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 {
 	const struct drm_display_mode *mode = &crtc->crtc.state->adjusted_mode;
@@ -66,6 +86,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 	 */
 	crtc->group->need_restart = true;
 
+	vsp1_du_register_callback(crtc->vsp->vsp, rcar_du_vsp_complete, crtc);
+
 	vsp1_du_setup_lif(crtc->vsp->vsp, mode->hdisplay, mode->vdisplay);
 }
 
@@ -81,7 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
 
 void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
 {
-	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
+	struct drm_device *dev = crtc->crtc.dev;
+	struct drm_pending_vblank_event *event;
+	unsigned long flags;
+
+	/* Move the event to the VSP, track it locally as 'pending' */
+	spin_lock_irqsave(&dev->event_lock, flags);
+	event = crtc->pending = crtc->event;
+	crtc->event = NULL;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	vsp1_du_atomic_flush(crtc->vsp->vsp, event);
 }
 
 /* Keep the two tables in sync. */
-- 
git-series 0.9.1

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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 01 Mar 2017 13:12:56 Kieran Bingham wrote:
> Updating the state in a running VSP1 requires two interrupts from the
> VSP. Initially, the updated state will be committed - but only after the
> VSP1 has completed processing it's current frame will the new state be
> taken into account. As such, the committed state will only be 'completed'
> after an extra frame completion interrupt.
> 
> Track this delay, by passing the frame flip event through the VSP
> module; It will be returned only when the frame has completed and can be
> returned to the caller.

I'll check the interrupt sequence logic tomorrow, it's a bit too late now. 
Please see below for additional comments.

> 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  | 34 ++++++++++++++++++++++++++-
>  3 files changed, 41 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..0a824633a012
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -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 != NULL);
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> 
>  	return pending;
> @@ -578,6 +578,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
> rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
> 
>  	if (status & DSSR_FRM) {
> +
> +		if (rcrtc->pending) {
> +			trace_printk("VBlank loss due to VSP Overrun\n");
> +			return IRQ_HANDLED;
> +		}
> +
>  		drm_crtc_handle_vblank(&rcrtc->crtc);
>  		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..8374a858446a
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -46,6 +46,7 @@ struct rcar_du_crtc {
>  	bool started;
> 
>  	struct drm_pending_vblank_event *event;
> +	struct drm_pending_vblank_event *pending;
>  	wait_queue_head_t flip_wait;
> 
>  	unsigned int outputs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 71e70e1e0881..408375aff1a0
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,26 @@
>  #include "rcar_du_kms.h"
>  #include "rcar_du_vsp.h"
> 
> +static void rcar_du_vsp_complete(void *private, void *data)
> +{
> +	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
> +	struct drm_device *dev = crtc->crtc.dev;
> +	struct drm_pending_vblank_event *event;
> +	bool match;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	event = crtc->event;
> +	crtc->event = data;
> +	match = (crtc->event == crtc->pending);
> +	crtc->pending = NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	/* Safety checks */
> +	WARN(event, "Event lost by VSP completion callback\n");
> +	WARN(!match, "Stored pending event, does not match completion\n");

I understand you want to be safe, and I assume these have never been triggered 
in your tests. I'd rather replace them by a mechanism that doesn't require 
passing the event to the VSP driver, and that wouldn't require adding a 
pending field to the rcar_du_crtc structure. Wouldn't adding a WARN_ON(rcrtc-
>event) in rcar_du_crtc_atomic_begin() in the if (crtc->state->event) block do 
the job ?

Would this get in the way of your trace_printk() debugging in 
rcar_du_crtc_irq() ? Could you implement the debugging in a separate patch 
without requiring to pass the event to the VSP driver ?

> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>  	const struct drm_display_mode *mode = &crtc->crtc.state-
>adjusted_mode;
> @@ -66,6 +86,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  	 */
>  	crtc->group->need_restart = true;
> 
> +	vsp1_du_register_callback(crtc->vsp->vsp, rcar_du_vsp_complete, crtc);
> +
>  	vsp1_du_setup_lif(crtc->vsp->vsp, mode->hdisplay, mode->vdisplay);
>  }
> 
> @@ -81,7 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> 
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
> -	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
> +	struct drm_device *dev = crtc->crtc.dev;
> +	struct drm_pending_vblank_event *event;
> +	unsigned long flags;
> +
> +	/* Move the event to the VSP, track it locally as 'pending' */
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	event = crtc->pending = crtc->event;
> +	crtc->event = NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	vsp1_du_atomic_flush(crtc->vsp->vsp, event);
>  }
> 
>  /* Keep the two tables in sync. */

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wednesday 01 Mar 2017 13:12:56 Kieran Bingham wrote:
> Updating the state in a running VSP1 requires two interrupts from the
> VSP. Initially, the updated state will be committed - but only after the
> VSP1 has completed processing it's current frame will the new state be
> taken into account. As such, the committed state will only be 'completed'
> after an extra frame completion interrupt.
> 
> Track this delay, by passing the frame flip event through the VSP
> module; It will be returned only when the frame has completed and can be
> returned to the caller.

I'll check the interrupt sequence logic tomorrow, it's a bit too late now. 
Please see below for additional comments.

> 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  | 34 ++++++++++++++++++++++++++-
>  3 files changed, 41 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..0a824633a012
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -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 != NULL);
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
> 
>  	return pending;
> @@ -578,6 +578,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
> rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
> 
>  	if (status & DSSR_FRM) {
> +
> +		if (rcrtc->pending) {
> +			trace_printk("VBlank loss due to VSP Overrun\n");
> +			return IRQ_HANDLED;
> +		}
> +
>  		drm_crtc_handle_vblank(&rcrtc->crtc);
>  		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..8374a858446a
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -46,6 +46,7 @@ struct rcar_du_crtc {
>  	bool started;
> 
>  	struct drm_pending_vblank_event *event;
> +	struct drm_pending_vblank_event *pending;
>  	wait_queue_head_t flip_wait;
> 
>  	unsigned int outputs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 71e70e1e0881..408375aff1a0
> 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,26 @@
>  #include "rcar_du_kms.h"
>  #include "rcar_du_vsp.h"
> 
> +static void rcar_du_vsp_complete(void *private, void *data)
> +{
> +	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
> +	struct drm_device *dev = crtc->crtc.dev;
> +	struct drm_pending_vblank_event *event;
> +	bool match;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	event = crtc->event;
> +	crtc->event = data;
> +	match = (crtc->event == crtc->pending);
> +	crtc->pending = NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	/* Safety checks */
> +	WARN(event, "Event lost by VSP completion callback\n");
> +	WARN(!match, "Stored pending event, does not match completion\n");

I understand you want to be safe, and I assume these have never been triggered 
in your tests. I'd rather replace them by a mechanism that doesn't require 
passing the event to the VSP driver, and that wouldn't require adding a 
pending field to the rcar_du_crtc structure. Wouldn't adding a WARN_ON(rcrtc-
>event) in rcar_du_crtc_atomic_begin() in the if (crtc->state->event) block do 
the job ?

Would this get in the way of your trace_printk() debugging in 
rcar_du_crtc_irq() ? Could you implement the debugging in a separate patch 
without requiring to pass the event to the VSP driver ?

> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>  	const struct drm_display_mode *mode = &crtc->crtc.state-
>adjusted_mode;
> @@ -66,6 +86,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  	 */
>  	crtc->group->need_restart = true;
> 
> +	vsp1_du_register_callback(crtc->vsp->vsp, rcar_du_vsp_complete, crtc);
> +
>  	vsp1_du_setup_lif(crtc->vsp->vsp, mode->hdisplay, mode->vdisplay);
>  }
> 
> @@ -81,7 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
> 
>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>  {
> -	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
> +	struct drm_device *dev = crtc->crtc.dev;
> +	struct drm_pending_vblank_event *event;
> +	unsigned long flags;
> +
> +	/* Move the event to the VSP, track it locally as 'pending' */
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	event = crtc->pending = crtc->event;
> +	crtc->event = NULL;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	vsp1_du_atomic_flush(crtc->vsp->vsp, event);
>  }
> 
>  /* Keep the two tables in sync. */

-- 
Regards,

Laurent Pinchart

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

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

[Kieran Bingham]

Hi Laurent,

On 03/03/17 02:17, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Wednesday 01 Mar 2017 13:12:56 Kieran Bingham wrote:
>> Updating the state in a running VSP1 requires two interrupts from the
>> VSP. Initially, the updated state will be committed - but only after the
>> VSP1 has completed processing it's current frame will the new state be
>> taken into account. As such, the committed state will only be 'completed'
>> after an extra frame completion interrupt.
>>
>> Track this delay, by passing the frame flip event through the VSP
>> module; It will be returned only when the frame has completed and can be
>> returned to the caller.
> 
> I'll check the interrupt sequence logic tomorrow, it's a bit too late now. 
> Please see below for additional comments.

No worries

> 
>> 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  | 34 ++++++++++++++++++++++++++-
>>  3 files changed, 41 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..0a824633a012
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
>> @@ -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 != NULL);
>>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>>
>>  	return pending;
>> @@ -578,6 +578,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>> rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);
>>
>>  	if (status & DSSR_FRM) {
>> +
>> +		if (rcrtc->pending) {
>> +			trace_printk("VBlank loss due to VSP Overrun\n");
>> +			return IRQ_HANDLED;
>> +		}
>> +
>>  		drm_crtc_handle_vblank(&rcrtc->crtc);
>>  		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..8374a858446a
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
>> @@ -46,6 +46,7 @@ struct rcar_du_crtc {
>>  	bool started;
>>
>>  	struct drm_pending_vblank_event *event;
>> +	struct drm_pending_vblank_event *pending;
>>  	wait_queue_head_t flip_wait;
>>
>>  	unsigned int outputs;
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 71e70e1e0881..408375aff1a0
>> 100644
>> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
>> @@ -28,6 +28,26 @@
>>  #include "rcar_du_kms.h"
>>  #include "rcar_du_vsp.h"
>>
>> +static void rcar_du_vsp_complete(void *private, void *data)
>> +{
>> +	struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private;
>> +	struct drm_device *dev = crtc->crtc.dev;
>> +	struct drm_pending_vblank_event *event;
>> +	bool match;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&dev->event_lock, flags);
>> +	event = crtc->event;
>> +	crtc->event = data;
>> +	match = (crtc->event == crtc->pending);
>> +	crtc->pending = NULL;
>> +	spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
>> +	/* Safety checks */
>> +	WARN(event, "Event lost by VSP completion callback\n");
>> +	WARN(!match, "Stored pending event, does not match completion\n");
> 
> I understand you want to be safe, and I assume these have never been triggered 
> in your tests. I'd rather replace them by a mechanism that doesn't require 
> passing the event to the VSP driver, and that wouldn't require adding a 
> pending field to the rcar_du_crtc structure. 

Ok - understandable, I started this way - but hit problems, which I think were
unrelated. Anyway, I switched to 'moving' the event so that I could be sure
rcar_du_crtc_finish_page_flip() couldn't have an event to send.

I can switch back to keeping it's 'ownership' in rcar-du.

> Wouldn't adding a WARN_ON(rcrtc-
>> event) in rcar_du_crtc_atomic_begin() in the if (crtc->state->event) block do 
> the job ?

Yes, that would :D - I put those in just to be sure things were going as
expected, and there weren't any code paths I hadn't considered. But I think they
are 'unreachable' warnings anyway (at least at the moment).



> Would this get in the way of your trace_printk() debugging in 
> rcar_du_crtc_irq() ? Could you implement the debugging in a separate patch 
> without requiring to pass the event to the VSP driver ?

Moving the event wasn't necessarily about trace_printk debugging, but perhaps it
was part of me debugging why I hadn't got things working correctly.

Moving the event out meant I was certain it couldn't let userspace flip until
after the VSP had returned the event. I can adjust this now.


So the next issue is in the performance, which I am currently investigating:

We are currently halving the throughput due to requiring two interrupts on any
update in the VSP1.

We have interrupt timings such as the following:
                   (loops)
VSP-FRE : |          |          |          |          |
	  0uS
VSP-DFE : |          |          |          |          |
          4uS
VSP-DST :  |          |          |          |          |
           58uS
DU-DSSR :   |          |          |          |          |
          16187uS

(From the timings shown in http://paste.ubuntu.com/24089285/)

Currently, an atomic flush committed between A-B, commences at B, and completes
at C.

VSP-FRE : |          |          |          |          |
VSP-DFE : |          A          B          C          |
VSP-DST :  |          |          |          |          |
DU-DSSR :   |          |F         |          |          |

The atomic flush occurs immediately after, DU-DSSR has 'flipped' a page, at 'F'
in the above diagram.

This gives plenty of time for processing the lists, but of course means that
actually processing is a full frame latency behind.

Full timing log can be seen at http://paste.ubuntu.com/24101386/

--
Regards

Kieran


>> +}
>> +
>>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>>  {
>>  	const struct drm_display_mode *mode = &crtc->crtc.state-
>> adjusted_mode;
>> @@ -66,6 +86,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>>  	 */
>>  	crtc->group->need_restart = true;
>>
>> +	vsp1_du_register_callback(crtc->vsp->vsp, rcar_du_vsp_complete, crtc);
>> +
>>  	vsp1_du_setup_lif(crtc->vsp->vsp, mode->hdisplay, mode->vdisplay);
>>  }
>>
>> @@ -81,7 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc)
>>
>>  void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc)
>>  {
>> -	vsp1_du_atomic_flush(crtc->vsp->vsp, NULL);
>> +	struct drm_device *dev = crtc->crtc.dev;
>> +	struct drm_pending_vblank_event *event;
>> +	unsigned long flags;
>> +
>> +	/* Move the event to the VSP, track it locally as 'pending' */
>> +	spin_lock_irqsave(&dev->event_lock, flags);
>> +	event = crtc->pending = crtc->event;
>> +	crtc->event = NULL;
>> +	spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
>> +	vsp1_du_atomic_flush(crtc->vsp->vsp, event);
>>  }
>>
>>  /* Keep the two tables in sync. */
>