On Wednesday, January 7, 2009 10:36 am Michel Dänzer wrote:
> 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. :)
Yeah I could be misremembering too, maybe it was just a fleeting comment on
IRC and maybe not even from you. :)
> > + /* 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);
Ah yeah, good call. I'll fix that up.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
------------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel
