Hi Kieran,
Thank you for the patch.
On Sunday 05 Mar 2017 16:00:02 Kieran Bingham wrote:
> If we try to commit the display list while an update is pending, we have
> missed our opportunity. The display list manager will hold the commit
> until the next interrupt.
>
> In this event, we skip the pipeline completion callback handler so that
> the pipeline will not mistakenly report frame completion to the user.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/platform/vsp1/vsp1_dl.c | 19 +++++++++++++++++--
> drivers/media/platform/vsp1/vsp1_dl.h | 2 +-
> drivers/media/platform/vsp1/vsp1_pipe.c | 13 ++++++++++++-
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index b9e5027778ff..f449ca689554
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -562,9 +562,19 @@ void vsp1_dlm_irq_display_start(struct vsp1_dl_manager
> *dlm) spin_unlock(&dlm->lock);
> }
>
> -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> +/**
> + * vsp1_dlm_irq_frame_end - Display list handler for the frame end
> interrupt + * @dlm: the display list manager
> + *
> + * Return true if the previous display list has completed at frame end, or
> false + * if it has been delayed by one frame because the display list
> commit raced + * with the frame end interrupt. The function always returns
> true in header mode + * as display list processing is then not continuous
> and races never occur. + */
> +bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> {
> struct vsp1_device *vsp1 = dlm->vsp1;
> + bool completed = false;
>
> spin_lock(&dlm->lock);
>
> @@ -576,8 +586,10 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager
> *dlm) * perform any operation as there can't be any new display list queued
> * in that case.
> */
> - if (dlm->mode == VSP1_DL_MODE_HEADER)
> + if (dlm->mode == VSP1_DL_MODE_HEADER) {
> + completed = true;
> goto done;
> + }
>
> /*
> * The UPD bit set indicates that the commit operation raced with the
> @@ -597,6 +609,7 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
> if (dlm->queued) {
> dlm->active = dlm->queued;
> dlm->queued = NULL;
> + completed = true;
> }
>
> /*
> @@ -619,6 +632,8 @@ void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm)
>
> done:
> spin_unlock(&dlm->lock);
> +
> + return completed;
> }
>
> /* Hardware Setup */
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 7131aa3c5978..6ec1380a10af
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -28,7 +28,7 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device
> *vsp1, void vsp1_dlm_destroy(struct vsp1_dl_manager *dlm);
> void vsp1_dlm_reset(struct vsp1_dl_manager *dlm);
> void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm);
> -void vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
> +bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
>
> struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
> void vsp1_dl_list_put(struct vsp1_dl_list *dl);
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 35364f594e19..d15327701ad8
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -304,10 +304,21 @@ bool vsp1_pipeline_ready(struct vsp1_pipeline *pipe)
>
> void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
> {
> + bool completed;
> +
> if (pipe == NULL)
> return;
>
> - vsp1_dlm_irq_frame_end(pipe->output->dlm);
> + completed = vsp1_dlm_irq_frame_end(pipe->output->dlm);
> + if (!completed) {
> + /*
> + * If the DL commit raced with the frame end interrupt, the
> + * commit ends up being postponed by one frame. Return
> + * immediately without calling the pipeline's frame end
handler
> + * or incrementing the sequence number.
> + */
> + return;
> + }
>
> if (pipe->frame_end)
> pipe->frame_end(pipe);
--
Regards,
Laurent Pinchart
