| Message ID | 1418911135-5207-2-git-send-email-gustavo@padovan.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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'? Thanks, Inki Dae > > + if (ctx->i80_if) { > /* Exits triggering mode */ > atomic_set(&ctx->triggering, 0); > } else { > - drm_handle_vblank(ctx->drm_dev, ctx->pipe); > - exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe); > - > /* set wait vsync event to zero and wake up queue. */ > if (atomic_read(&ctx->wait_vsync_event)) { > atomic_set(&ctx->wait_vsync_event, 0); > diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h > index a20e4a3..f81d081 100644 > --- a/include/video/samsung_fimd.h > +++ b/include/video/samsung_fimd.h > @@ -291,6 +291,7 @@ > > /* Video buffer addresses */ > #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) > +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) > #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) > #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8)) > #define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8)) >
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
Hi, On 12 January 2015 at 21:13, Gustavo Padovan <gustavo@padovan.org> wrote: > 2014-12-30 Inki Dae <inki.dae@samsung.com>: > > On 2014? 12? 18? 22:58, Gustavo Padovan wrote: > > > + * 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); > > 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? > Right, you'd need to keep the vblank IRQ alive until start == start_s. As an aside, there is currently some refcounting code (e.g. in the DPMS off and/or framebuffer final-unref paths if I remember correctly) which assumes that waiting one vblank is enough to be able to unpin resources. Obviously, as this patch shows, this is not actually true. This patch doesn't make this any better or worse; just something to bear in mind. (I have WIP code which fixes this.) Cheers, Daniel
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. + * 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); + if (ctx->i80_if) { /* Exits triggering mode */ atomic_set(&ctx->triggering, 0); } else { - drm_handle_vblank(ctx->drm_dev, ctx->pipe); - exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe); - /* set wait vsync event to zero and wake up queue. */ if (atomic_read(&ctx->wait_vsync_event)) { atomic_set(&ctx->wait_vsync_event, 0); diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h index a20e4a3..f81d081 100644 --- a/include/video/samsung_fimd.h +++ b/include/video/samsung_fimd.h @@ -291,6 +291,7 @@ /* Video buffer addresses */ #define VIDW_BUF_START(_buff) (0xA0 + ((_buff) * 8)) +#define VIDW_BUF_START_S(_buff) (0x40A0 + ((_buff) * 8)) #define VIDW_BUF_START1(_buff) (0xA4 + ((_buff) * 8)) #define VIDW_BUF_END(_buff) (0xD0 + ((_buff) * 8)) #define VIDW_BUF_END1(_buff) (0xD4 + ((_buff) * 8))