summaryrefslogtreecommitdiff
path: root/drivers/gpu/drm/i915/i915_irq.c
AgeCommit message (Collapse)Author
2016-08-10drm/i915: Move missed interrupt detection from hangcheck to breadcrumbsChris Wilson
In commit 2529d57050af ("drm/i915: Drop racy markup of missed-irqs from idle-worker") the racy detection of missed interrupts was removed when we went idle. This however opened up the issue that the stuck waiters were not being reported, causing a test case failure. If we move the stuck waiter detection out of hangcheck and into the breadcrumb mechanims (i.e. the waiter) itself, we can avoid this issue entirely. This leaves hangcheck looking for a stuck GPU (inspecting for request advancement and HEAD motion), and breadcrumbs looking for a stuck waiter - hopefully make both easier to understand by their segregation. v2: Reduce the error message as we now run independently of hangcheck, and the hanging batch used by igt also counts as a stuck waiter causing extra warnings in dmesg. v3: Move the breadcrumb's hangcheck kickstart to the first missed wait. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97104 Fixes: 2529d57050af (waiter"drm/i915: Drop racy markup of missed-irqs...") Testcase: igt/drv_missed_irq Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1470761272-1245-2-git-send-email-chris@chris-wilson.co.uk
2016-08-05drm/i915: Use drm official vblank_no_hw_counter callback.Rodrigo Vivi
No functional change. Instead of defining a new empty function let's use what is available on drm. It gets cleaner, and easy to read, and understand. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
2016-08-05drm/i915: Enable i915_gem_wait_for_idle() without holding struct_mutexChris Wilson
The principal motivation for this was to try and eliminate the struct_mutex from i915_gem_suspend - but we still need to hold the mutex current for the i915_gem_context_lost(). (The issue there is that there may be an indirect lockdep cycle between cpu_hotplug (i.e. suspend) and struct_mutex via the stop_machine().) For the moment, enabling last request tracking for the engine, allows us to do busyness checking and waiting without requiring the struct_mutex - which is useful in its own right. As a side-effect of having a robust means for tracking engine busyness, we can replace our other busyness heuristic, that of comparing against the last submitted seqno. For paranoid reasons, we have a semi-ordered check of that seqno inside the hangchecker, which we can now improve to an ordered check of the engine's busyness (removing a locked xchg in the process). v2: Pass along "bool interruptible" as being unlocked we cannot rely on i915->mm.interruptible being stable or even under our control. v3: Replace check Ironlake i915_gpu_busy() with the common precalculated value Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1470388464-28458-6-git-send-email-chris@chris-wilson.co.uk
2016-08-02drm/i915: Rename struct intel_ringbuffer to struct intel_ringChris Wilson
The state stored in this struct is not only the information about the buffer object, but the ring used to communicate with the hardware. Using buffer here is overly specific and, for me at least, conflates with the notion of buffer objects themselves. s/struct intel_ringbuffer/struct intel_ring/ s/enum intel_ring_hangcheck/enum intel_engine_hangcheck/ s/describe_ctx_ringbuf()/describe_ctx_ring()/ s/intel_ring_get_active_head()/intel_engine_get_active_head()/ s/intel_ring_sync_index()/intel_engine_sync_index()/ s/intel_ring_init_seqno()/intel_engine_init_seqno()/ s/ring_stuck()/engine_stuck()/ s/intel_cleanup_engine()/intel_engine_cleanup()/ s/intel_stop_engine()/intel_engine_stop()/ s/intel_pin_and_map_ringbuffer_obj()/intel_pin_and_map_ring()/ s/intel_unpin_ringbuffer()/intel_unpin_ring()/ s/intel_engine_create_ringbuffer()/intel_engine_create_ring()/ s/intel_ring_flush_all_caches()/intel_engine_flush_all_caches()/ s/intel_ring_invalidate_all_caches()/intel_engine_invalidate_all_caches()/ s/intel_ringbuffer_free()/intel_ring_free()/ Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1469432687-22756-15-git-send-email-chris@chris-wilson.co.uk Link: http://patchwork.freedesktop.org/patch/msgid/1470174640-18242-4-git-send-email-chris@chris-wilson.co.uk
2016-07-27drm/i915: Update a couple of hangcheck comments to talk about enginesChris Wilson
We still have lots of comments that refer to the old ring when we mean struct intel_engine_cs and its hardware correspondence. This patch fixes an instance inside hangcheck to talk about engines. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1469432687-22756-10-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1469606850-28659-5-git-send-email-chris@chris-wilson.co.uk
2016-07-20drm/i915: Rename ring->virtual_start as ring->vaddrChris Wilson
Just a different colour to better match virtual addresses elsewhere. s/ring->virtual_start/ring->vaddr/ Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1469005202-9659-9-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Dave Gordon <david.s.gordon@intel.com> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1469017917-15134-8-git-send-email-chris@chris-wilson.co.uk
2016-07-20drm/i915: Treat ringbuffer writes as write to normal memoryChris Wilson
Ringbuffers are now being written to either through LLC or WC paths, so treating them as simply iomem is no longer adequate. However, for the older !llc hardware, the hardware is documentated as treating the TAIL register update as serialising, so we can relax the barriers when filling the rings (but even if it were not, it is still an uncached register write and so serialising anyway.). For simplicity, let's ignore the iomem annotation. v2: Remove iomem from ringbuffer->virtual_address v3: And for good measure add iomem elsewhere to keep sparse happy Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> #v2 Link: http://patchwork.freedesktop.org/patch/msgid/1469005202-9659-8-git-send-email-chris@chris-wilson.co.uk Link: http://patchwork.freedesktop.org/patch/msgid/1469017917-15134-7-git-send-email-chris@chris-wilson.co.uk
2016-07-14drm/i915: Define a separate variable and control for RPS waitboost frequencyChris Wilson
To allow the user finer control over waitboosting, allow them to set the frequency we request for the boost. This also them allows to effectively disable the boosting by setting the boost request to a low frequency. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1468397438-21226-5-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
2016-07-07drm/i915: Introduce Kabypoint PCH for Kabylake H/DT.Rodrigo Vivi
Some Kabylake SKUs are going to use Kabypoint PCH. It is mainly for Halo and DT ones. >From our specs it doesn't seem that KBP brings any change on the display south engine. So let's consider this as a continuation of SunrisePoint, i.e., SPT+. Since it is easy to get confused by a letter change: KBL = Kabylake - CPU/GPU codename. KBP = Kabypoint - PCH codename. Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96826 Link: http://patchwork.freedesktop.org/patch/msgid/1467418032-15167-1-git-send-email-rodrigo.vivi@intel.com Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
2016-07-06drm/i915: Group the irq breadcrumb variables into the same cachelineChris Wilson
As we inspect both the tasklet (to check for an active bottom-half) and set the irq-posted flag at the same time (both in the interrupt handler and then in the bottom-halt), group those two together into the same cacheline. (Not having total control over placement of the struct means we can't guarantee the cacheline boundary, we need to align the kmalloc and then each struct, but the grouping should help.) v2: Try a couple of different names for the state touched by the user interrupt handler. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467805142-22219-3-git-send-email-chris@chris-wilson.co.uk
2016-07-05drm/i915: Convert dev_priv->dev backpointers to dev_priv->drmChris Wilson
Since drm_i915_private is now a subclass of drm_device we do not need to chase the drm_i915_private->dev backpointer and can instead simply access drm_i915_private->drm directly. text data bss dec hex filename 1068757 4565 416 1073738 10624a drivers/gpu/drm/i915/i915.ko 1066949 4565 416 1071930 105b3a drivers/gpu/drm/i915/i915.ko Created by the coccinelle script: @@ struct drm_i915_private *d; identifier i; @@ ( - d->dev->i + d->drm.i | - d->dev + &d->drm ) and for good measure the dev_priv->dev backpointer was removed entirely. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Matthew Auld <matthew.auld@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467711623-2905-4-git-send-email-chris@chris-wilson.co.uk
2016-07-05drm/i915: Clean up GPU hang messageChris Wilson
Remove some redundant kernel messages as we deduce a hung GPU and capture the error state. v2: Fix "hang" vs "no progress" message whilst I was there v3: s/snprintf/scnprintf/ Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467618513-4966-2-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
2016-07-05drm/i915: Replace lockless_dereference(bool) with READ_ONCE()Chris Wilson
After Joonas complained about using READ_ONCE() on the only use of the variable in the function, where the intent was to simply document that the read was intentionally racy and unlocked, I switched the READ_ONCE() over to lockless_dereference(). However, in linux-next that has a stronger type-check to only allow pointers and is no longer interchangeable with READ_ONCE(), see commit 331b6d8c7afc ("locking/barriers: Validate lockless_dereference() is used on a pointer type") Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Fixes: 67d97da34917 ("drm/i915: Only start retire worker when idle") Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1467705276-707-1-git-send-email-chris@chris-wilson.co.uk Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
2016-07-04drm/i915: Mass convert dev->dev_private to to_i915(dev)Chris Wilson
Since we now subclass struct drm_device, we can save pointer dances by noting the equivalence of struct drm_device and struct drm_i915_private, i.e. by using to_i915(). text data bss dec hex filename 1073824 4562 416 1078802 107612 drivers/gpu/drm/i915/i915.ko 1068976 4562 416 1073954 106322 drivers/gpu/drm/i915/i915.ko Created by the coccinelle script: @@ expression E; identifier p; @@ - struct drm_i915_private *p = E->dev_private; + struct drm_i915_private *p = to_i915(E); Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Dave Gordon <david.s.gordon@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467628477-25379-1-git-send-email-chris@chris-wilson.co.uk
2016-07-04drm/i915: Flush the RPS bottom-half when the GPU idlesChris Wilson
Make sure that the RPS bottom-half is flushed before we set the idle frequency when we decide the GPU is idle. This should prevent any races with the bottom-half and setting the idle frequency, and ensures that the bottom-half is bounded by the GPU's rpm reference taken for when it is active (i.e. between gen6_rps_busy() and gen6_rps_idle()). v2: Avoid recursively using the i915->wq - RPS does not touch the struct_mutex so has no place being on the ordered i915->wq. v3: Enable/disable interrupts for RPS busy/idle in order to prevent further HW access from RPS outside of the wakeref. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Imre Deak <imre.deak@intel.com> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> References: https://bugs.freedesktop.org/show_bug.cgi?id=89728 Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467616119-4093-6-git-send-email-chris@chris-wilson.co.uk
2016-07-04drm/i915: Only start retire worker when idleChris Wilson
The retire worker is a low frequency task that makes sure we retire outstanding requests if userspace is being lax. We only need to start it once as it remains active until the GPU is idle, so do a cheap test before the more expensive queue_work(). A consequence of this is that we need correct locking in the worker to make the hot path of request submission cheap. To keep the symmetry and keep hangcheck strictly bound by the GPU's wakelock, we move the cancel_sync(hangcheck) to the idle worker before dropping the wakelock. v2: Guard against RCU fouling the breadcrumbs bottom-half whilst we kick the waiter. v3: Remove the wakeref assertion squelching (now we hold a wakeref for the hangcheck, any rpm error there is genuine). v4: To prevent excess work when retiring requests, we split the busy flag into two, a boolean to denote whether we hold the wakeref and a bitmask of active engines. v5: Reorder cancelling hangcheck upon idling to avoid a race where we might cancel a hangcheck after being preempted by a new task Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> References: https://bugs.freedesktop.org/show_bug.cgi?id=88437 Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467616119-4093-1-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Remove debug noise on detecting fault-injection of missed interruptsChris Wilson
Since the tests can and do explicitly check debugfs/i915_ring_missed_irqs for the handling of a "missed interrupt", adding it to the dmesg at INFO is just noise. When it happens for real, we still class it as an ERROR. Note that I have chose to remove it entirely because when we detect the "missed interrupt" is irrelevant and the message contains no more information than we glean from looking in debugfs. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-20-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Move the get/put irq locking into the callerChris Wilson
With only a single callsite for intel_engine_cs->irq_get and ->irq_put, we can reduce the code size by moving the common preamble into the caller, and we can also eliminate the reference counting. For completeness, as we are no longer doing reference counting on irq, rename the get/put vfunctions to enable/disable respectively and are able to review the use of posting reads. We only require the serialisation with hardware when enabling the interrupt (i.e. so we cannot miss an interrupt by going to sleep before the hardware truly enables it). Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-18-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Only apply one barrier after a breadcrumb interrupt is postedChris Wilson
If we flag the seqno as potentially stale upon receiving an interrupt, we can use that information to reduce the frequency that we apply the heavyweight coherent seqno read (i.e. if we wake up a chain of waiters). v2: Use cmpxchg to replace READ_ONCE/WRITE_ONCE for more explicit control of the ordering wrt to interrupt generation and interrupt checking in the bottom-half. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-14-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Add a delay between interrupt and inspecting the final seqno (ilk)Chris Wilson
On Ironlake, there is no command nor register to ensure that the write from a MI_STORE command is completed (and coherent on the CPU) before the command parser continues. This means that the ordering between the seqno write and the subsequent user interrupt is undefined (like gen6+). So to ensure that the seqno write is completed after the final user interrupt we need to delay the read sufficiently to allow the write to complete. This delay is undefined by the bspec, and empirically requires 75us even though a register read combined with a clflush is less than 500ns. Hence, the delay is due to an on-chip buffer rather than the latency of the write to memory. Note that the render ring controls this by filling the PIPE_CONTROL fifo with stalling commands that force the earliest pipe-control with the seqno to be completed before the command parser continues. Given that we need a barrier operation for BSD, we may as well forgo the extra per-batch latency by using a common per-interrupt barrier. Studying the impact of adding the usleep shows that in both sequences of and individual synchronous no-op batches is negligible for the media engine (where the write now is unordered with the interrupt). Converting the render engine over from the current glutton of pie-controls over to the per-interrupt delays speeds up both the sequential and individual synchronous no-ops by 20% and 60%, respectively. This speed up holds even when looking at the throughput of small copies (4KiB->4MiB), both serial and synchronous, by about 20%. This is because despite adding a significant delay to the interrupt, in all likelihood we will see the seqno write without having to apply the barrier (only in the rare corner cases where the write is delayed on the last required is the delay necessary). Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94307 Testcase: igt/gem_sync #ilk Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-12-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Use HWS for seqno tracking everywhereChris Wilson
By using the same address for storing the HWS on every platform, we can remove the platform specific vfuncs and reduce the get-seqno routine to a single read of a cached memory location. v2: Fix semaphore_passed() to look at the signaling engine (not the waiter's) Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-8-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Slaughter the thundering i915_wait_request herdChris Wilson
One particularly stressful scenario consists of many independent tasks all competing for GPU time and waiting upon the results (e.g. realtime transcoding of many, many streams). One bottleneck in particular is that each client waits on its own results, but every client is woken up after every batchbuffer - hence the thunder of hooves as then every client must do its heavyweight dance to read a coherent seqno to see if it is the lucky one. Ideally, we only want one client to wake up after the interrupt and check its request for completion. Since the requests must retire in order, we can select the first client on the oldest request to be woken. Once that client has completed his wait, we can then wake up the next client and so on. However, all clients then incur latency as every process in the chain may be delayed for scheduling - this may also then cause some priority inversion. To reduce the latency, when a client is added or removed from the list, we scan the tree for completed seqno and wake up all the completed waiters in parallel. Using igt/benchmarks/gem_latency, we can demonstrate this effect. The benchmark measures the number of GPU cycles between completion of a batch and the client waking up from a call to wait-ioctl. With many concurrent waiters, with each on a different request, we observe that the wakeup latency before the patch scales nearly linearly with the number of waiters (before external factors kick in making the scaling much worse). After applying the patch, we can see that only the single waiter for the request is being woken up, providing a constant wakeup latency for every operation. However, the situation is not quite as rosy for many waiters on the same request, though to the best of my knowledge this is much less likely in practice. Here, we can observe that the concurrent waiters incur extra latency from being woken up by the solitary bottom-half, rather than directly by the interrupt. This appears to be scheduler induced (having discounted adverse effects from having a rbtree walk/erase in the wakeup path), each additional wake_up_process() costs approximately 1us on big core. Another effect of performing the secondary wakeups from the first bottom-half is the incurred delay this imposes on high priority threads - rather than immediately returning to userspace and leaving the interrupt handler to wake the others. To offset the delay incurred with additional waiters on a request, we could use a hybrid scheme that did a quick read in the interrupt handler and dequeued all the completed waiters (incurring the overhead in the interrupt handler, not the best plan either as we then incur GPU submission latency) but we would still have to wake up the bottom-half every time to do the heavyweight slow read. Or we could only kick the waiters on the seqno with the same priority as the current task (i.e. in the realtime waiter scenario, only it is woken up immediately by the interrupt and simply queues the next waiter before returning to userspace, minimising its delay at the expense of the chain, and also reducing contention on its scheduler runqueue). This is effective at avoid long pauses in the interrupt handler and at avoiding the extra latency in realtime/high-priority waiters. v2: Convert from a kworker per engine into a dedicated kthread for the bottom-half. v3: Rename request members and tweak comments. v4: Use a per-engine spinlock in the breadcrumbs bottom-half. v5: Fix race in locklessly checking waiter status and kicking the task on adding a new waiter. v6: Fix deciding when to force the timer to hide missing interrupts. v7: Move the bottom-half from the kthread to the first client process. v8: Reword a few comments v9: Break the busy loop when the interrupt is unmasked or has fired. v10: Comments, unnecessary churn, better debugging from Tvrtko v11: Wake all completed waiters on removing the current bottom-half to reduce the latency of waking up a herd of clients all waiting on the same request. v12: Rearrange missed-interrupt fault injection so that it works with igt/drv_missed_irq_hang v13: Rename intel_breadcrumb and friends to intel_wait in preparation for signal handling. v14: RCU commentary, assert_spin_locked v15: Hide BUG_ON behind the compiler; report on gem_latency findings. v16: Sort seqno-groups by priority so that first-waiter has the highest task priority (and so avoid priority inversion). v17: Add waiters to post-mortem GPU hang state. v18: Return early for a completed wait after acquiring the spinlock. Avoids adding ourselves to the tree if the is already complete, and skips the awkward question of why we don't do completion wakeups for waits earlier than or equal to ourselves. v19: Prepare for init_breadcrumbs to fail. Later patches may want to allocate during init, so be prepared to propagate back the error code. Testcase: igt/gem_concurrent_blit Testcase: igt/benchmarks/gem_latency Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: "Rogozhkin, Dmitry V" <dmitry.v.rogozhkin@intel.com> Cc: "Gong, Zhipeng" <zhipeng.gong@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Dave Gordon <david.s.gordon@intel.com> Cc: "Goel, Akash" <akash.goel@intel.com> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> #v18 Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-6-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Separate GPU hang waitqueue from advanceChris Wilson
Currently __i915_wait_request uses a per-engine wait_queue_t for the dual purpose of waking after the GPU advances or for waking after an error. In the future, we may add even more wake sources and require greater separation, but for now we can conceptually simplify wakeups by separating the two sources. In particular, this allows us to use different wait-queues (e.g. one on the engine advancement, a global one for errors and one on each requests) without any hassle. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-5-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Make queueing the hangcheck work inlineChris Wilson
Since the function is a small wrapper around schedule_delayed_work(), move it inline to remove the function call overhead for the principle caller. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-4-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Remove the dedicated hangcheck workqueueChris Wilson
The queue only ever contains at most one item and has no special flags. It is just a very simple wrapper around the system-wq - a complication with no benefits. v2: Use the system_long_wq as we may wish to capture the error state after detecting the hang - which may take a bit of time. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-3-git-send-email-chris@chris-wilson.co.uk
2016-07-01drm/i915: Delay queuing hangcheck to wait-requestChris Wilson
We can forgo queuing the hangcheck from the start of every request to until we wait upon a request. This reduces the overhead of every request, but may increase the latency of detecting a hang. However, if nothing every waits upon a hang, did it ever hang? It also improves the robustness of the wait-request by ensuring that the hangchecker is indeed running before we sleep indefinitely (and thereby ensuring that we never actually sleep forever waiting for a dead GPU). As pointed out by Tvrtko, it is possible for a GPU hang to go unnoticed for as long as nobody is waiting for the GPU. Though this rare, during that time we may be consuming more power than if we had promptly recovered, and in the most extreme case we may exhaust all memory before forcing the hangcheck. Something to be wary off in future. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1467390209-3576-2-git-send-email-chris@chris-wilson.co.uk
2016-06-06drm/i915: Fix a buch of kerneldoc warningsTvrtko Ursulin
Just a bunch of stale kerneldocs generating warnings when building the docs. Mostly function parameters so not very useful but still. v2: Tidy. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1464958937-23344-1-git-send-email-tvrtko.ursulin@linux.intel.com
2016-05-31drm/i915: Update GEN6_PMINTRMSK setup with GuC enabledSagar Arun Kamble
On Loading, GuC sets PM interrupts routing (bit 31) and clears ARAT expired interrupt (bit 9). Host turbo also updates this register in RPS flows. This patch ensures bit 31 and bit 9 setup by GuC persists. ARAT timer interrupt is needed in GuC for various features. It also facilitates halting GuC and hence achieving RC6. PM interrupt routing will not impact RPS interrupt reception by host as GuC will redirect them. This patch fixes igt test pm_rc6_residency that was failing with guc load/submission enabled. Tested with SKL GuC v6.1 and BXT GuC v5.1 and v8.7. v2: i915_irq/i915_pm decoupling from intel_guc. (ChrisW) v3: restructuring the mask update and rebase w.r.t Ville's patch. (ChrisW) v4: Updating the pm_intr_keep during direct_interrupts_to_guc. (Sagar) Cc: Chris Harris <chris.harris@intel.com> Cc: Zhe Wang <zhe1.wang@intel.com> Cc: Deepak S <deepak.s@intel.com> Cc: Satyanantha, Rama Gopal M <rama.gopal.m.satyanantha@intel.com> Cc: Akash Goel <akash.goel@intel.com> Testcase: igt/pm_rc6_residency Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Tested-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1464683307-19475-1-git-send-email-sagar.a.kamble@intel.com
2016-05-25drm/i915: Revert async unpin and nonblocking atomic commitDaniel Vetter
This reverts the following patches: d55dbd06bb5e1399aba9ab5227465339d1bbefff drm/i915: Allow nonblocking update of pageflips. 15c86bdb760185e871c7a0f559978328aa500971 drm/i915: Check for unpin correctness. 95c2ccdc82d520f59ae3b6fdc097b63c9b7082bb Reapply "drm/i915: Avoid stalling on pending flips for legacy cursor updates" a6747b7304a9d66758a196d885dab8bbfa5e7d1f drm/i915: Make unpin async. 03f476e1fcb42fca88fc50b94b0d3adbdbe887f0 drm/i915: Prepare connectors for nonblocking checks. 2099deffef4404f949ba1b68d2b17e0608190bc2 drm/i915: Pass atomic states to fbc update functions. ee7171af72c39c18b7d7571419a4ac6ca30aea66 drm/i915: Remove reset_counter from intel_crtc. 2ee004f7c59b2e642f0bb2834f847d756f2dd7b7 drm/i915: Remove queue_flip pointer. b8d2afae557dbb9b9c7bc6f6ec4f5278f3c4c34e drm/i915: Remove use_mmio_flip kernel parameter. 8dd634d922615ec3a9af7976029110ec037f8b50 drm/i915: Remove cs based page flip support. 143f73b3bf48c089b40f58462dd7f7c199fd4f0f drm/i915: Rework intel_crtc_page_flip to be almost atomic, v3. 84fc494b64e8c591be446a966b7447a9db519c88 drm/i915: Add the exclusive fence to plane_state. 6885843ae164e11f6c802209d06921e678a3f3f3 drm/i915: Convert flip_work to a list. aa420ddd8eeaa5df579894a412289e4d07c2fee9 drm/i915: Allow mmio updates on all platforms, v2. afee4d8707ab1f21b7668de995be3a5961e83582 Revert "drm/i915: Avoid stalling on pending flips for legacy cursor updates" "drm/i915: Allow nonblocking update of pageflips" should have been split up, misses a proper commit message and seems to cause issues in the legacy page_flip path as demonstrated by kms_flip. "drm/i915: Make unpin async" doesn't handle the unthrottled cursor updates correctly, leading to an apparent pin count leak. This is caught by the WARN_ON in i915_gem_object_do_pin which screams if we have more than DRM_I915_GEM_OBJECT_MAX_PIN_COUNT pins. Unfortuantely we can't just revert these two because this patch series came with a built-in bisect breakage in the form of temporarily removing the unthrottled cursor update hack for legacy cursor ioctl. Therefore there's no other option than to revert the entire pile :( There's one tiny conflict in intel_drv.h due to other patches, nothing serious. Normally I'd wait a bit longer with doing a maintainer revert, but since the minimal set of patches we need to revert (due to the bisect breakage) is so big, time is running out fast. And very soon (especially after a few attempts at fixing issues) it'll be really hard to revert things cleanly. Lessons learned: - Not a good idea to rush the review (done by someone fairly new to the area) and not make sure domain experts had a chance to read it. - Patches should be properly split up. I only looked at the two patches that should be reverted in detail, but both look like the mix up different things in one patch. - Patches really should have proper commit messages. Especially when doing more than one thing, and especially when touching critical and tricky core code. - Building a patch series and r-b stamping it when it has a built-in bisect breakage is not a good idea. - I also think we need to stop building up technical debt by postponing atomic igt testcases even longer. I think it's clear that there's enough corner cases in this beast that we really need to have the testcases _before_ the next step lands. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com> Cc: John Harrison <John.C.Harrison@Intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Acked-by: Dave Airlie <airlied@redhat.com> Acked-by: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
2016-05-23drm/i915: Enable GSE interrupt on BDW+Ville Syrjälä
We've never actually enabled or unmasked the GSE interrupt on BDW+, even though the interrupt handler was always prepared for it. Let's enable it and see what happens. Credit to Mark Kettenis who fixed this in the OpenBSD fork of the driver. He reports that it fixed the "ACPI _BCM/_BCQ-based brightness mechanism on a MacBookPro12,1 and a 3rd gen Lenovo X1 Carbon" for them. Mark says: "FWIW, this *is* needed if you want ACPI-based backlight control to work. On Linux you probably don't notice, since "hardware" backlight control is preferred over "firmware" or "platform" backlight control. It would help me if this did land in the Linux tree though, as it will make future imports of the i915 driver into OpenBSD easier." So even though we don't really need this, let's put it in to help Mark with future porting efforts. Should be harmless to have it enabled in any case. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> References: http://lists.freedesktop.org/archives/intel-gfx/2015-December/081799.html Reported-by: Mark Kettenis <mark.kettenis@xs4all.nl> Cc: Mark Kettenis <mark.kettenis@xs4all.nl> Cc: Jani Nikula <jani.nikula@linux.intel.com> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1463649283-28698-1-git-send-email-ville.syrjala@linux.intel.com Acked-by: Jani Nikula <jani.nikula@intel.com>
2016-05-19drm/i915: Remove cs based page flip support.Maarten Lankhorst
With mmio flips now available on all platforms it's time to remove support for cs flips. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1463490484-19540-13-git-send-email-maarten.lankhorst@linux.intel.com Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
2016-05-19drm/i915: Unify unpin_work and mmio_work into flip_work, v2.Maarten Lankhorst
Rename intel_unpin_work to intel_flip_work and use it for mmio flips and unpinning. Use flip_queued_req to hold the wait request in the mmio case, and the vblank counter from intel_crtc_get_vblank_counter. MMIO flips get their own path through intel_finish_page_flip_mmio, handled on vblank. CS page flips go through *_cs. Changes since v1: - Clean up destinction between MMIO and CS flips. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1463490484-19540-7-git-send-email-maarten.lankhorst@linux.intel.com Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
2016-05-19drm/i915: Remove intel_prepare_page_flip, v3.Maarten Lankhorst
Instead of calling prepare_flip right before calling finish_page_flip do everything from prepare_page_flip in finish_page_flip. Putting prepare and finish page_flip in a single step removes the need for INTEL_FLIP_COMPLETE, so it can be removed. This simplifies the code slightly. Changes since v1: - Invert if case to simplify code. - Add missing barrier. - Reword commit message. Changes since v2: - intel_page_flip_plane is removed. - work->pending is turned into a bool. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1463490484-19540-5-git-send-email-maarten.lankhorst@linux.intel.com Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
2016-05-19drm/i915: Remove intel_finish_page_flip_plane.Maarten Lankhorst
This function is duplicated with intel_finish_page_flip, and is only ever used from planes that could use the other function anyway. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1463490484-19540-4-git-send-email-maarten.lankhorst@linux.intel.com Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
2016-05-11drm/i915: Replace "INTEL_INFO->gen == x" checks with IS_GENxTvrtko Ursulin
This way optimization from a previous patch works even better. v2: Rebase. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
2016-05-10drm/i915: Use drm_i915_private as the native pointer for intel_uncore.cChris Wilson
Pass drm_i915_private to the uncore init/fini routines and their subservients as it is their native type. text data bss dec hex filename 6309978 3578778 696320 10585076 a183f4 vmlinux 6309530 3578778 696320 10584628 a18234 vmlinux a modest 400 bytes of saving, but 60 lines of code deleted! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1462885804-26750-1-git-send-email-chris@chris-wilson.co.uk
2016-05-09drm/i915: Store a i915 backpointer from engine, and use itChris Wilson
text data bss dec hex filename 6309351 3578714 696320 10584385 a18141 vmlinux 6308391 3578714 696320 10583425 a17d81 vmlinux Almost 1KiB of code reduction. v2: More s/INTEL_INFO()->gen/INTEL_GEN()/ and IS_GENx() conversions text data bss dec hex filename 6304579 3578778 696320 10579677 a16edd vmlinux 6303427 3578778 696320 10578525 a16a5d vmlinux Now over 1KiB! Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1462545621-30125-3-git-send-email-chris@chris-wilson.co.uk
2016-05-09drm/i915: Small display interrupt handlers tidyTvrtko Ursulin
I have noticed some of our interrupt handlers use both dev and dev_priv while they could get away with only dev_priv in the huge majority of cases. Tidying that up had a cascading effect on changing functions prototypes, so relatively big churn factor, but I think it is for the better. For example even where changes cascade out of i915_irq.c, for functions prefixed with intel_, genX_ or <plat>_, it makes more sense to take dev_priv directly anyway. This allows us to eliminate local variables and intermixed usage of dev and dev_priv where only one is good enough. End result is shrinkage of both source and the resulting binary. i915.ko: - .text 000b0899 + .text 000b0619 Or if we look at the Gen8 display irq chain: -00000000000006ad t gen8_irq_handler +0000000000000663 t gen8_irq_handler -0000000000000028 T intel_opregion_asle_intr +0000000000000024 T intel_opregion_asle_intr -000000000000008c t ilk_hpd_irq_handler +000000000000007f t ilk_hpd_irq_handler -0000000000000116 T intel_check_page_flip +0000000000000112 T intel_check_page_flip -000000000000011a T intel_prepare_page_flip +0000000000000119 T intel_prepare_page_flip -0000000000000014 T intel_finish_page_flip_plane +0000000000000013 T intel_finish_page_flip_plane -0000000000000053 t hsw_pipe_crc_irq_handler +000000000000004c t hsw_pipe_crc_irq_handler -000000000000022e t cpt_irq_handler +0000000000000213 t cpt_irq_handler So small shrinkage but it is all fast paths so doesn't harm. Situation is similar in other interrupt handlers as well. v2: Tidy intel_queue_rps_boost_for_request as well. (Chris Wilson) Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-20drm/i915/gen8+: Do not enable DPF interrupt since the handler does not existTvrtko Ursulin
Looks like DPF was not implemented for gen8+ but the IER and IMR are still enabled on initialization. Since there is no code to handle this interrupt, gate the irq enablement behind HAS_L3_DPF in case the feature gets enabled in the future. Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Chris Wilson <chris@chris-wilson.co.uk> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
2016-04-14drm/i915: Split gen8_gt_irq_handler into ack+handleVille Syrjälä
As we did on VLV, split the gt irq handling to ack and handler phases on CHV. Leave the BDW+ codepath mostly intact for now. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-13-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Eliminate passing dev+dev_priv to {snb,ilk}_gt_irq_handler()Ville Syrjälä
It looks silly to pass both dev and dev_priv to the snb/ilk gt irq handlers. Just pass dev_priv. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-12-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Move gt/pm irq handling out from irq disabled section on VLVVille Syrjälä
No need to actually handle the GT/PM interrupt while we have interrupt sources disabled. Move the actual processing to happen after we've restored VLV_IER and master interrupt enable. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-11-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Split VLV/CVH PIPESTAT handling into ack+handlerVille Syrjälä
Minimize the amount of stuff we do with interrupt sources disabled by splitting the PIPESTAT irq handling into ack+handler phases. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-10-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Split PORT_HOTPLUG_STAT ack out from i9xx_hpd_irq_handler()Ville Syrjälä
Split the VLV/CHV hoplug irq handling to ack and handler phases. This way we can move the actual irq handling outside the section where we have disabled the interrupt sources. For now, we leave things as is for pre-VLV GMCH platforms, but eventually they could get the same treatment. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-9-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Move variables to narrower scope in VLV/CHV irq handlersVille Syrjälä
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-8-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Eliminate loop from VLV irq handlerVille Syrjälä
Now that we've dealt with the races in clearing IIR bits via VLV_IER and the master interrupt enable, we can go ahead aliminate the loop from the VLV interrupt handler. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-7-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Clear VLV_IER around irq processingVille Syrjälä
On VLV/CHV the master interrupt enable bit only affects GT/PM interrupts. Display interrupts are not affected by the master irq control. Also it seems that the CPU interrupt will only be generated when the combined result of all GT/PM/display interrupts has a 0->1 edge. We already use the master interrupt enable bit to make sure GT/PM interrupt can generate such an edge if we don't end up clearing all IIR bits. We must do the same for display interrupts, and for that we can simply clear out VLV_IER, and restore after we've acked all the interrupts we are about to process. So with both master interrupt enable and VLV_IER cleared out, we will guarantee that there will be a 0->1 edge if any IIR bits are still set at the end, and thus another CPU interrupt will be generated. Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Fixes: 579de73b048a ("drm/i915: Exit cherryview_irq_handler() after one pass") Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-6-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Clear VLV_MASTER_IER around irq processingVille Syrjälä
Like on CHV, let's clear out the master irq enable bit when we ack GT/PM interrupts. This will allow GT/PM interrupts to re-raise the CPU interrupt if we fail to clear all the bits from the IIR(s). Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-5-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Clear VLV_IIR after PIPESTATVille Syrjälä
On VLV/CHV VLV_IIR is not double double buffered, and it doesn't detect edges from PIPESTAT & co. like it does on gen4. Instead it just directly latches the level from PIPESTAT & co. That means we must clear VLV_IIR after PIPESTAT & co. or else we'll get a spurious bit in VLV_IIR every single time. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-4-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
2016-04-14drm/i915: Set up VLV_MASTER_IER consistentlyVille Syrjälä
We're lacking VLV_MASTER_IER setup from valleyview_irq_preinstall(), so add it there. Also cargo cult in some POSTING_READ()s to match the other platforms. Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1460571598-24452-3-git-send-email-ville.syrjala@linux.intel.com Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>