Keith Packard wrote: > On Mon, 2008-06-16 at 10:24 +0200, Thomas Hellström wrote: > > >> IMHO this seems like a very odd commit. >> > > We're now using libdrm for all X server device interaction, which means > having every return deal correctly with signals (as the X server > receives a lot of signals). What we want is restartable ioctls, but > POSIX doesn't allow those. > > Are there DRM functions which can be interrupted and which shouldn't be > restarted? It seems crazy to me. However, it's likely that we should > only restart on EINTR and not on EAGAIN. EINTR should only be returned > when interrupted by a signal, and restarting makes the ioctls look like > other 'sane' syscalls like read and write. > > My main concern is with ioctls that aren't interruptible, like DRM_IOCTL_VERSION. Why should we have a check on those, when they never return -EAGAIN or -EINTR? That looks bad and will confuse people (new drm developers) to think restarting is always needed.
Better to restart only the GEM ioctls that _are_ interruptible and only restart on -EINTR. Old TTM actually confuses this and uses -EAGAIN in these cases, which is bad. On a side note, doesn't read and write return -EINTR on signals? >> If the "wait_event_interruptible" hits a signal, it will return an >> -ERESTARTSYS. >> In that case no error message should be printed out, and the code should >> perform all cleanup necessary to be able to restart from the ioctl entry >> point, and then return an appropriate error message (-EINTR or perhaps >> -EAGAIN) to user-space. The above code might kill the X server if it >> hits a mouse-movement signal during the wait. >> > > All users of this path do correctly clean up and return -EINTR from the > kernel; the error message is actually useful precisely when this occurs > as the wait is almost always interrupted when the user hits ^C because > the application has locked up inside this wait. > Any dri app that uses a lot of signals will probably generate a lot of error messages that aren't really errors and that will confuse users. Not that I know of many such apps, but this will probably cause that error printout to be removed sooner or later. > >> Also if a mutex is locked around a long GPU wait, it is preferrable to, >> when possible (not just around GPU waits) , lock that mutex using >> mutex_lock_interruptible >> with proper error handling, to avoid long signal delivery latencies when >> a process is waiting uninterruptible for the mutex. >> > > That's a good suggestion; most of the paths in the driver take the mutex > on entrance to the syscall, making the 'cleanup' trivial too. > > There's one slight problem, though, and that is that the struct_mutex is locked throughout drm. Converting to mutex_lock_interruptible would be made easier if there was a dev->gem_mutex. This is, of course, assuming that the mutex _is_ indeed held around gpu waits. /Thomas ------------------------------------------------------------------------- Check out the new SourceForge.net Marketplace. It's the best place to buy or sell services for just about anything Open Source. http://sourceforge.net/services/buy/index.php -- _______________________________________________ Dri-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/dri-devel
