| Message ID | 1386758111-3446-29-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 11, 2013 at 8:34 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > Since this is all old ums stuff (including the one in the radeon > driver) I've just tried to perfectly replicate the existing semantics. > > Reinventing wait queue code with semantics that differ from all the > standard linux wait functions is just hairy, so now we can get rid of > this and so make sure it'll never again be used. > > Oh and: via futexes ... *shudder* This one worries me, I've had numerous attempts to rip this out in the past and they always changed semantics on some devices and broke stuff. The sneaky timeout is essential for a lot of the hardware http://marc.info/?l=dri-devel&m=111383274225047&w=1 So I think I'd like a few more people to review this one before I go near it again :-) Dave.
On Wed, Dec 18, 2013 at 11:39:27AM +1000, Dave Airlie wrote: > On Wed, Dec 11, 2013 at 8:34 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > Since this is all old ums stuff (including the one in the radeon > > driver) I've just tried to perfectly replicate the existing semantics. > > > > Reinventing wait queue code with semantics that differ from all the > > standard linux wait functions is just hairy, so now we can get rid of > > this and so make sure it'll never again be used. > > > > Oh and: via futexes ... *shudder* > > This one worries me, I've had numerous attempts to rip this out in the > past and they always changed semantics on some devices and broke > stuff. > > The sneaky timeout is essential for a lot of the hardware > > http://marc.info/?l=dri-devel&m=111383274225047&w=1 > > So I think I'd like a few more people to review this one before I go > near it again :-) Oh, and I've thought the only tricky stuff is mapping the errno's correctly. I think if we have such cases of racy drivers then the right thing to do is to shovel this macro into drivers and slap a big comment about signalling races on top of that code. Thomas report doesn't say so, but I guess it's for via. Thomas, do you still have a vague recollection of what this has been about? Dave, do you want me to just respin this one with via left as-is (and the macro pushed into the via driver)? Thanks, Daniel
On 12/18/2013 09:25 AM, Daniel Vetter wrote: > On Wed, Dec 18, 2013 at 11:39:27AM +1000, Dave Airlie wrote: >> On Wed, Dec 11, 2013 at 8:34 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >>> Since this is all old ums stuff (including the one in the radeon >>> driver) I've just tried to perfectly replicate the existing semantics. >>> >>> Reinventing wait queue code with semantics that differ from all the >>> standard linux wait functions is just hairy, so now we can get rid of >>> this and so make sure it'll never again be used. >>> >>> Oh and: via futexes ... *shudder* >> This one worries me, I've had numerous attempts to rip this out in the >> past and they always changed semantics on some devices and broke >> stuff. >> >> The sneaky timeout is essential for a lot of the hardware >> >> https://urldefense.proofpoint.com/v1/url?u=http://marc.info/?l%3Ddri-devel%26m%3D111383274225047%26w%3D1&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=wgfMdGK1q8n48mRclOyEBEeb4OB1u5PwhRxswGxwjGY%3D%0A&s=6e87c42702c679927c4e0fab27ef8874b2a22707cb860f0de000df4798199e63 >> >> So I think I'd like a few more people to review this one before I go >> near it again :-) > Oh, and I've thought the only tricky stuff is mapping the errno's > correctly. I think if we have such cases of racy drivers then the right > thing to do is to shovel this macro into drivers and slap a big comment > about signalling races on top of that code. > > Thomas report doesn't say so, but I guess it's for via. Thomas, do you > still have a vague recollection of what this has been about? Yes, this was with via, but IIRC there were other drivers affected as well, but OTOH, I think the kernel wait code was fixed for races some time after that.... /Thomas > > Dave, do you want me to just respin this one with via left as-is (and the > macro pushed into the via driver)? > > Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index be8d4720d961..7f653ab59db4 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -783,8 +783,15 @@ static int i915_wait_irq(struct drm_device * dev, int irq_nr) master_priv->sarea_priv->perf_boxes |= I915_BOX_WAIT; if (ring->irq_get(ring)) { - DRM_WAIT_ON(ret, ring->irq_queue, 3 * HZ, - READ_BREADCRUMB(dev_priv) >= irq_nr); + ret = wait_event_interruptible_timeout(ring->irq_queue, + READ_BREADCRUMB(dev_priv) >= irq_nr, + 3 * HZ); + if (ret == 0) + ret = -EBUSY; + else if (ret == -ERESTARTSYS) + ret = -EINTR; + else + ret = 0; ring->irq_put(ring); } else if (wait_for(READ_BREADCRUMB(dev_priv) >= irq_nr, 3000)) ret = -EBUSY; diff --git a/drivers/gpu/drm/mga/mga_irq.c b/drivers/gpu/drm/mga/mga_irq.c index 1b071b8ff9dc..8ceeb5fc6d97 100644 --- a/drivers/gpu/drm/mga/mga_irq.c +++ b/drivers/gpu/drm/mga/mga_irq.c @@ -128,13 +128,19 @@ int mga_driver_fence_wait(struct drm_device *dev, unsigned int *sequence) * by about a day rather than she wants to wait for years * using fences. */ - DRM_WAIT_ON(ret, dev_priv->fence_queue, 3 * HZ, + ret = wait_event_interruptible_timeout(dev_priv->fence_queue, (((cur_fence = atomic_read(&dev_priv->last_fence_retired)) - - *sequence) <= (1 << 23))); + - *sequence) <= (1 << 23)), + 3 * HZ); *sequence = cur_fence; - return ret; + if (ret == 0) + return -EBUSY; + else if (ret == -ERESTARTSYS) + return -EINTR; + else + return 0; } void mga_driver_irq_preinstall(struct drm_device *dev) diff --git a/drivers/gpu/drm/radeon/radeon_irq.c b/drivers/gpu/drm/radeon/radeon_irq.c index 244b19bab2e7..b8eeadecd5d9 100644 --- a/drivers/gpu/drm/radeon/radeon_irq.c +++ b/drivers/gpu/drm/radeon/radeon_irq.c @@ -249,10 +249,15 @@ static int radeon_wait_irq(struct drm_device * dev, int swi_nr) dev_priv->stats.boxes |= RADEON_BOX_WAIT_IDLE; - DRM_WAIT_ON(ret, dev_priv->swi_queue, 3 * HZ, - RADEON_READ(RADEON_LAST_SWI_REG) >= swi_nr); + ret = wait_event_interruptible_timeout(dev_priv->swi_queue, + RADEON_READ(RADEON_LAST_SWI_REG) >= swi_nr, 3 * HZ); - return ret; + if (ret == 0) + return -EBUSY; + else if (ret == -ERESTARTSYS) + return -EINTR; + else + return 0; } u32 radeon_get_vblank_counter(struct drm_device *dev, int crtc) diff --git a/drivers/gpu/drm/via/via_dmablit.c b/drivers/gpu/drm/via/via_dmablit.c index ba33cf679180..929767ec428e 100644 --- a/drivers/gpu/drm/via/via_dmablit.c +++ b/drivers/gpu/drm/via/via_dmablit.c @@ -436,13 +436,19 @@ via_dmablit_sync(struct drm_device *dev, uint32_t handle, int engine) int ret = 0; if (via_dmablit_active(blitq, engine, handle, &queue)) { - DRM_WAIT_ON(ret, *queue, 3 * HZ, - !via_dmablit_active(blitq, engine, handle, NULL)); + ret = wait_event_interruptible_timeout(*queue, + !via_dmablit_active(blitq, engine, handle, NULL), + 3 * HZ); } DRM_DEBUG("DMA blit sync handle 0x%x engine %d returned %d\n", handle, engine, ret); - return ret; + if (ret == 0) + return -EBUSY; + else if (ret == -ERESTARTSYS) + return -EINTR; + else + return 0; } @@ -688,9 +694,14 @@ via_dmablit_grab_slot(drm_via_blitq_t *blitq, int engine) while (blitq->num_free == 0) { spin_unlock_irqrestore(&blitq->blit_lock, irqsave); - DRM_WAIT_ON(ret, blitq->busy_queue, HZ, blitq->num_free > 0); - if (ret) - return (-EINTR == ret) ? -EAGAIN : ret; + ret = wait_event_interruptible_timeout(blitq->busy_queue, + blitq->num_free > 0, HZ); + if (ret == 0) + return -EBUSY; + else if (ret == -ERESTARTSYS) + return -EAGAIN; + else + return 0; spin_lock_irqsave(&blitq->blit_lock, irqsave); } diff --git a/drivers/gpu/drm/via/via_irq.c b/drivers/gpu/drm/via/via_irq.c index 1319433816d3..4e3afef9fa0b 100644 --- a/drivers/gpu/drm/via/via_irq.c +++ b/drivers/gpu/drm/via/via_irq.c @@ -239,18 +239,23 @@ via_driver_irq_wait(struct drm_device *dev, unsigned int irq, int force_sequence cur_irq = dev_priv->via_irqs + real_irq; if (masks[real_irq][2] && !force_sequence) { - DRM_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ, + ret = wait_event_interruptible_timeout(cur_irq->irq_queue, ((VIA_READ(masks[irq][2]) & masks[irq][3]) == - masks[irq][4])); + masks[irq][4]), 3 * HZ); cur_irq_sequence = atomic_read(&cur_irq->irq_received); } else { - DRM_WAIT_ON(ret, cur_irq->irq_queue, 3 * HZ, + ret = wait_event_interruptible_timeout(cur_irq->irq_queue, (((cur_irq_sequence = atomic_read(&cur_irq->irq_received)) - - *sequence) <= (1 << 23))); + *sequence) <= (1 << 23)), 3 * HZ); } *sequence = cur_irq_sequence; - return ret; + if (ret == 0) + return -EBUSY; + else if (ret == -ERESTARTSYS) + return -EINTR; + else + return 0; } diff --git a/drivers/gpu/drm/via/via_video.c b/drivers/gpu/drm/via/via_video.c index a9ffbad1cfdd..6944b675020f 100644 --- a/drivers/gpu/drm/via/via_video.c +++ b/drivers/gpu/drm/via/via_video.c @@ -82,9 +82,15 @@ int via_decoder_futex(struct drm_device *dev, void *data, struct drm_file *file_ switch (fx->func) { case VIA_FUTEX_WAIT: - DRM_WAIT_ON(ret, dev_priv->decoder_queue[fx->lock], - (fx->ms / 10) * (HZ / 100), *lock != fx->val); - return ret; + ret = wait_event_interruptible_timeout(dev_priv->decoder_queue[fx->lock], + *lock != fx->val, + (fx->ms / 10) * (HZ / 100)); + if (ret == 0) + return -EBUSY; + else if (ret == -ERESTARTSYS) + return -EINTR; + else + return 0; case VIA_FUTEX_WAKE: wake_up(&(dev_priv->decoder_queue[fx->lock])); return 0; diff --git a/include/drm/drm_os_linux.h b/include/drm/drm_os_linux.h index 86ab99bc0ac5..ad5f874195b1 100644 --- a/include/drm/drm_os_linux.h +++ b/include/drm/drm_os_linux.h @@ -39,27 +39,3 @@ static inline void writeq(u64 val, void __iomem *reg) #define DRM_READ64(map, offset) readq(((void __iomem *)(map)->handle) + (offset)) /** Write a qword into a MMIO region */ #define DRM_WRITE64(map, offset, val) writeq(val, ((void __iomem *)(map)->handle) + (offset)) - -#define DRM_WAIT_ON( ret, queue, timeout, condition ) \ -do { \ - DECLARE_WAITQUEUE(entry, current); \ - unsigned long end = jiffies + (timeout); \ - add_wait_queue(&(queue), &entry); \ - \ - for (;;) { \ - __set_current_state(TASK_INTERRUPTIBLE); \ - if (condition) \ - break; \ - if (time_after_eq(jiffies, end)) { \ - ret = -EBUSY; \ - break; \ - } \ - schedule_timeout((HZ/100 > 1) ? HZ/100 : 1); \ - if (signal_pending(current)) { \ - ret = -EINTR; \ - break; \ - } \ - } \ - __set_current_state(TASK_RUNNING); \ - remove_wait_queue(&(queue), &entry); \ -} while (0)
Since this is all old ums stuff (including the one in the radeon driver) I've just tried to perfectly replicate the existing semantics. Reinventing wait queue code with semantics that differ from all the standard linux wait functions is just hairy, so now we can get rid of this and so make sure it'll never again be used. Oh and: via futexes ... *shudder* Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_dma.c | 11 +++++++++-- drivers/gpu/drm/mga/mga_irq.c | 12 +++++++++--- drivers/gpu/drm/radeon/radeon_irq.c | 11 ++++++++--- drivers/gpu/drm/via/via_dmablit.c | 23 +++++++++++++++++------ drivers/gpu/drm/via/via_irq.c | 15 ++++++++++----- drivers/gpu/drm/via/via_video.c | 12 +++++++++--- include/drm/drm_os_linux.h | 24 ------------------------ 7 files changed, 62 insertions(+), 46 deletions(-)