Re: [PATCH 01/29] drm/exynos/fimd: only finish pageflip if START == START_S

[Gustavo Padovan <gustavo@padovan.org>]

2014-12-30 Inki Dae <inki.dae@samsung.com>:

> On 2014년 12월 18일 22:58, Gustavo Padovan wrote:
> > From: Daniel Kurtz <djkurtz@chromium.org>
> > 
> > A framebuffer gets committed to FIMD's default window like this:
> >  exynos_drm_crtc_update()
> >   exynos_plane_commit()
> >    fimd_win_commit()
> > 
> > fimd_win_commit() programs BUF_START[0].  At each vblank, FIMD hardware
> > copies the value from BUF_START to BUF_START_S (BUF_START's shadow
> > register), starts scanning out from BUF_START_S, and asserts its irq.
> > 
> > This irq is handled by fimd_irq_handler(), which calls
> > exynos_drm_crtc_finish_pageflip() to free the old buffer that FIMD just
> > finished scanning out, and potentially commit the next pending flip.
> > 
> > There is a race, however, if fimd_win_commit() programs BUF_START(0)
> > between the actual vblank irq, and its corresponding fimd_irq_handler().
> > 
> >  => FIMD vblank: BUF_START_S[0] := BUF_START[0], and irq asserted
> >  | => fimd_win_commit(0) writes new BUF_START[0]
> >  |    exynos_drm_crtc_try_do_flip() marks exynos_fb as prepared
> >  => fimd_irq_handler()
> >     exynos_drm_crtc_finish_pageflip() sees prepared exynos_fb,
> >          and unmaps "old" fb
> >  ==> but, since BUF_START_S[0] still points to that "old" fb...
> >  ==> FIMD iommu fault
> > 
> > This patch ensures that fimd_irq_handler() only calls
> > exynos_drm_crtc_finish_pageflip() if any previously scheduled flip
> > has really completed.
> > 
> > This works because exynos_drm_crtc's flip fifo ensures that
> > fimd_win_commit() is never called more than once per
> > exynos_drm_crtc_finish_pageflip().
> > 
> > Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> > Reviewed-by: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 26 ++++++++++++++++++++++----
> >  include/video/samsung_fimd.h             |  1 +
> >  2 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > index e5810d1..b379182 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > @@ -55,6 +55,7 @@
> >  #define VIDOSD_D(win)		(VIDOSD_BASE + 0x0C + (win) * 16)
> >  
> >  #define VIDWx_BUF_START(win, buf)	(VIDW_BUF_START(buf) + (win) * 8)
> > +#define VIDWx_BUF_START_S(win, buf)     (VIDW_BUF_START_S(buf) + (win) * 8)
> >  #define VIDWx_BUF_END(win, buf)		(VIDW_BUF_END(buf) + (win) * 8)
> >  #define VIDWx_BUF_SIZE(win, buf)	(VIDW_BUF_SIZE(buf) + (win) * 4)
> >  
> > @@ -1039,6 +1040,7 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
> >  {
> >  	struct fimd_context *ctx = (struct fimd_context *)dev_id;
> >  	u32 val, clear_bit;
> > +	u32 start, start_s;
> >  
> >  	val = readl(ctx->regs + VIDINTCON1);
> >  
> > @@ -1050,15 +1052,31 @@ static irqreturn_t fimd_irq_handler(int irq, void *dev_id)
> >  	if (ctx->pipe < 0 || !ctx->drm_dev)
> >  		goto out;
> >  
> > -	if (ctx->i80_if) {
> > +	if (!ctx->i80_if)
> > +		drm_handle_vblank(ctx->drm_dev, ctx->pipe);
> > +
> > +	/*
> > +	 * Ensure finish_pageflip is called iff a pending flip has completed.
> 
> Maybe s/iff/if
> 
> > +	 * This works around a race between a page_flip request and the latency
> > +	 * between vblank interrupt and this irq_handler:
> > +	 *   => FIMD vblank: BUF_START_S[0] := BUF_START[0], and asserts irq
> > +	 *   | => fimd_win_commit(0) writes new BUF_START[0]
> > +	 *   |    exynos_drm_crtc_try_do_flip() marks exynos_fb as prepared
> > +	 *   => fimd_irq_handler()
> > +	 *       exynos_drm_crtc_finish_pageflip() sees prepared exynos_fb,
> > +	 *           and unmaps "old" fb
> > +	 *   ==> but, since BUF_START_S[0] still points to that "old" fb...
> > +	 *   ==> FIMD iommu fault
> > +	 */
> > +	start = readl(ctx->regs + VIDWx_BUF_START(0, 0));
> > +	start_s = readl(ctx->regs + VIDWx_BUF_START_S(0, 0));
> > +	if (start == start_s)
> >  		exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
> 
> As I already mentioned before, above codes should be called only in case
> of RGB mode until checked for i80 mode. Have you ever tested above codes
> on i80 mode?

I haven't tested it as I don't have any hardware that does i80 mode.
Let's keep this check only for !i80 then. 

> 
> And what if 'start_s' has a value different from one of 'start'? Is it
> ok to skip finish_pageflip request this time? Shouldn't it ensure to
> wait for that until 'start_s' has a value same as one of 'start'?

I think it is okay to skip finish_pageflip, but we could return directly
if they are different, so we keep the wait_vsync_queue running until the next
irq happens or it timeouts. How does this look to you?

	Gustavo