subject:"\[PATCH v2\] drm\/exynos\/fimd\: only finish pageflip if START == START

[PATCH v2] drm/exynos/fimd: only finish pageflip if START == START_S

2014-12-16 Thread Inki Dae

On 2014ë 12ì 11ì¼ 01:57, Gustavo Padovan wrote:
> From: Daniel Kurtz 
> 
> 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 
> Reviewed-by: Sean Paul 
> Signed-off-by: Gustavo Padovan 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c | 31 ---
>  include/video/samsung_fimd.h |  1 +
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index e5810d1..95bac27 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);
>  
> @@ -1051,19 +1053,34 @@ static irqreturn_t fimd_irq_handler(int irq, void 
> *dev_id)
>   goto out;
>  
>   if (ctx->i80_if) {
> - exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
> -
>   /* Exits triggering mode */
>   atomic_set(>triggering, 0);
>   } else {
>   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);

Did you test abvoe codes on I80 mode? If not so, I think this function
should be called only in case of RGB mode until checked.

>  
> - /* set wait vsync event to zero and wake up queue. */
> - if (atomic_read(>wait_vsync_event)) {
> - atomic_set(>wait_vsync_event, 0);
> - wake_up(>wait_vsync_queue);
> - }
> + /* set wait vsync event to zero and wake up queue. */
> + if (atomic_read(>wait_vsync_event)) {
> + atomic_set(>wait_vsync_event, 0);
> + wake_up(>wait_vsync_queue);

And in case of i80 mode, wait_vsync_queue will be waked up by
fimd_te_handler so I think above relevant codes should be called only in
case of RGB mode.

Thanks,
Inki Dae

>   }
>  
>  out:
> diff --git a/include/video/samsung_fimd.h b/include/video/samsung_fimd.h
> index a20e4a3..f81d081 100644
> --- a/include/video/samsung_fimd.h
> +++

[PATCH v2] drm/exynos/fimd: only finish pageflip if START == START_S

2014-12-10 Thread Gustavo Padovan

From: Daniel Kurtz 

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 
Reviewed-by: Sean Paul 
Signed-off-by: Gustavo Padovan 
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c | 31 ---
 include/video/samsung_fimd.h |  1 +
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c 
b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index e5810d1..95bac27 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);

@@ -1051,19 +1053,34 @@ static irqreturn_t fimd_irq_handler(int irq, void 
*dev_id)
goto out;

if (ctx->i80_if) {
-   exynos_drm_crtc_finish_pageflip(ctx->drm_dev, ctx->pipe);
-
/* Exits triggering mode */
atomic_set(>triggering, 0);
} else {
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);

-   /* set wait vsync event to zero and wake up queue. */
-   if (atomic_read(>wait_vsync_event)) {
-   atomic_set(>wait_vsync_event, 0);
-   wake_up(>wait_vsync_queue);
-   }
+   /* set wait vsync event to zero and wake up queue. */
+   if (atomic_read(>wait_vsync_event)) {
+   atomic_set(>wait_vsync_event, 0);
+   wake_up(>wait_vsync_queue);
}

 out:
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))
-- 
1.9.3

[PATCH v2] drm/exynos/fimd: only finish pageflip if START == START_S

[PATCH v2] drm/exynos/fimd: only finish pageflip if START == START_S

2 matches

Site Navigation

Mail list logo

Footer information