Bug 74882

Just made a quick test. The problem is present with intel driver 2.21.15, but not reproducible (or at least not triggered that easily) with 2.20.13. Guess I'll have to bisect...

This looks reminiscent of an Ironlake flushing bug. Can you please pull the latest tip from xf86-video-intel.git and try

diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
index a9db697..aa8167b 100644
--- a/src/sna/gen5_render.c
+++ b/src/sna/gen5_render.c
@@ -54,7 +54,7 @@
 #define DBG_NO_STATE_CACHE 0
 #define DBG_NO_SURFACE_CACHE 0
 
-#define ALWAYS_FLUSH 0
+#define ALWAYS_FLUSH 1
 
 #define MAX_3D_SIZE 8192

(In reply to comment #2)
> This looks reminiscent of an Ironlake flushing bug. Can you please pull the
> latest tip from xf86-video-intel.git and try
> 
> diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
> index a9db697..aa8167b 100644
> --- a/src/sna/gen5_render.c
> +++ b/src/sna/gen5_render.c
> @@ -54,7 +54,7 @@
>  #define DBG_NO_STATE_CACHE 0
>  #define DBG_NO_SURFACE_CACHE 0
>  
> -#define ALWAYS_FLUSH 0
> +#define ALWAYS_FLUSH 1
>  
>  #define MAX_3D_SIZE 8192

I followed this instructions, but it did not help.

That's a little scary. How about

diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
index a9db697..0755fd2 100644
--- a/src/sna/gen5_render.c
+++ b/src/sna/gen5_render.c
@@ -54,7 +54,7 @@
 #define DBG_NO_STATE_CACHE 0
 #define DBG_NO_SURFACE_CACHE 0
 
-#define ALWAYS_FLUSH 0
+#define ALWAYS_FLUSH 1
 
 #define MAX_3D_SIZE 8192
 
@@ -1049,7 +1049,7 @@ gen5_emit_state(struct sna *sna,
        }
        gen5_emit_vertex_elements(sna, op);
 
-       if (kgem_bo_is_dirty(op->src.bo) || kgem_bo_is_dirty(op->mask.bo)) {
+       if (ALWAYS_FLUSH || kgem_bo_is_dirty(op->src.bo) || kgem_bo_is_dirty(op->mask.bo)) {
                DBG(("%s: flushing dirty (%d, %d)\n", __FUNCTION__,
                     kgem_bo_is_dirty(op->src.bo),
                     kgem_bo_is_dirty(op->mask.bo)));
@@ -1058,7 +1058,7 @@ gen5_emit_state(struct sna *sna,
                kgem_bo_mark_dirty(op->dst.bo);
                flush = false;
        }
-       if (flush || ALWAYS_FLUSH) {
+       if (flush) {
                DBG(("%s: forcing flush\n", __FUNCTION__));
                gen5_emit_pipe_flush(sna);

instead?

i.e. enable ALWAYS_FLUSH and use it to force the full flush rather than the pipeline flush.

(In reply to comment #4)
> That's a little scary. How about
> 
> diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
> index a9db697..0755fd2 100644
> --- a/src/sna/gen5_render.c
> +++ b/src/sna/gen5_render.c
> @@ -54,7 +54,7 @@
>  #define DBG_NO_STATE_CACHE 0
>  #define DBG_NO_SURFACE_CACHE 0
>  
> -#define ALWAYS_FLUSH 0
> +#define ALWAYS_FLUSH 1
>  
>  #define MAX_3D_SIZE 8192
>  
> @@ -1049,7 +1049,7 @@ gen5_emit_state(struct sna *sna,
>         }
>         gen5_emit_vertex_elements(sna, op);
>  
> -       if (kgem_bo_is_dirty(op->src.bo) || kgem_bo_is_dirty(op->mask.bo)) {
> +       if (ALWAYS_FLUSH || kgem_bo_is_dirty(op->src.bo) ||
> kgem_bo_is_dirty(op->mask.bo)) {
>                 DBG(("%s: flushing dirty (%d, %d)\n", __FUNCTION__,
>                      kgem_bo_is_dirty(op->src.bo),
>                      kgem_bo_is_dirty(op->mask.bo)));
> @@ -1058,7 +1058,7 @@ gen5_emit_state(struct sna *sna,
>                 kgem_bo_mark_dirty(op->dst.bo);
>                 flush = false;
>         }
> -       if (flush || ALWAYS_FLUSH) {
> +       if (flush) {
>                 DBG(("%s: forcing flush\n", __FUNCTION__));
>                 gen5_emit_pipe_flush(sna);
> 
> instead?
> 
> i.e. enable ALWAYS_FLUSH and use it to force the full flush rather than the
> pipeline flush.

Part of this patch is already in git isn't it? Should I pull latest changes and simply switch ALWAYS_FLUSH to 1 then?

Created attachment 93929 [details]
git bisect log

Just finished bisecting. Here is the result:

37eb7343be1aeeb90a860096756603a577df1a77 is the first bad commit
commit 37eb7343be1aeeb90a860096756603a577df1a77
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat Dec 1 09:40:11 2012 +0000

    sna/gen5: Inspired by gen4, reorder the flushing

    This may not be totally safe, but it is a nicer explanation for random
    single character corruption.

    References: https://bugs.freedesktop.org/show_bug.cgi?id=51422
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

:040000 040000 7ea290b24bf00fc9bc722384318a2b15c2a1305e 18a700a43ad61d484fc4c17f9509b3777ec4b3ec M      src

(In reply to comment #4)
> That's a little scary. How about
> 
> diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
> index a9db697..0755fd2 100644
> --- a/src/sna/gen5_render.c
> +++ b/src/sna/gen5_render.c
> @@ -54,7 +54,7 @@
>  #define DBG_NO_STATE_CACHE 0
>  #define DBG_NO_SURFACE_CACHE 0
>  
> -#define ALWAYS_FLUSH 0
> +#define ALWAYS_FLUSH 1
>  
>  #define MAX_3D_SIZE 8192
>  
> @@ -1049,7 +1049,7 @@ gen5_emit_state(struct sna *sna,
>         }
>         gen5_emit_vertex_elements(sna, op);
>  
> -       if (kgem_bo_is_dirty(op->src.bo) || kgem_bo_is_dirty(op->mask.bo)) {
> +       if (ALWAYS_FLUSH || kgem_bo_is_dirty(op->src.bo) ||
> kgem_bo_is_dirty(op->mask.bo)) {
>                 DBG(("%s: flushing dirty (%d, %d)\n", __FUNCTION__,
>                      kgem_bo_is_dirty(op->src.bo),
>                      kgem_bo_is_dirty(op->mask.bo)));
> @@ -1058,7 +1058,7 @@ gen5_emit_state(struct sna *sna,
>                 kgem_bo_mark_dirty(op->dst.bo);
>                 flush = false;
>         }
> -       if (flush || ALWAYS_FLUSH) {
> +       if (flush) {
>                 DBG(("%s: forcing flush\n", __FUNCTION__));
>                 gen5_emit_pipe_flush(sna);
> 
> instead?
> 
> i.e. enable ALWAYS_FLUSH and use it to force the full flush rather than the
> pipeline flush.

This patch is not appliable on top of 27663f31163c22f7dfaf8f5a3e45fa1c93a7d9e4. Will pull and try again.

(In reply to comment #4)

I've pulled latest git (3b43630f5525a60e935dbc46e70354bb45444814) and switched ALWAYS_FLUSH to 1 since other changes from the patch you've suggested are already in tree. With this changes this issue is not reproducible.

Can you please try

diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
index a89d31a..3c3cf28 100644
--- a/src/sna/gen5_render.c
+++ b/src/sna/gen5_render.c
@@ -216,6 +216,8 @@ static bool gen5_magic_ca_pass(struct sna *sna,
        assert(op->mask.bo != NULL);
        assert(op->has_component_alpha);
 
+       OUT_BATCH(MI_FLUSH);
+
        gen5_emit_pipelined_pointers
                (sna, op, PictOpAdd,
                 gen5_choose_composite_kernel(PictOpAdd,


just to rule out the problem being introduced in a similar path elsewhere.

(In reply to comment #9)
> Can you please try
> 
> diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
> index a89d31a..3c3cf28 100644
> --- a/src/sna/gen5_render.c
> +++ b/src/sna/gen5_render.c
> @@ -216,6 +216,8 @@ static bool gen5_magic_ca_pass(struct sna *sna,
>         assert(op->mask.bo != NULL);
>         assert(op->has_component_alpha);
>  
> +       OUT_BATCH(MI_FLUSH);
> +
>         gen5_emit_pipelined_pointers
>                 (sna, op, PictOpAdd,
>                  gen5_choose_composite_kernel(PictOpAdd,
> 
> 
> just to rule out the problem being introduced in a similar path elsewhere.

Should I enable ALWAYS_FLUSH with it?

(In reply to comment #10)
> Should I enable ALWAYS_FLUSH with it?

No. It needs to be tested standalone. (I am not optimistic the test will succeed, but it will once again narrow down where the flush needs to be inserted.)

(In reply to comment #11)
> No. It needs to be tested standalone. (I am not optimistic the test will
> succeed, but it will once again narrow down where the flush needs to be
> inserted.)

You were right. This patch unfortunately does not help.

Thanks. Next step:

diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
index a89d31a..8789f0b 100644
--- a/src/sna/gen5_render.c
+++ b/src/sna/gen5_render.c
@@ -1039,7 +1039,7 @@ gen5_emit_state(struct sna *sna,
        assert(op->dst.bo->exec);
 
        /* drawrect must be first for Ironlake BLT workaround */
-       if (gen5_emit_drawing_rectangle(sna, op))
+       if (gen5_emit_drawing_rectangle(sna, op) && 0)
                offset &= ~1;
        gen5_emit_binding_table(sna, offset & ~1);
        if (gen5_emit_pipelined_pointers(sna, op, op->op, op->u.gen5.wm_kernel)){

(In reply to comment #13)
This patch does not help either.

Oh well. Next,

diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
index a89d31a..aa820b8 100644
--- a/src/sna/gen5_render.c
+++ b/src/sna/gen5_render.c
@@ -1045,7 +1045,8 @@ gen5_emit_state(struct sna *sna,
        if (gen5_emit_pipelined_pointers(sna, op, op->op, op->u.gen5.wm_kernel)){
                DBG(("%s: changed blend state, flush required? %d\n",
                     __FUNCTION__, (offset & 1) && op->op > PictOpSrc));
-               flush = (offset & 1) && op->op > PictOpSrc;
+               //flush = (offset & 1) && op->op > PictOpSrc;
+               flush = op->op > PictOpSrc;
        }
        gen5_emit_vertex_elements(sna, op);

(In reply to comment #15)
The bug still persists after this change.

Hmm, it really wants that flush of the render cache. Ok, last try before applying the big hammer:

diff --git a/src/sna/gen5_render.c b/src/sna/gen5_render.c
index c5ccaac..d5856ea 100644
--- a/src/sna/gen5_render.c
+++ b/src/sna/gen5_render.c
@@ -1017,7 +1017,7 @@ gen5_emit_vertex_elements(struct sna *sna,
 inline static void
 gen5_emit_pipe_flush(struct sna *sna)
 {
-#if 0
+#if 1
        OUT_BATCH(GEN5_PIPE_CONTROL |
                  GEN5_PIPE_CONTROL_WC_FLUSH |
                  (4 - 2));

(In reply to comment #17)
This change helps. I was not able to reproduce it anymore.

Created attachment 93947 [details] [review]
Flush the render cache between operations

This is hopefully the fix identified above.

Created attachment 93948 [details] [review]
Try not to flush so often

But this is a risky patch to try and reduce the number of flushes. It seems to work on later generation...

Unfortunately the combination of two patches above does not solve this issue. Tested on top of 564a766a6bbe8bcf2480248a54059e7d6727440d.

But with just the first patch, everything is ok?

(In reply to comment #21)
> Unfortunately the combination of two patches above does not solve this
> issue. Tested on top of 564a766a6bbe8bcf2480248a54059e7d6727440d.

Oops, sorry. I've applied patches in one directory, but built in another. Will patch and build properly this time and give actual results.

(In reply to comment #22)
> But with just the first patch, everything is ok?

Sorry, I've made a mistake during last patch'n'build iteration, will report proper results in a couple of minutes.

(In reply to comment #21)
> Unfortunately the combination of two patches above does not solve this
> issue. Tested on top of 564a766a6bbe8bcf2480248a54059e7d6727440d.

This time I've done everything properly, but results are the same: two patches combined do not solve this issue.

(In reply to comment #22)
> But with just the first patch, everything is ok?

Yes, with just the first patch, everything is fine.

So hopefully fixed once again:

commit fb89bfc73f4103ca6116c8f91970f4bfa491636c
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Feb 12 16:41:51 2014 +0000

    sna/gen5: Flush the render cache between operations
    
    When we change the blend mode between operations, it appears that we
    must flush the render cache or else we risk render corruption. This is
    usually noticeable in rendering of single glyphs.
    
    This was originally fixed for bug 51422, but was reintroduced by
    commit 37eb7343be1aeeb90a860096756603a577df1a77
    Author: Chris Wilson <chris@chris-wilson.co.uk>
    Date:   Sat Dec 1 09:40:11 2012 +0000
    
        sna/gen5: Inspired by gen4, reorder the flushing
    and the desire to reduce the impact of this w/a.
    
    Reported-by: itumaykin@gmail.com
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=74882
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Thank you very much for your help and attention to this problem.