On Wed, 2009-01-07 at 10:15 -0800, Jesse Barnes wrote:
> On Wednesday, January 7, 2009 9:49 am Michel Dänzer wrote:
> > On Tue, 2009-01-06 at 22:49 -0800, Keith Packard wrote:
> > > On Tue, 2009-01-06 at 10:41 -0800, Jesse Barnes wrote:
> > > > This patch adds a sanity check to drmWaitVBlank to prevent hangs.
> > > > Since the server interrupts syscalls pretty frequently with SIGALM, we
> > > > tend to restart syscalls when EINTR is returned. That means when a
> > > > vblank timeout happens, we probably won't catch it (it happens after
> > > > 3s, and SIGALM happens every few ms), so we can fairly easily get stuck
> > > > restarting the vblank wait ioctl if something fishy is going on (like
> > > > vblank interrupts are disabled for some reason).
> > > >
> > > > So this patch removes the top level restart code, pushing it into the
> > > > vblank wait ioctl wrapper, while adding a timeout. If there are cases
> > > > where more than 1s waits are desirable, we'd probably need to check the
> > > > sequence numbers and come up with a more reasonable value, or add a new
> > > > call that takes a timeout parameter.
> > >
> > > Uh, changing drmIoctl to return on EINTR seems like a bad plan. I'd hack
> > > up drmWaitVBlank to do the ioctl directly instead.
> >
> > Agreed - ignore the comment on this in my previous post, I got that
> > backwards of course. :)
>
> Ok; I messed with drmIoctl because I thought I remembered you saying we
> shouldn't put the retry code there but rather in the individual ioctls that
> needed it.
I don't remember saying that, but maybe I just lost my recollection of
it over the holidays or something. :)
> + /* Timeout after 1s */
> + if (cur.tv_sec > timeout.tv_sec + 1 ||
> + cur.tv_sec == timeout.tv_sec && cur.tv_nsec >= timeout.tv_nsec) {
> + errno = EBUSY;
> + ret = -1;
> + break;
> + }
Only just noticed this now: This should have additional tests to avoid
clobbering different error conditions or even success, something like
do {
ret = ioctl(fd, DRM_IOCTL_WAIT_VBLANK, vbl);
vbl->request.type &= ~DRM_VBLANK_RELATIVE;
if (ret && errno == EINTR) {
clock_gettime(CLOCK_MONOTONIC, &cur);
/* Timeout after 1s */
if (cur.tv_sec > timeout.tv_sec + 1 ||
cur.tv_sec == timeout.tv_sec && cur.tv_nsec >= timeout.tv_nsec)
{
errno = EBUSY;
ret = -1;
break;
}
}
} while (ret && errno == EINTR);
--
Earthling Michel Dänzer | http://www.vmware.com
Libre software enthusiast | Debian, X and DRI developer
------------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It is the best place to buy or sell services for
just about anything Open Source.
http://p.sf.net/sfu/Xq1LFB
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
