On Mon, Dec 14, 2015 at 12:21:32PM +0000, Tvrtko Ursulin wrote:
> >@@ -1229,18 +1219,13 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
> > s64 *timeout,
> > struct intel_rps_client *rps)
> > {
> >- struct intel_engine_cs *ring = i915_gem_request_get_ring(req);
> >- struct drm_device *dev = ring->dev;
> >- struct drm_i915_private *dev_priv = dev->dev_private;
> >- const bool irq_test_in_progress =
> >- ACCESS_ONCE(dev_priv->gpu_error.test_irq_rings) & intel_ring_flag(ring);
> > int state = interruptible ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
> >- DEFINE_WAIT(wait);
> >- unsigned long timeout_expire;
> >+ struct intel_breadcrumb wait;
> >+ unsigned long timeout_remain;
> > s64 before, now;
> >- int ret;
> >+ int ret = 0;
> >
> >- WARN(!intel_irqs_enabled(dev_priv), "IRQs disabled");
> >+ might_sleep();
>
> Why this suddenly?
No reason other than reading other patches and thought this would make
good annotation for this function.
> > /* Wake up __wait_seqno, potentially holding dev->struct_mutex. */
> > for_each_ring(ring, dev_priv, i)
> >- wake_up_all(&ring->irq_queue);
> >+ intel_engine_wakeup(ring);
>
> This is from process context without a lock or synchronize rcu. I'll
> comment on it at the implementation site as well.
True. We could just do rcu_read_lock() here.
> >
> > /* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */
> > wake_up_all(&dev_priv->pending_flip_queue);
> >@@ -2986,16 +2985,17 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
> > if (ring_idle(ring, seqno)) {
> > ring->hangcheck.action = HANGCHECK_IDLE;
> >
> >- if (waitqueue_active(&ring->irq_queue)) {
> >+ if (intel_engine_has_waiter(ring)) {
> > /* Issue a wake-up to catch stuck h/w. */
> > if (!test_and_set_bit(ring->id, &dev_priv->gpu_error.missed_irq_rings)) {
> >- if (!(dev_priv->gpu_error.test_irq_rings & intel_ring_flag(ring)))
> >+ if (!test_bit(ring->id, &dev_priv->gpu_error.test_irq_rings))
> > DRM_ERROR("Hangcheck timer elapsed... %s idle\n",
> > ring->name);
> > else
> > DRM_INFO("Fake missed irq on %s\n",
> > ring->name);
> >- wake_up_all(&ring->irq_queue);
> >+
> >+ intel_engine_enable_fake_irq(ring);
> > }
> > /* Safeguard against driver failure */
> > ring->hangcheck.score += BUSY;
> >diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >new file mode 100644
> >index 000000000000..a9a199bca2c2
> >--- /dev/null
> >+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> >@@ -0,0 +1,274 @@
> >+/*
> >+ * Copyright © 2015 Intel Corporation
> >+ *
> >+ * Permission is hereby granted, free of charge, to any person obtaining a
> >+ * copy of this software and associated documentation files (the "Software"),
> >+ * to deal in the Software without restriction, including without limitation
> >+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >+ * and/or sell copies of the Software, and to permit persons to whom the
> >+ * Software is furnished to do so, subject to the following conditions:
> >+ *
> >+ * The above copyright notice and this permission notice (including the next
> >+ * paragraph) shall be included in all copies or substantial portions of the
> >+ * Software.
> >+ *
> >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >+ * IN THE SOFTWARE.
> >+ *
> >+ */
> >+
> >+#include "i915_drv.h"
> >+
> >+static void intel_breadcrumbs_fake_irq(unsigned long data)
> >+{
> >+ struct intel_breadcrumbs *b = (struct intel_breadcrumbs *)data;
> >+ struct task_struct *task;
> >+
> >+ /*
> >+ * The timer persists in case we cannot enable interrupts,
> >+ * or if we have previously seen seqno/interrupt incoherency
> >+ * ("missed interrupt" syndrome). Here the worker will wake up
> >+ * every jiffie in order to kick the oldest waiter to do the
> >+ * coherent seqno check.
> >+ */
> >+
> >+ task = READ_ONCE(b->first_waiter);
> >+ if (task) {
> >+ wake_up_process(task);
>
> Put a comment here describing why a task cannot exit and become
> invalid between sampling and wakeup?
>
> Or we could afford a spinlock here since this fires really infrequently?
This is softirq context, so means we have to bump the weight
of all our locks. I didn't want to do that, so...
> Or even intel_engine_wakeup?
Only because I was using intel_breadcrumbs here. I was thinking of
if (intel_engine_wakeup(engine))
mod_timer(&engine->breadcrumbs.fake_irq, jiffies + 1);
Ok, that looks better. I'll have to check up on softirq context vs rcu,
I think it is safe as the RCU grace period cannot expire, but I will
have to double check.
> >+static void irq_enable(struct intel_engine_cs *engine)
> >+{
> >+ WARN_ON(!engine->irq_get(engine));
> >+}
> >+
> >+static void irq_disable(struct intel_engine_cs *engine)
> >+{
> >+ engine->irq_put(engine);
> >+}
>
> These helpers don't do much and only have one call site each?
Later patches.
> >+static void __intel_breadcrumbs_disable_irq(struct intel_breadcrumbs *b)
> >+{
> >+ struct intel_engine_cs *engine =
> >+ container_of(b, struct intel_engine_cs, breadcrumbs);
> >+
> >+ if (!b->rpm_wakelock)
> >+ return;
> >+
> >+ if (b->irq_enabled) {
> >+ irq_disable(engine);
> >+ b->irq_enabled = false;
> >+ }
> >+
> >+ intel_runtime_pm_put(engine->i915);
> >+ b->rpm_wakelock = false;
> >+}
>
> Maybe put assert_spin_locked in the above two.
Ok.
> >+inline struct intel_breadcrumb *to_crumb(struct rb_node *node)
> >+{
> >+ return container_of(node, struct intel_breadcrumb, node);
> >+}
> >+
> >+bool intel_engine_add_breadcrumb(struct intel_engine_cs *engine,
> >+ struct intel_breadcrumb *wait)
> >+{
> >+ struct intel_breadcrumbs *b = &engine->breadcrumbs;
> >+ u32 seqno = engine->get_seqno(engine, true);
> >+ struct rb_node **p, *parent, *completed;
> >+ bool first;
> >+
> >+ spin_lock(&b->lock);
> >+
> >+ /* Insert the request into the retirment ordered list
> >+ * of waiters by walking the rbtree. If we are the oldest
> >+ * seqno in the tree (the first to be retired), then
> >+ * set ourselves as the bottom-half.
> >+ *
> >+ * As we descend the tree, prune completed branches since we hold the
> >+ * spinlock we know that the first_waiter must be delayed and can
> >+ * reduce some of the sequential wake up latency if we take action
> >+ * ourselves and wake up the copmleted tasks in parallel.
> >+ */
>
> Why it is interesting to do it both from add and remove breadcrumb
> paths? Wouldn't it be sufficient to do it only on remove?
Observation of heavily contended scenarios show processes getting
between in the interrupt and the bottom-half. So by seeing if we can
prune the tree, we may be able to advance the bottom-half quicker.
> >+ if (next && next != &wait->node) {
> >+ smp_store_mb(b->first_waiter, to_crumb(next)->task);
> >+ __intel_breadcrumbs_enable_irq(b);
> >+ wake_up_process(to_crumb(next)->task);
>
> I don't get this, why it is waking up the one after completed? Just
> to anticipate it might be completed soon?
No. Because we may miss the interrupt in the process of enabling it.
> >+ }
> >+
> >+ do {
> >+ struct intel_breadcrumb *crumb = to_crumb(completed);
> >+ completed = rb_prev(completed);
> >+
> >+ rb_erase(&crumb->node, &b->requests);
> >+ RB_CLEAR_NODE(&crumb->node);
> >+ wake_up_process(crumb->task);
> >+ } while (completed != NULL);
> >+ }
> >+
> >+ if (first)
> >+ smp_store_mb(b->first_waiter, wait->task);
> >+ BUG_ON(b->first_waiter == NULL);
>
> I object your honour! :) Let the user ssh in and reboot cleanly even
> if graphics stack is stuck.
You should still be able to ssh and kill the box. I object to using
WARN_ON inappropriately.
> >+static inline void intel_engine_wakeup(struct intel_engine_cs *engine)
> >+{
> >+ struct task_struct *task = READ_ONCE(engine->breadcrumbs.first_waiter);
> >+ if (task)
> >+ wake_up_process(task);
>
> And here definitely put a comment saying why this is safe without
> the spinlock.
>
> Actually seeing how it is called from irq context and process
> context I think it will need a lock.
Ok, we can look at improving the commentary.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
