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