XV race condition with xf86XVReputImage, Expose events and Unichrome driver

[Jon Nettleton]

On Thu, 2007-05-24 at 13:22 +0100, Barry Scott wrote:
> Jon Nettleton wrote:
> > On Thu, 2007-05-24 at 09:43 +0200, Michel Dänzer wrote:
> >   
> >> On Wed, 2007-05-23 at 16:46 -0400, Jon Nettleton wrote:
> >>     
> >>> After some testing on this I have tracked down the issue to the commit
> >>> of this patch.
> >>>
> >>> http://gitweb.freedesktop.org/?p=xorg/xserver.git;a=commitdiff;h=dc914ced69e1e619a944c7dcb940c25e195cd4f3
> >>>
> >>> Specifically these changes
> >>>
> >>> @@ -1739,9 +1746,13 @@ xf86XVPutImage(
> >>>       REGION_UNINIT(pScreen, &VPReg)
> >>>    }
> >>>
> >>> -  if(portPriv->pDraw) {
> >>> +  /* If we are changing windows, unregister our port in the old window */
> >>> +  if(portPriv->pDraw && (portPriv->pDraw != pDraw))
> >>>       xf86XVRemovePortFromWindow((WindowPtr)(portPriv->pDraw), portPriv);
> >>> -  }
> >>> +
> >>> +  /* Register our port with the new window */
> >>> +  ret =  xf86XVEnlistPortInWindow((WindowPtr)pDraw, portPriv);
> >>> +  if(ret != Success) goto PUT_IMAGE_BAILOUT;
> >>>
> >>>    if(!REGION_NOTEMPTY(pScreen, &ClipRegion)) {
> >>>       clippedAway = TRUE;
> >>> @@ -1772,7 +1783,6 @@ xf86XVPutImage(
> >>>    if((ret == Success) &&
> >>>          (portPriv->AdaptorRec->flags & VIDEO_OVERLAID_IMAGES)) {
> >>>
> >>> -     xf86XVEnlistPortInWindow((WindowPtr)pDraw, portPriv);
> >>>       portPriv->isOn = XV_ON;
> >>>       portPriv->pDraw = pDraw;
> >>>       portPriv->drw_x = drw_x;  portPriv->drw_y = drw_y;
> >>>
> >>>
> >>> I haven't had any time to fully debug where the failure is, but
> >>> backing this patch out fixes the problem.
> >>>       
> >> Does this patch fix it?
> >>
> >>     
> > This is the part of the patch that caused the problem.  If you back out
> > this patch it allows Xvideo to work properly on VIA video chipsets (Not
> > sure if it happens on other chipsets).  Alternatively you can download
> > the patch I use to revert it in my rpm's
> > http://www.hekanetworks.com/~jnettlet/xserver-1.3.0-xv-minimize-crash.patch
> >
> > Hopefully this weekend I can get some time to figure out why this patch
> > has such ill effects.
> >   
> Maybe you are seeing the same bug that we found and reported 2 weeks ago.
> We worked around this by changing the driver to not support reput.
> 
> Barry
> 
> -------- Original Message --------
> Subject: 	XV race condition with xf86XVReputImage, Expose events and 
> Unichrome driver
> Date: 	Thu, 10 May 2007 15:34:43 +0100
> From: 	Barry Scott <barry.scott at onelan.co.uk>
> To: 	xorg at lists.freedesktop.org
> 
> 
> 
> I'm sending this report from Simon Farnsworth to the list.
> I'll raise a bug report about this unless you don't want one raised.
> 
> I've been tracking down an X server crash in our system, which
> appears to be triggered by bugs in xf86xv.c.
> 
> Our hardware is a VIA EPIA M10000 (CLE266 graphics), using the
> driver from unichrome.sf.net, xorg-server 1.3.0, and Linux 2.6.
> We have a single instance of Xine running, using the xv output driver.
> 
> When we tell xine to stop playing one movie and to start playing
> another movie we see the following sequence of events:
> 
> xf86XVClipNotify is called, and the test at line 1135
> succeeds as visible is set to 0. This causes pPriv->pDraw
> to be set to NULL (line 1143). Trapping X here in the
> debugger for a couple of minutes is sufficient to fix the bug.
> 
> If we don't stall X, the next call is to
> xf86XVWindowExposures; this ends up calling
> xf86XVReputImage (line 1082).
> 
> xf86XVReputImage assumes that pPriv->pDraw is not NULL,
> resulting in a SIGSEGV when it dereferences it (line 871 in an optimised 
> build).
> 
> If we stall X in xf86XVClipNotify for long enough, the next call
> we see is to xf86XVStopVideo, which closes down Xv, ensuring
> that we don't see the crash.
> 
> For our system, the workaround is to remove ReputImage support
> from the device driver, which prevents the call to xf86XVReputImage,
> and thus avoids the crash.

I guess that is my next question.  Is the via/unichrome/openchrome
driver the last one to use xf86XVReputImage?  If so should we just
remove it from the driver to keep things more standard across driver
implementations?  Possibly remove some code from xorg-server?

Jon