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

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 kieran.bingham+renesas@ideasonboard.com

---

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

}