Created attachment 93922 [details] Screenshot showing the problem Hello. I have a quite long-standing problem with corrupted rendering of Stylish entry in Firefox Add-ons tab. This happens with Stylish entry only and this issue was present since at least Firefox 25.0. I am running xf86-video-intel-2.99.910 patched up to 5142d1ca3fd877f2ca9277af314f6432580370de commit, but this problem was present with previous releases of intel driver as well. Step to reproduce: 1. Open Firefox 2. Open Add-ons tab 3. Click any entry except for selected once. For example, if upon opening add-ons tab the current selected entry is extensions, then click on Stylish entry (User styles) and vice versa. 4. See that "User styles" text is rendered corrupted My OS is Gentoo amd64, Firefox version is 27.0 and I have latest version of Stylish available. My WM is Openbox and I have no compositing enabled. If you need any other info I am ready to ptovide it. Please fix.
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.