Re: intel driver crash (was: Re: mplayer restarting openbsd)

On Thu, Dec 10, 2009 at 03:29:04PM +0000, Owain Ainsworth wrote:
> On Thu, Dec 10, 2009 at 02:38:02AM +0100, frantisek holop wrote:
> > 
> > hi,
> > 
> > please reply to this thread on x11@ only
> > after this message, i'd like to move it off ports@
> > 
> > thanks for everyone's suggestions.
> > this should be it..
> 
> ok, i think i've seen a fix for this upstream.
> 
> expect a diff in a few hours.

from upstream:

Author: Barry Scott <[email protected]>
Date:   Tue Jun 23 14:14:50 2009 +0100

    Fix segv for clipped movie window
    
    When playing a movie that is clipped on its left and right edges the
    Xorg server will SEGV sometimes. This is because the intel driver
    ignores the clipping info when it copies the planes out of the XV
    data.  The check for the optimised copy was wrong to ignore the
    width required.  Which leads to too much data being copied by the
    memcpy. It the source buffe happens to end exactly on a page
    boundary the server will SEGV.
    
    As we reviewed the code we checked the calculation of src1, src2 and
    src3.  The patch includes additional comments to make it clear what
    the elements of the calculation are.
    
    This bug exists in git head and we also see it in 2.4.1.

-0-

diff follows:

Index: src/i830_video.c
===================================================================
RCS file: /cvs/xenocara/driver/xf86-video-intel/src/i830_video.c,v
retrieving revision 1.9
diff -u -p -r1.9 i830_video.c
--- src/i830_video.c	25 Jun 2009 20:16:43 -0000	1.9
+++ src/i830_video.c	10 Dec 2009 15:35:33 -0000
@@ -1395,7 +1395,8 @@ I830CopyPlanarData(ScrnInfoPtr pScrn, I8
 
     switch (pPriv->rotation) {
     case RR_Rotate_0:
-	if (srcPitch == dstPitch2)
+       /* optimise for the case of no clipping */
+	if (srcPitch == dstPitch2 && srcPitch == w)
 	    memcpy (dst1, src1, srcPitch * h);
 	else
 	    for (i = 0; i < h; i++) {
@@ -1434,7 +1435,11 @@ I830CopyPlanarData(ScrnInfoPtr pScrn, I8
     }
 
     /* Copy V data for YV12, or U data for I420 */
-    src2 = buf + (srcH * srcPitch) + ((top * srcPitch) >> 2) + (left >> 1);
+    src2 = buf +                            /* start of YUV data */
+                (srcH * srcPitch) +         /* move over Luma plane */
+                ((top * srcPitch) >> 2) +   /* move down from by top lines */
+                    (left >> 1);            /* move left by left pixels */
+
 #if 0
     ErrorF("src2 is %p, offset is %ld\n", src2,
 	   (unsigned long)src2 - (unsigned long)buf);
@@ -1453,7 +1458,8 @@ I830CopyPlanarData(ScrnInfoPtr pScrn, I8
 
     switch (pPriv->rotation) {
     case RR_Rotate_0:
-	if (srcPitch2 == dstPitch)
+       /* optimise for the case of no clipping */
+	if (srcPitch2 == dstPitch && srcPitch2 == (w/2))
 	    memcpy (dst2, src2, h/2 * srcPitch2);
 	else
 	    for (i = 0; i < h / 2; i++) {
@@ -1492,8 +1498,11 @@ I830CopyPlanarData(ScrnInfoPtr pScrn, I8
     }
 
     /* Copy U data for YV12, or V data for I420 */
-    src3 = buf + (srcH * srcPitch) + ((srcH >> 1) * srcPitch2) +
-    ((top * srcPitch) >> 2) + (left >> 1);
+    src3 = buf +                            /* start of YUV data */
+                (srcH * srcPitch) +         /* move over Luma plane */
+                ((srcH >> 1) * srcPitch2) + /* move over Chroma plane */
+                ((top * srcPitch) >> 2) +   /* move down from by top lines */
+                    (left >> 1);            /* move left by left pixels */
 #if 0
     ErrorF("src3 is %p, offset is %ld\n", src3,
 	   (unsigned long)src3 - (unsigned long)buf);
@@ -1512,7 +1521,8 @@ I830CopyPlanarData(ScrnInfoPtr pScrn, I8
 
     switch (pPriv->rotation) {
     case RR_Rotate_0:
-	if (srcPitch2 == dstPitch)
+       /* optimise for the case of no clipping */
+	if (srcPitch2 == dstPitch && srcPitch2 == (w/2))
 	    memcpy (dst3, src3, srcPitch2 * h/2);
 	else
 	    for (i = 0; i < h / 2; i++) {

-- 
I have learned
To spell hors d'oeuvres
Which still grates on
Some people's n'oeuvres.
		-- Warren Knox