Bug 37068

Created attachment 46542 [details]
Xorg log running today's git intel driver

Created attachment 46543 [details]
/var/log/dmesg

Great! Thanks for the bisection, it means that I've already fixed it with:

commit 3145530feed879082bcfab11ffc8e7fd0911c920
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat May 7 19:51:04 2011 +0100

    Ensure that the partial batch is flushed upon the blockhandler
    
    Currently, we require that a batch containing a dirty bo be submitted
    before we mark the device as requiring a flush. So if we never submit a
    batch between block handlers, we can end up sleeping without ever
    flushing either the partial batch or the rendering to the scanout.
    
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36776
    Tested-by: Vasily Khoruzhick <anarsoul@gmail.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I'm afraid it's not fixed in current git, the last commit in the git version I'm running is:

commit fd1ebd44fb72e7bdf57d00f8941cd6110a529cac
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue May 10 07:30:58 2011 +0100

    module: Adopt IVB's more detailed naming convention for SNB

*g* That means that your bisection only pointed to a commit that made it more prevalent and not the actual bug. :-p

Can you do a reverse bisection instead and find the commit that appeared to fix it?

With "good" meaning I can see the problem, starting with 2.13.0 as good and 2.14.0 as bad, I ended up with:

4a186a612376bdd6f86c026e8b8b442108868a0a is the first bad commit
commit 4a186a612376bdd6f86c026e8b8b442108868a0a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Dec 7 16:56:57 2010 +0000

    Always flush the batch before blocking for new X requests
    
    This should prevent any lag when waiting upon user input, for example
    whilst logging in with gdm.
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Hmm. Ok, that really does confirm that the bug is the batch is simply not being flushed... Oh...

diff --git a/src/intel_dri.c b/src/intel_dri.c
index cd72f45..48d0f56 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -182,6 +182,8 @@ static PixmapPtr fixup_shadow(DrawablePtr drawable, PixmapPt
        /* And redirect the pixmap to the new bo (for 3D). */
        intel_set_pixmap_private(old, priv);
        old->refcnt++;
+
+       intel_get_screen_private(xf86Screens[screen->myNum])->needs_flush = TRUE
        return old;
 }

It's conspiracy, I tell you!

(In reply to comment #7)
> Hmm. Ok, that really does confirm that the bug is the batch is simply not being
> flushed... Oh...
> 
> diff --git a/src/intel_dri.c b/src/intel_dri.c
> index cd72f45..48d0f56 100644
> --- a/src/intel_dri.c
> +++ b/src/intel_dri.c
> @@ -182,6 +182,8 @@ static PixmapPtr fixup_shadow(DrawablePtr drawable,
> PixmapPt
>         /* And redirect the pixmap to the new bo (for 3D). */
>         intel_set_pixmap_private(old, priv);
>         old->refcnt++;
> +
> +       intel_get_screen_private(xf86Screens[screen->myNum])->needs_flush =
> TRUE
>         return old;
>  }
> 
> It's conspiracy, I tell you!

This patch doesn't fix anything.

In case it matters, I'm currently running openbox at the moment - running xcompmgr hides the problem, it's probably not visible under gnome-shell either.

And for good measure:

diff --git a/src/intel_video.c b/src/intel_video.c
index 499614f..021ca5f 100644
--- a/src/intel_video.c
+++ b/src/intel_video.c
@@ -1599,6 +1599,7 @@ I830PutImageTextured(ScrnInfoPtr scrn,
                                         pixmap);
        }
 
+       intel_get_screen_private(scrn)->needs_flush = TRUE;
        DamageDamageRegion(drawable, clipBoxes);
 
        return Success;

(In reply to comment #9)
> And for good measure:
> 
> diff --git a/src/intel_video.c b/src/intel_video.c
> index 499614f..021ca5f 100644
> --- a/src/intel_video.c
> +++ b/src/intel_video.c
> @@ -1599,6 +1599,7 @@ I830PutImageTextured(ScrnInfoPtr scrn,
>                                          pixmap);
>         }
> 
> +       intel_get_screen_private(scrn)->needs_flush = TRUE;
>         DamageDamageRegion(drawable, clipBoxes);
> 
>         return Success;

Yes, this one fixes it. Thanks!

Thanks, you very much for your report, persistence and thorough testing!


commit 0b4ca9313cc7eb4845cf7f4e87c869c0c6d6ff0d
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue May 10 20:36:10 2011 +0100

    video: Flush the batch on the next blockhandler after queuing
    
    In order to avoid video lag and jerky playback we need to ensure that
    any queued video is flushed before we go to sleep.
    
    Fixes regression from 6f104189bb.
    
    Reported-and-tested-by: Edward Sheldrake <ejsheldrake@gmail.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=37068
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

commit 895a46e8ff70195c1a4bdccbeb652e330376f64a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue May 10 20:38:25 2011 +0100

    dri: Flush the batch after a DRI swap/copy event
    
    To minimise lag in those every so critical games, we want to ensure that
    the copy happens as soon as it is received, so we need to flush the
    batch after processing a swap event and before we go to sleep.
    
    References: https://bugs.freedesktop.org/show_bug.cgi?id=37068
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>