There is currently no protection against walking a hash (using
_mesa_HashWalk()) and modifying it at the same time, for instance by inserting
or deleting elements. This leads to segfaults in multithreaded code if e.g.
someone calls glTexImage2D (which may have to walk the list of FBOs) while
another thread is calling glDeleteFramebuffers on another thread with the two
contexts sharing lists.
The reason for this is that _mesa_HashWalk() doesn't actually take the mutex
that normally protects the hash; it takes an entirely different mutex.
Thus, walks are only protected against other walks, and there is also no
outer lock taking this. There is an old comment saying that this is to fix
problems with deadlock if the callback needs to take a mutex; we solve this
by changing the mutex to be recursive.
A demonstration Helgrind hit from a real application:
==13412== Possible data race during write of size 8 at 0x3498C6A8 by thread #1
==13412== Locks held: 2, at addresses 0x1AF09530 0x2B3DF400
==13412== at 0x1F040C99: _mesa_hash_table_remove (hash_table.c:395)
==13412== by 0x1EE98174: _mesa_HashRemove_unlocked (hash.c:350)
==13412== by 0x1EE98174: _mesa_HashRemove (hash.c:365)
==13412== by 0x1EE2372D: _mesa_DeleteFramebuffers (fbobject.c:2669)
==13412== by 0x6105AA4: movit::ResourcePool::cleanup_unlinked_fbos(void*) (resource_pool.cpp:473)
==13412== by 0x610615B: movit::ResourcePool::release_fbo(unsigned int) (resource_pool.cpp:442)
[...]
==13412== This conflicts with a previous read of size 8 by thread #20
==13412== Locks held: 2, at addresses 0x1AF09558 0x1AF73318
==13412== at 0x1F040CD9: _mesa_hash_table_next_entry (hash_table.c:415)
==13412== by 0x1EE982A8: _mesa_HashWalk (hash.c:426)
==13412== by 0x1EED6DFD: _mesa_update_fbo_texture.part.33 (teximage.c:2683)
==13412== by 0x1EED9410: _mesa_update_fbo_texture (teximage.c:3043)
==13412== by 0x1EED9410: teximage (teximage.c:3073)
==13412== by 0x1EEDA28F: _mesa_TexImage2D (teximage.c:3105)
==13412== by 0x166A68: operator() (mixer.cpp:454)
There are many more interactions than just these two possible.
Cc: 11.2 12.0 13.0 <mesa-stable@lists.freedesktop.org>
Signed-off-by: Steinar H. Gunderson <steinar+mesa@gunderson.no>
Reviewed-by: Timothy Arceri <timothy.arceri@collabora.com>
Now that we have switched to using nir_shader_gather_info() we
can remove the hacks and just use the system variable.
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
This takes us one step closer to being able to drop the GLSL IR
optimisation passes during linking in favour of the NIR passes.
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
This allows for gl_PrimitiveId to come in as a system value rather than as
an input. This is the way it will come in from SPIR-V. We keeps the input
path working for now so we don't break GL.
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
Cc: "13.0" <mesa-stable@lists.freedesktop.org>
this will be shared between OpenGL and Vulkan drivers
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
It looks like I added this version as a short-hand for users that didn't
need the fuller version. I don't think there's any real utility in
that. I'm not sure what my thinking was there. Maybe if those users
overloaded the recursion function could just call the compact version to
avoid passing some parameters? None of the users do that.
Either way, having this extra overload is not useful. Delete it.
Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
Half float support already exists for desktop GL. Reuse the
ARB_half_float_vertex enable bit and account for the different enum to
enable the extension for ES2.
Signed-off-by: Kevin Strasser <kevin.strasser@intel.com>
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
This patch should have been the part of commit e592f7df.
In a situation when there are multiple render targets with alpha testing
enabled, if fragment shader doesn't write to draw buffer zero, it causes
the GPU hang on SKL. No GPU hang is seen on HSW. Simulator gives a
warning for all gen6+ h/w:
"Illegal render target write message length 0xa expected 0xc"
This patch fixes the GPU hang as well as the simulator warning with
new piglit test fbo-mrt-alphatest-no-buffer-zero-write:
https://patchwork.freedesktop.org/patch/118212
No regressions in Jenkins CI system.
Cc: "12.0 13.0" <mesa-stable@lists.freedesktop.org>
Signed-off-by: Anuj Phogat <anuj.phogat@gmail.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
For gen9+ this will indicate when we should allow hiz based sampling
during rendering.
Improves performance in :
- Synmark's OglDeferred by 2.2% (n=20)
- Synmark's OglShMapPcf by 0.44% (n=20)
v2 by Ben: Add spec reference, and make it fix with some of the changes made on
the previous patches
Change the check from mt->aux_buf to mt->num_samples. The presence of an aux_buf
isn't enough to determine there isn't a HiZ buffer to use.
v3: It seems all depth surface end up with num_samples = 0 by default,
so allow sampling from depth HiZ if num_samples <= 1. (Lionel)
Allow sampling from HiZ only if all LOD are available from the HiZ
buffer. (Lionel)
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> (v2)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> (v3)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
The original functionality this patch introduces was authored by a patch from
Ken (the commit subject was the same). Since I ended up changing so many patches
in the code before this one, I had some non-trivial decisions to make, and I
didn't feel it was appropriate to keeps Ken's name as author (mostly because he
might not like what I've done). Ken's original patch was like 2 LOC :-)
In either case, some credit needs to go to Ken, and to Jordan for a few small
other changes in that original patch.
v2: Back to a smaller diff now that ISL handles most of the actual
programming (Lionel)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> (v1)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> (v2)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
Currently it indicates that this is never supported, but soon it will
be supported for gen8+^w gen9+
v2 by Ben:
- Explicitly disable aux_hiz for gen < 9 (with comment)
- squashed in next patch to avoid unused and useless functions
i965: Support sampling with hiz during rendering
For gen8, we can sample from depth while using the hiz buffer. This
allows us to sample depth without resolving from hiz to the depth
texture.
To do this we must resolve to hiz before drawing so we can use the hiz
buffer to sample while rendering. Hopefully the hiz buffer will
already be resolved in most cases because it was previously rendered,
meaning the hiz resolve is a no-op.
Note that this is still controlled by the
intel_miptree_sample_with_hiz function, and we will enable hiz
sampling for gen8 in a separate patch.
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> (v2)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
This seems counter to the goal of consolidating hiz, mcs, and later ccs buffers.
Unfortunately, hiz on gen6 is a thing the code supports, and this wart will be
helpful to achieve that. Overall, I believe it does help unify AUX buffers on
gen7+.
I updated the size field which I introduced in the previous patch, even though
we have no use for it.
XXX: As I mentioned in the last patch, the height given to the MCS buffer
allocation in intel_miptree_alloc_mcs() looks wrong, but I don't claim to fully
understand how the MCS buffer is laid out.
v2: rebase on master (Lionel)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> (v1)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> (v2)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
This patch will preserve the BO & offset, and not the miptree for the
aux_mcs buffer. Eventually it might make sense to pull put the sizing
function in miptree creation, but for now this should be sufficient
and not too hideous.
v2: Save BO's offset too (Lionel)
v3: Squash previous patch storing the size of the allocated aux buffer
(Lionel)
Fix memory leak with mcs_buf->bo (Lionel)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> (v1)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> (v2)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
The next patch will change the map type, and this will make sure there are no
regressions as a result of the other stuff. Since the miptree is newly created,
I believe it is always safe to just map.
It is possible to CPU map this buffer on LLC platforms (it additionally requires
rounding up to tile size). I did experiment with that patch, and found no
performance gains to be had.
I've added in error handling while here. Generally GTT mapping is an operation
which is highly unlikely to fail, but we may as well handle it when it does.
v2: rebase on master (Lionel)
v3: print out error if gtt mapping fails (Topi)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com> (v1)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> (v2)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
This will allow us to treat HiZ and MCS the same when using as an
auxiliary surface buffer.
v2: (Ben) Minor rebase conflict resolution.
Rename mcs_buf to aux_buf to address upcoming change for hiz specific buffers.
That second thing is essentially a squash of:
i965/gen8: Use intel_miptree_aux_buffer for auxiliary buffer - which didn't need
to be separate in my opinion.
v3: rebase on master (Lionel)
Signed-off-by: Jordan Justen <jordan.l.justen@intel.com> (v1)
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>a (v2)
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> (v3)
Reviewed-by: Topi Pohjolainen <topi.pohjolainen@intel.com>
This fixes a problem seen with gallium drivers vs android wallpaper.
Basically, what happens is:
EGLSurface tmpSurface = mEgl.eglCreatePbufferSurface(mEglDisplay, mEglConfig, attribs);
mEgl.eglMakeCurrent(mEglDisplay, tmpSurface, tmpSurface, mEglContext);
int[] maxSize = new int[1];
Rect frame = surfaceHolder.getSurfaceFrame();
glGetIntegerv(GL_MAX_TEXTURE_SIZE, maxSize, 0);
mEgl.eglMakeCurrent(mEglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
mEgl.eglDestroySurface(mEglDisplay, tmpSurface);
... check maxSize vs frame size and bail if needed ...
mEglSurface = mEgl.eglCreateWindowSurface(mEglDisplay, mEglConfig, surfaceHolder, null);
... error checking ...
mEgl.eglMakeCurrent(mEglDisplay, mEglSurface, mEglSurface, mEglContext);
When the window-surface is created, it ends up with the same ptr address
as the recently freed tmpSurface pbuffer surface. Which after many
levels of indirection, results in st_framebuffer_validate() ending up with
the same/old framebuffer object, and in the end never calling the
DRIimageLoaderExtension::getBuffers(). Then in droid_swap_buffers(), the
dri2_surf is still the old pbuffer surface (with dri2_surf->buffer being
NULL, obviously, so when wallpaper app calls eglSwapBuffers() nothing
gets enqueued to the compositor). Resulting in a black/blank background
layer.
Note that at the EGL layer, when the context is unbound, EGL drops it's
references to the draw and read buffer as well.
Signed-off-by: Rob Clark <robdclark@gmail.com>
Tested-by: Robert Foss <robert.foss@collabora.com>
Acked-by: Tapani Pälli <tapani.palli@intel.com>
This fixes random crashes with MSVC release builds. It seems the
members are implicitly initialized to zero with gcc, but not MSVC.
In particular, the tex_offset_num_offset field was non-zero causing
a loop over the NULL tex_offsets array to crash.
Zero-init those fields and a few others to be safe.
The regression began with acc23b04cf "ralloc: remove memset from
ralloc_size".
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
GL_ARB_ES3_compatibility brings ETC2/EAC formats to desktop GL.
The meaning of the GL compressed format list is pretty vague - it's
supposed to return formats for "general-purpose usage". (GL 4.2
deprecates the list because of this.) Basically everyone interprets
this as "linear RGB/RGBA".
ETC2/EAC meets that criteria, so while we shouldn't be required to add
it to the list, there's also little harm in doing so, at least on
platforms with native support. I doubt anyone is using this list for
much anyway, so even on platforms without native support, it's probably
not a big deal.
Makes the following GL45-CTS.gtf43 tests pass:
* GL3Tests.eac_compression_r11.gl_compressed_r11_eac
* GL3Tests.eac_compression_rg11.gl_compressed_rg11_eac
* GL3Tests.eac_compression_signed_r11.gl_compressed_signed_r11_eac
* GL3Tests.eac_compression_signed_rg11.gl_compressed_signed_rg11_eac
* GL3Tests.etc2_compression_rgb8.gl_compressed_rgb8_etc2
* GL3Tests.etc2_compression_rgb8_pt_alpha1.gl_compressed_rgb8_pt_alpha1_etc2
* GL3Tests.etc2_compression_rgba8.gl_compressed_rgba8_etc2
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Reviewed-by: Eduardo Lima Mitev <elima@igalia.com>
A (latent) bug in VDPAU interop was exposed by commit
e5cc84dd43.
Before that commit, the st_vdpau code created samplers with
first_layer == last_layer == 1 that the general texture handling code
would immediately delete and re-create, because the layer does not match
the information in the GL texture object.
This was correct behavior at least in the DMABUF case, because the imported
resource is supposed to have the correct offset already applied. In the
non-DMABUF case, this was just plain wrong but apparently nobody noticed.
After that commit, the state tracker assumes that an existing sampler is
correct at all times. Existing samplers are supposed to be deleted when
they may become invalid, and they will be created on-demand. This meant
that the sampler with first_layer == last_layer == 1 stuck around, leading
to rendering artefacts (on radeonsi), command stream failures (on r600), and
assertions (in debug builds everywhere).
This patch fixes the problem by simply not creating a sampler at all in
st_vdpau_map_surface. We rely on the generic texture code to do the right
thing, adding the layer_override to make the non-DMABUF case work.
v2: add the layer_override
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98512
Cc: 13.0 <mesa-stable@lists.freedesktop.org>
Cc: Christian König <deathsimple@vodafone.de>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Reviewed-by: Marek Olšák <marek.olsak@amd.com> (v1)
Reviewed-by: Christian König <christian.koenig@amd.com>
When splitting up loads, we have to add 16 bytes to the offset for
the high components, just like already happens for stores.
Fixes arb_gpu_shader_fp64@shader_storage@layout-std140-fp64-shader.
Cc: 13.0 <mesa-stable@lists.freedesktop.org>
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
Valgrind reports that we use cfg.cycle_count uninitialised, so zero the
cfg_t on construction.
Fixes: 52d2b28f7f ("ralloc: use rzalloc where it's necessary")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Marek Olšák <marek.olsak@amd.com>
commit cc6aa1d161 changed to using rzalloc
for gl_program creation but one instance for program creation was still
using calloc.
Signed-off-by: Tapani Pälli <tapani.palli@intel.com>
Reviewed-by: Juan A. Suarez <jasuarez@igalia.com>
Reviewed-by: Iago Toral Quiroga <itoral@igalia.com>
Reviewed-by: Timothy Arceri <timothy.arceri@collabora.com>
This moves the delete linked shaders call to
_mesa_clear_shader_program_data() which makes sure we delete them
before returning due to any validation problems.
It also reduces some code duplication.
From the OpenGL 4.5 Core spec:
"If LinkProgram failed, any information about a previous link of
that program object is lost. Thus, a failed link does not restore
the old state of program.
...
If one of these commands is called with a program for which
LinkProgram failed, no error is generated unless otherwise noted.
Implementations may return information on variables and interface
blocks that would have been active had the program been linked
successfully. In cases where the link failed because the program
required too many resources, these commands may help applications
determine why limits were exceeded."
Therefore it's expected that we shouldn't be able to query the
program that failed to link and retrieve information about a
previously successful link.
Before this change the linker was doing validation before freeing
the previously linked shaders and therefore could exit on failure
before they were freed.
This change also fixes an issue in compat profile where a program
with no shaders attached is expect to fall back to fixed function
but was instead trying to relink IR from a previous link.
Reviewed-by: Tapani Pälli <tapani.palli@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97715
Cc: "13.0" <mesa-stable@lists.freedesktop.org>
This will allow use to use ralloc_parent() on the info field and fix
a regression in nir_sweep() caused by e1af20f18a.
This is intended to be a temporary requirement that will be removed
when we finish separating shader_info from nir_shader.
Reviewed-by: Eric Anholt <eric@anholt.net>
This allows us to use ralloc_parent() to see which data structure owns
shader_info which allows us to fix a regression in nir_sweep().
This will also allow us to move some fields from gl_linked_shader to
gl_program, which will allow us to do some clean-ups like storing
gl_program directly in the CurrentProgram array in gl_pipeline_object
enabling some small validation optimisations at draw time.
Also it is error prone to depend on the gl_linked_shader for
programs in current use because a failed linking attempt will free
infomation about the current program. In i965 we could be trying
to recompile a shader variant but may have lost some required fields.
Reviewed-by: Eric Anholt <eric@anholt.net>
The emission of vertex attributes corresponding to dvec3 and dvec4
vertex shader input variables was not correct when the <size> passed
to the VertexAttribL* commands was <= 2.
This was because we were using the vertex array size when emitting vertices
to decide if we uploaded a 64-bit floating point attribute as 1 slot (128-bits)
for sizes 1 and 2, or 2 slots (256-bits) for sizes 3 and 4. This caused problems
when mapping the input variables to registers because, for deciding which
registers contain the values uploaded for a certain variable, we use the size
and type given to the variable in the shader, so we will be assigning 256-bits
to dvec3/4 variables, even if we only uploaded 128-bits for them, which happened
when the vertex array size was <= 2.
The patch uses the shader information to only emit as 128-bits those 64-bit floating
point variables that were declared as double or dvec2 in the vertex shader. Dvec3 and
dvec4 variables will be always uploaded as 256-bits, independently of the <size> given
to the VertexAttribL* command.
From the ARB_vertex_attrib_64bit specification:
"For the 64-bit double precision types listed in Table X.1, no default
attribute values are provided if the values of the vertex attribute variable
are specified with fewer components than required for the attribute
variable. For example, the fourth component of a variable of type dvec4
will be undefined if specified using VertexAttribL3dv or using a vertex
array specified with VertexAttribLPointer and a size of three."
We are filling these unspecified components with zeros, which coincidentally is
also what the GL44-CTS.vertex_attrib_binding.basic-inputL-case1 expects.
v2: Do not use bitcount (Kenneth Graunke)
Fixes: GL44-CTS.vertex_attrib_binding.basic-inputL-case1 test
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97287
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
3DSTATE_WM_CHROMAKEY isn't programmed anywhere else.
3DSTATE_WM_HZ_OP is programmed, then cleared by blorp during a
HZ op, so repeatedly clearing it after every blorp execution is
redundant.
Signed-off-by: Nanley Chery <nanley.g.chery@intel.com>
Reviewed-by: Anuj Phogat <anuj.phogat@gmail.com>
This packet is non-pipelined and doesn't ever change across emissions.
Signed-off-by: Nanley Chery <nanley.g.chery@intel.com>
Reviewed-by: Anuj Phogat <anuj.phogat@gmail.com>
No change in behavior. ralloc_size is equivalent to rzalloc_size.
That will change though.
Calls not switched to rzalloc_size:
- ralloc_vasprintf
- glsl_type::name allocation (it's filled with snprintf)
- C++ classes where valgrind didn't show uninitialized values
I switched most of non-glsl stuff to rzalloc without checking whether
it's really needed.
Reviewed-by: Edward O'Callaghan <funfunctor@folklore1984.net>
Tested-by: Edmondo Tommasina <edmondo.tommasina@gmail.com>
Reviewed-by: Nicolai Hähnle <nicolai.haehnle@amd.com>
The address immediate field is only 9 bits and, since the value is in
bytes, the highest GRF we can point to with it is g15. This makes it
pretty close to useless for MOV_INDIRECT. There were already piles of
restrictions preventing us from using it prior to Broadwell, so let's get
rid of the gen8+ code path entirely.
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97779
Cc: "12.0 13.0" <mesa-stable@lists.freedesktop.org>
Reviewed-by: Matt Turner <mattst88@gmail.com>
Commit 66fcfa6894 changed the vec4 version of offset() to have 3
parameters instead of 2 but the vec4_cmod_propagation test was never
updated.
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Juan A. Suarez Romero
Steinar H. Gunderson
Kenneth Graunke
Brian Paul
Timothy Arceri
Jason Ekstrand
Tapani Pälli
Ian Romanick
Marek Olšák
Kevin Strasser
Jordan Justen
Anuj Phogat
Ben Widawsky
Rob Clark
Nicolai Hähnle
Chris Wilson
Antia Puentes
Nanley Chery
Juha-Pekka Heikkila