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