- Jul 09, 2021
-
-
Jason Ekstrand authored
The current context uAPI allows for two methods of setting context parameters: SET_CONTEXT_PARAM and CONTEXT_CREATE_EXT_SETPARAM. The former is allowed to be called at any time while the later happens as part of GEM_CONTEXT_CREATE. Currently, everything settable via one is settable via the other. While some params are fairly simple and setting them on a live context is harmless such as the context priority, others are far trickier such as the VM or the set of engines. In order to swap out the VM, for instance, we have to delay until all current in-flight work is complete, swap in the new VM, and then continue. This leads to a plethora of potential race conditions we'd really rather avoid. In previous patches, we added a i915_gem_proto_context struct which is capable of storing and tracking all such create parameters. This commit delays the creation of the actual context until after the client is done configuring it with SET_CONTEXT_PARAM. From the perspective of the client, it has the same u32 context ID the whole time. From the perspective of i915, however, it's an i915_gem_proto_context right up until the point where we attempt to do something which the proto-context can't handle. Then the real context gets created. This is accomplished via a little xarray dance. When GEM_CONTEXT_CREATE is called, we create a proto-context, reserve a slot in context_xa but leave it NULL, the proto-context in the corresponding slot in proto_context_xa. Then, whenever we go to look up a context, we first check context_xa. If it's there, we return the i915_gem_context and we're done. If it's not, we look in proto_context_xa and, if we find it there, we create the actual context and kill the proto-context. In order for this dance to work properly, everything which ever touches a proto-context is guarded by drm_i915_file_private::proto_context_lock, including context creation. Yes, this means context creation now takes a giant global lock but it can't really be helped and that should never be on any driver's fast-path anyway. v2 (Daniel Vetter): - Commit message grammatical fixes. - Use WARN_ON instead of GEM_BUG_ON - Rename lazy_create_context_locked to finalize_create_context_locked - Rework the control-flow logic in the setparam ioctl - Better documentation all around v3 (kernel test robot): - Make finalize_create_context_locked static Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-25-jason@jlekstrand.net
-
Jason Ekstrand authored
There's a big comment saying how useful it is but no one is using this for anything anymore. It was added in 2bfa996e ("drm/i915: Store owning file on the i915_address_space") and used for debugfs at the time as well as telling the difference between the global GTT and a PPGTT. In f6e8aa38 ("drm/i915: Report the number of closed vma held by each context in debugfs") we removed one use of it by switching to a context walk and comparing with the VM in the context. Finally, VM stats for debugfs were entirely nuked in db80a129 ("drm/i915/gem: Remove per-client stats from debugfs/i915_gem_objects") v2 (Daniel Vetter): - Delete a struct drm_i915_file_private pre-declaration - Add a comment to the commit message about history Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-24-jason@jlekstrand.net
-
Jason Ekstrand authored
We're about to start doing lazy context creation which means contexts get created in i915_gem_context_lookup and we may start having more errors than -ENOENT. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-23-jason@jlekstrand.net
-
Jason Ekstrand authored
This means that the proto-context needs to grow support for engine configuration information as well as setparam logic. Fortunately, we'll be deleting a lot of setparam logic on the primary context shortly so it will hopefully balance out. There's an extra bit of fun here when it comes to setting SSEU and the way it interacts with PARAM_ENGINES. Unfortunately, thanks to SET_CONTEXT_PARAM and not being allowed to pick the order in which we handle certain parameters, we have think about those interactions. v2 (Daniel Vetter): - Add a proto_context_free_user_engines helper - Comment on SSEU in the commit message - Use proto_context_set_persistence in set_proto_ctx_param v3 (Daniel Vetter): - Fix a doc comment - Do an explicit HAS_FULL_PPGTT check in set_proto_ctx_vm instead of relying on pc->vm != NULL. - Handle errors for CONTEXT_PARAM_PERSISTENCE - Don't allow more resetting user engines - Rework initialization of UCONTEXT_PERSISTENCE v4 (Jason Ekstrand): - Move hand-rolled initialization of UCONTEXT_PERSISTENCE to an earlier patch v5 (Jason Ekstrand): - Move proto_context_set_persistence to this patch Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-22-jason@jlekstrand.net
-
Jason Ekstrand authored
What we really want to check is that size of the engines array, i.e. args->size - sizeof(*user) is divisible by the element size, i.e. sizeof(*user->engines) because that's what's required for computing the array length right below the check. However, we're currently not doing this and instead doing a compile-time check that sizeof(*user) is divisible by sizeof(*user->engines) and avoiding the subtraction. As far as I can tell, the only reason for the more confusing pair of checks is to avoid a single subtraction of a constant. The other thing the BUILD_BUG_ON might be trying to implicitly check is that offsetof(user->engines) == sizeof(*user) and we don't have any weird padding throwing us off. However, that's not the check it's doing and it's not even a reliable way to do that check. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-21-jason@jlekstrand.net
-
Jason Ekstrand authored
This is the VM equivalent of i915_gem_context_lookup. It's only used once in this patch but future patches will need to duplicate this lookup code so it's better to have it in a helper. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-20-jason@jlekstrand.net
-
Jason Ekstrand authored
For now this is a no-op because everyone passes in a null SSEU but it lets us get some of the error handling and selftest refactoring plumbed through. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-19-jason@jlekstrand.net
-
Jason Ekstrand authored
Since free_engines works for partially constructed engine sets, we can use the usual goto pattern. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-18-jason@jlekstrand.net
-
Jason Ekstrand authored
The current context uAPI allows for two methods of setting context parameters: SET_CONTEXT_PARAM and CONTEXT_CREATE_EXT_SETPARAM. The former is allowed to be called at any time while the later happens as part of GEM_CONTEXT_CREATE. Currently, everything settable via one is settable via the other. While some params are fairly simple and setting them on a live context is harmless such the context priority, others are far trickier such as the VM or the set of engines. In order to swap out the VM, for instance, we have to delay until all current in-flight work is complete, swap in the new VM, and then continue. This leads to a plethora of potential race conditions we'd really rather avoid. Unfortunately, both methods of setting the VM and the engine set are in active use today so we can't simply disallow setting the VM or engine set vial SET_CONTEXT_PARAM. In order to work around this wart, this commit adds a proto-context struct which contains all the context create parameters. v2 (Daniel Vetter): - Better commit message - Use __set/clear_bit instead of set/clear_bit because there's no race and we don't need the atomics v3 (Daniel Vetter): - Use manual bitops and BIT() instead of __set_bit v4 (Daniel Vetter): - Add a changelog to the commit message - Better hyperlinking in docs - Create the default PPGTT in i915_gem_create_context v5 (Daniel Vetter): - Hand-roll the initialization of UCONTEXT_PERSISTENCE Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-17-jason@jlekstrand.net
-
Jason Ekstrand authored
In order to prevent kernel doc warnings, also fill out docs for any missing fields and fix those that forgot the "@". Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-16-jason@jlekstrand.net
-
Jason Ekstrand authored
With the proto-context stuff added later in this series, we end up having to duplicate set_priority. This lets us avoid duplicating the validation logic. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-15-jason@jlekstrand.net
-
Jason Ekstrand authored
As far as I can tell, the only real reason for this is to avoid taking a reference to the i915_gem_context. The cost of those two atomics probably pales in comparison to the cost of the ioctl itself so we're really not buying ourselves anything here. We're about to make context lookup a tiny bit more complicated, so let's get rid of the one hand- rolled case. Some usermode drivers such as our Vulkan driver call GET_RESET_STATS on every execbuf so the perf here could theoretically be an issue. If this ever does become a performance issue for any such userspace drivers, they can use set CONTEXT_PARAM_RECOVERABLE to false and look for -EIO coming from execbuf to check for hangs instead. v2 (Daniel Vetter): - Add a comment in the commit message about recoverable contexts Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-14-jason@jlekstrand.net
-
Jason Ekstrand authored
There's no sense in allowing userspace to create more engines than it can possibly access via execbuf. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-13-jason@jlekstrand.net
-
Jason Ekstrand authored
This was only ever used for FENCE_SUBMIT automatic engine selection which was removed in the previous commit. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-12-jason@jlekstrand.net
-
Jason Ekstrand authored
Even though FENCE_SUBMIT is only documented to wait until the request in the in-fence starts instead of waiting until it completes, it has a bit more magic than that. If FENCE_SUBMIT is used to submit something to a balanced engine, we would wait to assign engines until the primary request was ready to start and then attempt to assign it to a different engine than the primary. There is an IGT test (the bonded-slice subtest of gem_exec_balancer) which exercises this by submitting a primary batch to a specific VCS and then using FENCE_SUBMIT to submit a secondary which can run on any VCS and have i915 figure out which VCS to run it on such that they can run in parallel. However, this functionality has never been used in the real world. The media driver (the only user of FENCE_SUBMIT) always picks exactly two physical engines to bond and never asks us to pick which to use. v2 (Daniel Vetter): - Mention the exact IGT test this breaks Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-11-jason@jlekstrand.net
-
Jason Ekstrand authored
This adds a bunch of complexity which the media driver has never actually used. The media driver does technically bond a balanced engine to another engine but the balanced engine only has one engine in the sibling set. This doesn't actually result in a virtual engine. This functionality was originally added to handle cases where we may have more than two video engines and media might want to load-balance their bonded submits by, for instance, submitting to a balanced vcs0-1 as the primary and then vcs2-3 as the secondary. However, no such hardware has shipped thus far and, if we ever want to enable such use-cases in the future, we'll use the up-and-coming parallel submit API which targets GuC submission. This makes I915_CONTEXT_ENGINES_EXT_BOND a total no-op. We leave the validation code in place in case we ever decide we want to do something interesting with the bonding information. v2 (Jason Ekstrand): - Don't delete quite as much code. v3 (Tvrtko Ursulin): - Add some history to the commit message Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-10-jason@jlekstrand.net
-
Jason Ekstrand authored
This has never been used by any userspace except IGT and provides no real functionality beyond parroting back parameters userspace passed in as part of context creation or via setparam. If the context is in legacy mode (where you use I915_EXEC_RENDER and friends), it returns success with zero data so it's not useful for discovering what engines are in the context. It's also not a replacement for the recently removed I915_CONTEXT_CLONE_ENGINES because it doesn't return any of the balancing or bonding information. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-9-jason@jlekstrand.net
-
Jason Ekstrand authored
This API is entirely unnecessary and I'd love to get rid of it. If userspace wants a single timeline across multiple contexts, they can either use implicit synchronization or a syncobj, both of which existed at the time this feature landed. The justification given at the time was that it would help GL drivers which are inherently single-timeline. However, neither of our GL drivers actually wanted the feature. i965 was already in maintenance mode at the time and iris uses syncobj for everything. Unfortunately, as much as I'd love to get rid of it, it is used by the media driver so we can't do that. We can, however, do the next-best thing which is to embed a syncobj in the context and do exactly what we'd expect from userspace internally. This isn't an entirely identical implementation because it's no longer atomic if userspace races with itself by calling execbuffer2 twice simultaneously from different threads. It won't crash in that case; it just doesn't guarantee any ordering between those two submits. It also means that sync files exported from different engines on a SINGLE_TIMELINE context will have different fence contexts. This is visible to userspace if it looks at the obj_name field of sync_fence_info. Moving SINGLE_TIMELINE to a syncobj emulation has a couple of technical advantages beyond mere annoyance. One is that intel_timeline is no longer an api-visible object and can remain entirely an implementation detail. This may be advantageous as we make scheduler changes going forward. Second is that, together with deleting the CLONE_CONTEXT API, we should now have a 1:1 mapping between intel_context and intel_timeline which may help us reduce locking. v2 (Tvrtko Ursulin): - Update the comment on i915_gem_context::syncobj to mention that it's an emulation and the possible race if userspace calls execbuffer2 twice on the same context concurrently. v2 (Jason Ekstrand): - Wrap the checks for eb.gem_context->syncobj in unlikely() - Drop the dma_fence reference - Improved commit message v3 (Jason Ekstrand): - Move the dma_fence_put() to before the error exit v4 (Tvrtko Ursulin): - Add a comment about fence contexts to the commit message Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-8-jason@jlekstrand.net
-
Jason Ekstrand authored
This API allows one context to grab bits out of another context upon creation. It can be used as a short-cut for setparam(getparam()) for things like I915_CONTEXT_PARAM_VM. However, it's never been used by any real userspace. It's used by a few IGT tests and that's it. Since it doesn't add any real value (most of the stuff you can CLONE you can copy in other ways), drop it. There is one thing that this API allows you to clone which you cannot clone via getparam/setparam: timelines. However, timelines are an implementation detail of i915 and not really something that needs to be exposed to userspace. Also, sharing timelines between contexts isn't obviously useful and supporting it has the potential to complicate i915 internally. It also doesn't add any functionality that the client can't get in other ways. If a client really wants a shared timeline, they can use a syncobj and set it as an in and out fence on every submit. v2 (Jason Ekstrand): - More detailed commit message Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-7-jason@jlekstrand.net
-
Jason Ekstrand authored
None of the callbacks we use with it return an error code anymore; they all return 0 unconditionally. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-6-jason@jlekstrand.net
-
Jason Ekstrand authored
Instead of handling it like a context param, unconditionally set it when intel_contexts are created. For years we've had the idea of a watchdog uAPI floating about. The aim was for media, so that they could set very tight deadlines for their transcodes jobs, so that if you have a corrupt bitstream (especially for decoding) you don't hang your desktop too hard. But it's been stuck in limbo since forever, and this simplifies things a bit in preparation for the proto-context work. If we decide to actually make said uAPI a reality, we can do it through the proto- context easily enough. This does mean that we move from reading the request_timeout_ms param once per engine when engines are created instead of once at context creation. If someone changes request_timeout_ms between creating a context and setting engines, it will mean that they get the new timeout. If someone races setting request_timeout_ms and context creation, they can theoretically end up with different timeouts. However, since both of these are fairly harmless and require changing kernel params, we don't care. v2 (Tvrtko Ursulin): - Add a comment about races with request_timeout_ms Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-5-jason@jlekstrand.net
-
Jason Ekstrand authored
The idea behind this param is to support OpenCL drivers with relocations because OpenCL reserves 0x0 for NULL and, if we placed memory there, it would confuse CL kernels. It was originally sent out as part of a patch series including libdrm [1] and Beignet [2] support. However, the libdrm and Beignet patches never landed in their respective upstream projects so this API has never been used. It's never been used in Mesa or any other driver, either. Dropping this API allows us to delete a small bit of code. [1]: https://lists.freedesktop.org/archives/intel-gfx/2015-May/067030.html [2]: https://lists.freedesktop.org/archives/intel-gfx/2015-May/067031.html Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-4-jason@jlekstrand.net
-
Jason Ekstrand authored
Previously, we were storing the ring size in the ring pointer before it was actually allocated. We would then guard setting the ring size on checking for CONTEXT_ALLOC_BIT. This is error-prone at best and really only saves us a few bytes on something that already burns at least 4K. Instead, this patch adds a new ring_size field and makes everything use that. v2 (Daniel Vetter): - Replace 512 * SZ_4K with SZ_2M v2 (Jason Ekstrand): - Rebase on top of page migration code Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-3-jason@jlekstrand.net
-
Jason Ekstrand authored
This reverts commit 88be76cd ("drm/i915: Allow userspace to specify ringsize on construction"). This API was originally added for OpenCL but the compute-runtime PR has sat open for a year without action so we can still pull it out if we want. I argue we should drop it for three reasons: 1. If the compute-runtime PR has sat open for a year, this clearly isn't that important. 2. It's a very leaky API. Ring size is an implementation detail of the current execlist scheduler and really only makes sense there. It can't apply to the older ring-buffer scheduler on pre-execlist hardware because that's shared across all contexts and it won't apply to the GuC scheduler that's in the pipeline. 3. Having userspace set a ring size in bytes is a bad solution to the problem of having too small a ring. There is no way that userspace has the information to know how to properly set the ring size so it's just going to detect the feature and always set it to the maximum of 512K. This is what the compute-runtime PR does. The scheduler in i915, on the other hand, does have the information to make an informed choice. It could detect if the ring size is a problem and grow it itself. Or, if that's too hard, we could just increase the default size from 16K to 32K or even 64K instead of relying on userspace to do it. Let's drop this API for now and, if someone decides they really care about solving this problem, they can do it properly. Signed-off-by:
Jason Ekstrand <jason@jlekstrand.net> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-2-jason@jlekstrand.net
-
John Harrison authored
Add ADL-P to the list of supported GuC and HuC firmware versions. For HuC, it reuses the existing TGL firmware file. For GuC, there is a dedicated firmware release. Signed-off-by:
John Harrison <John.C.Harrison@Intel.com> Reviewed-by:
Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210626004522.1699509-3-John.C.Harrison@Intel.com
-
John Harrison authored
A new HuC is available for TGL and compatible platforms, so switch to using it. Signed-off-by:
John Harrison <John.C.Harrison@Intel.com> Reviewed-by:
Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210626004522.1699509-2-John.C.Harrison@Intel.com
-
- Jul 08, 2021
-
-
Tejas Upadhyay authored
46 bit addressing enables you to use 4 bits to support some MKTME features, and 3 more bits for Optane support that uses a subset of MTKME for persistent memory. But GTT addressing sticking to 39 bit addressing, thus setting dma_mask_size to 39 fixes below tests : igt@i915_selftest@live@mman igt@kms_big_fb@linear-32bpp-rotate-0 igt@gem_create@create-clear igt@gem_mmap_offset@clear igt@gem_mmap_gtt@cpuset-big-copy In a way solves Gitlab#3142 https://gitlab.freedesktop.org/drm/intel/-/issues/3142, which had following errors : DMAR: DRHD: handling fault status reg 2 DMAR: [DMA Write] Request device [00:02.0] PASID ffffffff fault addr 7effff9000 [fault reason 05] PTE Write access is not set 0x7effff9000 is suspiciously exactly 39 bits, so it seems likely that the HW just ends up masking off those extra bits hence DMA errors. Changes since V2 : - dim checkpatch error solved Changes since V1 : - Added more details to commit message - Matthew Auld Signed-off-by:
Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com> Acked-by:
Matthew Auld <matthew.auld@intel.com> Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210708071222.955455-1-tejaskumarx.surendrakumar.upadhyay@intel.com
-
Lucas De Marchi authored
Commit c816723b ("drm/i915/gt: replace IS_GEN and friends with GRAPHICS_VER") converted INTEL_GEN and friends to the new version check macros. Meanwhile, some changes sneaked in to use INTEL_GEN. Remove the last users so we can remove the macros. Signed-off-by:
Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by:
Matt Roper <matthew.d.roper@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210707181325.2130821-2-lucas.demarchi@intel.com
-
- Jul 07, 2021
-
-
Matthew Auld authored
If mock_region_create fails then mem will be an error pointer. Instead we just need to use the correct ordering for the onion unwind. igt_mock_reserve() error: 'mem' dereferencing possible ERR_PTR() Reported-by:
kernel test robot <lkp@intel.com> Reported-by:
Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Reviewed-by:
Ramalingam C <ramalingam.c@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210702104642.1189978-2-matthew.auld@intel.com
-
Matthew Auld authored
The block here can't be NULL, especially since we already dereferenced it earlier, so remove the redundant check. igt_check_blocks() warn: variable dereferenced before check 'block' (see line 126) Reported-by:
Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Reviewed-by:
Ramalingam C <ramalingam.c@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210702104642.1189978-1-matthew.auld@intel.com
-
- Jul 06, 2021
-
-
Daniel Vetter authored
We're not consistently recommending these for developers only. I stumbled over this due to DRM_I915_LOW_LEVEL_TRACEPOINTS, which was added in commit 354d036f Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Date: Tue Feb 21 11:01:42 2017 +0000 drm/i915/tracepoints: Add request submit and execute tracepoints to "alleviate the performance impact concerns." Which is nonsense. Tvrtko and Joonas pointed out on irc that the real (but undocumented reason) was stable abi concerns for tracepoints, see https://lwn.net/Articles/705270/ and the specific change that was blocked around tracepoints: https://lwn.net/Articles/442113/ Anyway to make it a notch clearer why we have this Kconfig option consistly add the "Recommended for driver developers only." to it and all the other debug options we have. Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Matthew Brost <matthew.brost@intel.com> Reviewed-by:
Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Signed-off-by:
Daniel Vetter <daniel.vetter@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210702201708.2075793-1-daniel.vetter@ffwll.ch
-
- Jul 03, 2021
-
-
Thomas Zimmermann authored
Remove all references to DRM's IRQ midlayer. i915 uses Linux' interrupt functions directly. v2: * also remove an outdated comment * move IRQ fix into separate patch * update Fixes tag (Daniel) Signed-off-by:
Thomas Zimmermann <tzimmermann@suse.de> Fixes: b318b824 ("drm/i915: Nuke drm_driver irq vfuncs") Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: intel-gfx@lists.freedesktop.org Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210701173618.10718-3-tzimmermann@suse.de
-
Thomas Zimmermann authored
The code in xcs_resume() probably didn't work as intended. It uses struct drm_device.irq, which is allocated to 0, but never initialized by i915 to the device's interrupt number. Change all calls to synchronize_hardirq() to intel_synchronize_irq(), which uses the correct interrupt. _hardirq() functions are not needed in this context. v5: * go back to _hardirq() after PCI probe reported wrong context; add rsp comment v4: * switch everything to intel_synchronize_irq() (Daniel) v3: * also use intel_synchronize_hardirq() at another callsite v2: * wrap irq code in intel_synchronize_hardirq() (Ville) Signed-off-by:
Thomas Zimmermann <tzimmermann@suse.de> Fixes: 536f77b1 ("drm/i915/gt: Call stop_ring() from ring resume, again") Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210701173618.10718-2-tzimmermann@suse.de
-
- Jun 30, 2021
-
-
Matthew Auld authored
The min_page_size is only needed for pages inserted into the GTT, and for our paging structures we only need at most 4K bytes, so simply ignore the min_page_size restrictions here, otherwise we might see some severe overallocation on some devices. v2(Thomas): add some commentary Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Reviewed-by:
Thomas Hellström <thomas.hellstrom@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210625103824.558481-2-matthew.auld@intel.com
-
Matthew Auld authored
For some specialised objects we might need something larger than the regions min_page_size due to some hw restriction, and slightly more hairy is needing something smaller with the guarantee that such objects will never be inserted into any GTT, which is the case for the paging structures. This also fixes how we setup the BO page_alignment, if we later migrate the object somewhere else. For example if the placements are {SMEM, LMEM}, then we might get this wrong. Pushing the min_page_size behaviour into the manager should fix this. v2(Thomas): push the default page size behaviour into buddy_man, and let the user override it with the page-alignment, which looks cleaner v3: rebase on ttm sys changes Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Reviewed-by:
Thomas Hellström <thomas.hellstrom@linux.intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210625103824.558481-1-matthew.auld@intel.com
-
Thomas Hellström authored
Objects intended to be used as display framebuffers must reside in LMEM for discrete. If they happen to not do that, migrate them to LMEM before pinning. Signed-off-by:
Thomas Hellström <thomas.hellstrom@linux.intel.com> Reviewed-by:
Matthew Auld <matthew.auld@intel.com> Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210629151203.209465-4-thomas.hellstrom@linux.intel.com
-
Matthew Auld authored
A selftest for the gem object migrate functionality. Slightly adapted from the original by Matthew to the new interface and new fill blit code. v4: - Initialize buffers and check contents after migration (Suggested by Matthew Auld) - Perform async migration (if implemented) in the igt_lmem_pages_migrate test - Test also migration to the current region. Co-developed-by:
Thomas Hellström <thomas.hellstrom@linux.intel.com> Signed-off-by:
Thomas Hellström <thomas.hellstrom@linux.intel.com> Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@intel.com> #v3 Link: https://patchwork.freedesktop.org/patch/msgid/20210629151203.209465-3-thomas.hellstrom@linux.intel.com
-
Thomas Hellström authored
Introduce an interface to migrate objects between regions. This is primarily intended to migrate objects to LMEM for display and to SYSTEM for dma-buf, but might be reused in one form or another for performance-based migration. v2: - Verify that the memory region given as an id really exists. (Reported by Matthew Auld) - Call i915_gem_object_{init,release}_memory_region() when switching region to handle also switching region lists. (Reported by Matthew Auld) v3: - Fix i915_gem_object_can_migrate() to return true if object is already in the correct region, even if the object ops doesn't have a migrate() callback. - Update typo in commit message. - Fix kerneldoc of i915_gem_object_wait_migration(). v4: - Improve documentation (Suggested by Mattew Auld and Michael Ruhl) - Always assume TTM migration hits a TTM move and unsets the pages through move_notify. (Reported by Matthew Auld) - Add a dma_fence_might_wait() annotation to i915_gem_object_wait_migration() (Suggested by Daniel Vetter) v5: - Re-add might_sleep() instead of __dma_fence_might_wait(), Sent v4 with the wrong version, didn't compile and __dma_fence_might_wait() is not exported. - Added an R-B. Reported-by:
kernel test robot <lkp@intel.com> Signed-off-by:
Thomas Hellström <thomas.hellstrom@linux.intel.com> Reviewed-by:
Michael J. Ruhl <michael.j.ruhl@intel.com> Reviewed-by:
Matthew Auld <matthew.auld@intel.com> Signed-off-by:
Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210629151203.209465-2-thomas.hellstrom@linux.intel.com
-
Matthew Brost authored
Add entry for i915 new parallel submission uAPI plan. v2: (Daniel Vetter): - Expand logical order explaination - Add dummy header - Only allow N BBs in execbuf IOCTL - Configure parallel submission per slot not per gem context v3: (Marcin Ślusarz): - Lot's of typos / bad english fixed (Tvrtko Ursulin): - Consistent pseudo code, clean up wording in descriptions v4: (Daniel Vetter) - Drop flags - Add kernel doc - Reword a few things / fix typos (Tvrtko) - Reword a few things / fix typos v5: (Checkpatch) - Fix typos (Docs) - Fix warning Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Tony Ye <tony.ye@intel.com> CC: Carl Zhang <carl.zhang@intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jason Ekstrand <jason@jlekstrand.net> Signed-off-by:
Matthew Brost <matthew.brost@intel.com> Acked-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Acked-by:
Tony Ye <tony.ye@intel.com> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210629193511.124099-3-matthew.brost@intel.com
-
Matthew Brost authored
Add entry for i915 GuC submission / DRM scheduler integration plan. Follow up patch with details of new parallel submission uAPI to come. v2: (Daniel Vetter) - Expand explaination of why bonding isn't supported for GuC submission - CC some of the DRM scheduler maintainers - Add priority inheritance / boosting use case - Add reasoning for removing in order assumptions (Daniel Stone) - Add links to priority spec v4: (Tvrtko) - Add TODOs section (Daniel Vetter) - Pull in 1 line from following patch v5: (Checkpatch) - Fix typos Cc: Christian König <christian.koenig@amd.com> Cc: Luben Tuikov <luben.tuikov@amd.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Steven Price <steven.price@arm.com> Cc: Jon Bloomfield <jon.bloomfield@intel.com> Cc: Dave Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Jason Ekstrand <jason@jlekstrand.net> Cc: dri-devel@lists.freedesktop.org Signed-off-by:
Matthew Brost <matthew.brost@intel.com> Reviewed-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Acked-by:
Dave Airlie <airlied@redhat.com> Signed-off-by:
Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210629193511.124099-2-matthew.brost@intel.com
-