Keith Packard wrote:
> libdrm/xf86drm.c | 172
> ++++++++++++++++++++++++++------------------------
> linux-core/drmP.h | 5 +
> linux-core/drm_gem.c | 7 ++
> linux-core/drm_proc.c | 4 +
> linux-core/i915_gem.c | 31 ++++++---
> 5 files changed, 130 insertions(+), 89 deletions(-)
>
> New commits:
> commit 8b9ab108ec1f2ba2b503f713769c4946849b3cb2
> Author: Keith Packard <[EMAIL PROTECTED]>
> Date: Fri Jun 13 16:03:22 2008 -0700
>
> [libdrm] Restart all ioctls on signal receipt
>
> Receiving a signal should be ignored by the library, so just restart any
> ioctl which returns EINTR or EAGAIN.
>
>
Keith,
IMHO this seems like a very odd commit. First, drm ioctls() don't
"automatically" return -EINTR or -EAGAIN when the process receives a
signal. The kernel code must detect that situation and take appropriate
action. To have low signal delivery latency (which is crucial for the X
server when waiting for the hardware), the kernel code should check the
return code of any interruptible wait that may block for a long time,
and, if appropriate, return an -EINTR (or perhaps an -EAGAIN) to be
catched by user-space.
To have a catch-all restarter on each libdrm IOCTL, regardless whether
the IOCTL is capable of returning an -EINTR or -EAGAIN looks very bad.
Unless you've determined that the underlying kernel code actually can
return those error codes, and that no libdrm client uses them for other
purposes, (EAGAIN may mean, temporaryily busy - try again), the
restarter should be limited to those GEM ioctls that explicitly require
it. I might be wrong, but I can't imagine the original libdrm writers
having missed this and we haven't detected it yet.
If we look at the following GEM code:
if (!i915_seqno_passed(i915_get_gem_seqno(dev), seqno)) {
i915_user_irq_on(dev_priv);
ret = wait_event_interruptible(dev_priv->irq_queue,
i915_seqno_passed(i915_get_gem_seqno(dev),
seqno));
i915_user_irq_off(dev_priv);
}
if (ret)
DRM_ERROR("%s returns %d (awaiting %d at %d)\n",
__func__, ret, seqno, i915_get_gem_seqno(dev));
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.
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.
A good way to test these error paths is to add code to fake an
-ERESTARTSYS from wait_event_interruptible() every 20 or so waits, run a
number of simultaneous dri clients and make sure this doesn't
destabilize the system.
/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
