Bug 80560

Created attachment 101807 [details]
original-icon.png

This is how it should look like.

Any tricks required to reproduce?

Not that I know of:
    Option "AccelMethod" "sna"
    Option "PageFlip" "true"
    Option "TripleBuffer" "true"
    Option "TearFree" "true"
    Option "VSync" "true"

Created attachment 101809 [details]
Xorg.log

It is definitely the right icon, but distorted? Do you see sometimes other junk in the icons (even just incorrect highlight/focus state)?

Yes.

1) It is 100% reproducible when highlighting/selecting and not reverting the commit.

2) It also happens occasionally with the commit reverted, but is not so easily reproducible. Also, the corruptions are not the same; About 3/4 of the icon are corrupted with a garbled black mess. I'll attach another image for comparison if I can make a screenshot. The corruptions will disappear on a key press or mouse click, or when switching to another window.

Maybe 2) is the real cause and the commit makes it only worse?
I will investigate in 2) on the machine with the radeon card to see if it happens there too...

Created attachment 101815 [details]
other-corruption.png

Ok, a bit different than what I described. The lower part of the icon is still visible. In other cases the whole icon is garbled, and sometimes the corruption is different for the same icon but will be the same on the next occurence.

(In reply to comment #6) 
> Maybe 2) is the real cause and the commit makes it only worse?
> I will investigate in 2) on the machine with the radeon card to see if it
> happens there too...

That makes more sense. The commit in question tries hard to reuse an active buffer, so if there is some race condition it would exacerbate it. Something else to check is different kernel versions (just not 3.10!) to see if the race is actually a kernel bug (unlikely, but has happened!).

Something simple to try is compile with ./configure --enable-debug and see if any of the current asserts catch the error.

No luck reproducing this so far, trying pnv/snb/ivb with trusty, utopic and f20. But using current kernels. Hmm. Please do try a different kernel and recompiling with --enable-debug.

I've tried a new kernel (3.15.2), same problem. It is not reproducible on the other machine with a different driver. However, I did a new bisect and got this result:

git bisect start
# bad: [b6eca567c08fa1e14b06d751a13b714119a480a6] Revert "sna: Promote better active buffer reuse"
git bisect bad b6eca567c08fa1e14b06d751a13b714119a480a6
# good: [7468a6b740af14d95e8f9bacd2e352ec98a9acf2] 2.99.906 snapshot
git bisect good 7468a6b740af14d95e8f9bacd2e352ec98a9acf2
# good: [2fb2cd092dadba40b5ad2da57943c660a0c9bc14] 2.99.910 snapshot
git bisect good 2fb2cd092dadba40b5ad2da57943c660a0c9bc14
# good: [df297e9fafe1a7de1036f3151f93de67661c2a4e] sna: Userptr lands upstream, enable.
git bisect good df297e9fafe1a7de1036f3151f93de67661c2a4e
# bad: [b41a51e0f54c983ef72bc31125419382d666d48c] sna: Tweak self-copy boxes to hit the GPU more often
git bisect bad b41a51e0f54c983ef72bc31125419382d666d48c
# bad: [7f28b1cfa20eb4fc11c1a9c1e0c0fac7847be72e] uxa: Support 64-bit MSC values. Handle kernel vageries about MSC reporting
git bisect bad 7f28b1cfa20eb4fc11c1a9c1e0c0fac7847be72e
# good: [e8d8f754ebf77f17a4666c8f649907bee50313bd] sna/dri2: Make the swap-limit transitions more obvious
git bisect good e8d8f754ebf77f17a4666c8f649907bee50313bd
# bad: [bb49222a514b1d6041f3d9530a22f5701377118b] sna: Add DBG hints for using inplace CPU mmappings
git bisect bad bb49222a514b1d6041f3d9530a22f5701377118b
# bad: [562c47fc21b9029ebff29b53d2be590df4329264] Silence CLang (almost)
git bisect bad 562c47fc21b9029ebff29b53d2be590df4329264
# bad: [e6ee0679374f8fc80cb34693f80affe6ea676ddb] uxa: Silence compiler for warnings in Cursor API changes
git bisect bad e6ee0679374f8fc80cb34693f80affe6ea676ddb
# good: [2a0dc7ed4961d5bda08bb583a7a6a2cc633b27e4] sna/dri2: Move scanout processing from frame event to global
git bisect good 2a0dc7ed4961d5bda08bb583a7a6a2cc633b27e4
# good: [4321e8decd7be9daaa49c6ab4e8bb9bd66faae31] sna: Silence compiler for warnings in Cursor API changes
git bisect good 4321e8decd7be9daaa49c6ab4e8bb9bd66faae31
# first bad commit: [e6ee0679374f8fc80cb34693f80affe6ea676ddb] uxa: Silence compiler for warnings in Cursor API changes

Reproducability varies between bisect steps, but in general it's not hard to notice the issue. I'm using gcc-4.8.3. If it's a compiler problem, it might explain why it's not reproducible everywhere. However, the other machine uses gcc-4.8.3 too but has a sandy bridge processor.

CFLAGS="-O2 -march=native -pipe -fno-ident -ggdb"
LDFLAGS="-Wl,-O1 -Wl,--hash-style=gnu -Wl,--as-needed -Wl,--sort-common -Wl,-z,now"

Hmm, not sure yet. Can you try:

diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index a559907..8241be2 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -75,7 +75,7 @@
 #define USE_USERPTR_UPLOADS 1
 #define USE_USERPTR_DOWNLOADS 1
 #define USE_COW 1
-#define UNDO 1
+#define UNDO 0
 
 #define MIGRATE_ALL 0
 #define DBG_NO_PARTIAL_MOVE_TO_CPU 0

Modifying UNDO does not make any difference at all.

But I noticed something else: What both issues have in common is that these corruptions do not appear with smaller icon sizes. I use the largest setting in thunar, I'm not sure how big that is, but probably 128px. Going one size lower (96px or 80px maybe?) gets completely rid of the issue. This is 100% reproducible. Maybe that sheds more light on this?

How do you change icon size? I see side-pane and tree-pane in the preferences? Which icons are affected (I was assuming the files)?

Easy. It's in menu view->zoom in/out. This is for the icons in the main view.

...or you press <C>-<+> or <C>-<-> to adjust the size, <C>-<0> restores the original size.

Ok, I wasn't sure if zooming in there was doing what you wanted. But now I can see a little bit of corruption in the selection box, so I'll start digging. Thanks.

The corruption I see is related to manual detiling it seems. Haven't pieced together the exact cause, I am just tracing symptoms.

So far, I can "fix" it by:

1. disabling X-tiling for 512 byte wide tiles
2. disabling gpu_bo_download() (manual detiling)
3. disabling try_upload__inplace()

It is the 128x128 icon that gets manually tiled inplace, then hits a fallback, and gets detiled inplace. So far that explains why it is only the largest size that triggers it (it has to be 128 to hit this path, and I guess no other element is rendered quite like those icons).

To confirm you have a similar bug, can you please test:

diff --git a/src/sna/kgem.c b/src/sna/kgem.c
index 898f943..d143b42 100644
--- a/src/sna/kgem.c
+++ b/src/sna/kgem.c
@@ -4194,7 +4194,7 @@ int kgem_choose_tiling(struct kgem *kgem, int tiling, int width, int height, int
                goto done;
        }
 
-       if (tiling == I915_TILING_X && width * bpp <= 8*8*512/10) {
+       if (tiling == I915_TILING_X && width * bpp <= 8*512) {
                DBG(("%s: too thin [width %d, %d bpp] for TILING_X\n",
                     __FUNCTION__, width, bpp));
                tiling = I915_TILING_NONE;

Yes, this small change fixes both issues, so I can confirm the bug is similar.

I think I understand the bug now. I changed the transfer from GPU bo to CPU bo paths, but missed that implied a change in synchronisation requirement. This fixes the bug, but I want to see if I can choose the allocation flags more accurately before committing;


diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index dd5c108..6efc5f3 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -2157,7 +2157,8 @@ _sna_pixmap_move_to_cpu(PixmapPtr pixmap, unsigned int flags)
 skip_inplace_map:
                sna_damage_destroy(&priv->gpu_damage);
                priv->clear = false;
-               if (priv->cpu_bo && !priv->cpu_bo->flush &&
+               if ((flags & MOVE_ASYNC_HINT) == 0 &&
+                   priv->cpu_bo && !priv->cpu_bo->flush &&
                    __kgem_bo_is_busy(&sna->kgem, priv->cpu_bo)) {
                        DBG(("%s: discarding busy CPU bo\n", __FUNCTION__));
                        assert(!priv->shm);
@@ -2269,8 +2270,8 @@ skip_inplace_map:
 
        assert(priv->mapped == MAPPED_NONE);
        if (pixmap->devPrivate.ptr == NULL &&
-           !sna_pixmap_alloc_cpu(sna, pixmap, priv,
-                                 flags & MOVE_READ ? priv->gpu_damage && !priv->clear : 0))
+           !sna_pixmap_alloc_cpu(sna, pixmap, priv, 0))
+                                 //((flags & (MOVE_READ | MOVE_ASYNC_HINT)) == MOVE_READ) ? priv->gpu_damage && !priv->clear : 0))
                return false;
        assert(priv->mapped == MAPPED_NONE);
        assert(pixmap->devPrivate.ptr == PTR(priv->ptr));
@@ -2594,7 +2595,7 @@ sna_drawable_move_region_to_cpu(DrawablePtr drawable,
                sna_pixmap_unmap(pixmap, priv);
                assert(priv->mapped == MAPPED_NONE);
                if (pixmap->devPrivate.ptr == NULL &&
-                   !sna_pixmap_alloc_cpu(sna, pixmap, priv, false))
+                   !sna_pixmap_alloc_cpu(sna, pixmap, priv, flags & MOVE_ASYNC_HINT))
                        return false;
                assert(priv->mapped == MAPPED_NONE);
                assert(pixmap->devPrivate.ptr == PTR(priv->ptr));
@@ -2780,8 +2781,8 @@ move_to_cpu:
 
        assert(priv->mapped == MAPPED_NONE);
        if (pixmap->devPrivate.ptr == NULL &&
-           !sna_pixmap_alloc_cpu(sna, pixmap, priv,
-                                 flags & MOVE_READ ? priv->gpu_damage && !priv->clear : 0)) {
+           !sna_pixmap_alloc_cpu(sna, pixmap, priv, 0)) {
+                                 //flags & MOVE_READ ? priv->gpu_damage && !priv->clear : 0)) {
                DBG(("%s: CPU bo allocation failed, trying full move-to-cpu\n", __FUNCTION__));
                goto move_to_cpu;
        }

commit b961d7323369284ea2c3db47d30c27ffe01a9040
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sun Jun 29 07:00:58 2014 +0100

    sna: Sync CPU bo before writes
    
    Fixes regression from
    
    commit 961139f5878572ebea268a0bbf47caf05af9093f [2.99.912]
    Author: Chris Wilson <chris@chris-wilson.co.uk>
    Date:   Fri May 30 09:45:15 2014 +0100
    
        sna: Use manual detiling for downloads
    
    Reported-by: Harald Judt <h.judt@gmx.at>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80560
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks, I confirm problem fixed.