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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
