Bug 77178

Please confirm your bug with upstream and attach an Xorg.log and screenshot if possible.

Created attachment 97073 [details]
Xorg log

Created attachment 97074 [details]
Screenshot

This is firefox window on Qt5-based kwin. Happens for all windows in Qt5-kwin, and for Qt5 applications in KDE4.

Not sure what you mean by upstream, I tried latest git and is still reproducible.

That's what I needed you to verify, that the bug is still reproducible with git. Can you do a checkout of say 2.99.910 or 2.21.15 and see if it still fails?

Presumably you use KDE as well, what setup? i.e. kwin desktop effects, engines etc.

Are you also using mesa-10.1?

Yes, I'm using mesa 10.1, KDE desktop effects enabled and it happens with both openGL 2 and 3 backends.

I've bisected and the guilty commit is:

commit 1de1104064b5898cbed37e836901694a381c1266
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Feb 21 22:43:04 2014 +0000

    sna: Use a hint to do whole image uploads inplace
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

That should have been fixed by

commit 3310ee89c1f1a663de5f5b12b8125809a213996f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat Apr 5 12:18:31 2014 +0100

    sna: Avoid discarding damage when applying WHOLE hint to pixmap migration

Please do double check before I start pulling my hair out.

Tested again with:

1) Arch linux package 2.99.911-2 (which does contain the mentioned fix)
2) Latest git (commit 617e96f)

They both show the issue.

Created attachment 97084 [details] [review]
Refactor CPU migration promotion

Please test this patch.

(In reply to comment #9)

> Please test this patch.

No improvement.

Let's disable the two WHOLE hints and see which is the culprit.

A:

diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index 8abfdd0..639985c 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -2525,7 +2525,7 @@ sna_drawable_move_region_to_cpu(DrawablePtr drawable,
                return _sna_pixmap_move_to_cpu(pixmap, flags);
        }
 
-       if (flags & MOVE_WHOLE_HINT) {
+       if (flags & MOVE_WHOLE_HINT && 0) {
                DBG(("%s: region (%d, %d), (%d, %d) marked with WHOLE hint, pixmap %dx%d\n",
                       __FUNCTION__,
                       region->extents.x1,


B (on top of the attached patch):
diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c
index 8abfdd0..7a7499a 100644
--- a/src/sna/sna_accel.c
+++ b/src/sna/sna_accel.c
@@ -2500,11 +2500,13 @@ sna_drawable_move_region_to_cpu(DrawablePtr drawable,
                flags |= MOVE_INPLACE_HINT;
        }
 
+#if 0
        if (flags & MOVE_READ || (priv->gpu_damage == NULL && priv->cpu_damage == NULL)) {
                if (flags & MOVE_WHOLE_HINT ||
                    (flags & MOVE_WRITE && (priv->create & KGEM_CAN_CREATE_GPU) == 0))
                        return _sna_pixmap_move_to_cpu(pixmap, flags);
        }
+#endif
 
        if (get_drawable_deltas(drawable, pixmap, &dx, &dy)) {
                DBG(("%s: delta=(%d, %d)\n", __FUNCTION__, dx, dy));

Do these patches go on top of git master? patch A doesn't apply here.

Yes, they should both apply to git master.

(In reply to comment #13)
> Yes, they should both apply to git master.

Was a whitespace issue, they apply with patch -l

So:
patch A alone -> bad
patch A + patch B -> good

(In reply to comment #14)
> (In reply to comment #13)
> > Yes, they should both apply to git master.
> 
> Was a whitespace issue, they apply with patch -l

Apologies, I should learn not to paste diffs into bugzilla.

> So:
> patch A alone -> bad
> patch A + patch B -> good

That implies that is that first chunk, the promotion of the empty pixmap to whole that is troublesome. Thanks, time to mull it over.

Is there any chance you can reproduce this issue with --enable-debug=full and send me the Xorg.0.log file? (Please grab the latest DBG patch from xf86-video-intel.git first.)

Here you go

https://dl.dropboxusercontent.com/u/2693818/Xorg.0.log.tar.gz

I think

commit 510e5cf3fa4e1db8297be62461b602b13d18d8fc
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Apr 9 12:26:53 2014 +0100

    sna: Tweak application of WHOLE hint for uploads
    
    This help with the continuing saga of
    commit 1de1104064b5898cbed37e836901694a381c1266 [2.99.911]
    Author: Chris Wilson <chris@chris-wilson.co.uk>
    Date:   Fri Feb 21 22:43:04 2014 +0000
    
        sna: Use a hint to do whole image uploads inplace
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=77178
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

should do the trick, but I have not finished digesting your debug log.

(In reply to comment #18)

> should do the trick, but I have not finished digesting your debug log.

So, should I test it, or are there more fixes coming?

That should fix the regression identified, I'm just not finding evidence of it in the debug log. :|

Not to worry, my grepping was wrong.

As, I see where my assumptions broke down. Here it is uploading the whole image, but the drawable is clipped.

commit f14d6cbff3b9766f5fa834282a4eca6636b6fd7d
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Apr 9 13:14:37 2014 +0100

    sna: Handle clipped PutImage uploads more carefully
    
    If the upload is clipped, we do not want to apply the WHOLE migration hints.
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=77178
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

That should behave better by not blindly applying the optimisation in cases where we are uploading less than the whole image.

Everything OK in f14d6cb, thanks!

So which commits need to be backported to fix this downstream?

The fix is 510e5cf3fa4e1db8297be62461b602b13d18d8fc.