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; }

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__ */

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. */