- 09 Aug, 2016 5 commits
-
-
Chris Wilson authored
When using RCU lookup for the request, commit 0eafec6d ("drm/i915: Enable lockless lookup of request tracking via RCU"), we acknowledge that we may race with another thread that could have reallocated the request. In order for the first thread not to blow up, the second thread must not clear the request completed before overwriting it. In the RCU lookup, we allow for the engine/seqno to be replaced but we do not allow for it to be zeroed. The choice we make is to either add extra checking to the RCU lookup, or embrace the inherent races (as intended). It is more complicated as we need to manually clear everything we depend upon being zero initialised, but we benefit from not emiting the memset() to clear the entire frequently allocated structure (that memset turns up in throughput profiles). And at the same time, the lookup remains flexible for future adjustments. v2: Old style LRC requires another variable to be initialize. (The danger inherent in not zeroing everything.) v3: request->batch also needs to be cleared v4: signaling.tsk is no long used unset, but pid still exists Fixes: 0eafec6d ("drm/i915: Enable lockless lookup of request...") Signed-off-by:
Chris Wilson <chris@chris-wilson.co.uk> Cc: "Goel, Akash" <akash.goel@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: http://patchwork.freedesktop.org/patch/msgid/1470731014-6894-2-git-send-email-chris@chris-wilson.co.uk
-
Chris Wilson authored
In the debate as to whether the second read of active->request is ordered after the dependent reads of the first read of active->request, just give in and throw a smp_rmb() in there so that ordering of loads is assured. v2: Explain the manual smp_rmb() Signed-off-by:
-
Chris Wilson authored
When we force the cleanup after a GPU hang, we want to retire all requests, or else we may leak them if truly wedged (and the GPU never advances again). Converting to the active request helpers had the issue of doing the check against busyness before reporting the request, so if we claim the GPU had hung but this engine hadn't we could potential skip the request cleanup - triggering the self-check BUG. Fixes: dcff85c8 ("drm/i915: Enable i915_gem_wait_for_idle() ...") Signed-off-by:
-
Chris Wilson authored
As we do the lockdep protected RCU lookup in a couple of places, refactor that code to a common helper i915_gem_active_raw(). Signed-off-by:
-
Chris Wilson authored
This was originally introduced to be used by the busy-ioctl, but in the end busy ioctl performed a different dance. Since there are no users, and no likely users, remove an unwanted chunk of the API. Suggested-by:
-
- 08 Aug, 2016 1 commit
-
-
Daniel Vetter authored
Signed-off-by:
-
- 05 Aug, 2016 27 commits
-
-
Matthew Auld authored
In i915_ggtt_cleanup_hw we need to remember to free aliasing_ppgtt. This fixes the following kmemleak message: unreferenced object 0xffff880213cca000 (size 8192): comm "modprobe", pid 1298, jiffies 4294745402 (age 703.930s) hex dump (first 32 bytes): 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ backtrace: [<ffffffff817c808e>] kmemleak_alloc+0x4e/0xb0 [<ffffffff8121f9c2>] kmem_cache_alloc_trace+0x142/0x1d0 [<ffffffffa06d11ef>] i915_gem_init_ggtt+0x10f/0x210 [i915] [<ffffffffa06d71bb>] i915_gem_init+0x5b/0xd0 [i915] [<ffffffffa069749a>] i915_driver_load+0x97a/0x1460 [i915] [<ffffffffa06a26ef>] i915_pci_probe+0x4f/0x70 [i915] [<ffffffff81423015>] local_pci_probe+0x45/0xa0 [<ffffffff81424463>] pci_device_probe+0x103/0x150 [<ffffffff81515e6c>] driver_probe_device+0x22c/0x440 [<ffffffff81516151>] __driver_attach+0xd1/0xf0 [<ffffffff8151379c>] bus_for_each_dev+0x6c/0xc0 [<ffffffff8151555e>] driver_attach+0x1e/0x20 [<ffffffff81514fa3>] bus_add_driver+0x1c3/0x280 [<ffffffff81516aa0>] driver_register+0x60/0xe0 [<ffffffff8142297c>] __pci_register_driver+0x4c/0x50 [<ffffffffa013605b>] 0xffffffffa013605b Signed-off-by:Matthew Auld <matthew.auld@intel.com> Reviewed-by:
-
Daniel Vetter authored
~jiffie and a few usecs is 3 orders of magnitude different. A bit much. This was changed in commit ca5b721e Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Dec 11 11:32:58 2015 +0000 drm/i915: Limit the busy wait on requests to 5us not 10ms! But probably missed the comment since the change was non-local to the comment. v2: Polish comment more (Chris). Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Acked-by:
Daniel Vetter <daniel.vetter@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1470413484-23775-1-git-send-email-daniel.vetter@ffwll.ch
-
Rodrigo Vivi authored
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>
-
Rodrigo Vivi authored
Copy to user return the number of bytes it couldn't write and zero on success. So any number different than 0 should be considered a fault, not only when it doesn't write the full size. v2: fixed the inverted logic. (Ville) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Reviewed-by:
-
Ville Syrjälä authored
This reverts commit f64425a8. active_streams will get totally out of whack with SST unless we sync up with the hw state at readout, obviously! We don't yet do that, so now the WARNs fire all the time. Let's revert :( Signed-off-by:
-
Matthew Auld authored
As pointed out by Chris Harris, we are using the wrong WA name, it should in fact be WaToEnableHwFixForPushConstHWBug, also it should be applied from C0 onwards for both BXT and KBL. Fixes: 7b9005cd ("drm/i915: Add WaInsertDummyPushConstP for bxt and kbl") Cc: Chris Harris <chris.harris@intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Reported-by:
Chris Harris <chris.harris@intel.com> Signed-off-by:
Arun Siluvery <arun.siluvery@linux.intel.com> Signed-off-by:
Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1470127013-29653-1-git-send-email-matthew.auld@intel.com
-
Chris Wilson authored
With all callers now not playing tricks with dropping the struct_mutex between waiting and retiring, we can assert that the request is ready to be retired. Signed-off-by:
-
Chris Wilson authored
In the previous commit, we moved the obj->tiling_mode out of a bitfield and into its own integer so that we could safely use READ_ONCE(). Let us now repair some of that damage by sharing the tiling_mode with its companion, the fence stride. v2: New magic Signed-off-by:
-
Chris Wilson authored
Through the GTT interface to the fence registers, we can only handle linear, X and Y tiling. The more esoteric tiling patterns are ignored. Document that the tiling ABI only supports upto Y tiling, and reject any attempts to set a tiling mode other than NONE, X or Y. Signed-off-by:
-
Chris Wilson authored
Since we are not concerned with userspace racing itself with set-tiling (the order is indeterminant even if we take a lock), then we can safely read back the single obj->tiling_mode and do the static lookup of swizzle mode without having to take a lock. get-tiling is reasonably frequent due to the back-channel passing around of tiling parameters in DRI2/DRI3. v2: Make tiling_mode a full unsigned int so that we can trivially use it with READ_ONCE(). Separating it out into manual control over the flags field was too noisy for a simple patch. Note that we could use the lower bits of obj->stride for the tiling mode. Signed-off-by:
-
Chris Wilson authored
We don't need to incur the overhead of checking whether the object is pinned prior to changing its madvise. If the object is pinned, the madvise will not take effect until it is unpinned and so we cannot free the pages being pointed at by hardware. Marking a pinned object with allocated pages as DONTNEED will not trigger any undue warnings. The check is therefore superfluous, and by removing it we can remove a linear walk over all the vma the object has. Still despite it being an overzealous check, that error code is part of the current ABI and so we must proceed with caution. Signed-off-by:
-
Chris Wilson authored
We only need to take the struct_mutex if the object is pinned to the display engine and so requires checking for clflush. (The race with userspace pinning the object to a framebuffer is irrelevant.) v2: Use access once for compiler hints (or not as it is a bitfield) v3: READ_ONCE, obj->pin_display is not a bitfield anymore v4: Don't be creative with goto. Signed-off-by:
-
Chris Wilson authored
By applying the same logic as for wait-ioctl, we can query whether a request has completed without holding struct_mutex. The biggest impact system-wide is removing the flush_active and the contention that causes. Testcase: igt/gem_busy Signed-off-by:
-
Chris Wilson authored
With a bit of care (and leniency) we can iterate over the object and wait for previous rendering to complete with judicial use of atomic reference counting. The ABI requires us to ensure that an active object is eventually flushed (like the busy-ioctl) which is guaranteed by our management of requests (i.e. everything that is submitted to hardware is flushed in the same request). All we have to do is ensure that we can detect when the requests are complete for reporting when the object is idle (without triggering ETIME), locklessly - this is handled by i915_gem_active_wait_unlocked(). The impact of this is actually quite small - the return to userspace following the wait was already lockless and so we don't see much gain in latency improvement upon completing the wait. What we do achieve here is completing an already finished wait without hitting the struct_mutex, our hold is quite short and so we are typically just a victim of contention rather than a cause - but it is still one less contention point! v2: Break up a long line. Signed-off-by:
-
Chris Wilson authored
If we try and read or write to an active request, we first must wait upon the GPU completing that request. Let's do that without holding the mutex (and so allow someone else to access the GPU whilst we wait). Upon completion, we will acquire the mutex and only then start the operation (i.e. we do not rely on state from before the initial wait). v2: Repaint the goto labels v3: Move the tracepoints back to the start of the ioctls Signed-off-by:
-
Chris Wilson authored
After removing the user of this wart, we can remove the wart entirely. Signed-off-by:
-
Chris Wilson authored
If we make the observation that mmap-offsets are only released when we free an object, we can then deduce that the shrinker only creates free space in the mmap arena indirectly by flushing the request list and freeing expired objects. If we combine this with the lockless vma-manager and lockless idling, we can avoid taking our big struct_mutex until we need to actually free the requests. One side-effect is that we defer the madvise checking until we need the pages (i.e. the fault handler). This brings us into line with the other delayed checks (and madvise in general). v2: s/ret/err/ and use if (!err) rather than if (ret == 0) Signed-off-by:
-
Chris Wilson authored
We can now wait for the GPU (all engines) to become idle without requiring the struct_mutex. Inside the shrinker, we need to currently take the struct_mutex in order to purge objects and to purge the objects we need the GPU to be idle - causing a stall whilst we hold the struct_mutex. We can hide most of that stall by performing the wait before taking the struct_mutex and only doing essential waits for new rendering on objects to be freed. Signed-off-by:
-
Chris Wilson authored
Now that we pass along the expected interruptible nature for the wait-for-idle, we do not need to modify the global i915->mm.interruptible for this single call. (Only the immediate call to i915_gem_wait_for_idle() takes the interruptible status as the other action, dma_map_sg(), is independent of i915.ko) Signed-off-by:
-
Chris Wilson authored
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 authored
Before suspending (or unloading), we would first wait upon all rendering to be completed and then disable the rings. This later step is a remanent from DRI1 days when we did not use request tracking for all operations upon the ring. Now that we are sure we are waiting upon the very last operation by the engine, we can forgo clobbering the ring registers, though we do keep the assert that the engine is indeed idle before sleeping. Signed-off-by:
-
Chris Wilson authored
Inside the kthread context, we can't be interrupted by signals so touching the mm.interruptible flag is pointless and wait-request now consumes EIO itself. Signed-off-by:
-
Chris Wilson authored
We can completely avoid taking the struct_mutex around the non-blocking waits by switching over to the RCU request management (trading the mutex for a RCU read lock and some complex atomic operations). The improvement is that we gain further contention reduction, and overall the code become simpler due to the reduced mutex dancing. Signed-off-by:
-
Chris Wilson authored
We can completely avoid taking the struct_mutex around the non-blocking waits by switching over to the RCU request management (trading the mutex for a RCU read lock and some complex atomic operations). The improvement is that we gain further contention reduction, and overall the code become simpler due to the reduced mutex dancing. v2: Move i915_gem_fault tracepoint back to the start of the function, before the unlocked wait. Signed-off-by:
-
Chris Wilson authored
It is useful to be able to wait on pending rendering without grabbing the struct_mutex. We can do this by using the i915_gem_active_get_rcu() primitive to acquire a reference to the pending request without requiring struct_mutex, just the RCU read lock, and then call i915_wait_request(). v2: Rebase onto new i915_gem_active_get_unlocked() semantics that take the RCU read lock on behalf of the caller. Signed-off-by:
-
Daniel Vetter authored
Backmerge the 4.8 pull request state from Dave - conflicts were getting out of hand, and Chris has some patches which outright don't apply without everything merged together again. Signed-off-by: -
Ville Syrjälä authored
The spec was recently fixed to have the correct iboost setting for the SKL Y/U DP DDI buffer translation table entry 2. Update our tables to match. Cc: David Weinehall <david.weinehall@linux.intel.com> Signed-off-by:
David Weinehall <david.weinehall@linux.intel.com>
-
- 04 Aug, 2016 7 commits
-
-
Matt Roper authored
The bspec was updated a couple weeks ago to add an extra block per line to plane watermark calculations for linear pixel formats. Bspec update 115327 description: "Gen9+ - Updated the plane blocks per line calculation for linear cases. Adds +1 for all linear cases to handle the non-block aligned stride cases." Cc: Lyude <cpaul@redhat.com> Cc: drm-intel-fixes@lists.freedesktop.org Signed-off-by:
Matt Roper <matthew.d.roper@intel.com> Link: http://patchwork.freedesktop.org/patch/msgid/1470344880-27394-1-git-send-email-matthew.d.roper@intel.comReviewed-by:
Lyude <cpaul@redhat.com>
-
Chris Wilson authored
If the GEM objects being rendered with in this request have been exported via dma-buf to a third party, hook ourselves into the dma-buf reservation object so that the third party can serialise with our rendering via the dma-buf fences. Testcase: igt/prime_busy Signed-off-by:
-
Chris Wilson authored
If we enable RCU for the requests (providing a grace period where we can inspect a "dead" request before it is freed), we can allow callers to carefully perform lockless lookup of an active request. However, by enabling deferred freeing of requests, we can potentially hog a lot of memory when dealing with tens of thousands of requests per second - with a quick insertion of a synchronize_rcu() inside our shrinker callback, that issue disappears. v2: Currently, it is our responsibility to handle reclaim i.e. to avoid hogging memory with the delayed slab frees. At the moment, we wait for a grace period in the shrinker, and block for all RCU callbacks on oom. Suggested alternatives focus on flushing our RCU callback when we have a certain number of outstanding request frees, and blocking on that flush after a second high watermark. (So rather than wait for the system to run out of memory, we stop issuing requests - both are nondeterministic.) Paul E. McKenney wrote: Another approach is synchronize_rcu() after some largish number of requests. The advantage of this approach is that it throttles the production of callbacks at the source. The corresponding disadvantage is that it slows things up. Another approach is to use call_rcu(), but if the previous call_rcu() is still in flight, block waiting for it. Yet another approach is the get_state_synchronize_rcu() / cond_synchronize_rcu() pair. The idea is to do something like this: cond_synchronize_rcu(cookie); cookie = get_state_synchronize_rcu(); You would of course do an initial get_state_synchronize_rcu() to get things going. This would not block unless there was less than one grace period's worth of time between invocations. But this assumes a busy system, where there is almost always a grace period in flight. But you can make that happen as follows: cond_synchronize_rcu(cookie); cookie = get_state_synchronize_rcu(); call_rcu(&my_rcu_head, noop_function); Note that you need additional code to make sure that the old callback has completed before doing a new one. Setting and clearing a flag with appropriate memory ordering control suffices (e.g,. smp_load_acquire() and smp_store_release()). v3: More comments on compiler and processor order of operations within the RCU lookup and discover we can use rcu_access_pointer() here instead. v4: Wrap i915_gem_active_get_rcu() to take the rcu_read_lock itself. Signed-off-by: -
Chris Wilson authored
Just move it earlier so that we can use the companion nonblocking version in a couple of more callsites without having to add a forward declaration. Signed-off-by:
-
Chris Wilson authored
We are motivated to avoid using a bitfield for obj->active for a couple of reasons. Firstly, we wish to document our lockless read of obj->active using READ_ONCE inside i915_gem_busy_ioctl() and that requires an integral type (i.e. not a bitfield). Secondly, gcc produces abysmal code when presented with a bitfield and that shows up high on the profiles of request tracking (mainly due to excess memory traffic as it converts the bitfield to a register and back and generates frequent AGI in the process). v2: BIT, break up a long line in compute the other engines, new paint for i915_gem_object_is_active (now i915_gem_object_get_active). Signed-off-by:
-
Chris Wilson authored
Rather than a mismash of struct drm_device *dev and struct drm_i915_private *dev_priv being used freely within a function, be consistent and only pass along dev_priv. Signed-off-by:
-
Chris Wilson authored
The individual bits inside obj->frontbuffer_bits are protected by each plane->mutex, but the whole bitfield may be accessed by multiple KMS operations simultaneously and so the RMW need to be under atomics. However, for updating the single field we do not need to mandate that it be under the struct_mutex, one more step towards its removal as the de facto BKL. Signed-off-by:
-
