Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Hello Benedikt, Am Sonntag, den 29.04.2018, 00:06 +0200 schrieb Benedikt Schemmer: > Hi Gert > > Am 28.04.2018 um 23:51 schrieb Gert Wollny: > > Am Samstag, den 28.04.2018, 22:43 +0200 schrieb Benedikt Schemmer: > > > The patches apply cleanly, however I just did a shader-db test > > > run > > > and can't find a difference with your patch > > > applied, am I doing something wrong? > > > > AFAIK radeonsi doesn't use the register-merge optimizer in TGSI. > > > > Ah, ok. Was wondering why your debug code doesn't output anything. > Makes sense now ;) Not exactly, the reason there is no output is because -DNDEBUG is set. Without it the statistics should also be printed out on radeonsi, but thinking of it I should probably disable it when register_merge is not accessed, because without this the numbers will be inflated and don't have much meaning. > So is this useless on radeonsi? Indeed. > Seemed interesting to me. :) it certainly helps on r600 > > > compile times went up though: > > > > This is strange, because "see above". Did you compile with debug > > information and c++11 or higher enables? ... > > > > > not intentionally: Then you should actually not run any code that this series adds to mesa. I checked again, apart from the debugging output nothing will ever be run if a drivers that report PIPE_SHADER_CAP_TGSI_SKIP_MERGE_REGISTERS != 0 (as does radeonsi). Best, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/linker: Add nir_link_uniforms()
On 29/04/18 16:48, Alejandro Piñeiro wrote: From: Eduardo Lima Mitev This function will be the entry point for linking the uniforms from the nir_shader objects associated with the gl_linked_shaders of a program. This patch includes initial support for linking uniforms from NIR shaders. It is tailored for the ARB_gl_spirv needs, and it is far from complete, but it should handle most cases of uniforms, array uniforms, structs, samplers and images. There are some FIXMEs related to specific features that will be implemented in following patches, like atomic counters, UBOs and SSBOs. Also, note that ARB_gl_spirv makes mandatory explicit location for normal uniforms, so this code only handles uniforms with explicit location. But there are cases, like uniform atomic counters, that doesn't have a location from the OpenGL point of view (they have a binding), but that Mesa assign internally a location. That will be handled on following patches. A nir_linker.h file is also added. More NIR-linking related API will be added in subsequent patches and those will include stuff from Mesa, so reusing nir.h didn't seem a good idea. These files should actually be src/compiler/glsl/nir_link_uniforms.c etc these are not general purpose nir helpers they are GLSL specific. v2: update var->data.location with UniformStorage index when the storage was set up on a previous stage (Neil) Signed-off-by: Eduardo Lima Signed-off-by: Neil Roberts --- src/compiler/Makefile.sources| 2 + src/compiler/nir/meson.build | 2 + src/compiler/nir/nir_link_uniforms.c | 461 +++ src/compiler/nir/nir_linker.h| 41 4 files changed, 506 insertions(+) create mode 100644 src/compiler/nir/nir_link_uniforms.c create mode 100644 src/compiler/nir/nir_linker.h diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index bb86972ea1a..dba70a5e5a1 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -206,6 +206,8 @@ NIR_FILES = \ nir/nir_inline_functions.c \ nir/nir_instr_set.c \ nir/nir_instr_set.h \ + nir/nir_link_uniforms.c \ + nir/nir_linker.h \ nir/nir_linking_helpers.c \ nir/nir_liveness.c \ nir/nir_loop_analyze.c \ diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build index b28a565d0c6..baf5682d388 100644 --- a/src/compiler/nir/meson.build +++ b/src/compiler/nir/meson.build @@ -99,6 +99,8 @@ files_libnir = files( 'nir_inline_functions.c', 'nir_instr_set.c', 'nir_instr_set.h', + 'nir_link_uniforms.c', + 'nir_linker.h', 'nir_linking_helpers.c', 'nir_liveness.c', 'nir_loop_analyze.c', diff --git a/src/compiler/nir/nir_link_uniforms.c b/src/compiler/nir/nir_link_uniforms.c new file mode 100644 index 000..450a5ce2e21 --- /dev/null +++ b/src/compiler/nir/nir_link_uniforms.c @@ -0,0 +1,461 @@ +/* + * Copyright © 2018 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "nir.h" +#include "nir_linker.h" +#include "compiler/glsl/ir_uniform.h" /* for gl_uniform_storage */ +#include "compiler/linker_util.h" +#include "main/context.h" +#include "main/mtypes.h" + +/* This file do the common link for GLSL uniforms, using NIR, instead of IR as + * the counter-part glsl/link_uniforms.cpp + * + * Also note that this is tailored for ARB_gl_spirv needs and particularities + * (like need to work/link without name available, explicit location for + * normal uniforms as mandatory, and so on). + */ + +static void +nir_setup_uniform_remap_tables(struct gl_context *ctx, + struct gl_shader_program *prog) +{ + prog->UniformRemapTable = rzalloc_array(prog, + struct gl_uniform_storage *, + prog->NumUniformRemapTable); +
Re: [Mesa-dev] [PATCH 17/22] nir/linker: Add nir_build_program_resource_list()
Again this should be moved to src/compiler/glsl/ somewhere. On 18/04/18 00:36, Alejandro Piñeiro wrote: From: Eduardo Lima Mitev This function is equivalent to the compiler/glsl build_program_resource_list() but will extract the resources from NIR shaders instead. For now, only uniforms and program inputs are implemented. Signed-off-by: Eduardo Lima Signed-off-by: Alejandro Piñeiro --- src/compiler/nir/nir_linker.h | 3 ++ src/compiler/nir/nir_linking_helpers.c | 63 ++ 2 files changed, 66 insertions(+) diff --git a/src/compiler/nir/nir_linker.h b/src/compiler/nir/nir_linker.h index 09f7042dffa..0e3b609a4d2 100644 --- a/src/compiler/nir/nir_linker.h +++ b/src/compiler/nir/nir_linker.h @@ -38,6 +38,9 @@ void nir_set_uniform_initializers(struct gl_context *ctx, struct gl_shader_program *prog); +void nir_build_program_resource_list(struct gl_context *ctx, + struct gl_shader_program *prog); + #ifdef __cplusplus } /* extern "C" */ #endif diff --git a/src/compiler/nir/nir_linking_helpers.c b/src/compiler/nir/nir_linking_helpers.c index 2b0a2668a33..8733fffb4f3 100644 --- a/src/compiler/nir/nir_linking_helpers.c +++ b/src/compiler/nir/nir_linking_helpers.c @@ -24,6 +24,10 @@ #include "nir.h" #include "util/set.h" #include "util/hash_table.h" +#include "nir_linker.h" +#include "compiler/glsl/ir_uniform.h" /* for gl_uniform_storage */ +#include "compiler/linker_util.h" +#include "main/context.h" /* This file contains various little helpers for doing simple linking in * NIR. Eventually, we'll probably want a full-blown varying packing @@ -503,3 +507,62 @@ nir_compact_varyings(nir_shader *producer, nir_shader *consumer, compact_components(producer, consumer, comps, interp_type, interp_loc, default_to_smooth_interp); } + +void +nir_build_program_resource_list(struct gl_context *ctx, +struct gl_shader_program *prog) +{ + /* Rebuild resource list. */ + if (prog->data->ProgramResourceList) { + ralloc_free(prog->data->ProgramResourceList); + prog->data->ProgramResourceList = NULL; + prog->data->NumProgramResourceList = 0; + } + + struct set *resource_set = _mesa_set_create(NULL, + _mesa_hash_pointer, + _mesa_key_pointer_equal); + + /* Add uniforms +* +* Here, it is expected that nir_link_uniforms() has already been +* called, so that UniformStorage table is already available. +*/ + for (unsigned i = 0; i < prog->data->NumUniformStorage; i++) { + struct gl_uniform_storage *uniform = &prog->data->UniformStorage[i]; + + if (!add_program_resource(prog, resource_set, GL_UNIFORM, uniform, +uniform->active_shader_mask)) { + return; + } + } + + /* Add inputs */ + struct gl_linked_shader *sh = prog->_LinkedShaders[MESA_SHADER_VERTEX]; + if (sh) { + nir_shader *nir = sh->Program->nir; + assert(nir); + + nir_foreach_variable(var, &nir->inputs) { + struct gl_shader_variable *sh_var = +rzalloc(prog, struct gl_shader_variable); + + /* ARB_gl_spirv: names are considered optional debug info, so the linker + * needs to work without them, and returning them is optional. For + * simplicity we ignore names. + */ + sh_var->name = NULL; + sh_var->type = var->type; + sh_var->location = var->data.location; + + /* @TODO: Fill in the rest of gl_shader_variable data. */ + + if (!add_program_resource(prog, resource_set, GL_PROGRAM_INPUT, + sh_var, 1 << MESA_SHADER_VERTEX)) { +return; + } + } + } + + _mesa_set_destroy(resource_set, NULL); +} ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 15/22] nir/linker: Set the uniform initial values
This too should be moved to src/compiler/glsl/ somewhere. On 18/04/18 00:36, Alejandro Piñeiro wrote: From: Neil Roberts This is based on link_uniform_initializers.cpp. --- src/compiler/Makefile.sources| 1 + src/compiler/nir/meson.build | 1 + src/compiler/nir/nir_link_uniform_initializers.c | 292 +++ src/compiler/nir/nir_link_uniforms.c | 1 + src/compiler/nir/nir_linker.h| 4 + 5 files changed, 299 insertions(+) create mode 100644 src/compiler/nir/nir_link_uniform_initializers.c diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index dba70a5e5a1..305ea30b2ab 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -206,6 +206,7 @@ NIR_FILES = \ nir/nir_inline_functions.c \ nir/nir_instr_set.c \ nir/nir_instr_set.h \ + nir/nir_link_uniform_initializers.c \ nir/nir_link_uniforms.c \ nir/nir_linker.h \ nir/nir_linking_helpers.c \ diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build index baf5682d388..4f7a2f853e8 100644 --- a/src/compiler/nir/meson.build +++ b/src/compiler/nir/meson.build @@ -101,6 +101,7 @@ files_libnir = files( 'nir_instr_set.h', 'nir_link_uniforms.c', 'nir_linker.h', + 'nir_link_uniform_initializers.c', 'nir_linking_helpers.c', 'nir_liveness.c', 'nir_loop_analyze.c', diff --git a/src/compiler/nir/nir_link_uniform_initializers.c b/src/compiler/nir/nir_link_uniform_initializers.c new file mode 100644 index 000..94e213330ce --- /dev/null +++ b/src/compiler/nir/nir_link_uniform_initializers.c @@ -0,0 +1,292 @@ +/* + * Copyright © 2018 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "nir.h" +#include "nir_linker.h" +#include "compiler/glsl/ir_uniform.h" /* for gl_uniform_storage */ +#include "main/context.h" +#include "main/mtypes.h" + +struct set_opaque_binding_closure { + struct gl_shader_program *shader_prog; + struct gl_program *prog; + const nir_variable *var; + int binding; + int location; +}; + +static void +set_opaque_binding(struct set_opaque_binding_closure *data, + const struct glsl_type *type) +{ + if (glsl_type_is_array(type) && + glsl_type_is_array(glsl_get_array_element(type))) { + const struct glsl_type *element_type = glsl_get_array_element(type); + + for (unsigned int i = 0; i < glsl_get_length(type); i++) + set_opaque_binding(data, element_type); + + return; + } + + if (data->location < 0 || + data->location >= data->prog->sh.data->NumUniformStorage) + return; + + struct gl_uniform_storage *storage = + data->prog->sh.data->UniformStorage + data->location++; + + const unsigned elements = MAX2(storage->array_elements, 1); + + for (unsigned int i = 0; i < elements; i++) + storage->storage[i].i = data->binding++; + + for (int sh = 0; sh < MESA_SHADER_STAGES; sh++) { + struct gl_linked_shader *shader = data->shader_prog->_LinkedShaders[sh]; + + if (!shader) + continue; + if (!storage->opaque[sh].active) + continue; + + if (glsl_type_is_sampler(storage->type)) { + for (unsigned i = 0; i < elements; i++) { +const unsigned index = storage->opaque[sh].index + i; + +if (storage->is_bindless) { + if (index >= shader->Program->sh.NumBindlessSamplers) + break; + shader->Program->sh.BindlessSamplers[index].unit = + storage->storage[i].i; + shader->Program->sh.BindlessSamplers[index].bound = true; + shader->Program->sh.HasBoundBindlessSampler = true; +} else { + if (index >= ARRAY_SIZE(shader->Program->SamplerUnits)) + break; +
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Hi Gert, couldn't resist at least to try what would happen if I enable register merge for radeonsi: PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits piglit 80732 -0.16 % -0.02 % . .0.87 % 0.86 %0.04 % . . -- All affected 513 -17.58 % -2.30 % . .4.12 % 5.87 %1.73 %0.10 % . -- Total 80732 -0.16 % -0.02 % . .0.87 % 0.86 %0.04 % . . I had already removed the defines around the debug code so thats also happily outputting data. fails with two piglit shaders: [require] GLSL >= 3.30 [fragment shader] // [config] // expect_result: pass // glsl_version: 3.30 // require_extensions: GL_ARB_bindless_texture GL_ARB_shader_image_load_store // [end config] #version 330 #extension GL_ARB_bindless_texture: require #extension GL_ARB_shader_image_load_store: enable #extension GL_ARB_arrays_of_arrays: enable struct s { writeonly image2D img[3][2]; int y; }; void main() { s a[2][4]; imageStore(a[0][0].img[0][0], ivec2(0, 0), vec4(1, 2, 3, 4)); } and [require] GLSL >= 3.30 [fragment shader] // [config] // expect_result: pass // glsl_version: 3.30 // require_extensions: GL_ARB_bindless_texture // [end config] #version 330 #extension GL_ARB_bindless_texture: require #extension GL_ARB_arrays_of_arrays: enable struct s { sampler2D tex[3][2]; int y; }; out vec4 color; void main() { s a[2][4]; color = texture2D(a[0][0].tex[0][0], vec2(0, 0)); } Real world is a little different: Max Increase: SGPRS: 72 -> 96 (33.33 %) (in shaders/cat/1787.shader_test) VGPRS: 64 -> 84 (31.25 %) (in shaders/dirtrally/0859b69789591d7046e211400b1edd9a7cfca734_742.shader_test) Spilled SGPRs: 0 -> 16 (0.00 %) (in shaders/deusex_mankind/d64e2084204e29749639e8fbd9a1e507c7e5e1dd_6840.shader_test) Spilled VGPRs: 0 -> 0 (0.00 %) Private memory VGPRs: 24 -> 32 (33.33 %) (in shaders/deusex_mankind/28cac87049d8c833e72296a5a02ea6118f1144e5_5876.shader_test) Scratch size: 28 -> 36 (28.57 %) dwords per thread (in shaders/deusex_mankind/28cac87049d8c833e72296a5a02ea6118f1144e5_5876.shader_test) Code Size: 6988 -> 8036 (15.00 %) bytes (in shaders/cat/1847.shader_test) LDS: 0 -> 0 (0.00 %) blocks Max Waves: 5 -> 7 (40.00 %) (in shaders/ruiner/0967c5fce7fc456496b1cfa25fbb1d1c4dcf9bed_2958.shader_test) Wait states: 0 -> 0 (0.00 %) Max Decrease: SGPRS: 104 -> 64 (-38.46 %) (in shaders/deusex_mankind/480ddf21b1076d36f9ffd9911389656b5d8e12cb_2878.shader_test) VGPRS: 44 -> 36 (-18.18 %) (in shaders/ruiner/0967c5fce7fc456496b1cfa25fbb1d1c4dcf9bed_2958.shader_test) Spilled SGPRs: 19 -> 0 (-100.00 %) (in shaders/deusex_mankind/0749c9ae23417f918c7286fe502ff5de4cb8e1a0_3276.shader_test) Spilled VGPRs: 0 -> 0 (0.00 %) Private memory VGPRs: 0 -> 0 (0.00 %) Scratch size: 0 -> 0 (0.00 %) dwords per thread Code Size: 17576 -> 17276 (-1.71 %) bytes (in shaders/ruiner/75b96ff36f5328b9ff9366f0d0fd58a1046f51bc_3053.shader_test) LDS: 0 -> 0 (0.00 %) blocks Max Waves: 8 -> 5 (-37.50 %) (in shaders/deusex_mankind/8dabec49e5b6c3b1cbcbaee194eff69f164d72f4_3968.shader_test) Wait states: 0 -> 0 (0.00 %) PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits 0ad6 . . . . . . . . . aer 590 .0.26 % -20.00 % . . .0.34 % . . alien_isolation 1414 . . . . . . . . . anholt10 . . . . . . . . . bioshock_infinite 2581 -0.02 %0.03 % . . . .0.13 % . . blackmesa584 . . . . . . . . . cat 573 -0.06 % -0.12 % . . . .0.20 %0.05 % . csgo1392 . . -0.88 % . . . -0.03 % . . deadisland_definitive 17760.06 % . . . . .0.15 %0.01 % . deadisland_original11602 . . . . . .0.05 % . . deadisland_riptide_..293 -0.06 %0.06 % . . . .0.32 % . . deusex_mankind 50510.08 %
Re: [Mesa-dev] [PATCH] nir/linker: Add nir_link_uniforms()
On 29/04/18 10:13, Timothy Arceri wrote: > > > On 29/04/18 16:48, Alejandro Piñeiro wrote: >> From: Eduardo Lima Mitev >> >> This function will be the entry point for linking the uniforms from >> the nir_shader objects associated with the gl_linked_shaders of a >> program. >> >> This patch includes initial support for linking uniforms from NIR >> shaders. It is tailored for the ARB_gl_spirv needs, and it is far from >> complete, but it should handle most cases of uniforms, array >> uniforms, structs, samplers and images. >> >> There are some FIXMEs related to specific features that will be >> implemented in following patches, like atomic counters, UBOs and >> SSBOs. >> >> Also, note that ARB_gl_spirv makes mandatory explicit location for >> normal uniforms, so this code only handles uniforms with explicit >> location. But there are cases, like uniform atomic counters, that >> doesn't have a location from the OpenGL point of view (they have a >> binding), but that Mesa assign internally a location. That will be >> handled on following patches. >> >> A nir_linker.h file is also added. More NIR-linking related API will >> be added in subsequent patches and those will include stuff from Mesa, >> so reusing nir.h didn't seem a good idea. > > These files should actually be src/compiler/glsl/nir_link_uniforms.c > etc these are not general purpose nir helpers they are GLSL specific. Yes, it is true that are not general purpose nir helpers, but they are not GLSL specific either. As mentioned on the commit message and on the introductory comment, it is heavily tailored for ARB_gl_spirv, so they are SPIR-V specific. In fact, they fit better at src/compiler/spirv than on src/compiler/glsl. But as that directory is even more general purpose, so we placed them at src/compiler/nir as it using the nir-shader to do the linkage. The alternative would be place them on a new directory, something like src/compiler/gl_spirv, but that seems somewhat an overkill. > >> >> v2: update var->data.location with UniformStorage index when the >> storage was set up on a previous stage (Neil) >> >> Signed-off-by: Eduardo Lima >> Signed-off-by: Neil Roberts > Signed-off-by: Alejandro Piñeiro >> --- >> src/compiler/Makefile.sources | 2 + >> src/compiler/nir/meson.build | 2 + >> src/compiler/nir/nir_link_uniforms.c | 461 >> +++ >> src/compiler/nir/nir_linker.h | 41 >> 4 files changed, 506 insertions(+) >> create mode 100644 src/compiler/nir/nir_link_uniforms.c >> create mode 100644 src/compiler/nir/nir_linker.h >> >> diff --git a/src/compiler/Makefile.sources >> b/src/compiler/Makefile.sources >> index bb86972ea1a..dba70a5e5a1 100644 >> --- a/src/compiler/Makefile.sources >> +++ b/src/compiler/Makefile.sources >> @@ -206,6 +206,8 @@ NIR_FILES = \ >> nir/nir_inline_functions.c \ >> nir/nir_instr_set.c \ >> nir/nir_instr_set.h \ >> + nir/nir_link_uniforms.c \ >> + nir/nir_linker.h \ >> nir/nir_linking_helpers.c \ >> nir/nir_liveness.c \ >> nir/nir_loop_analyze.c \ >> diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build >> index b28a565d0c6..baf5682d388 100644 >> --- a/src/compiler/nir/meson.build >> +++ b/src/compiler/nir/meson.build >> @@ -99,6 +99,8 @@ files_libnir = files( >> 'nir_inline_functions.c', >> 'nir_instr_set.c', >> 'nir_instr_set.h', >> + 'nir_link_uniforms.c', >> + 'nir_linker.h', >> 'nir_linking_helpers.c', >> 'nir_liveness.c', >> 'nir_loop_analyze.c', >> diff --git a/src/compiler/nir/nir_link_uniforms.c >> b/src/compiler/nir/nir_link_uniforms.c >> new file mode 100644 >> index 000..450a5ce2e21 >> --- /dev/null >> +++ b/src/compiler/nir/nir_link_uniforms.c >> @@ -0,0 +1,461 @@ >> +/* >> + * Copyright © 2018 Intel Corporation >> + * >> + * Permission is hereby granted, free of charge, to any person >> obtaining a >> + * copy of this software and associated documentation files (the >> "Software"), >> + * to deal in the Software without restriction, including without >> limitation >> + * the rights to use, copy, modify, merge, publish, distribute, >> sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom >> the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice (including >> the next >> + * paragraph) shall be included in all copies or substantial >> portions of the >> + * Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, >> EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF >> MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO >> EVENT SHALL >> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES >> OR OTHER >> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> ARISING >> + * FROM, OUT OF OR IN CONN
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Am Sonntag, den 29.04.2018, 10:43 +0200 schrieb Benedikt Schemmer: > Hi Gert, > > couldn't resist at least to try what would happen if I enable > register merge for radeonsi: > > PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR > SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits > piglit 80732 -0.16 % -0.02 > % . .0.87 %0.86 %0.04 % . . > -- > > All affected 513 -17.58 % -2.30 > % . .4.12 %5.87 %1.73 %0.10 % . > -- > > Total 80732 -0.16 % -0.02 > % . .0.87 %0.86 %0.04 % . . > > I had already removed the defines around the debug code so thats also > happily outputting data. > > fails with two piglit shaders: Which are the names of these test? I'd like to check this on r600, because here I didn't see any regressions last time I checked. > Real world is a little different: I guess these tests refer to enabled register_merge - without and with this patch set, no? Out of curiosity, did you also look at how enabling register_merge (before this series) impacts the result as compared to the normal operation of radeonsi? > If theres an easy way to figure out when your code makes it worse and > when its an improvement this would be really nice. My insentive for this series was, that on r600 the arrays are allocated before the final optimization pass on the byte code that requires that the number of registers is <= 124. When I started this no spilling was implemented, and shaders with too many arrays and registers would simply fail. Now spilling is impelmented, but AFAIK reducing the numbers of registers in the final optimization pass does not result in changed spilling, so bringing down the number of registers before tgsi- to-bytecode is still of interest. For radeonsi my guess would be that the llvm optimizer works better when the registers are not yet merged, and that would be the reason why register_merge is disabled. In any case, Timothy wrote in this thread [1] (last message) that he had similar patches for NIR. Best, Gert [1] https://patchwork.freedesktop.org/patch/189842/ ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/linker: Add nir_link_uniforms()
On 29/04/18 19:05, Alejandro Piñeiro wrote: On 29/04/18 10:13, Timothy Arceri wrote: On 29/04/18 16:48, Alejandro Piñeiro wrote: From: Eduardo Lima Mitev This function will be the entry point for linking the uniforms from the nir_shader objects associated with the gl_linked_shaders of a program. This patch includes initial support for linking uniforms from NIR shaders. It is tailored for the ARB_gl_spirv needs, and it is far from complete, but it should handle most cases of uniforms, array uniforms, structs, samplers and images. There are some FIXMEs related to specific features that will be implemented in following patches, like atomic counters, UBOs and SSBOs. Also, note that ARB_gl_spirv makes mandatory explicit location for normal uniforms, so this code only handles uniforms with explicit location. But there are cases, like uniform atomic counters, that doesn't have a location from the OpenGL point of view (they have a binding), but that Mesa assign internally a location. That will be handled on following patches. A nir_linker.h file is also added. More NIR-linking related API will be added in subsequent patches and those will include stuff from Mesa, so reusing nir.h didn't seem a good idea. These files should actually be src/compiler/glsl/nir_link_uniforms.c etc these are not general purpose nir helpers they are GLSL specific. Yes, it is true that are not general purpose nir helpers, but they are not GLSL specific either. As mentioned on the commit message and on the introductory comment, it is heavily tailored for ARB_gl_spirv, so they are SPIR-V specific. But why? Why not try to make it reusable as a linker for GLSL? What exactly is tailored for ARB_gl_spirv? And does this really block us reusing it for GLSL? I've expressed my opinion on this long ago, we are very close to being able to implement a NIR linker for GLSL, spending a little effort designing this linker code as share-able seems like a no brainer to me. Ignoring the above issue I'm fairly certain adding a dependency on core mesa GL to NIR is a no no so either way these files don't belong in src/compiler/nir In fact, they fit better at src/compiler/spirv than on src/compiler/glsl. But as that directory is even more general purpose, so we placed them at src/compiler/nir as it using the nir-shader to do the linkage. The alternative would be place them on a new directory, something like src/compiler/gl_spirv, but that seems somewhat an overkill. v2: update var->data.location with UniformStorage index when the storage was set up on a previous stage (Neil) Signed-off-by: Eduardo Lima Signed-off-by: Neil Roberts --- src/compiler/Makefile.sources | 2 + src/compiler/nir/meson.build | 2 + src/compiler/nir/nir_link_uniforms.c | 461 +++ src/compiler/nir/nir_linker.h | 41 4 files changed, 506 insertions(+) create mode 100644 src/compiler/nir/nir_link_uniforms.c create mode 100644 src/compiler/nir/nir_linker.h diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources index bb86972ea1a..dba70a5e5a1 100644 --- a/src/compiler/Makefile.sources +++ b/src/compiler/Makefile.sources @@ -206,6 +206,8 @@ NIR_FILES = \ nir/nir_inline_functions.c \ nir/nir_instr_set.c \ nir/nir_instr_set.h \ + nir/nir_link_uniforms.c \ + nir/nir_linker.h \ nir/nir_linking_helpers.c \ nir/nir_liveness.c \ nir/nir_loop_analyze.c \ diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build index b28a565d0c6..baf5682d388 100644 --- a/src/compiler/nir/meson.build +++ b/src/compiler/nir/meson.build @@ -99,6 +99,8 @@ files_libnir = files( 'nir_inline_functions.c', 'nir_instr_set.c', 'nir_instr_set.h', + 'nir_link_uniforms.c', + 'nir_linker.h', 'nir_linking_helpers.c', 'nir_liveness.c', 'nir_loop_analyze.c', diff --git a/src/compiler/nir/nir_link_uniforms.c b/src/compiler/nir/nir_link_uniforms.c new file mode 100644 index 000..450a5ce2e21 --- /dev/null +++ b/src/compiler/nir/nir_link_uniforms.c @@ -0,0 +1,461 @@ +/* + * Copyright © 2018 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNE
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Am 29.04.2018 um 11:34 schrieb Gert Wollny: > Am Sonntag, den 29.04.2018, 10:43 +0200 schrieb Benedikt Schemmer: >> Hi Gert, >> >> couldn't resist at least to try what would happen if I enable >> register merge for radeonsi: >> >> PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR >> SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits >> piglit 80732 -0.16 % -0.02 >> % . .0.87 %0.86 %0.04 % . . >> -- >> >> All affected 513 -17.58 % -2.30 >> % . .4.12 %5.87 %1.73 %0.10 % . >> -- >> >> Total 80732 -0.16 % -0.02 >> % . .0.87 %0.86 %0.04 % . . >> >> I had already removed the defines around the debug code so thats also >> happily outputting data. >> >> fails with two piglit shaders: > Which are the names of these test? I'd like to check this on r600, > because here I didn't see any regressions last time I checked. > might of course be different on r600 (is bindless available?), also shader-db is more sensitive to problems than piglit 1. tests/spec/arb_bindless_texture/compiler/images/arrays-of-struct.frag 2. tests/spec/arb_bindless_texture/compiler/samplers/arrays-of-struct.frag > >> Real world is a little different: > > I guess these tests refer to enabled register_merge - without and with > this patch set, no? > > Out of curiosity, did you also look at how enabling register_merge > (before this series) impacts the result as compared to the normal > operation of radeonsi? no I didn't but if I do (old vs new) nothing vs register merge: PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits piglit 80732 . . . . . .0.06 % . . -- All affected 4350.89 %0.36 % . . . .3.61 % -0.06 % . -- Total 80732 . . . . . .0.06 % . . register merge vs yours: PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits piglit 80732 -0.16 % -0.02 % . .0.87 % 0.86 % -0.02 % . . -- All affected 83 -56.92 % -14.22 % . . 11.67 % 16.67 % -2.86 %0.93 % . -- Total 80732 -0.16 % -0.02 % . .0.87 % 0.86 % -0.02 % . . nothing vs yours: PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits piglit 80732 -0.16 % -0.02 % . .0.87 % 0.86 %0.04 % . . -- All affected 513 -17.58 % -2.30 % . .4.12 % 5.87 %1.73 %0.10 % . -- Total 80732 -0.16 % -0.02 % . .0.87 % 0.86 %0.04 % . . > > >> If theres an easy way to figure out when your code makes it worse and >> when its an improvement this would be really nice. > > My insentive for this series was, that on r600 the arrays are allocated > before the final optimization pass on the byte code that requires that > the number of registers is <= 124. When I started this no spilling was > implemented, and shaders with too many arrays and registers would > simply fail. Now spilling is impelmented, but AFAIK reducing the > numbers of registers in the final optimization pass does not result in > changed spilling, so bringing down the number of registers before tgsi- > to-bytecode is still of interest. > > For radeonsi my guess would be that the llvm optimizer works better > when the registers are not yet merged, and that would be the reason why > register_merge is disabled. well at least sometimes it doesn't, low hanging fruit m
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Real world (old vs new) nothing vs register merge: PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits 0ad6 . . . . . . . . . aer 590 .0.26 % -20.00 % . . .0.34 % . . alien_isolation 1414 . . . . . . . . . anholt10 . . . . . . . . . bioshock_infinite 2581 -0.02 % . . . . .0.12 % . . blackmesa584 . . . . . . . . . cat 573 . -0.02 % . . . .0.11 % . . csgo1392 . . -0.88 % . . . -0.03 % . . deadisland_definitive 17760.06 % . . . . .0.15 %0.01 % . deadisland_original11602 . . . . . .0.05 % . . deadisland_riptide_..293 -0.06 %0.06 % . . . .0.32 % . . deusex_mankind 50510.14 % -0.01 % -1.57 % . . .0.18 % . . dirtrally787 -0.04 %0.15 %1.08 % . . .0.27 % -0.07 % . dolphin 22 . . . . . . . . . dyinglight 4012 .0.05 % . . . .0.34 % -0.01 % . eurotruck2 216 . . . . . . . . . f1_2015 746 -0.04 %0.02 %0.93 % . . .0.08 % . . glamor16 -2.33 % . . . . .3.97 % . . hl2ep1 294 . . . . . . . . . hl2ep2 154 . . . . . . . . . hl2lostcoast 66 . . . . . . . . . hlsl3582 . . . . . . -0.14 % . . humus-celshading 4 . . . . . . . . . humus-domino 6 . . . . . . . . . humus-dynamicbranching24 . . . . . . . . . humus-hdr 10 . . . . . . . . . humus-portals 2 . . . . . . . . . humus-volumetricfog.. 6 . . . . . . . . . kerbal 1016 .0.11 % . . . .0.31 % . . larago 664 . . . . . .0.01 % . . madmax 354 . -0.08 % . . . . -0.02 %0.04 % . metro2033redux 4410 .0.03 % . . . .0.06 % -0.01 % . nexuiz80 . . . . . . . . . ruiner 685 . -0.02 % . . . .0.04 % . . sauerbraten7 . . . . . . . . . serioussam2017 736 . .7.59 % . . .0.05 % . . soma 436 . . . . . . . . . specops 1814 . . . . . .0.35 % . . stellaris434 . . . . . .0.11 % . . supertuxkart 4 . . . . . . . . . talos762 -0.02 % .0.09 % . . .0.01 % . . tesseract430
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
sorry the last one wasnt correct: register merge vs yours Max Increase: SGPRS: 80 -> 96 (20.00 %) (in shaders/deusex_mankind/7c39f71090a9db19ac2e1542ea12804ae6c6495b_4864.shader_test) VGPRS: 64 -> 84 (31.25 %) (in shaders/dirtrally/0859b69789591d7046e211400b1edd9a7cfca734_742.shader_test) Spilled SGPRs: 0 -> 16 (0.00 %) (in shaders/deusex_mankind/d64e2084204e29749639e8fbd9a1e507c7e5e1dd_6840.shader_test) Spilled VGPRs: 0 -> 0 (0.00 %) Private memory VGPRs: 24 -> 32 (33.33 %) (in shaders/deusex_mankind/28cac87049d8c833e72296a5a02ea6118f1144e5_5876.shader_test) Scratch size: 28 -> 36 (28.57 %) dwords per thread (in shaders/deusex_mankind/28cac87049d8c833e72296a5a02ea6118f1144e5_5876.shader_test) Code Size: 6988 -> 8036 (15.00 %) bytes (in shaders/cat/1847.shader_test) LDS: 0 -> 0 (0.00 %) blocks Max Waves: 5 -> 7 (40.00 %) (in shaders/ruiner/0967c5fce7fc456496b1cfa25fbb1d1c4dcf9bed_2958.shader_test) Wait states: 0 -> 0 (0.00 %) Max Decrease: SGPRS: 96 -> 72 (-25.00 %) (in shaders/deusex_mankind/c1f098c7b14b1ba291cfa9bba4e41ba91acaba30_3630.shader_test) VGPRS: 80 -> 68 (-15.00 %) (in shaders/dirtrally/710d3319bc986ea003f1a84ec6d3c01b2a8b9ded_2482.shader_test) Spilled SGPRs: 19 -> 0 (-100.00 %) (in shaders/deusex_mankind/0749c9ae23417f918c7286fe502ff5de4cb8e1a0_3276.shader_test) Spilled VGPRs: 0 -> 0 (0.00 %) Private memory VGPRs: 0 -> 0 (0.00 %) Scratch size: 0 -> 0 (0.00 %) dwords per thread Code Size: 9080 -> 8712 (-4.05 %) bytes (in shaders/dirtrally/2e1a8e04b49dee3e39b6f30a7c707f395abf_2480.shader_test) LDS: 0 -> 0 (0.00 %) blocks Max Waves: 8 -> 5 (-37.50 %) (in shaders/deusex_mankind/8dabec49e5b6c3b1cbcbaee194eff69f164d72f4_3968.shader_test) Wait states: 0 -> 0 (0.00 %) PERCENTAGE DELTASShaders SGPRs VGPRs SpillSGPR SpillVGPR PrivVGPR Scratch CodeSize MaxWaves Waits 0ad6 . . . . . . . . . aer 590 . . . . . . . . . alien_isolation 1414 . . . . . . . . . anholt10 . . . . . . . . . bioshock_infinite 2581 .0.04 % . . . .0.01 % . . blackmesa584 . . . . . . . . . cat 573 -0.06 % -0.09 % . . . .0.08 %0.05 % . csgo1392 . . . . . . . . . deadisland_definitive 1776 . . . . . . . . . deadisland_original11602 . . . . . . . . . deadisland_riptide_..293 . . . . . . . . . deusex_mankind 5051 -0.06 % . -4.64 % . 33.33 % 28.57 % . -0.01 % . dirtrally7870.04 %0.49 % -0.46 % . . .0.03 % -0.23 % . dolphin 22 . . . . . . . . . dyinglight 4012 . . . . . . . . . eurotruck2 216 . . . . . . . . . f1_2015 746 . -0.03 %1.77 % . . .0.05 % . . glamor16 . . . . . . . . . hl2ep1 294 . . . . . . . . . hl2ep2 154 . . . . . . . . . hl2lostcoast 66 . . . . . . . . . hlsl3582 . . . . . . . . . humus-celshading 4 . . . . . . . . . humus-domino 6 . . . . . . . . . humus-dynamicbranching24 . . . . . . . . . humus-hdr 10 . . . . . . . . . humus-portals 2 . . . . . . . .
[Mesa-dev] [Bug 106180] [bisected] radv vulkan smoke test black screen (Add support for DRI3 v1.2)
https://bugs.freedesktop.org/show_bug.cgi?id=106180 --- Comment #5 from mercuriete --- Hi I tried that patch on top of mesa-18.1.0_rc2 it fixes some issues with resizing windows and with alt+tab in csgo+opengl But it does not fix this problem. * vulkan smoketest on intel haswell is OK (now doesnt crash when resizing window) * vulkan smoketest on radv is a BLACK window and crashes when maximize window. terminate called after throwing an instance of 'std::runtime_error' what(): VkResult -101004 returned * dota2 on radv only shows the cursor and cpu usage is 100% on cpu0 but no crash. * dota2 on anv haswell same behaviour (show cursor and 100% usage on cpu0) Perf information with radv+PRIME with dota2 # To display the perf.data header info, please use --header/--header-only options. # # # Total Lost Samples: 0 # # Samples: 29K of event 'cycles:uppp' # Event count (approx.): 5618911073 # # Overhead Command Shared Object Symbol # ... ... # 93.64% VKRenderThread [kernel.vmlinux] [.] syscall_return_via_sysret 2.00% VKRenderThread libtier0.so [.] CThreadFastMutex::Lock 1.03% VKRenderThread libpthread-2.25.so[.] pthread_yield 0.58% VKRenderThread libc-2.25.so [.] __sched_yield 0.47% VKRenderThread libtier0.so [.] ThreadSleep@plt 0.26% VKRenderThread [kernel.vmlinux] [.] native_irq_return_iret 0.25% VKRenderThread [unknown] [k] 0x7f2d7dd06495 0.24% VKRenderThread libtier0.so [.] pthread_yield@plt 0.23% SDLAudioP2 libm-2.25.so [.] __powf_finite 0.17% SDLAudioP2 [kernel.vmlinux] [.] syscall_return_via_sysret 0.15% AsyncScheduledF [kernel.vmlinux] [.] syscall_return_via_sysret 0.10% VKRenderThread libpthread-2.25.so[.] sched_yield@plt 0.09% dota2[kernel.vmlinux] [.] syscall_return_via_sysret 0.08% AsyncScheduledF libm-2.25.so [.] __powf_finite 0.05% VKRenderThread libtier0.so [.] ThreadSleep 0.04% SDLAudioP2 libm-2.25.so [.] powf 0.02% SDLAudioP2 libm-2.25.so [.] log10f 0.02% AsyncScheduledF libm-2.25.so [.] powf 0.01% SDLAudioP2 [kernel.vmlinux] [.] native_irq_return_iret 0.01% AsyncScheduledF libtier0.so [.] Plat_FloatTime 0.01% SDLAudioP2 libsoundsystem.so [.] 0x00169aa4 0.01% AsyncScheduledF libpthread-2.25.so[.] pthread_cond_timedwait@@GLIBC_2.3.2 0.01% SDLAudioP2 libm-2.25.so [.] 0x6d91 0.01% AsyncScheduledF libm-2.25.so [.] log10f 0.01% SDLAudioP2 libm-2.25.so [.] 0x81f9 0.01% SDLAudioP2 libc-2.25.so [.] 0x0013e29f 0.01% SDLAudioP2 libc-2.25.so [.] 0x0013e3c0 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 106283] Shader replacements works only for limited use cases
https://bugs.freedesktop.org/show_bug.cgi?id=106283 --- Comment #6 from Mark Janes --- FrameRetrace supports gl shader replacement. I'll enhance it to support glProgramString. It will be easier to debug shaders with FrameRetrace than using the environment variables. https://fosdem.org/2018/schedule/event/apitrace/ -- You are receiving this mail because: You are the assignee for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Hello Benedict, thanks for all the testing! On 29.04.2018 12:12, Benedikt Schemmer wrote: >> Which are the names of these test? I'd like to check this on r600, >> because here I didn't see any regressions last time I checked. >> > might of course be different on r600 (is bindless available?), > also shader-db is more sensitive to problems than piglit > > 1. tests/spec/arb_bindless_texture/compiler/images/arrays-of-struct.frag > 2. tests/spec/arb_bindless_texture/compiler/samplers/arrays-of-struct.frag Indeed, bindless testures are not available on r600, so it is quite difficult for me to test this. I would guess that parameters related to this might be stored in the TGSI declaration that I currently don't check. If you have time for it, could you send me a TGSI dump of one of these shaders? With "ST_DEBUG=tgsi" this should be possible. > >> For radeonsi my guess would be that the llvm optimizer works better >> when the registers are not yet merged, and that would be the reason why >> register_merge is disabled. > well at least sometimes it doesn't, low hanging fruit maybe? Unfortunately, I can't test on radeonsi Best, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 106315] The witness + dxvk suffers flickering garbage
https://bugs.freedesktop.org/show_bug.cgi?id=106315 Bug ID: 106315 Summary: The witness + dxvk suffers flickering garbage Product: Mesa Version: git Hardware: Other OS: Linux (All) Status: NEW Severity: normal Priority: medium Component: Drivers/Vulkan/radeon Assignee: mesa-dev@lists.freedesktop.org Reporter: nota...@gmail.com QA Contact: mesa-dev@lists.freedesktop.org The witness + wine + dxvk suffers from flickering garbage. It's most visible with MSAA off, when it's on the problem is only visible in water reflections. The problem doesn't show up under Windows + dxvk, which suggests it's a driver issue (probably LLVM?) Renderdoc capture with MSAA off: https://drive.google.com/file/d/1MZ3GaxQaKgdJ-FZHRj7hnpmimv9bKodU/view?usp=sharing Recorded using renderdoc nightly (2018_04_28) because dxvk requires VK_KHR_descriptor_update_template now. GPU: POLARIS10 (RX 470) LLVM: 7.0.0 r330470 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
Am 29.04.2018 um 21:44 schrieb Gert Wollny: > Hello Benedict, > > thanks for all the testing! thanks for all the developing ;) > > On 29.04.2018 12:12, Benedikt Schemmer wrote: >>> Which are the names of these test? I'd like to check this on r600, >>> because here I didn't see any regressions last time I checked. >>> >> might of course be different on r600 (is bindless available?), >> also shader-db is more sensitive to problems than piglit >> >> 1. tests/spec/arb_bindless_texture/compiler/images/arrays-of-struct.frag >> 2. tests/spec/arb_bindless_texture/compiler/samplers/arrays-of-struct.frag > Indeed, bindless testures are not available on r600, so it is quite > difficult for me to test this. I would guess that parameters related to > this might be stored in the TGSI declaration that I currently don't check. > > If you have time for it, could you send me a TGSI dump of one of these > shaders? > With "ST_DEBUG=tgsi" this should be possible. I created 'R600_DEBUG=merge' so I can switch without having to recompile. $ST_DEBUG=tgsi ./run wollny/ ATTENTION: default value of option allow_glsl_extension_directive_midshader overridden by environment. FRAG DCL TEMP[0..55], ARRAY(1), LOCAL IMM[0] INT32 {0, 0, 0, 0} IMM[1] FLT32 {1., 2., 3., 4.} 0: STORE TEMP[0], IMM[0]., IMM[1], 2D 1: END wollny/41d88325fd2a57cb6af40de02dc281ee0683cc40_2.shader_test - LLVM diagnostic (remark): :0:0: 12 instructions in function wollny/41d88325fd2a57cb6af40de02dc281ee0683cc40_2.shader_test - Shader Stats: SGPRS: 16 VGPRS: 16 Code Size: 80 LDS: 0 Scratch: 0 Max Waves: 8 Spilled SGPRs: 0 Spilled VGPRs: 0 PrivMem VGPRs: 0 FRAG DCL OUT[0], COLOR DCL TEMP[0..1], LOCAL DCL TEMP[2..57], ARRAY(1), LOCAL IMM[0] FLT32 {0., 0., 0., 0.} 0: MOV TEMP[0].xy, IMM[0]. 1: TEX TEMP[1], TEMP[0], TEMP[2].xyxy, 2D 2: MOV OUT[0], TEMP[1] 3: END wollny/a115868a349cd666b842a0e70f47451b4463903a_2.shader_test - LLVM diagnostic (remark): :0:0: 11 instructions in function wollny/a115868a349cd666b842a0e70f47451b4463903a_2.shader_test - Shader Stats: SGPRS: 24 VGPRS: 16 Code Size: 72 LDS: 0 Scratch: 0 Max Waves: 8 Spilled SGPRs: 0 Spilled VGPRs: 0 PrivMem VGPRs: 0 Thread 0 took 0.13 seconds and compiled 2 shaders (not including SIMD16) with 1 GL context switches $R600_DEBUG=merge ST_DEBUG=tgsi ./run wollny/ ATTENTION: default value of option allow_glsl_extension_directive_midshader overridden by environment. run: state_tracker/st_glsl_to_tgsi.cpp:5783: ureg_dst dst_register(st_translate*, gl_register_file, unsigned int, unsigned int): Assertion `array_id && array_id <= t->num_temp_arrays' failed. => CRASHED <= while processing these shaders: wollny/41d88325fd2a57cb6af40de02dc281ee0683cc40_2.shader_test >> >>> For radeonsi my guess would be that the llvm optimizer works better >>> when the registers are not yet merged, and that would be the reason why >>> register_merge is disabled. >> well at least sometimes it doesn't, low hanging fruit maybe? > Unfortunately, I can't test on radeonsi I can, if you dont mind waiting for an answer sometimes. But maybe even easier: is there an implicit/explicit magic number I can play with to see if it changes anything? ATM it seems like your code improves half the shaders its affecting a lot and hurting the other half bad like it hits an invisible wall. I think one problem could be the relationship between VGPRs and SGPRs used and max Wavefronts achieved. This is somewhat similar to NIR although that changes things all over the place. > > Best, > Gert > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 106246] radv: VK_PIPELINE_CREATE_DISABLE_OPTIMIZATION_BIT and bringing down initial pipeline compile times
https://bugs.freedesktop.org/show_bug.cgi?id=106246 --- Comment #6 from Timothy Arceri --- (In reply to Timothy Arceri from comment #4) > (In reply to Nicolai Hähnle from comment #3) > > As long as scratch buffer support is robust, removing LLVM IR optimization > > passes is probably not a problem, though you really do want mem2reg and I > > don't think we spend much time in the others (at least radeonsi didn't, last > > time I checked). > > > > Using the -O0 settings for the codegen backend is a lot riskier. Our compute > > folks have done some work fixing bugs there, but I really wouldn't recommend > > it. > > Yeah I've done some experimenting with the Blacksmith demo. I'm not sure we > can get much benefit implementing > VK_PIPELINE_CREATE_DISABLE_OPTIMIZATION_BIT with the current state of > things. > > Default: > Sum of shader compile times: 325933 ms > > With only LLVM DCE opt (compilation fails without this): > Sum of shader compile times: 326451 ms > > No NIR linking plus single pass over NIR opts (compilation fails without > this): > Sum of shader compile times: 294788 ms I've done some playing around with the LLVM cogegen opt levels: LLVMCodeGenLevelNone + LLVMAddEarlyCSEMemSSAPass (compilation fails without this): Sum of shader compile times: 211403 ms However there are all sorts of rendering issues when running the demo. No NIR linking plus single pass over NIR opts (compilation fails without this), LLVMCodeGenLevelNone + LLVMAddEarlyCSEMemSSAPass(compilation fails without this): Sum of shader compile times: 179775 ms With this the demo doesn't actually display the graphics it just shows a flickering Unity logo throughout the run. -- You are receiving this mail because: You are the QA Contact for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] ac/nir: expand 64-bit vec3 loads to fix shuffling.
From: Dave Airlie If loading 64-bit vec3 values, a 4 component load would be followed by a 2 component load and the resulting shuffle would fail as it requires 2 4 components. This just expands the second results vector out to 4 components. This fixes 100 CTS tests: dEQP-VK.spirv_assembly.type.vec3.*64* --- src/amd/common/ac_nir_to_llvm.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/amd/common/ac_nir_to_llvm.c b/src/amd/common/ac_nir_to_llvm.c index e4ae6ef49ad..b77d62a39b0 100644 --- a/src/amd/common/ac_nir_to_llvm.c +++ b/src/amd/common/ac_nir_to_llvm.c @@ -1572,6 +1572,11 @@ static LLVMValueRef visit_load_buffer(struct ac_nir_context *ctx, LLVMConstInt(ctx->ac.i32, 6, false), LLVMConstInt(ctx->ac.i32, 7, false) }; + if (num_components == 6) { + /* we end up with a v4f32 and v2f32 but shuffle fails on that */ + results[1] = ac_build_expand_to_vec4(&ctx->ac, results[1], 4); + } + LLVMValueRef swizzle = LLVMConstVector(masks, num_components); ret = LLVMBuildShuffleVector(ctx->ac.builder, results[0], results[num_components > 4 ? 1 : 0], swizzle, ""); -- 2.14.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 106246] radv: VK_PIPELINE_CREATE_DISABLE_OPTIMIZATION_BIT and bringing down initial pipeline compile times
https://bugs.freedesktop.org/show_bug.cgi?id=106246 --- Comment #7 from Timothy Arceri --- (In reply to Timothy Arceri from comment #6) > (In reply to Timothy Arceri from comment #4) > > (In reply to Nicolai Hähnle from comment #3) > > > As long as scratch buffer support is robust, removing LLVM IR optimization > > > passes is probably not a problem, though you really do want mem2reg and I > > > don't think we spend much time in the others (at least radeonsi didn't, > > > last > > > time I checked). > > > > > > Using the -O0 settings for the codegen backend is a lot riskier. Our > > > compute > > > folks have done some work fixing bugs there, but I really wouldn't > > > recommend > > > it. > > > > Yeah I've done some experimenting with the Blacksmith demo. I'm not sure we > > can get much benefit implementing > > VK_PIPELINE_CREATE_DISABLE_OPTIMIZATION_BIT with the current state of > > things. > > > > Default: > > Sum of shader compile times: 325933 ms > > > > With only LLVM DCE opt (compilation fails without this): > > Sum of shader compile times: 326451 ms > > > > No NIR linking plus single pass over NIR opts (compilation fails without > > this): > > Sum of shader compile times: 294788 ms > > I've done some playing around with the LLVM cogegen opt levels: > > LLVMCodeGenLevelNone + LLVMAddEarlyCSEMemSSAPass (compilation fails without > this): > Sum of shader compile times: 211403 ms > However there are all sorts of rendering issues when running the demo. > > No NIR linking plus single pass over NIR opts (compilation fails without > this), > LLVMCodeGenLevelNone + LLVMAddEarlyCSEMemSSAPass(compilation fails without > this): > Sum of shader compile times: 179775 ms > With this the demo doesn't actually display the graphics it just shows a > flickering Unity logo throughout the run. Ok so it seems this speed up (and the display issues that go with it) and due to switching from the GreedyRegisterAllocator to the FastRegisterAllocator. -- You are receiving this mail because: You are the QA Contact for the bug.___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 00/13] TGSI: improved live range tracking, also including arrays
[...] > > $R600_DEBUG=merge ST_DEBUG=tgsi ./run wollny/ > > ATTENTION: default value of option > allow_glsl_extension_directive_midshader overridden by environment. > run: state_tracker/st_glsl_to_tgsi.cpp:5783: ureg_dst > dst_register(st_translate*, gl_register_file, unsigned int, unsigned > int): Assertion `array_id && array_id <= t->num_temp_arrays' failed. This would indicate that the array is split, but still addressed somewhere, i.e. I don't look at all possible locations where arrays are used. I think I debug this by letting the R600 driver advertise bindless texture support (obvioulsy it will not pass the tests, because it is not implemented and the hardware may not even support it, but the GLSL->TGSI conversion should still work. > > > I can, if you dont mind waiting for an answer sometimes. > I will rely on this, but I also will take some time do work on it. > But maybe even easier: is there an implicit/explicit magic number I > can play with to see if it changes anything? Unfortunately, in my code there are no specific magic numbers to change, if there is anything then it is probably in the LLVM backend (that is not used with r600). The only think you could try is to disable one of the two parts in my code: one is the split_array(), called around st_glsl_to_tgsi:7018 and the other is where the array merging and interleaving are applied, i.e. line st_glsl_to_tgsi:5537 My take is that this latter step is responsible for the regressions, especially the array interleaving could make it more difficult to optimize, because if the optimizer pass doesn't look at a per-component access, but on the higher level per-register base, then register live- ranges for the array elements will become longer, making it more difficult to schedule everything without spilling. > ATM it seems like your code improves half the shaders its affecting a > lot and hurting the other half bad like it hits an invisible wall. > I think one problem could be the relationship between VGPRs and SGPRs > used and max Wavefronts achieved. Unfortunately, on r600 such fine grained information is not available, so that I also have no real clue. Cheer, Gert ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/3] vulkan/wsi: VK_EXT_hdr_metadata plumbing
From: Ville Syrjälä Add the common wsi infrastructure for VK_EXT_hdr_metadata. Signed-off-by: Ville Syrjälä Signed-off-by: Tapani Pälli --- src/vulkan/wsi/wsi_common.c | 12 src/vulkan/wsi/wsi_common.h | 5 + src/vulkan/wsi/wsi_common_private.h | 2 ++ 3 files changed, 19 insertions(+) diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c index fe262b4968..676346f0a2 100644 --- a/src/vulkan/wsi/wsi_common.c +++ b/src/vulkan/wsi/wsi_common.c @@ -773,6 +773,18 @@ wsi_common_destroy_swapchain(VkDevice device, swapchain->destroy(swapchain, pAllocator); } +void +wsi_common_set_hdr_metadata(uint32_t swapchainCount, +const VkSwapchainKHR *pSwapchain, +const VkHdrMetadataEXT *pMetadata) +{ + for (uint32_t i = 0; i < swapchainCount; i++) { + WSI_FROM_HANDLE(wsi_swapchain, swapchain, pSwapchain[i]); + if (swapchain->set_hdr_metadata) + swapchain->set_hdr_metadata(swapchain, &pMetadata[i]); + } +} + VkResult wsi_common_get_images(VkSwapchainKHR _swapchain, uint32_t *pSwapchainImageCount, diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h index 6cf729ba02..b41a633e02 100644 --- a/src/vulkan/wsi/wsi_common.h +++ b/src/vulkan/wsi/wsi_common.h @@ -202,6 +202,11 @@ wsi_common_destroy_swapchain(VkDevice device, VkSwapchainKHR swapchain, const VkAllocationCallbacks *pAllocator); +void +wsi_common_set_hdr_metadata(uint32_t swapchainCount, +const VkSwapchainKHR* pSwapchains, +const VkHdrMetadataEXT* pMetadata); + VkResult wsi_common_queue_present(const struct wsi_device *wsi, VkDevice device_h, diff --git a/src/vulkan/wsi/wsi_common_private.h b/src/vulkan/wsi/wsi_common_private.h index b608119b96..c0210f5588 100644 --- a/src/vulkan/wsi/wsi_common_private.h +++ b/src/vulkan/wsi/wsi_common_private.h @@ -67,6 +67,8 @@ struct wsi_swapchain { VkResult (*queue_present)(struct wsi_swapchain *swap_chain, uint32_t image_index, const VkPresentRegionKHR *damage); + void (*set_hdr_metadata)(struct wsi_swapchain *swapchain, +const VkHdrMetadataEXT *metadata); }; VkResult -- 2.13.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/3] common bits from HDR POC
Hi; I took Ville's HDR POC Mesa patches, fixed some issues and removed all Wayland specific parts. Here are the bits that could be already integrated, rest will be window system specific parts. // Tapani Tapani Pälli (1): egl: implement EXT_surface_SMPTE2086_metadata and EXT_surface_CTA861_3_metadata Ville Syrjälä (2): vulkan/wsi: VK_EXT_hdr_metadata plumbing anv: Plug in VK_EXT_hdr_metadata src/egl/drivers/dri2/egl_dri2.c | 3 + src/egl/main/eglapi.c | 2 + src/egl/main/egldisplay.h | 2 + src/egl/main/eglsurface.c | 169 src/egl/main/eglsurface.h | 19 src/intel/vulkan/anv_extensions.py | 1 + src/intel/vulkan/anv_wsi.c | 9 ++ src/vulkan/wsi/wsi_common.c | 12 +++ src/vulkan/wsi/wsi_common.h | 5 ++ src/vulkan/wsi/wsi_common_private.h | 2 + 10 files changed, 224 insertions(+) -- 2.13.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] egl: implement EXT_surface_SMPTE2086_metadata and EXT_surface_CTA861_3_metadata
Patch implements common bits for EXT_surface_SMPTE2086_metadata and EXT_surface_CTA861_3_metadata extensions by adding new required attributes and eglQuerySurface + eglSurfaceAttrib changes. Currently none of the drivers are utilizing this data but this patch is enabler in getting there. Following dEQP cases pass with these changes: dEQP-EGL.functional.hdr_metadata.smpte2086 dEQP-EGL.functional.hdr_metadata.cta861_3 Signed-off-by: Ville Syrjälä Signed-off-by: Tapani Pälli --- src/egl/drivers/dri2/egl_dri2.c | 3 + src/egl/main/eglapi.c | 2 + src/egl/main/egldisplay.h | 2 + src/egl/main/eglsurface.c | 169 src/egl/main/eglsurface.h | 19 + 5 files changed, 195 insertions(+) diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c index 45d0c7275c..fcfd3852d9 100644 --- a/src/egl/drivers/dri2/egl_dri2.c +++ b/src/egl/drivers/dri2/egl_dri2.c @@ -712,6 +712,9 @@ dri2_setup_screen(_EGLDisplay *disp) disp->Extensions.EXT_create_context_robustness = EGL_TRUE; } + disp->Extensions.EXT_surface_SMPTE2086_metadata = EGL_TRUE; + disp->Extensions.EXT_surface_CTA861_3_metadata = EGL_TRUE; + if (dri2_dpy->no_error) disp->Extensions.KHR_create_context_no_error = EGL_TRUE; diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c index c110349119..493fd6d1b9 100644 --- a/src/egl/main/eglapi.c +++ b/src/egl/main/eglapi.c @@ -488,6 +488,8 @@ _eglCreateExtensionsString(_EGLDisplay *dpy) _EGL_CHECK_EXTENSION(EXT_create_context_robustness); _EGL_CHECK_EXTENSION(EXT_image_dma_buf_import); _EGL_CHECK_EXTENSION(EXT_image_dma_buf_import_modifiers); + _EGL_CHECK_EXTENSION(EXT_surface_CTA861_3_metadata); + _EGL_CHECK_EXTENSION(EXT_surface_SMPTE2086_metadata); _EGL_CHECK_EXTENSION(EXT_swap_buffers_with_damage); _EGL_CHECK_EXTENSION(IMG_context_priority); diff --git a/src/egl/main/egldisplay.h b/src/egl/main/egldisplay.h index d7e51991a4..6dc77ebe3d 100644 --- a/src/egl/main/egldisplay.h +++ b/src/egl/main/egldisplay.h @@ -105,6 +105,8 @@ struct _egl_extensions EGLBoolean EXT_image_dma_buf_import; EGLBoolean EXT_image_dma_buf_import_modifiers; EGLBoolean EXT_pixel_format_float; + EGLBoolean EXT_surface_CTA861_3_metadata; + EGLBoolean EXT_surface_SMPTE2086_metadata; EGLBoolean EXT_swap_buffers_with_damage; unsigned int IMG_context_priority; diff --git a/src/egl/main/eglsurface.c b/src/egl/main/eglsurface.c index 3bd14a8cd0..1bf4723243 100644 --- a/src/egl/main/eglsurface.c +++ b/src/egl/main/eglsurface.c @@ -86,6 +86,90 @@ _eglParseSurfaceAttribList(_EGLSurface *surf, const EGLint *attrib_list) break; surf->GLColorspace = val; break; + case EGL_SMPTE2086_DISPLAY_PRIMARY_RX_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.display_primary_r.x = val; + break; + case EGL_SMPTE2086_DISPLAY_PRIMARY_RY_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.display_primary_r.y = val; + break; + case EGL_SMPTE2086_DISPLAY_PRIMARY_GX_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.display_primary_g.x = val; + break; + case EGL_SMPTE2086_DISPLAY_PRIMARY_GY_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.display_primary_g.y = val; + break; + case EGL_SMPTE2086_DISPLAY_PRIMARY_BX_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.display_primary_b.x = val; + break; + case EGL_SMPTE2086_DISPLAY_PRIMARY_BY_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.display_primary_b.y = val; + break; + case EGL_SMPTE2086_WHITE_POINT_X_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.white_point.x = val; + break; + case EGL_SMPTE2086_WHITE_POINT_Y_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + } + surf->HdrMetadata.white_point.y = val; + break; + case EGL_SMPTE2086_MAX_LUMINANCE_EXT: + if (!dpy->Extensions.EXT_surface_SMPTE2086_metadata) { +err = EGL_BAD_ATTRIBUTE; +break; + }
[Mesa-dev] [PATCH 3/3] anv: Plug in VK_EXT_hdr_metadata
From: Ville Syrjälä The common wsi stuff for VK_EXT_hdr_metadata is in place, and we shouldn't need any hardware specifics, so let's expose the extension. Signed-off-by: Ville Syrjälä --- src/intel/vulkan/anv_extensions.py | 1 + src/intel/vulkan/anv_wsi.c | 9 + 2 files changed, 10 insertions(+) diff --git a/src/intel/vulkan/anv_extensions.py b/src/intel/vulkan/anv_extensions.py index b5bee0881c..a3416b3e31 100644 --- a/src/intel/vulkan/anv_extensions.py +++ b/src/intel/vulkan/anv_extensions.py @@ -112,6 +112,7 @@ EXTENSIONS = [ Extension('VK_EXT_global_priority', 1, 'device->has_context_priority'), Extension('VK_EXT_shader_viewport_index_layer', 1, True), +Extension('VK_EXT_hdr_metadata', 1, True), ] class VkVersion: diff --git a/src/intel/vulkan/anv_wsi.c b/src/intel/vulkan/anv_wsi.c index 20094f93e1..61a4f42535 100644 --- a/src/intel/vulkan/anv_wsi.c +++ b/src/intel/vulkan/anv_wsi.c @@ -191,6 +191,15 @@ void anv_DestroySwapchainKHR( wsi_common_destroy_swapchain(_device, swapchain, alloc); } +void anv_SetHdrMetadataEXT( + VkDevice device, + uint32_t swapchainCount, + const VkSwapchainKHR* pSwapchains, + const VkHdrMetadataEXT* pMetadata) +{ + wsi_common_set_hdr_metadata(swapchainCount, pSwapchains, pMetadata); +} + VkResult anv_GetSwapchainImagesKHR( VkDevice device, VkSwapchainKHR swapchain, -- 2.13.6 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] dri3: Only update number of back buffers in loader_dri3_get_buffers
Hello, with my simple tests it works, but have some remarks (but don't have much experience with dri3): 1. It looks little dangerous to move logic of destroying buffers and seems in your version we are loosing some logic with 'busy' and 'num_back' 2. And also looks like buffers just will not be destroyed on time for some cases. 3. From my opinion it will be safer to use in function 'dri3_get_buffer' call (with my patch): 'dri3_fence_await(draw->conn, NULL, buffer)' instead of 'dri3_fence_await(draw->conn, draw, buffer)' On Fri, Apr 27, 2018 at 6:56 PM, Michel Dänzer wrote: > From: Michel Dänzer > > And only free no longer needed back buffers there as well. > > We want to stick to the same back buffer throughout a frame, otherwise > we can run into various issues. > > Bugzilla: https://bugs.freedesktop.org/105906 > Fixes: 3160cb86aa92 "egl/x11: Re-allocate buffers if format is suboptimal" > Signed-off-by: Michel Dänzer > --- > > Sergii, Eero, here's an alternative fix which I think is cleaner, can > you test it? > > src/loader/loader_dri3_helper.c | 19 +++ > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_ > helper.c > index 23729f7ecb2..6db8303d26d 100644 > --- a/src/loader/loader_dri3_helper.c > +++ b/src/loader/loader_dri3_helper.c > @@ -420,13 +420,6 @@ dri3_handle_present_event(struct > loader_dri3_drawable *draw, > > if (buf && buf->pixmap == ie->pixmap) > buf->busy = 0; > - > - if (buf && draw->cur_blit_source != b && !buf->busy && > - (buf->reallocate || > - (draw->num_back <= b && b < LOADER_DRI3_MAX_BACK))) { > -dri3_free_render_buffer(draw, buf); > -draw->buffers[b] = NULL; > - } >} >break; > } > @@ -559,7 +552,6 @@ dri3_find_back(struct loader_dri3_drawable *draw) > /* Check whether we need to reuse the current back buffer as new back. > * In that case, wait until it's not busy anymore. > */ > - dri3_update_num_back(draw); > num_to_consider = draw->num_back; > if (!loader_dri3_have_image_blit(draw) && draw->cur_blit_source != > -1) { >num_to_consider = 1; > @@ -1815,6 +1807,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable, > { > struct loader_dri3_drawable *draw = loaderPrivate; > struct loader_dri3_buffer *front, *back; > + int buf_id; > > buffers->image_mask = 0; > buffers->front = NULL; > @@ -1826,6 +1819,16 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable, > if (!dri3_update_drawable(driDrawable, draw)) >return false; > > + dri3_update_num_back(draw); > + > + /* Free no longer needed back buffers */ > + for (buf_id = draw->num_back; buf_id < LOADER_DRI3_MAX_BACK; buf_id++) > { > + if (draw->cur_blit_source != buf_id && draw->buffers[buf_id]) { > + dri3_free_render_buffer(draw, draw->buffers[buf_id]); > + draw->buffers[buf_id] = NULL; > + } > + } > + > /* pixmaps always have front buffers. > * Exchange swaps also mandate fake front buffers. > */ > -- > 2.17.0 > > -- Sergii Romantsov GlobalLogic Inc. www.globallogic.com ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir/linker: Add nir_link_uniforms()
On 29/04/18 12:09, Timothy Arceri wrote: > On 29/04/18 19:05, Alejandro Piñeiro wrote: >> On 29/04/18 10:13, Timothy Arceri wrote: >>> On 29/04/18 16:48, Alejandro Piñeiro wrote: From: Eduardo Lima Mitev This function will be the entry point for linking the uniforms from the nir_shader objects associated with the gl_linked_shaders of a program. This patch includes initial support for linking uniforms from NIR shaders. It is tailored for the ARB_gl_spirv needs, and it is far from complete, but it should handle most cases of uniforms, array uniforms, structs, samplers and images. There are some FIXMEs related to specific features that will be implemented in following patches, like atomic counters, UBOs and SSBOs. Also, note that ARB_gl_spirv makes mandatory explicit location for normal uniforms, so this code only handles uniforms with explicit location. But there are cases, like uniform atomic counters, that doesn't have a location from the OpenGL point of view (they have a binding), but that Mesa assign internally a location. That will be handled on following patches. A nir_linker.h file is also added. More NIR-linking related API will be added in subsequent patches and those will include stuff from Mesa, so reusing nir.h didn't seem a good idea. >>> >>> These files should actually be src/compiler/glsl/nir_link_uniforms.c >>> etc these are not general purpose nir helpers they are GLSL specific. >> >> Yes, it is true that are not general purpose nir helpers, but they are >> not GLSL specific either. As mentioned on the commit message and on the >> introductory comment, it is heavily tailored for ARB_gl_spirv, so they >> are SPIR-V specific. > > But why? Why not try to make it reusable as a linker for GLSL? What > exactly is tailored for ARB_gl_spirv? And does this really block us > reusing it for GLSL? > > I've expressed my opinion on this long ago, we are very close to being > able to implement a NIR linker for GLSL, spending a little effort > designing this linker code as share-able seems like a no brainer to me. Yes, it is true that we didn't explain in detail that decision on the mailing list. We briefly mentioned that on the patches that we were sending to the list, and explained on my presentation on FOSDEM [1], where Nicolai mentioned during the Q&A section that they agreed to us (at least with going for a nir-based linking). In fact, at the beginning we also hoped that this work would be more aligned with a more general nir-linker, and more easily reused for a nir-based GLSL linker, but our opinion changed as we started to code the needs for this extension. In summary the main reason are the names. Right now GLSL linking is based on the names. Uniform name, ubo name, and so on. So for example, from link_uniform_blocks.cpp: /* This hash table will track all of the uniform blocks that have been * encountered. Since blocks with the same block-name must be the same, * the hash is organized by block-name. */ And most the validation rules on GLSL are based, or include somehow, the name of the variables. But on ARB_gl_spirv, everything should work without names. Read as example issues 12, 21 and 22 of the spec [2] as a reference. In fact names are optional even if the SPIR-V include them (see issue 19). So the linking for nir shaders coming from ARB_gl_spirv should work based on the location, binding, offsets, etc. With the previous example, ubos are linked using the binding. So explicit bindings are now mandatory (so the validation rule here change, as not having a explicit binding can be raised as an error). So in order to work for ARB_gl_spirv it should work without names. In order to reuse it for GLSL it should work with names, in fact based on them. Validation rules for both are different. And getting both working at the same time would just add a complexity that IMHO we don't need right now. We already have a GLSL linker, so it is not really worth right now to get this new NIR linker to work there too. Right now we are focusing on getting ARB_gl_spirv linkage working. Another reason is that ARB_gl_spirv GLSL supported features are not exactly any existing GLSL. It is based on GL_KHR_vulkan_glsl, but at the same time, slightly tweaked. See "Differences Relative to Other Specifications" on the ARB_gl_spirv spec [2]. So perhaps in the future some of this work could be reused for nir-base linker for GLSL. But, IMHO, that shouldn't be one of the features driving the development. I think that we should work first on what it is not supported. > > Ignoring the above issue I'm fairly certain adding a dependency on > core mesa GL to NIR is a no no so either way these files don't belong > in src/compiler/nir But we already have that dependency, right? I mean gl_program have a reference to nir_shader for some time now. And in any case, the implementation of ARB_g