[PATCH] Ensure blitter quiescience before reading pixels from the framebuffer

[Bernardo Innocenti]

Ping.  Could you please commit this fix or reject it with rationale?
Do you need more info?

Moreover, we're just scratching the surface of serious performance
bottlenecks in both EXA and XAA.  Is anyone interested in discussing
possible solutions?


-------- Original Message --------
Subject: [PATCH] Ensure blitter quiescience before reading pixels from the	framebuffer
Date: Tue, 17 Jul 2007 12:02:36 +0100
From: Bernardo Innocenti <bernie at codewiz.org>
Organization: http://www.codewiz.org/
To: keith.packard at intel.com, Xorg Dev <xorg at lists.freedesktop.org>

On my laptop, I sometimes see trapezoids and glyphs painted in
the wrong color.  It happens mostly in Gtk widgets, but I could
reproduce the problem also with a small cairo testcase.

I can't test this fix right now because I'm travelling and I
can't afford to download and build all the xserver prerequisites,
but the race I'm fixing here would perfectly justify the symptoms
I see.

Unfortunately, calling XSync() at this point will stall the GPU
and has therefore a huge impact on performance.  But I can't
think of another way to protect access.  If we're always going
to accesse these 1x1 repeated pixmaps with the CPU, a better
fix would be not uploading them in the first place.

diff --git a/hw/xfree86/xaa/xaaPict.c b/hw/xfree86/xaa/xaaPict.c
index 55f0f6e..52852df 100644
--- a/hw/xfree86/xaa/xaaPict.c
+++ b/hw/xfree86/xaa/xaaPict.c
@@ -237,6 +237,7 @@ XAADoComposite (
 
 	if((pSrc->pDrawable->width == 1) && (pSrc->pDrawable->height == 1)) {
 	   CARD16 red, green, blue, alpha;
+           XAASync();
            CARD32 pixel =
                 *((CARD32*)(((PixmapPtr)(pSrc->pDrawable))->devPrivate.ptr));
 
@@ -594,8 +595,9 @@ XAADoGlyphs (CARD8         op,
        !(infoRec->WriteBitmapFlags & NO_TRANSPARENCY))
     {
 	CARD16 red, green, blue, alpha;
-	CARD32 pixel =
-                *((CARD32*)(((PixmapPtr)(pSrc->pDrawable))->devPrivate.ptr));
+	CARD32 pixel;
+	XaaSync();
+	pixel = *((CARD32*)(((PixmapPtr)(pSrc->pDrawable))->devPrivate.ptr));
 	CARD32 *bits, *pntr, *pnt;
 	int x, y, i, n, left, top, right, bottom, width, height, pitch;
 	int L, T, R, B, X, Y, h, w, dwords, row, column, nbox;
-- 
1.5.2.3


Bernardo Innocenti wrote:
> Daniel Stone wrote:
> 
>> You might start by mentioning which driver you're using?
> 
> I use r200, in 16bpp.  But, as you can see from the patch, it's
> not really driver dependent.
> 
> The problem was that we were reading a pixel from a pixmap
> without first waiting for the blitter to quiesce.
> 
> I've found the very same problem in xf86-video-amd a few days
> ago.  Unfortunately, adding locking stalls the pipeline, so it
> has a big performance hit.
> 
> To avoid this, Ajax came up with an EXA patch to migrate the pixmap
> back to system memory when we try to read the top-left pixel.
> This is somewhat better, but still wasteful.
> 
> Perhaps we should design a way to prevent uploasing 1x1 pixmaps
> to the framebuffer altogether.  And we should probably make it
> driver dependent, because some drivers, notably r200, really uses
> the 1x1 pixmap as a little texture for solid fills.  I'm surprised
> r200 doesn't provide a faster way to specify a solid source color.

-- 
   // Bernardo Innocenti
 \X/  http://www.codewiz.org/