Hi Kieran, Thank you for the patch.
On Saturday 04 Mar 2017 02:01:19 Kieran Bingham wrote: > Currently we process page flip events on every display interrupt, > however this does not take into consideration the processing time needed > by the VSP1 utilised in the pipeline. > > Register a callback with the VSP driver to obtain completion events, and > track them so that we only perform page flips when the full display > pipeline has completed for the frame. > > Signed-off-by: Kieran Bingham <[email protected]> > > --- > v2: > - Commit message completely re-worded for patch re-work. > - drm_crtc_handle_vblank() re-instated in event of rcrtc->pending > - removed passing of unnecessary 'data' through callbacks > - perform page flips from the VSP completion handler > - add locking around pending flags > > drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 10 +++++++-- > drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 2 ++- > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 29 +++++++++++++++++++++++++++- > 3 files changed, 39 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 7391dd95c733..b7ff00bb45de > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c > @@ -299,7 +299,7 @@ static void rcar_du_crtc_update_planes(struct > rcar_du_crtc *rcrtc) * Page Flip > */ > > -static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc) > +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc) > { > struct drm_pending_vblank_event *event; > struct drm_device *dev = rcrtc->crtc.dev; > @@ -328,7 +328,7 @@ static bool rcar_du_crtc_page_flip_pending(struct > rcar_du_crtc *rcrtc) bool pending; > > spin_lock_irqsave(&dev->event_lock, flags); > - pending = rcrtc->event != NULL; > + pending = (rcrtc->event != NULL) || (rcrtc->pending); No need for parenthesis. > spin_unlock_irqrestore(&dev->event_lock, flags); > > return pending; > @@ -579,6 +579,12 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg) > > if (status & DSSR_FRM) { > drm_crtc_handle_vblank(&rcrtc->crtc); > + > + if (rcrtc->pending) { > + trace_printk("VBlank loss due to VSP Overrun\n"); > + return IRQ_HANDLED; > + } > + More than that, now that the VSP completion handler finishes the page flip, you should skip the rcar_du_crtc_finish_page_flip() call here unconditionally on Gen3. Something like struct rcar_du_crtc *rcrtc = arg; struct rcar_du_device *rcdu = rcrtc->group->dev; ... if (status & DSSR_FRM) { drm_crtc_handle_vblank(&rcrtc->crtc); if (rcdu->info->gen < 3) rcar_du_crtc_finish_page_flip(rcrtc); ret = IRQ_HANDLED; } > rcar_du_crtc_finish_page_flip(rcrtc); > ret = IRQ_HANDLED; > } > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index a7194812997e..b73ec6de7af4 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h > @@ -47,6 +47,7 @@ struct rcar_du_crtc { > > struct drm_pending_vblank_event *event; > wait_queue_head_t flip_wait; > + bool pending; > > unsigned int outputs; > > @@ -71,5 +72,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc); > > void rcar_du_crtc_route_output(struct drm_crtc *crtc, > enum rcar_du_output output); > +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc); > > #endif /* __RCAR_DU_CRTC_H__ */ > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1fcd311badb1 > 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c > @@ -28,6 +28,22 @@ > #include "rcar_du_kms.h" > #include "rcar_du_vsp.h" > > +static void rcar_du_vsp_complete(void *private) > +{ > + struct rcar_du_crtc *crtc = (struct rcar_du_crtc *)private; > + struct drm_device *dev = crtc->crtc.dev; > + unsigned long flags; > + bool pending; > + > + spin_lock_irqsave(&dev->event_lock, flags); > + pending = crtc->pending; > + crtc->pending = false; > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + if (pending) > + rcar_du_crtc_finish_page_flip(crtc); This seems to duplicate the synchronization mechanism based on events in rcar_du_crtc_atomic_begin(). I need to check that in more details. > +} > + > void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) > { > const struct drm_display_mode *mode = &crtc->crtc.state- >adjusted_mode; > @@ -35,6 +51,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) > struct vsp1_du_lif_config cfg = { > .width = mode->hdisplay, > .height = mode->vdisplay, > + .callback = rcar_du_vsp_complete, > + .callback_data = crtc, > }; > struct rcar_du_plane_state state = { > .state = { > @@ -85,6 +103,17 @@ void rcar_du_vsp_atomic_begin(struct rcar_du_crtc *crtc) > > void rcar_du_vsp_atomic_flush(struct rcar_du_crtc *crtc) > { > + struct drm_device *dev = crtc->crtc.dev; > + unsigned long flags; > + bool pending; > + > + spin_lock_irqsave(&dev->event_lock, flags); > + pending = crtc->pending; > + crtc->pending = true; > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + WARN_ON(pending); > + > vsp1_du_atomic_flush(crtc->vsp->vsp); > } -- Regards, Laurent Pinchart
