We jump out of the loops whenever result is not VK_SUCCESS, there is
no need to check for it there. I guess I missed this detail in the
most recent rework for this function.
Reviewed-by: Iván Briano <ivan.briano@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31698>
When the VkBuffer is of size 2^32 (which matches maxBufferSize), we
have vm_bind->size set to 2^32, which is fine because it fits in an
uint64_t. What is not fine is the 'i' variable being size_t, because
on 32bit systems it will loop forever since it will always be smaller
than 2^32.
Credits to Iván for not only reporting it, but also coming up with the
solution at the same time as I did, then testing it.
Cc: mesa-stable
Reported-by: Iván Briano <ivan.briano@intel.com>
Reviewed-by: Iván Briano <ivan.briano@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31698>
This will make things easier in situations where we don't want to use
the binding table at all (indirect draws/dispatches).
The mechanism is simple, upload a vec3 either through push constants
(<= Gfx12.0) or through the inline parameter register (>= Gfx12.5).
In the shader, do this :
if vec.x == 0xffffffff:
addr = pack64_2x32 vec.y, vec.z
vec = load_global addr
This works because we limit the maximum number of workgroup size to
0xffff in all dimension :
maxComputeWorkGroupCount = { 65535, 65535, 65535 },
So we can use the large values to signal the need for indirect
loading.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Caio Oliveira <caio.oliveira@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31508>
When flushing the render target cache for future operations, we need a
stall at pixel scoreboard. We likely didn't see any issue until now
because a change in render target added the pb-stall.
When using a 2 compute shaders with the following pattern :
vkCmdDispatch()
vkCmdPipelineBarrier() ImageBarrier with (src|dst)AccessMask=0 & identical layout
vkCmdDispatch()
we should ensure that the first dispatch is completed before executing
the second one, otherwise they can race to on resource accesses. This
fixes failures in some new CTS tests.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: mesa-stable
Reviewed-by: Ivan Briano <ivan.briano@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31676>
We have no cases where we intentionally pass a NULL layout when dynamic
offsets, and doing so would cause a null dereference. Le't asd an assert
for that.
CID: 1620447
Fixes: f39cd30f4f ("anv: Track all the descriptor sets")
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31638>
Coverity notices that we've insured that index index is < MAX_RTS in one
case, but that it must be greater in one case. Since `color_att_count`
is a uint32_t, it can easily exceed MAX_RTS (8), and would thus create
an out-of-bounds read situation. While the type system would allow this,
the actually implementation shouldn't, so an assert should make Coverity
happy and help us check our assumption.
CID: 1620440
Fixes: d2f7b6d5a7 ("anv: implement VK_KHR_dynamic_rendering_local_read")
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31640>
The code that initializes each queue got big enough that the
repetitive error handling is getting ugly and it could benefit from
being on its own function.
v2: Rebase, try to improve the comments.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
Since the L2 bug fix we've been overestimating l3l2_binds by a lot in
most of the cases: almost every single call to anv_sparse_bind_trtt
ends up using either 0 or 1 elements for l3l2_binds, with occasionally
something using 512 or more. By switching to util_dynarray we can
guarantee the best of every case:
- l1_binds will remain a stack array for the vast majority of the
calls
- even more than before, since STACK_ARRAY was limited to 8
elements and now we do 32
- l1 will be properly dimensioned without the need for reallocs
- l3l2_binds will be completely empty most of the times and only
trigger allocations when necessary
Here's the top 10 most common results of anv_sparse_bind_trtt() for a
trace of Assassin's Creed: Valhalla. The first column is how many
times we had that case while running the trace. After this patch, all
these cases will proceed without any memory allocations.
168 trtt_binds: num_vm_binds:04 l3l2:0000 l1:0004
344 trtt_binds: num_vm_binds:01 l3l2:0000 l1:0004
420 trtt_binds: num_vm_binds:01 l3l2:0000 l1:0012
422 trtt_binds: num_vm_binds:04 l3l2:0000 l1:0008
479 trtt_binds: num_vm_binds:01 l3l2:0000 l1:0024
560 trtt_binds: num_vm_binds:03 l3l2:0000 l1:0003
1005 trtt_binds: num_vm_binds:01 l3l2:0000 l1:0002
1024 trtt_binds: num_vm_binds:02 l3l2:0000 l1:0004
2145 trtt_binds: num_vm_binds:02 l3l2:0000 l1:0002
3735 trtt_binds: num_vm_binds:01 l3l2:0000 l1:0001
Only 70 out of total 11340 calls to anv_sparse_bind_trtt() contained
l3l2 elements.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
We use 2MB page table BOs, as defined by ANV_TRTT_PAGE_TABLE_BO_SIZE.
Each BO is enough to hold 512 pages, since each one has 4096 bytes.
Each L1 page can fit 1024 entries of 64kb size, which means our 512
pages should be able to fit a little less than 32gb of sparse resource
memory, since we also need some L2 pages and an L3 page. I don't see
any real world application using more than a single BO.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
Move it past the (n_l3l2_binds == 0 && n_l1_binds == 0) check so we
don't end up trying to do garbage collection more often than we submit
batches.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
Otherwise code such as anv_sparse_trtt_garbage_collect_batches() may
end up stuck waiting forever on a timeline of a submission that
failed.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
The application can submit bind operations where it simply resets
state that is already in our page tables, so there's nothing to do.
Before commit 7da5b1caef ("anv: move trtt submissions over to the
anv_async_submit") we would simply return and not submit any batches
when this happened, but the commit reorganized things in a way where
we started submitting empty batches instead.
Fix this by simply jumping out when we detect this case. Because of
this, rename the "error" labels to "out" as they can now happen on a
happy case.
It should be noted that an alternative to this implementation would be
to move all the handling of 'submit' to after the n_lX_binds check,
but this would put all the initialization inside the trtt->mutex,
creating extra contention even when we have stuff to bind. Since the
"there's nothing to bind" check is now rare (after we stopped doing
NULL binds during resource creation), it is probably better to reduce
lock contention in the common case at the expense of a little more CPU
in the rare case.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
We're missing a check for 'result' in the middle of a loop and we have
an unnecessary check for 'result' after the loop.
Fixes: 7da5b1caef ("anv: move trtt submissions over to the anv_async_submit")
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
There is a lot that can go wrong during initialization after we assign
trtt->l3_addr, and we use its value to check if trtt is initialized.
If an initialization fails after l3_addr is already assigned, the next
bind will attempt to use the leftover values from the failed
initialization attempt and will likely cause all sorts of random
errors. So when we fail, just set l3_addr back to 0, causing the next
bind to attempt to initialize everything again.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
Since everything is always NULL-bound at init and we always bind
things back to NULL in anv_free_sparse_bindings(), this means we don't
need to do NULL bindings during anv_init_sparse_bindings(), saving us
a bunch of time, espcially since we don't track L1 entries so we may
end up submitting TR-TT batches just to write zeroes on top of zeroes.
v2: Don't unnecessarily check for uses_relocs (Lionel).
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
- There's only one caller.
- The caller is rather small.
- We want to introduce initialization code that's not exactly queue
state and reuse the 'submit'.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
In the next commits we're going to move this out of
anv_sparse_bind_trtt() and we're also going to add more code to it.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
When we create sparse resources the first thing we do is a NULL bind
on them, as the Vulkan spec mandates certain behavior even for unbound
sparse resources. We do this with the minimal effort possible: if we
can get away with marking an L2 pointer as NULL in the L3 table, we
just do it and return, instead of going all the way to creating L1
tables and marking all the final entries as NULL.
The strategy we were using had a bug that could lead to previously
created NULL entries not being marked as NULL anymore. Let's give an
example:
(before proceeding, keep in mind that a NULL entry in the L3 and L2
tables has bit 1 set, it does *not* have the value 0)
- Create a 64mb buffer that uses an entire L1 table (needs to be
properly aligned), which triggers a NULL bind.
- Our algorithm will just set the L3 entry (pointing to the L2
table) as NULL.
- Create a 64kb buffer that uses the same L2 table (but a different
L1 table).
- The NULL bind triggered won't do anything as the L2 table is
already NULL.
- Bind the first buffer to actual memory. This will end up creating
the L2 table and the L1 table. The only entry we will set in the L2
table will be the one pointing to the L1 table. All the other
values will be 0 (so they won't have neither the NULL or Invalid
bits set: access to them will lead to page faults).
- Try to use the second buffer, which is still unbound. It was
relying on the fact that its L2 table pointer was NULL, but now
it's not anymore, so the page walker will fetch the L1 entries in
the L2 table and they will all be zero instead of having the NULL
bit set.
The fix is pretty simple: whenever we create a new L2 table, set every
entry to NULL (except the one we're about to set to non-NULL). This
preserves behavior for every other NULL resource relying on the L3
entry being set to NULL.
We don't need to do this for the L1 table because its entries are
different and instead of having bits to signal NULL entries we have
a special TR-TT register that we can set that gets compared to check
if an entry is NULL, and we conveniently program it to 0: see
ANV_TRTT_L1_NULL_TILE_VAL.
I am not aware of any real workloads that are triggering this
behavior, I found this issue while investigating something else,
running a custom sparse program in our pre-silicon environment, and it
told us about the page faults.
Cc: mesa-stable
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30953>
GFX_VER block matches both workarounds and while these workarounds are
almost about the same cause, other one applies only for LNL and other
one for BMG, need to check for both.
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31571>
These should be included according to table in Bspec 43904.
Patch removes PIPE_CONTROL_STATE_CACHE_INVALIDATE based on HSDES.
Cc: mesa-stable
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Nanley Chery <nanley.g.chery@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/29764>
It looks like this comment attempted to describe all the reasons we need
to pass the clear color address to BLORP. This comment actually isn't
exhaustive and some bits are out of date (e.g., BLORP no longer updates
the clear color address for us). Let's just delete it.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31136>
If a dmabuf is shared with a clear color, the raw clear color channels
generally won't be interpreted correctly during format reinterpretation.
So, prevent Vulkan apps from trying to use such dmabufs as mutable
format render targets. Also, prevent such apps from using such dmabufs
as blorp_copy() destinations if doing so would require format
reinterpretation.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31136>
blorp_copy() will sometimes use a complex shader if the source and
destination surface formats differ. For example, it will do this when
both formats support CCS_E, but have differing numbers of
bits-per-channel.
To reduce the chance of using this complex shader during transfers
between images and buffers, ensure the same format is used. We can't
completely prevent the complex shader because a copy may happen between
surface formats that have a different number of bits-per-pixel.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31136>
Patch converts MI_LOAD_REGISTER_MEM, MI_LOAD_REGISTER_IMM to use
mi_builder in CmdBeginTransformFeedbackEXT.
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31502>
This avoids sprinkling those all over the code base. Debug breakpoints
are put in there too.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Backport-to: 24.2
Reviewed-by: Rohan Garg <rohan.garg@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31481>
The workaround is already implemented by
batch_emit_pipe_control_write(), we don't need to do it here as well.
This was spotted by Lionel Landwerlin. The credits go to him, I just
wrote the patch.
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31412>
Commit a603cc0633 ("anv: move some pc was to
batch_emit_pipe_control_write") moved some WAs from
emit_apply_pipe_flushes() to batch_emit_pipe_control_write(), but it
turns out one of them was already there since cf7e1f3817 ("anv,
iris: add missing CS_STALL bit for GPGPU texture invalidation").
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31412>
This is also encouraged by another wa, Wa_14018813551.
Both workarounds state that StackIDControlOverride_RTGlobals should
always be set to 0 (i.e. 2k).
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/30937>
When allocating a buffer normally, this flag gets to the allocator from
the memory requirements, but when sparse bindings are created we were
checking for them but never setting them.
Fixes sparse descriptor buffers on Xe2.
Makes the failure on TRTT more obvious.
Fixes: c6a91f1695 ("anv: add new heap/pool for descriptor buffers")
Fixes: 692e1ab2c1 ("anv: get rid of the second dynamic state heap")
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31372>
Valgrind doesn't seem to know that drmSyncobjQuery() writes to the
variable that we pass as 'last_value'. This gets rid of:
==6275== Conditional jump or move depends on uninitialised value(s)
==6275== at 0x5308370: anv_sparse_trtt_garbage_collect_batches (anv_sparse.c:540)
==6275== by 0x53091E2: anv_sparse_bind_trtt (anv_sparse.c:825)
==6275== by 0x5309771: anv_sparse_bind (anv_sparse.c:953)
==6275== by 0x5309A3B: anv_free_sparse_bindings (anv_sparse.c:1041)
==6275== by 0x529FF21: anv_DestroyBuffer (anv_buffer.c:248)
==6275== by 0x932ADBD: ??? (in /usr/lib/x86_64-linux-gnu/libVkLayer_khronos_validation.so)
==6275== by 0x127AA2: MyVkBuffer::~MyVkBuffer() (sparse.cpp:364)
==6275== by 0x12B2D4: MyApp::test1_trivial_sparse() (sparse.cpp:1421)
==6275== by 0x13E01A: MyApp::run_test(int) (sparse.cpp:6594)
==6275== by 0x13E3B0: main (sparse.cpp:6656)
==6275== Uninitialised value was created by a stack allocation
==6275== at 0x53082D3: anv_sparse_trtt_garbage_collect_batches (anv_sparse.c:525)
An alternative to these Valgrind macros would simply have been to
zero-intialize last_value.
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31332>
I just noticed that my custom sparse program was not working correctly
when I used ANV_QUEUE_OVERRIDE (instead of enabling the compute queue
by default or using INTEL_ENGINE_CLASS_COMPUTE, which was removed by
commit 600d88ab3c ("intel: Remove INTEL_ENGINE_CLASS_COMPUTE and
INTEL_ENGINE_CLASS_COPY parameters").
It turns out we were not setting the same engine class type when using
ANV_QUEUE_OVERRIDE vs the other cases. Move the code around so the
behavior can stay the same.
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31332>
