[Xorg-driver-geode] Xv PutImage crash

[Daniel Drake]

On 2 August 2010 16:48, Daniel Drake <dsd at laptop.org> wrote:
> It gets weirder. After applying this fix, the crash goes away and my
> test video works fine.
> (http://proxy-48.dailymotion.com/video/791/905/21509197%3aogg_theora_vorbis%3a255.ogg?auth=1280796338-6f471aa2c1113efb638177362c0625cf&cache=0)
>
> But a low-resolution video (160x120) now hangs the machine before the
> first frame is even displayed.
> The 2 LXCopyFromSys calls complete fine, but it then hangs
> indefinitely in gp_wait_until_idle (called from LXDisplayVideo from
> LXPutImage).

A new observation: when playing YUYV video, everything is fine. The
codepath is then:
LXPutImage --> LXCopyPacked --> LXCopyFromSys --> gp_color_bitmap_to_screen_blt

And in this case, each pixel in the input image really is 2 bytes, so
the following calculation inside gp_color_bitmap_to_screen_blt
correctly determines the line size:

    /* CALCULATE THE SIZE OF ONE LINE */
    size = (width << gp3_pix_shift) + indent;

So, only I420 video is affected by this bug.


Considering this, and the fact that writing the correct number of
dwords for a low-resolution I420 video causes the engine to hang, I've
come up with a workaround for this bug: detect the buggy case when
we're about to write 2 lines per loop iteration (causing the overflow
at the end), but instead of writing 2 lines, simply write each line
twice. This way we avoid the hang (still writing the same number of
dwords as before) and never overflow the buffer.

This is just for illustration, it's obviously not the real fix.

Daniel
-------------- next part --------------
http://dev.laptop.org/ticket/10260

Index: xf86-video-geode-2.11.2/src/cim/cim_gp.c
===================================================================
--- xf86-video-geode-2.11.2.orig/src/cim/cim_gp.c
+++ xf86-video-geode-2.11.2/src/cim/cim_gp.c
@@ -1231,6 +1231,7 @@ gp_color_bitmap_to_screen_blt(unsigned l
     unsigned long dword_count, byte_count;
     unsigned long size = ((width << 16) | height);
     unsigned long srcoffset;
+    int write_twice = 0;
 
     /* ASSUME BITMAPS ARE DWORD ALIGNED */
     /* We will offset into the source data in DWORD increments.  We */
@@ -1287,6 +1288,20 @@ gp_color_bitmap_to_screen_blt(unsigned l
     dword_count = (size >> 2);
     byte_count = (size & 3);
 
+    /* The above calculation is buggy, because it doesn't consider pitch of
+     * the input image. In the current codebase at time of writing, the one
+     * case when this can be incorrect is when each pixel is 1 byte (from
+     * planar image copy). Previously, we were overflowing the buffer in this
+     * case (writing 2 lines per loop iteration below).
+     * Unfortunately correcting the calculation in this case causes the BLT
+     * engine to freeze for some video resolutions.
+     * So, detect the case and adjust the behaviour to write each line twice in
+     * this case, which is better than overflowing the buffer at least.
+     * http://lists.x.org/archives/xorg-driver-geode/2010-August/000938.html
+     */
+    if (size != pitch)
+        write_twice = 1;
+
     /* CHECK FOR SMALL BLT CASE */
 
     if (((total_dwords << 2) * height) <= GP3_BLT_1PASS_SIZE &&
@@ -1319,7 +1334,14 @@ gp_color_bitmap_to_screen_blt(unsigned l
         while (height--) {
             /* WRITE DATA */
 
-            WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count);
+            if (write_twice) {
+                WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count >> 1);
+                WRITE_COMMAND_STRING32(8 + (dword_count << 1), data, srcoffset,
+                    dword_count >> 1);
+            } else {
+                WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count);
+            }
+
             WRITE_COMMAND_STRING8(8 + (dword_count << 2), data,
                 srcoffset + (dword_count << 2), byte_count);
 
@@ -1370,7 +1392,13 @@ gp_color_bitmap_to_screen_blt(unsigned l
 
             /* WRITE DATA */
 
-            WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count);
+            if (write_twice) {
+                WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count >> 1);
+                WRITE_COMMAND_STRING32(8 + (dword_count << 1), data, srcoffset, dword_count >> 1);
+            } else {
+                WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count);
+            }
+
             WRITE_COMMAND_STRING8(8 + (dword_count << 2), data,
                 srcoffset + (dword_count << 2), byte_count);