Re: [PATCH v2 02/14] v4l: vsp1: Don't recycle active list at display start

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 18:02:20 +0100
Kieran Bingham  escreveu:

> Hi Laurent,
> 
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > When the display start interrupt occurs, we know that the hardware has
> > finished loading the active display list. The driver then proceeds to
> > recycle the list, assuming it won't be needed anymore.
> > 
> > This assumption holds true for headerless display lists, as the VSP
> > doesn't reload the list for the next frame if it hasn't changed.
> > However, this isn't true anymore for header display lists, as they are
> > loaded at every frame start regardless of whether they have been
> > updated.
> > 
> > To prepare for header display lists usage in display pipelines, we need
> > to postpone recycling the list until it gets replaced by a new one
> > through a page flip. The driver already does so in the frame end
> > interrupt handler, so all we need is to skip list recycling in the
> > display start interrupt handler.
> > 
> > While the active list can be recycled at display start for headerless
> > display lists, there's no real harm in postponing that to the frame end
> > interrupt handler in all cases. This simplifies interrupt handling as we
> > don't need to process the display start interrupt anymore.
> > 
> > Signed-off-by: Laurent Pinchart  
> >  
> 
> Ok, I had skipped this one as I was concerned about its effects in relation to
> 11/14 but I see how that's working now.
> 
> Reviewed-by: Kieran Bingham 
Acked-by: Mauro Carvalho Chehab 

Thanks,
Mauro
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 02/14] v4l: vsp1: Don't recycle active list at display start

[Kieran Bingham]

Hi Laurent,

On 26/06/17 19:12, Laurent Pinchart wrote:
> When the display start interrupt occurs, we know that the hardware has
> finished loading the active display list. The driver then proceeds to
> recycle the list, assuming it won't be needed anymore.
> 
> This assumption holds true for headerless display lists, as the VSP
> doesn't reload the list for the next frame if it hasn't changed.
> However, this isn't true anymore for header display lists, as they are
> loaded at every frame start regardless of whether they have been
> updated.
> 
> To prepare for header display lists usage in display pipelines, we need
> to postpone recycling the list until it gets replaced by a new one
> through a page flip. The driver already does so in the frame end
> interrupt handler, so all we need is to skip list recycling in the
> display start interrupt handler.
> 
> While the active list can be recycled at display start for headerless
> display lists, there's no real harm in postponing that to the frame end
> interrupt handler in all cases. This simplifies interrupt handling as we
> don't need to process the display start interrupt anymore.
> 
> Signed-off-by: Laurent Pinchart 

Ok, I had skipped this one as I was concerned about its effects in relation to
11/14 but I see how that's working now.

Reviewed-by: Kieran Bingham 

> ---
>  drivers/media/platform/vsp1/vsp1_dl.c  | 16 
>  drivers/media/platform/vsp1/vsp1_dl.h  |  1 -
>  drivers/media/platform/vsp1/vsp1_drm.c | 12 
>  drivers/media/platform/vsp1/vsp1_drm.h |  2 --
>  drivers/media/platform/vsp1/vsp1_drv.c |  8 
>  5 files changed, 4 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index dc47e236c780..bb92be4fe0f0 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -547,22 +547,6 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
>   * Display List Manager
>   */
>  
> -/* Interrupt Handling */
> -void vsp1_dlm_irq_display_start(struct vsp1_dl_manager *dlm)
> -{
> - spin_lock(>lock);
> -
> - /*
> -  * The display start interrupt signals the end of the display list
> -  * processing by the device. The active display list, if any, won't be
> -  * accessed anymore and can be reused.
> -  */
> - __vsp1_dl_list_put(dlm->active);
> - dlm->active = NULL;
> -
> - spin_unlock(>lock);
> -}
> -
>  /**
>   * vsp1_dlm_irq_frame_end - Display list handler for the frame end interrupt
>   * @dlm: the display list manager
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h 
> b/drivers/media/platform/vsp1/vsp1_dl.h
> index 6ec1380a10af..ee3508172f0a 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -27,7 +27,6 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device 
> *vsp1,
>   unsigned int prealloc);
>  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);
>  bool vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm);
>  
>  struct vsp1_dl_list *vsp1_dl_list_get(struct vsp1_dl_manager *dlm);
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 9377aafa8996..bc3fd9bc7126 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -32,11 +32,6 @@
>   * Interrupt Handling
>   */
>  
> -void vsp1_drm_display_start(struct vsp1_device *vsp1)
> -{
> - vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
> -}
> -
>  static void vsp1_du_pipeline_frame_end(struct vsp1_pipeline *pipe)
>  {
>   struct vsp1_drm *drm = to_vsp1_drm(pipe);
> @@ -224,6 +219,10 @@ int vsp1_du_setup_lif(struct device *dev, const struct 
> vsp1_du_lif_config *cfg)
>   return ret;
>   }
>  
> + /* Disable the display interrupts. */
> + vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
> + vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
> +
>   dev_dbg(vsp1->dev, "%s: pipeline enabled\n", __func__);
>  
>   return 0;
> @@ -529,13 +528,10 @@ void vsp1_du_atomic_flush(struct device *dev)
>  
>   /* Start or stop the pipeline if needed. */
>   if (!vsp1->drm->num_inputs && pipe->num_inputs) {
> - vsp1_write(vsp1, VI6_DISP_IRQ_STA, 0);
> - vsp1_write(vsp1, VI6_DISP_IRQ_ENB, VI6_DISP_IRQ_ENB_DSTE);
>   spin_lock_irqsave(>irqlock, flags);
>   vsp1_pipeline_run(pipe);
>   spin_unlock_irqrestore(>irqlock, flags);
>   } else if (vsp1->drm->num_inputs && !pipe->num_inputs) {
> - vsp1_write(vsp1, VI6_DISP_IRQ_ENB, 0);
>   vsp1_pipeline_stop(pipe);
>   }
>  }
> diff --git