Three reasons for that:
0. The operation we're doing here is actually a reallocation.
1. The newer code is, IMHO, easier to read.
2. Realloc has this property where sometimes, when possible, it will
expand your array without moving it somewhere else, so it doesn't
need to copy the memory contents, returning the original pointer
back to you. I did some analysis and while that case is not common,
it does happen sometimes in real world applications (I could see it
happening in Shootergame and Aztec Ruins, but not Dota 2), so we're
able to save a few CPU cycles.
v2: Rebase.
Reviewed-by: Ivan Briano <ivan.briano@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/20703>
Because this is counting the array length of the things related to the
BOs, just like syncobj_array_length is counting the array length of
the things related to syncobjs.
v2: Rebase.
Reviewed-by: Ivan Briano <ivan.briano@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/20703>
This is the only code path where we don't run anv_execbuf_finish() in
case anv_execbuf_add_bo() fails. While there is not a bug in the
current tree, I recently made an (uncommitted) modification that
started leaking memory and made me realize the lack of cleanup here.
If we had anv_execbuf_finish() being called upon error like we're
going to have after this patch my modification wouldn't have caused
the memory leak.
I think it's much safer and future-proof if we're able to operate
under the assumption that whatever is allocated and set to anv_execbuf
will be dealt with upon failure of anything else related to it, so
functions that fail should only be required to free pointers not yet
assigned to anv_execbuf.
The dEQP-VK 'alloc_callback_fail' tests should exercise this code
path. The one I was specifically using here is:
dEQP-VK.api.object_management.alloc_callback_fail.device_group
v2: Rebase.
Reviewed-by: Ivan Briano <ivan.briano@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/20703>
Because anv_execbuf_add_bo_bitset() calls anv_execbuf_add_bo(), which
can fail if its memory allocations fail.
I have seen dEQP tests exercising memory allocation failures during
anv_execbuf_add_bo(), but I don't think the path coming from
add_bo_biset() was specifically exercised. Anyway, add the error check
just in case.
v2: Rebase.
Cc: mesa-stable
Reviewed-by: Ivan Briano <ivan.briano@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/20703>
In anv_execbuf_add_syncobj(), we try to not create or use
exec->syncobj_values if we don't need to. But when we figure we're
going to need it (i.e., when timeline_value is not zero), then we
create exec->syncobj_values with vk_zalloc, which means every previous
value is set to zero, as it should be. This is all correct.
The problem starts when we add a 16th element. In this case we double
exec->syncobj_array_length and realloc the buffer by using vk_alloc
and copying the old array to the new one. After that, we write the
timeline_value to the array only if it's not zero, and that's the
problem: since we just used vkalloc and memcpy, we don't have any
guarantees that the new array will be zero after the 16th element, and
if timeline_value is zero we write nothing to that position.
Once we start using exec->syncobj_values we have to commit to using
it, so the "if (timeline_value)" check near the end of the function
has to be changed to "if (exec->syncobj_values)", so we actually set
elements after the 16th to zero when they need to be zero. Another
approach to fix this would be to memset the new elements once we
double syncobj_array_length.
In practice, I couldn't find any application or deqp test that used
more than 3 elements in exec->syncobj_array_length, and we need more
than 16 elements in order to be able to reproduce the bug, so I'm not
aware of any real-world bug that goes away with this patch. This issue
was found while reading code.
If we craft a little Vulkan program that submits a ton of timeline and
binary semaphores on vkQueueSubmit, then waits for them, we get the
following error without this patch:
MESA: error: ../../src/intel/vulkan/anv_batch_chain.c:1910: execbuf2 failed: Invalid argument (VK_ERROR_DEVICE_LOST)
v2: Rebase.
Cc: mesa-stable
Reviewed-by: Ivan Briano <ivan.briano@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/20703>
There is no change in behavior here.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Acked-by: Rohan Garg <rohan.garg@intel.com>
Acked-by: Marcin Ślusarz <marcin.slusarz@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20428>
This functions will be used by i915 and Xe KMD.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Acked-by: Rohan Garg <rohan.garg@intel.com>
Acked-by: Marcin Ślusarz <marcin.slusarz@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20428>
More code re-organization to separate i915_drm.h specific code from
the rest.
No behavior changes here.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Acked-by: Rohan Garg <rohan.garg@intel.com>
Acked-by: Marcin Ślusarz <marcin.slusarz@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20428>
We still maintain our own table of formats but all of the conversion and
sampling info we pull from common code.
Acked-by: Alejandro Piñeiro <apinheiro@igalia.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/19950>
Using a pointer to an internal data structure works but it's a bit
sketchy. Since every anv_format maps to a VkFormat, we may as well
just use the VkFormat.
Reviewed-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/19950>
We already show the address range, which is most of why I'd think you'd be
looking at hex values. I find a more human-readable number nice for
debugging, instead of counting zeroes to decide if it's 1.5MB or 96kb.
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20540>
Some binding table entries have been identify as unused in the shaders
by the push constant analysis pass. We can just put the null entry in
there.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: b49b18f0b7 ("anv: reduce BT emissions & surface state writes with push descriptors")
Reviewed-by: Emma Anholt <emma@anholt.net>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20555>
We can't just check the load_ubo range is contained in the push entry,
we also need to check that the push entry set/binding matches the
load_ubo set/binding.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: ff91c5ca42 ("anv: add analysis for push descriptor uses and store it in shader cache")
Reviewed-by: Emma Anholt <emma@anholt.net>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20555>
We'll use those to fill the push constant addresses, so we can't have
them turned to null.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: ff91c5ca42 ("anv: add analysis for push descriptor uses and store it in shader cache")
Reviewed-by: Emma Anholt <emma@anholt.net>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20555>
This 2 PIPELINE_CONTROL flushes are not necessary for TGL and newer
and also it have different requirements of flush, so here doing
this two changes at the same time.
As no ANV_PIPE_INVALIDATE_BITS is set as parameter of
anv_add_pending_pipe_bits(),
genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer) will only emit one
PIPELINE_CONTROL.
BSpec: 44505
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20501>
For INTEL_MEASURE, ensure all prior instructions completed before
timestamp taken. Continue to support no CS flush case for Perfetto.
CS stall was dropped from pipecontrol when adding u_trace support.
Fixes: cc5843a573 ("anv: implement u_trace support")
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20502>
After using task shader, we need to emit a zero URB state and a
nullprim (empty pipe control) before rendering with primitives.
After this, a normal URB state needs to be returned, this will
happen when pipeline batch is emitted during pipeline switch.
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/20334>
To avoid the replication of code to properly emit PIPELINE_SELECT.
init_compute_queue_state() had a different emit of PIPELINE_SELECT but
as there is no compute engine in GFX VER 11 we are safe with the
differences.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20444>
The loop going from 0 to max_draw_count multiplies the value which
could potentially overflow.
Fixes Coverity CID 1517852
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: 3596a8ea7a ("anv: factor out some indirect draw count entry points")
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20436>
The issue we're addressing here is that we have 2 batches and the both
grow at different rate. We want to keep doubling the main batch size
as the application writes more and more commands to limit the number
of GEM BOs. But we don't want to have the generation batch size to be
linked to the main batch.
v2: remove gfx7 code
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Rohan Garg <rohan.garg@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/15642>
anv_bo has information that will be needed by a future patch in
anv_gem_mmap(), so here already preparing for that.
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20423>
Add the PSS stall bit to ANV_PIPE_STALL_BITS so that it get's cleared on
flush.
Fixes: f3c62973 ("anv,iris: PSS Stall Sync around color fast clears")
Signed-off-by: Rohan Garg <rohan.garg@intel.com>
Reviewed-by: Nanley Chery <nanley.g.chery@intel.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20317>
Pre-patch, anv_descriptor_pool used a free list for host allocations
that never merged adjacent free blocks. If the pool only allocated
fixed-sized blocks, then this would not be a problem. But the pool
allocations are variable-sized, and this caused over half of the pool's
memory to be consumed by unusable free blocks in some workloads, causing
unnecessary memory footprint.
Replacing the free list with util_vma_heap, which does merge adjacent
free blocks, fixes the memory explosion in the target workload.
Disdavantges of util_vma_heap compared to the free list:
- The heap calls malloc() when a new hole is created.
- The heap calls free() when a hole disappears or is merged with an
adjacent hole.
- The Vulkan spec expects descriptor set creation/destruction to be
thread-local lockless in the common case. For workloads that
create/destroy with high frequency, malloc/free may cause overhead.
Profiling is needed.
Tested with a ChromeOS internal TensorFlow benchmark, provided by
package 'tensorflow', running with its OpenCL backend on clvk.
cmdline: benchmark_model --graph=mn2.tflite --use_gpu=true --min_secs=60
gpu: adl
memory footprint from start of benchmark:
before: init=132.691MB max=227.684MB
after: init=134.988MB max=134.988MB
Reported-by: Romaric Jodin <rjodin@google.com>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20289>
This reverts commit b1126abb38.
This breaks all hell at least on DG2, as there are several cases left
where current_pipeline gets checked against GPGPU to decide what to do,
and the value doesn't match that of ANV_HW_PIPELINE_STATE_COMPUTE.
On top of that, it also misses checking for
ANV_HW_PIPELINE_STATE_RAYTRACING.
Then there's the fact that in some cases, current_pipeline will be
UINT32_MAX, because it's the original undefined state and also used
after executing a secondary command buffer because we are not tracking
on which pipeline did the secondary left us.
Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/7910
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/20349>
