[Mesa-dev] [Bug 89586] Drivers/DRI/swrast
https://bugs.freedesktop.org/show_bug.cgi?id=89586 --- Comment #10 from Dan Sebald --- OK, src/mesa/state_tracker/st_cb_drawpixels.c is it, thanks. Saved me a lot of time. And here's a comment about image size in the code: /* Limit the size of the glDrawPixels to the max texture size. * Strictly speaking, that's not correct but since we don't handle * larger images yet, this is better than crashing. */ I simply removed the limit and the image displays fine with no segfaults. There is a tiny discrepancy compared with the swrast/ result, however. There may still be a bug floating about. I will explain in a different bug report. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/23] mesa: glGetProgramResourceName
On 03/16/2015 07:16 PM, Ilia Mirkin wrote: On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli wrote: Patch adds required helper functions to shaderapi.h and the actual implementation. Name generation copied from '_mesa_get_uniform_name' which can be removed later by refactoring functions to use resource list. The added functionality can be tested by tests for following functions that are refactored by later patches: GetActiveUniformName GetActiveUniformBlockName Signed-off-by: Tapani Pälli --- src/mesa/main/program_resource.c | 22 + src/mesa/main/shader_query.cpp | 96 src/mesa/main/shaderapi.h| 10 + 3 files changed, 128 insertions(+) diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c index 4190f98..4fa6ac6 100644 --- a/src/mesa/main/program_resource.c +++ b/src/mesa/main/program_resource.c @@ -211,6 +211,28 @@ _mesa_GetProgramResourceName(GLuint program, GLenum programInterface, GLuint index, GLsizei bufSize, GLsizei *length, GLchar *name) { + GET_CURRENT_CONTEXT(ctx); + struct gl_shader_program *shProg = + _mesa_lookup_shader_program_err(ctx, program, + "glGetProgramResourceIndex"); + if (!shProg) + return; + + /* Set user friendly return values in case of errors. */ Should this be done above the if (!shProg) return thing then? Sure, for the case of consistency. + *name = '\0'; + if (length) + *length = 0; + + if (programInterface == GL_ATOMIC_COUNTER_BUFFER) { + _mesa_error(ctx, GL_INVALID_ENUM, + "glGetProgramResourceName(%s)", + _mesa_lookup_enum_by_nr(programInterface)); + return; + } + + _mesa_get_program_resource_name(shProg, programInterface, index, + bufSize, length, name, + "glGetProgramResourceName"); } void GLAPIENTRY diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index d1974a4..77a4af0 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -647,3 +647,99 @@ _mesa_program_resource_index(struct gl_shader_program *shProg, return calc_resource_index(shProg, res); } } + +/* Find a program resource with specific index in given interface. + */ +struct gl_program_resource * +_mesa_program_resource_find_index(struct gl_shader_program *shProg, + GLenum interface, GLuint index) I feel like I've seen a very similar function before. TBH I can't remember which patch it was in, but if there's any unification possibility, please do so. OK, will see if possible. +{ + struct gl_program_resource *res = shProg->ProgramResourceList; + int idx = -1; + + for (unsigned i = 0; i < shProg->NumProgramResourceList; i++, res++) { + if (res->Type != interface) + continue; + + switch (res->Type) { + case GL_UNIFORM_BLOCK: + case GL_ATOMIC_COUNTER_BUFFER: + if (_mesa_program_resource_index(shProg, res) == index) +return res; + + case GL_TRANSFORM_FEEDBACK_VARYING: + case GL_PROGRAM_INPUT: + case GL_PROGRAM_OUTPUT: + case GL_UNIFORM: + if (++idx == (int) index) +return res; + break; + default: + assert(!"not implemented for given interface"); + } + } + return NULL; +} + +/* Get full name of a program resource. + */ +bool +_mesa_get_program_resource_name(struct gl_shader_program *shProg, +GLenum interface, GLuint index, +GLsizei bufSize, GLsizei *length, +GLchar *name, const char *caller) +{ + GET_CURRENT_CONTEXT(ctx); + + /* Find resource with given interface and index. */ + struct gl_program_resource *res = + _mesa_program_resource_find_index(shProg, interface, index); + + /* The error INVALID_VALUE is generated if is greater than + * or equal to the number of entries in the active resource list for + * . + */ + if (!res) { + _mesa_error(ctx, GL_INVALID_VALUE, "%s(index %u)", caller, index); + return false; + } + + GLsizei localLength; + + if (length == NULL) + length = &localLength; + + _mesa_copy_string(name, bufSize, length, _mesa_program_resource_name(res)); + + /* Page 61 (page 73 of the PDF) in section 2.11 of the OpenGL ES 3.0 +* spec says: +* +* "If the active uniform is an array, the uniform name returned in +* name will always be the name of the uniform array appended with +* "[0]"." +* +* The same text also appears in the OpenGL 4.2 spec. It does not, +* however, appear in any previous spec. Previous specifications are +* ambiguous in this regard. However, either name can later be passed +* to glGetUniformLocation (and
Re: [Mesa-dev] [PATCH 06/23] mesa: glGetProgramResourceLocation
On 03/16/2015 07:08 PM, Ilia Mirkin wrote: On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli wrote: Patch adds required helper functions to shaderapi.h and the actual implementation. corresponding Piglit test: arb_program_interface_query-resource-location The added functionality can be tested by tests for following functions that are refactored by later patches: GetAttribLocation GetUniformLocation GetFragDataLocation Signed-off-by: Tapani Pälli --- src/mesa/main/program_resource.c | 81 +++- src/mesa/main/shader_query.cpp | 64 +++ src/mesa/main/shaderapi.h| 4 ++ 3 files changed, 148 insertions(+), 1 deletion(-) diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c index 4fa6ac6..87a0144 100644 --- a/src/mesa/main/program_resource.c +++ b/src/mesa/main/program_resource.c @@ -243,11 +243,90 @@ _mesa_GetProgramResourceiv(GLuint program, GLenum programInterface, { } +/** + * Function verifies syntax of given name for GetProgramResourceLocation + * and GetProgramResourceLocationIndex for the following cases: + * + * "array element portion of a string passed to GetProgramResourceLocation + * or GetProgramResourceLocationIndex must not have, a "+" sign, extra + * leading zeroes, or whitespace". + * + * Check is written to be compatible with GL_ARB_array_of_arrays. + */ +static bool +invalid_array_element_syntax(const GLchar *name) +{ + char *array = strrchr(name, '['); + + if (!array) + return false; + + /* No '+' or ' ' allowed anywhere. */ + if (strchr(name, '+') || strchr(name, ' ')) I guess it'd be mildly better to do a strchr('[') and use that for the second strchr's? You could do it like char *first = strchr(name, '['); char *last = strrchr(first, '['); if (strchr(first, '+') || ... ) That way you avoid iterating over the "name" portion of it unnecessarily. Probably doesn't amount to too much. Yes, it is more optimal, will change. + return true; + + /* Check that last array index is 0. */ + if (array[1] == '0' && array[2] != ']') + return true; + + return false; +} + +static struct gl_shader_program * +lookup_linked_program(GLuint program, const char *caller) +{ + GET_CURRENT_CONTEXT(ctx); + struct gl_shader_program *prog = + _mesa_lookup_shader_program_err(ctx, program, caller); + + if (!prog) + return NULL; + + if (prog->LinkStatus == GL_FALSE) { + _mesa_error(ctx, GL_INVALID_OPERATION, + "%s(program not linked)", caller); + return NULL; + } + return prog; +} + GLint GLAPIENTRY _mesa_GetProgramResourceLocation(GLuint program, GLenum programInterface, const GLchar *name) { - return -1; + GET_CURRENT_CONTEXT(ctx); + struct gl_shader_program *shProg = + lookup_linked_program(program, "glGetProgramResourceLocation"); + + if (!shProg || invalid_array_element_syntax(name)) + return -1; + + /* Validate programInterface. */ + switch (programInterface) { + case GL_UNIFORM: + case GL_PROGRAM_INPUT: + case GL_PROGRAM_OUTPUT: + break; + + /* For reference valid cases requiring addition extension support: +* GL_ARB_shader_subroutine +* GL_ARB_tessellation_shader +* GL_ARB_compute_shader +*/ + case GL_VERTEX_SUBROUTINE_UNIFORM: + case GL_TESS_CONTROL_SUBROUTINE_UNIFORM: + case GL_TESS_EVALUATION_SUBROUTINE_UNIFORM: + case GL_GEOMETRY_SUBROUTINE_UNIFORM: + case GL_FRAGMENT_SUBROUTINE_UNIFORM: + case GL_COMPUTE_SUBROUTINE_UNIFORM: + + default: + _mesa_error(ctx, GL_INVALID_ENUM, + "glGetProgramResourceLocation(%s %s)", + _mesa_lookup_enum_by_nr(programInterface), name); + } + + return _mesa_program_resource_location(shProg, programInterface, name); } GLint GLAPIENTRY diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index 77a4af0..4ae00a6 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -743,3 +743,67 @@ _mesa_get_program_resource_name(struct gl_shader_program *shProg, } return true; } + +static GLint +program_resource_location(struct gl_shader_program *shProg, + struct gl_program_resource *res, const char *name) +{ + unsigned index, offset; + int array_index = -1; And I suppose leaving off the initializer here makes gcc complain? Yep,"warning: 'array_index' may be used uninitialized in this function" + + if (res->Type == GL_PROGRAM_INPUT || + res->Type == GL_PROGRAM_OUTPUT) { put all on one line? will fix + array_index = array_index_of_resource(res, name); + if (array_index < 0) + return -1; + } + + switch (res->Type) { + case GL_PROGRAM_INPUT: + return RESOURCE_VAR(res)->data.location + array_index - VERT_ATTRIB_GENERIC0; + case GL_PROGRAM_OUTPUT: + return RESOURCE_VAR(res)->data.location
Re: [Mesa-dev] [PATCH 07/23] mesa: glGetProgramResourceLocationIndex
On 03/16/2015 08:08 PM, Ilia Mirkin wrote: On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli wrote: Patch adds required helper functions to shaderapi.h and the actual implementation. The added functionality can be tested by tests for following functions that are refactored by later patches: GetFragDataIndex Signed-off-by: Tapani Pälli --- src/mesa/main/program_resource.c | 25 - src/mesa/main/shader_query.cpp | 18 ++ src/mesa/main/shaderapi.h| 4 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/mesa/main/program_resource.c b/src/mesa/main/program_resource.c index 87a0144..ae987de 100644 --- a/src/mesa/main/program_resource.c +++ b/src/mesa/main/program_resource.c @@ -329,9 +329,32 @@ _mesa_GetProgramResourceLocation(GLuint program, GLenum programInterface, return _mesa_program_resource_location(shProg, programInterface, name); } +/** + * Returns output index for dual source blending. + */ GLint GLAPIENTRY _mesa_GetProgramResourceLocationIndex(GLuint program, GLenum programInterface, const GLchar *name) { - return -1; + GET_CURRENT_CONTEXT(ctx); + struct gl_shader_program *shProg = + lookup_linked_program(program, "glGetProgramResourceLocationIndex"); + + if (!shProg || invalid_array_element_syntax(name)) + return -1; + + /* From the GL_ARB_program_interface_query spec: +* +* "For GetProgramResourceLocationIndex, must be +* PROGRAM_OUTPUT." +*/ And presumably it must be a program with a fragment shader (which might not be there for a no-rast or compute pipeline). spec says that -1 is returned: "If has been successfully linked but contains no fragment shader, no error will be generated but -1 will be returned." this is what happens with the implementation. + if (programInterface != GL_PROGRAM_OUTPUT) { + _mesa_error(ctx, GL_INVALID_ENUM, + "glGetProgramResourceLocationIndex (%s)", + _mesa_lookup_enum_by_nr(programInterface)); + return -1; + } + + return _mesa_program_resource_location_index(shProg, programInterface, +name); } diff --git a/src/mesa/main/shader_query.cpp b/src/mesa/main/shader_query.cpp index 4ae00a6..d3264db 100644 --- a/src/mesa/main/shader_query.cpp +++ b/src/mesa/main/shader_query.cpp @@ -807,3 +807,21 @@ _mesa_program_resource_location(struct gl_shader_program *shProg, return program_resource_location(shProg, res, name); } + +/** + * Function implements following index queries: + *glGetFragDataIndex + */ +GLint +_mesa_program_resource_location_index(struct gl_shader_program *shProg, + GLenum interface, const char *name) +{ + struct gl_program_resource *res = + _mesa_program_resource_find_name(shProg, interface, name); + + /* Non-existent (inactive) variable. */ + if (!res) + return -1; + + return RESOURCE_VAR(res)->data.index; +} diff --git a/src/mesa/main/shaderapi.h b/src/mesa/main/shaderapi.h index 73ebf60..5046018 100644 --- a/src/mesa/main/shaderapi.h +++ b/src/mesa/main/shaderapi.h @@ -248,6 +248,10 @@ extern GLint _mesa_program_resource_location(struct gl_shader_program *shProg, GLenum interface, const char *name); +extern GLint +_mesa_program_resource_location_index(struct gl_shader_program *shProg, + GLenum interface, const char *name); + #ifdef __cplusplus } #endif -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/4] i965/state: Create separate dirty state bits for each pipeline
On Monday, March 16, 2015 10:21:31 PM Kristian Høgsberg wrote: > On Wed, Mar 11, 2015 at 11:53 AM, Jordan Justen > wrote: > > When uploading state for a pipeline, we will save changed state for > > the other pipelines. > > > > Signed-off-by: Jordan Justen > > In reviewing this again, I realize I'm not completely comfortable with > how this is done. The overall approach is great, I'm happy to see the > dual pipeline flags contained in brw_state_upload.c, but I think we > need to keep brw_upload_pipeline_state() a no-op in case it fails. Given that brw_upload_state() has always done: struct brw_state_flags *state = &brw->state.dirty; state->mesa |= brw->NewGLState; brw->NewGLState = 0; state->brw |= ctx->NewDriverState; ctx->NewDriverState = 0; I think there's a pretty good precedence for having it update the driver's notion of what's dirty. But you can still run it several times back to back with no issues. > I can't think of a case where it might bite us, but currently, the > expectation is that if it fails, nothing changes and you can call it > again after flushing the batch. This patch changes that in two ways: > 1) we merge the pipeline brw->state.dirty into state and clear the > pipeline state and 2) we copy the dirty state into the other pipeline. > Suppose we call with the render pipeline and fail, flush the batch, > then call with the compute pipeline and clear the dirty flags [1]. If > we then go back and try to emit render state again, there are no > longer any dirty bits and we fail to emit render state. I'm pretty comfortable with this. Your concerns seem to revolve around implicitly switching pipelines in the middle of state upload. I don't believe that can happen (and we'd better keep it that way!). You're always uploading state for some particular operation - drawing (on the render pipe), or glDispatchCompute (on the compute pipe). When processing an operation, we first save the batch state (bytes used for commands, number of relocations). Then we upload state. If we run out of batch space(*), or refer to more objects than we can fit in the aperture, we roll back to that saved state and flush the batch, executing everything queued except the most recent operation. We then re-upload state for that single failed operation in a fresh batch. There's no pipeline switch here - when we retry that same operation, it will dispatch to the same pipe as before. Notably, there are no _mesa_update_state calls, so we can never hit resolves. (We did those at the very beginning, before even beginning to process the operation at hand!) It's a pretty tight loop - 1. upload state, 2. check aperture space, 3. flush, go back to step 1. > This may be far-fetched and, as I said, I don't think we can hit this. So we agree :) > But I'd rather not break the contract that brw_upload_pipeline_state() > doesn't change state in case of failure - I know I'd hate to debug > that. > We can just introduce a brw_clear_pipeline_dirty_bits() function > that will be called on successful state upload and merge the state > bits to the other pipeline and then clear the selected pipeline dirty > flags as well as brw->state.dirty. And we need to use a local copy of > brw->state.dirty in brw_upload_pipeline_state() for merging in the > pipeline dirty flags so we don't modify brw->state.dirty on error. > > [1] I don't see how we'd get into that, but with fast clear resolve > using meta we end up running the render pipeline at unexpected places. But not here - and frankly, doing resolves in the middle of state upload would be crazy. Resolves inherently need to set render state that the actual draw will want to set, too. You have to serialize them. Which is what we do today - we handle resolves when Mesa first asks us to draw or dispatch a compute workload, before we ever start processing the operation itself. The resolve will space-check-and-retry...but it will get done. Only then do we try the next thing, which will space-check-and-retry independently. Thanks for thinking about this carefully - it's easy to mess up, and we really need to get it right. I think we have, though. > > --- > > src/mesa/drivers/dri/i965/brw_context.h | 1 + > > src/mesa/drivers/dri/i965/brw_state_upload.c | 42 > > ++-- > > 2 files changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_context.h > > b/src/mesa/drivers/dri/i965/brw_context.h > > index 91b4054..e693f50 100644 > > --- a/src/mesa/drivers/dri/i965/brw_context.h > > +++ b/src/mesa/drivers/dri/i965/brw_context.h > > @@ -1101,6 +1101,7 @@ struct brw_context > > GLuint NewGLState; > > struct { > >struct brw_state_flags dirty; > > + struct brw_state_flags pipelines[BRW_NUM_PIPELINES]; > > } state; > > > > struct brw_cache cache; > > diff --git a/src/mesa/drivers/dri/i965/brw_state_upload.c > > b/src/mesa/drivers/dri/i965/brw_state_upload.c > > index 4f210
Re: [Mesa-dev] [PATCH 15/23] mesa: refactor GetActiveUniformsiv, use _mesa_program_resource_prop
On 03/17/2015 12:38 AM, Ilia Mirkin wrote: On Fri, Mar 13, 2015 at 4:38 AM, Tapani Pälli wrote: Signed-off-by: Tapani Pälli --- src/mesa/main/uniform_query.cpp | 107 ++-- 1 file changed, 38 insertions(+), 69 deletions(-) diff --git a/src/mesa/main/uniform_query.cpp b/src/mesa/main/uniform_query.cpp index 9f82de9..217473a 100644 --- a/src/mesa/main/uniform_query.cpp +++ b/src/mesa/main/uniform_query.cpp @@ -79,6 +79,33 @@ _mesa_GetActiveUniform(GLuint program, GLuint index, } 8< - - case GL_UNIFORM_ATOMIC_COUNTER_BUFFER_INDEX: - if (!ctx->Extensions.ARB_shader_atomic_counters) -goto invalid_enum; - params[i] = uni->atomic_buffer_index; + if (!_mesa_program_resource_prop(shProg, res, uniformIndices[i], + res_prop, ¶ms[i], + "glGetActiveUniformsiv")) Will this return GL_INVALID_ENUM if res_prop == 0? If not, you need to handle that above. Yes, it results in INVALID_ENUM error. With that answered or taken care of, Reviewed-by: Ilia Mirkin break; - - default: - goto invalid_enum; - } } - - return; - - invalid_enum: - _mesa_error(ctx, GL_INVALID_ENUM, "glGetActiveUniformsiv(pname)"); } static struct gl_uniform_storage * -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] i965: Handle scratch accesses where reladdr also points to scratch space
This is a problem when we have IR like this: (array_ref (var_ref temps) (swiz x (expression ivec4 bitcast_f2i (swiz (array_ref (var_ref temps) (constant int (2)) ) )) )) ) ) where we are indexing an array with the result of an expression that accesses the same array. In this scenario, temps will be moved to scratch space and we will need to add scratch reads/writes for all accesses to temps, however, the current implementation does not consider the case where a reladdr pointer (obtained by indexing into temps trough a expression) points to a register that is also stored in scratch space (as in this case, where the expression used to index temps access temps[2]), and thus, requires a scratch read before it is accessed. v2 (Francisco Jerez): - Handle also recursive reladdr addressing. - Do not memcpy dst_reg into src_reg when rewriting reladdr. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89508 --- A couple of notes for reviewers: 1. The implementation resolves recursive reladdr scratch accesses in one go so we don't have to do multiple passes over the complete set of instructions. This is more performant than doing something similar to what we do in move_uniform_array_access_to_pull_constants. 2. Once we start handling recursive reladdr we are rewriting reladdr accesses to point to the destination registers of the scratch loads. This means that alloc.count increases and we can have a reladdr pointing to a register location beyond the original alloc.count, so we should take this into account when indexing scratch_loc[] with reladdr registers. I tested this for recursive reladdr accesses in both src and dst, including indexing different arrays: a[b[a[b[0, etc and seems to work well. No piglit regressions on IvyBridge. As a side note, I also noticed that opt_array_splitting.cpp does not handle these situations well and hits an assertion in some cases where it wrongly assumes that an array only has constant indexing. This problem is happening in master and is unrelated to this patch so I'll upload a bug report for that. I mention this just in case someone wants to test the patch and hits the problem. In my tests I had to disable that optimization pass for the more complex cases. src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 138 + 1 file changed, 117 insertions(+), 21 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp index 5bf9e1b..c776c7a 100644 --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp @@ -3391,7 +3391,8 @@ vec4_visitor::emit_scratch_write(bblock_t *block, vec4_instruction *inst, void vec4_visitor::move_grf_array_access_to_scratch() { - int scratch_loc[this->alloc.count]; + int alloc_count = alloc.count; + int scratch_loc[alloc_count]; memset(scratch_loc, -1, sizeof(scratch_loc)); /* First, calculate the set of virtual GRFs that need to be punted @@ -3400,19 +3401,29 @@ vec4_visitor::move_grf_array_access_to_scratch() */ foreach_block_and_inst(block, vec4_instruction, inst, cfg) { if (inst->dst.file == GRF && inst->dst.reladdr && - scratch_loc[inst->dst.reg] == -1) { -scratch_loc[inst->dst.reg] = c->last_scratch; -c->last_scratch += this->alloc.sizes[inst->dst.reg]; + scratch_loc[inst->dst.reg] == -1) { + scratch_loc[inst->dst.reg] = c->last_scratch; + c->last_scratch += this->alloc.sizes[inst->dst.reg]; + + src_reg *iter = inst->dst.reladdr; + while (iter->reladdr) { +if (iter->file == GRF && scratch_loc[iter->reg] == -1) { + scratch_loc[iter->reg] = c->last_scratch; + c->last_scratch += this->alloc.sizes[iter->reg]; +} +iter = iter->reladdr; + } } for (int i = 0 ; i < 3; i++) { -src_reg *src = &inst->src[i]; - -if (src->file == GRF && src->reladdr && -scratch_loc[src->reg] == -1) { - scratch_loc[src->reg] = c->last_scratch; - c->last_scratch += this->alloc.sizes[src->reg]; -} + src_reg *iter = &inst->src[i]; + while (iter->reladdr) { +if (iter->file == GRF && scratch_loc[iter->reg] == -1) { + scratch_loc[iter->reg] = c->last_scratch; + c->last_scratch += this->alloc.sizes[iter->reg]; +} +iter = iter->reladdr; + } } } @@ -3422,27 +3433,112 @@ vec4_visitor::move_grf_array_access_to_scratch() * we're processing. */ foreach_block_and_inst_safe(block, vec4_instruction, inst, cfg) { + bool nested_reladdr; + /* Set up the annotation tracking for new generated instructions. */ base_ir = inst->ir; current_annotation = inst->annotation; + /* First handle scratch access on the dst */ if (inst->dst.file == GRF && scratch_loc[ins
Re: [Mesa-dev] [PATCH 3/3] r600g: constify r600_shader_tgsi_instruction lists.
For the series: Reviewed-by: Marek Olšák Marek On Mon, Mar 16, 2015 at 3:47 PM, Emil Velikov wrote: > Massive list of constant data. Annotate it as such. > > Signed-off-by: Emil Velikov > --- > src/gallium/drivers/r600/r600_shader.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/src/gallium/drivers/r600/r600_shader.c > b/src/gallium/drivers/r600/r600_shader.c > index acac89f..28b290a 100644 > --- a/src/gallium/drivers/r600/r600_shader.c > +++ b/src/gallium/drivers/r600/r600_shader.c > @@ -283,7 +283,7 @@ struct r600_shader_ctx { > unsignedtype; > unsignedfile_offset[TGSI_FILE_COUNT]; > unsignedtemp_reg; > - struct r600_shader_tgsi_instruction *inst_info; > + const struct r600_shader_tgsi_instruction *inst_info; > struct r600_bytecode*bc; > struct r600_shader *shader; > struct r600_shader_src src[4]; > @@ -316,7 +316,7 @@ struct r600_shader_tgsi_instruction { > }; > > static int emit_gs_ring_writes(struct r600_shader_ctx *ctx, bool ind); > -static struct r600_shader_tgsi_instruction r600_shader_tgsi_instruction[], > eg_shader_tgsi_instruction[], cm_shader_tgsi_instruction[]; > +static const struct r600_shader_tgsi_instruction > r600_shader_tgsi_instruction[], eg_shader_tgsi_instruction[], > cm_shader_tgsi_instruction[]; > static int tgsi_helper_tempx_replicate(struct r600_shader_ctx *ctx); > static inline void callstack_push(struct r600_shader_ctx *ctx, unsigned > reason); > static void fc_pushlevel(struct r600_shader_ctx *ctx, int type); > @@ -7270,7 +7270,7 @@ static int tgsi_umad(struct r600_shader_ctx *ctx) > return 0; > } > > -static struct r600_shader_tgsi_instruction r600_shader_tgsi_instruction[] = { > +static const struct r600_shader_tgsi_instruction > r600_shader_tgsi_instruction[] = { > [TGSI_OPCODE_ARL] = { ALU_OP0_NOP, tgsi_r600_arl}, > [TGSI_OPCODE_MOV] = { ALU_OP1_MOV, tgsi_op2}, > [TGSI_OPCODE_LIT] = { ALU_OP0_NOP, tgsi_lit}, > @@ -7475,7 +7475,7 @@ static struct r600_shader_tgsi_instruction > r600_shader_tgsi_instruction[] = { > [TGSI_OPCODE_LAST] = { ALU_OP0_NOP, tgsi_unsupported}, > }; > > -static struct r600_shader_tgsi_instruction eg_shader_tgsi_instruction[] = { > +static const struct r600_shader_tgsi_instruction > eg_shader_tgsi_instruction[] = { > [TGSI_OPCODE_ARL] = { ALU_OP0_NOP, tgsi_eg_arl}, > [TGSI_OPCODE_MOV] = { ALU_OP1_MOV, tgsi_op2}, > [TGSI_OPCODE_LIT] = { ALU_OP0_NOP, tgsi_lit}, > @@ -7674,7 +7674,7 @@ static struct r600_shader_tgsi_instruction > eg_shader_tgsi_instruction[] = { > [TGSI_OPCODE_LAST] = { ALU_OP0_NOP, tgsi_unsupported}, > }; > > -static struct r600_shader_tgsi_instruction cm_shader_tgsi_instruction[] = { > +static const struct r600_shader_tgsi_instruction > cm_shader_tgsi_instruction[] = { > [TGSI_OPCODE_ARL] = { ALU_OP0_NOP, tgsi_eg_arl}, > [TGSI_OPCODE_MOV] = { ALU_OP1_MOV, tgsi_op2}, > [TGSI_OPCODE_LIT] = { ALU_OP0_NOP, tgsi_lit}, > -- > 2.3.1 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] linker: fix varying linking if SSO program has only gs and fs
Previously linker did not take in to account case where one would have only gs and fs (with SSO), patch adds the case by refactoring code around assign_varying_locations. This makes sure locations for gs get populated correctly. This was found with some of the SSO subtests of Martin's upcoming GetProgramInterfaceiv Piglit test which passes with the patch, no Piglit regressions. Signed-off-by: Tapani Pälli --- src/glsl/linker.cpp | 32 +++- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp index 0c44677..9135ca0 100644 --- a/src/glsl/linker.cpp +++ b/src/glsl/linker.cpp @@ -2736,10 +2736,19 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) goto done; } - unsigned first; - for (first = 0; first <= MESA_SHADER_FRAGMENT; first++) { - if (prog->_LinkedShaders[first] != NULL) -break; + unsigned first, last; + + first = MESA_SHADER_STAGES; + last = 0; + + /* Determine first and last stage. */ + for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) { + struct gl_shader *sh = prog->_LinkedShaders[i]; + if (!sh) + continue; + if (first == MESA_SHADER_STAGES) + first = i; + last = i; } if (num_tfeedback_decls != 0) { @@ -2768,13 +2777,9 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) * ensures that inter-shader outputs written to in an earlier stage are * eliminated if they are (transitively) not used in a later stage. */ - int last, next; - for (last = MESA_SHADER_FRAGMENT; last >= 0; last--) { - if (prog->_LinkedShaders[last] != NULL) - break; - } + int next; - if (last >= 0 && last < MESA_SHADER_FRAGMENT) { + if (first < MESA_SHADER_FRAGMENT) { gl_shader *const sh = prog->_LinkedShaders[last]; if (first == MESA_SHADER_GEOMETRY) { @@ -2786,13 +2791,14 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) * MESA_SHADER_GEOMETRY. */ if (!assign_varying_locations(ctx, mem_ctx, prog, - NULL, sh, + NULL, prog->_LinkedShaders[first], num_tfeedback_decls, tfeedback_decls, prog->Geom.VerticesIn)) goto done; } - if (num_tfeedback_decls != 0 || prog->SeparateShader) { + if (last != MESA_SHADER_FRAGMENT && + (num_tfeedback_decls != 0 || prog->SeparateShader)) { /* There was no fragment shader, but we still have to assign varying * locations for use by transform feedback. */ @@ -2814,7 +2820,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) while (do_dead_code(sh->ir, false)) ; } - else if (first == MESA_SHADER_FRAGMENT) { + else if (first == MESA_SHADER_FRAGMENT && first == last) { /* If the program only contains a fragment shader... */ gl_shader *const sh = prog->_LinkedShaders[first]; -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines.
-Original Message- From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of Matt Turner Sent: Thursday, March 12, 2015 12:02 AM To: Ilia Mirkin Cc: mesa-dev@lists.freedesktop.org Subject: Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines. On Wed, Mar 11, 2015 at 2:09 PM, Ilia Mirkin wrote: > On Wed, Mar 11, 2015 at 5:57 PM, wrote: >> From: Marius Predut > > Set your email from name correctly in git and then you won't have this > line in your git send-email results. > >> >> On SNB and IVB hw, for 1 pixel line thickness or less, >> the general anti-aliasing algorithm give up - garbage line is generated. >> Setting a Line Width of 0.0 specifies the rasterization >> of the “thinnest” (one-pixel-wide), non-antialiased lines. >> Lines rendered with zero Line Width are rasterized using >> Grid Intersection Quantization rules as specified by >> bspec section 6.3.12.1 Zero-Width (Cosmetic) Line Rasterization. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668 > > This seems like the wrong bug reference... I'm pretty sure it should be https://bugs.freedesktop.org/show_bug.cgi?id=28832 yes it is. thanks Matt ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 88806] nir/nir_constant_expressions.c:2754:15: error: controlling expression type 'unsigned int' not compatible with any generic association type
https://bugs.freedesktop.org/show_bug.cgi?id=88806 Jason Ekstrand changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #8 from Jason Ekstrand --- Then let's close this -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 79706] [TRACKER] Mesa regression tracker
https://bugs.freedesktop.org/show_bug.cgi?id=79706 Bug 79706 depends on bug 88806, which changed state. Bug 88806 Summary: nir/nir_constant_expressions.c:2754:15: error: controlling expression type 'unsigned int' not compatible with any generic association type https://bugs.freedesktop.org/show_bug.cgi?id=88806 What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/23] mesa: glGetProgramResourceLocationIndex
On Tue, Mar 17, 2015 at 5:13 AM, Tapani Pälli wrote: > > > On 03/16/2015 08:08 PM, Ilia Mirkin wrote: >> >> On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli >> wrote: >>> +/** >>> + * Returns output index for dual source blending. >>> + */ >>> GLint GLAPIENTRY >>> _mesa_GetProgramResourceLocationIndex(GLuint program, GLenum >>> programInterface, >>> const GLchar *name) >>> { >>> - return -1; >>> + GET_CURRENT_CONTEXT(ctx); >>> + struct gl_shader_program *shProg = >>> + lookup_linked_program(program, >>> "glGetProgramResourceLocationIndex"); >>> + >>> + if (!shProg || invalid_array_element_syntax(name)) >>> + return -1; >>> + >>> + /* From the GL_ARB_program_interface_query spec: >>> +* >>> +* "For GetProgramResourceLocationIndex, must be >>> +* PROGRAM_OUTPUT." >>> +*/ >> >> >> And presumably it must be a program with a fragment shader (which >> might not be there for a no-rast or compute pipeline). > > > spec says that -1 is returned: > > "If has been successfully linked but contains no fragment shader, > no error will be generated but -1 will be returned." > > this is what happens with the implementation. Can you explain how the current implementation takes care of that? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/4] i965/state: Create separate dirty state bits for each pipeline
On Tue, Mar 17, 2015 at 2:22 AM, Kenneth Graunke wrote: > On Monday, March 16, 2015 10:21:31 PM Kristian Høgsberg wrote: >> On Wed, Mar 11, 2015 at 11:53 AM, Jordan Justen >> wrote: >> > When uploading state for a pipeline, we will save changed state for >> > the other pipelines. >> > >> > Signed-off-by: Jordan Justen >> >> In reviewing this again, I realize I'm not completely comfortable with >> how this is done. The overall approach is great, I'm happy to see the >> dual pipeline flags contained in brw_state_upload.c, but I think we >> need to keep brw_upload_pipeline_state() a no-op in case it fails. > > Given that brw_upload_state() has always done: > >struct brw_state_flags *state = &brw->state.dirty; > >state->mesa |= brw->NewGLState; >brw->NewGLState = 0; > >state->brw |= ctx->NewDriverState; >ctx->NewDriverState = 0; > > I think there's a pretty good precedence for having it update the > driver's notion of what's dirty. But you can still run it several > times back to back with no issues. > >> I can't think of a case where it might bite us, but currently, the >> expectation is that if it fails, nothing changes and you can call it >> again after flushing the batch. This patch changes that in two ways: >> 1) we merge the pipeline brw->state.dirty into state and clear the >> pipeline state and 2) we copy the dirty state into the other pipeline. >> Suppose we call with the render pipeline and fail, flush the batch, >> then call with the compute pipeline and clear the dirty flags [1]. If >> we then go back and try to emit render state again, there are no >> longer any dirty bits and we fail to emit render state. > > I'm pretty comfortable with this. Your concerns seem to revolve around > implicitly switching pipelines in the middle of state upload. I don't > believe that can happen (and we'd better keep it that way!). > > You're always uploading state for some particular operation - drawing > (on the render pipe), or glDispatchCompute (on the compute pipe). When > processing an operation, we first save the batch state (bytes used for > commands, number of relocations). Then we upload state. > > If we run out of batch space(*), or refer to more objects than we can > fit in the aperture, we roll back to that saved state and flush the > batch, executing everything queued except the most recent operation. > > We then re-upload state for that single failed operation in a fresh > batch. There's no pipeline switch here - when we retry that same > operation, it will dispatch to the same pipe as before. Notably, > there are no _mesa_update_state calls, so we can never hit resolves. > (We did those at the very beginning, before even beginning to process > the operation at hand!) It's a pretty tight loop - 1. upload state, > 2. check aperture space, 3. flush, go back to step 1. > >> This may be far-fetched and, as I said, I don't think we can hit this. > > So we agree :) > >> But I'd rather not break the contract that brw_upload_pipeline_state() >> doesn't change state in case of failure - I know I'd hate to debug >> that. >> We can just introduce a brw_clear_pipeline_dirty_bits() function >> that will be called on successful state upload and merge the state >> bits to the other pipeline and then clear the selected pipeline dirty >> flags as well as brw->state.dirty. And we need to use a local copy of >> brw->state.dirty in brw_upload_pipeline_state() for merging in the >> pipeline dirty flags so we don't modify brw->state.dirty on error. >> >> [1] I don't see how we'd get into that, but with fast clear resolve >> using meta we end up running the render pipeline at unexpected places. > > But not here - and frankly, doing resolves in the middle of state upload > would be crazy. Resolves inherently need to set render state that the > actual draw will want to set, too. You have to serialize them. > > Which is what we do today - we handle resolves when Mesa first asks us > to draw or dispatch a compute workload, before we ever start processing > the operation itself. The resolve will space-check-and-retry...but it > will get done. Only then do we try the next thing, which will > space-check-and-retry independently. > > Thanks for thinking about this carefully - it's easy to mess up, and we > really need to get it right. I think we have, though. You've argued and agreed that the case I said couldn't happen can't happen. Fine. My point however was that with a small change we can eliminate any concerns like that and not have to think through all the different paths we can call brw_update_state(). I'm not asking for a big rewrite of this, just a small tweak in how and when we clear and propagate the state bits. And you ignored the part below about how we keep carrying dirty state around as we switch pipelines. Kristian > >> > --- >> > src/mesa/drivers/dri/i965/brw_context.h | 1 + >> > src/mesa/drivers/dri/i965/brw_state_upload.c | 42 >> > ++-- >> >
[Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN7
From: Marius Predut On SNB and IVB hw, for 1 pixel line thickness or less, the general anti-aliasing algorithm give up - garbage line is generated. Setting a Line Width of 0.0 specifies the rasterization of the “thinnest” (one-pixel-wide), non-antialiased lines. Lines rendered with zero Line Width are rasterized using Grid Intersection Quantization rules as specified by bspec section 6.3.12.1 Zero-Width (Cosmetic) Line Rasterization. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832 Signed-off-by: Marius Predut --- src/mesa/drivers/dri/i965/gen7_sf_state.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c b/src/mesa/drivers/dri/i965/gen7_sf_state.c index c9815b0..fbad889 100644 --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c @@ -198,9 +198,15 @@ upload_sf_state(struct brw_context *brw) float line_width = roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); uint32_t line_width_u3_7 = U_FIXED(line_width, 7); - /* TODO: line width of 0 is not allowed when MSAA enabled */ - if (line_width_u3_7 == 0) - line_width_u3_7 = 1; + + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { + if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1 ) +line_width_u3_7 = 0; + } else { + if (line_width_u3_7 = 0) + line_width_u3_7 = 1; + } + dw2 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; } if (ctx->Line.SmoothFlag) { -- 1.7.9.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN7
Hi, On 17 March 2015 at 16:37, wrote: > --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c > +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c > @@ -198,9 +198,15 @@ upload_sf_state(struct brw_context *brw) >float line_width = > roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); >uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > - /* TODO: line width of 0 is not allowed when MSAA enabled */ > - if (line_width_u3_7 == 0) > - line_width_u3_7 = 1; > + > + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { > + if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1 ) > +line_width_u3_7 = 0; > + } else { > + if (line_width_u3_7 = 0) > + line_width_u3_7 = 1; You almost certainly meant 'if (line_width_u3 == 0)', rather than an assignment - surprised the compiler didn't throw a warning here. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines.
Can you provide the output of git var -l And the headers of the patch file you're sending (or git show --format=raw for the commit in question if you're not using patch files as intermediates) I suspect that GIT_AUTHOR_IDENT will differ from whatever's in the "From:" header of the patch being sent. On Tue, Mar 17, 2015 at 12:36 PM, Predut, Marius wrote: > -Original Message- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of > Ilia Mirkin > Sent: Wednesday, March 11, 2015 11:09 PM > To: Predut, Marius > Cc: mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for > thinnest width lines. > > On Wed, Mar 11, 2015 at 5:57 PM, wrote: >> From: Marius Predut > > Set your email from name correctly in git and then you won't have this line > in your git send-email results. > > I have this config : > email = marius.pre...@intel.com > name = Marius Predut > > but still the first line appear > >> >> On SNB and IVB hw, for 1 pixel line thickness or less, the general >> anti-aliasing algorithm give up - garbage line is generated. >> Setting a Line Width of 0.0 specifies the rasterization of the >> “thinnest” (one-pixel-wide), non-antialiased lines. >> Lines rendered with zero Line Width are rasterized using Grid >> Intersection Quantization rules as specified by bspec section 6.3.12.1 >> Zero-Width (Cosmetic) Line Rasterization. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668 > > This seems like the wrong bug reference... > >> Signed-off-by: Marius Predut >> --- >> src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c >> b/src/mesa/drivers/dri/i965/gen6_sf_state.c >> index f9d8d27..1bed444 100644 >> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c >> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c >> @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw) >>float line_width = >> roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); >>uint32_t line_width_u3_7 = U_FIXED(line_width, 7); >> - /* TODO: line width of 0 is not allowed when MSAA enabled */ >> - if (line_width_u3_7 == 0) >> - line_width_u3_7 = 1; >> + >> + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { >> +if (ctx->Line.SmoothFlag && ctx->Line.Width <=1) >> + line_width_u3_7 = 0; >> + } else { >> +if (line_width_u3_7 == 0) >> +line_width_u3_7 = 1; >> + } >> + >>dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; >> } >> if (ctx->Line.SmoothFlag) { >> -- >> 1.7.9.5 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines.
-Original Message- From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of Ilia Mirkin Sent: Wednesday, March 11, 2015 11:09 PM To: Predut, Marius Cc: mesa-dev@lists.freedesktop.org Subject: Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines. On Wed, Mar 11, 2015 at 5:57 PM, wrote: > From: Marius Predut Set your email from name correctly in git and then you won't have this line in your git send-email results. I have this config : email = marius.pre...@intel.com name = Marius Predut but still the first line appear > > On SNB and IVB hw, for 1 pixel line thickness or less, the general > anti-aliasing algorithm give up - garbage line is generated. > Setting a Line Width of 0.0 specifies the rasterization of the > “thinnest” (one-pixel-wide), non-antialiased lines. > Lines rendered with zero Line Width are rasterized using Grid > Intersection Quantization rules as specified by bspec section 6.3.12.1 > Zero-Width (Cosmetic) Line Rasterization. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668 This seems like the wrong bug reference... > Signed-off-by: Marius Predut > --- > src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c > b/src/mesa/drivers/dri/i965/gen6_sf_state.c > index f9d8d27..1bed444 100644 > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c > @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw) >float line_width = > roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); >uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > - /* TODO: line width of 0 is not allowed when MSAA enabled */ > - if (line_width_u3_7 == 0) > - line_width_u3_7 = 1; > + > + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { > +if (ctx->Line.SmoothFlag && ctx->Line.Width <=1) > + line_width_u3_7 = 0; > + } else { > +if (line_width_u3_7 == 0) > +line_width_u3_7 = 1; > + } > + >dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; > } > if (ctx->Line.SmoothFlag) { > -- > 1.7.9.5 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radeonsi: increase coords array for radeon_llvm_emit_prepare_cube_coords
From: Marek Olšák radeon_llvm_emit_prepare_cube_coords uses coords[4] in some cases (TXB2 etc.) Discovered by Coverity. Reported by Ilia Mirkin. Cc: --- src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- src/gallium/drivers/radeonsi/si_shader.c| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c index d89e2b4..1690194 100644 --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c @@ -748,7 +748,7 @@ static void txp_fetch_args( const struct tgsi_full_instruction * inst = emit_data->inst; LLVMValueRef src_w; unsigned chan; - LLVMValueRef coords[4]; + LLVMValueRef coords[5]; emit_data->dst_type = LLVMVectorType(bld_base->base.elem_type, 4); src_w = lp_build_emit_fetch(bld_base, emit_data->inst, 0, TGSI_CHAN_W); diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c index de889ed..4dcf756 100644 --- a/src/gallium/drivers/radeonsi/si_shader.c +++ b/src/gallium/drivers/radeonsi/si_shader.c @@ -1572,7 +1572,7 @@ static void tex_fetch_args( const struct tgsi_full_instruction * inst = emit_data->inst; unsigned opcode = inst->Instruction.Opcode; unsigned target = inst->Texture.Texture; - LLVMValueRef coords[4]; + LLVMValueRef coords[5]; LLVMValueRef address[16]; int ref_pos; unsigned num_coords = tgsi_util_get_texture_coord_dim(target, &ref_pos); -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] Revert "main: _mesa_cube_level_complete checks NumLayers."
Reviewed-by: Jason Ekstrand On Thu, Mar 12, 2015 at 12:33 PM, Laura Ekstrand wrote: > This reverts commit 1ee000a0b6737d6c140d4f07b6044908b8ebfdc7. > Failures with the GLES3 conformance suite and Synmark2 OGLHdrBloom revealed > that this commit might be in error. A look at the offended test in GLES3 > conformance suite, NPOT gen mipmap, suggests that NumLayers may not actually > always be 6 for a cube complete texture with target = GL_TEXTURE_CUBE_MAP. > > Extensive testing with Piglit prior to patch review and upstreaming did not > reveal this problem because, in the few Piglit tests that test for cube > completeness, NumLayers = 6. So it appears that perhaps existing test > coverage in Piglit is inadequate. > --- > src/mesa/main/texobj.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/src/mesa/main/texobj.c b/src/mesa/main/texobj.c > index a99b108..e018ab9 100644 > --- a/src/mesa/main/texobj.c > +++ b/src/mesa/main/texobj.c > @@ -879,10 +879,6 @@ _mesa_cube_level_complete(const struct gl_texture_object > *texObj, > if (texObj->Target != GL_TEXTURE_CUBE_MAP) >return GL_FALSE; > > - /* Make sure we have enough image planes for a cube map. */ > - if (texObj->NumLayers < 6) > - return GL_FALSE; > - > if ((level < 0) || (level >= MAX_TEXTURE_LEVELS)) >return GL_FALSE; > > -- > 2.1.0 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3.5/5] i965/fs: Add unit tests for "Handle CMP.nz ... 0 and AND.nz ... 1 similarly in cmod propagation"
From: Ian Romanick This change should get squashed with the afore mentioned change. Tests suggested by Matt. Signed-off-by: Ian Romanick Cc: Matt Turner --- .../drivers/dri/i965/test_fs_cmod_propagation.cpp | 105 + 1 files changed, 105 insertions(+), 0 deletions(-) diff --git a/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp b/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp index cb92abf..8c6ff09 100644 --- a/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp +++ b/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp @@ -449,3 +449,108 @@ TEST_F(cmod_propagation_test, different_types_cmod_with_zero) EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 1)->opcode); EXPECT_EQ(BRW_CONDITIONAL_GE, instruction(block0, 1)->conditional_mod); } + +TEST_F(cmod_propagation_test, andnz_one) +{ + fs_reg dest = v->vgrf(glsl_type::int_type); + fs_reg src0 = v->vgrf(glsl_type::float_type); + fs_reg zero(0.0f); + fs_reg one(int(1)); + + v->emit(BRW_OPCODE_CMP, retype(dest, BRW_REGISTER_TYPE_F), src0, zero) + ->conditional_mod = BRW_CONDITIONAL_L; + v->emit(BRW_OPCODE_AND, v->reg_null_f, dest, one) + ->conditional_mod = BRW_CONDITIONAL_NZ; + + /* = Before = +* 0: cmp.l.f0(8) dest:D src0:F 0F +* 1: and.nz.f0(8)null:D dest:D 1D +* +* = After = +* 0: cmp.l.f0(8) dest:D src0:F 0F +*/ + + v->calculate_cfg(); + bblock_t *block0 = v->cfg->blocks[0]; + + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(1, block0->end_ip); + + EXPECT_TRUE(cmod_propagation(v)); + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(0, block0->end_ip); + EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 0)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_L, instruction(block0, 0)->conditional_mod); + EXPECT_TRUE(retype(dest, BRW_REGISTER_TYPE_F) + .equals(instruction(block0, 0)->dst)); +} + +TEST_F(cmod_propagation_test, andnz_non_one) +{ + fs_reg dest = v->vgrf(glsl_type::int_type); + fs_reg src0 = v->vgrf(glsl_type::float_type); + fs_reg zero(0.0f); + fs_reg nonone(int(38)); + + v->emit(BRW_OPCODE_CMP, retype(dest, BRW_REGISTER_TYPE_F), src0, zero) + ->conditional_mod = BRW_CONDITIONAL_L; + v->emit(BRW_OPCODE_AND, v->reg_null_f, dest, nonone) + ->conditional_mod = BRW_CONDITIONAL_NZ; + + /* = Before = +* 0: cmp.l.f0(8) dest:D src0:F 0F +* 1: and.nz.f0(8)null:D dest:D 1D +* +* = After = +* (no changes) +*/ + + v->calculate_cfg(); + bblock_t *block0 = v->cfg->blocks[0]; + + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(1, block0->end_ip); + + EXPECT_FALSE(cmod_propagation(v)); + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(1, block0->end_ip); + EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 0)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_L, instruction(block0, 0)->conditional_mod); + EXPECT_EQ(BRW_OPCODE_AND, instruction(block0, 1)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_NZ, instruction(block0, 1)->conditional_mod); +} + +TEST_F(cmod_propagation_test, andz_one) +{ + fs_reg dest = v->vgrf(glsl_type::int_type); + fs_reg src0 = v->vgrf(glsl_type::float_type); + fs_reg zero(0.0f); + fs_reg one(int(1)); + + v->emit(BRW_OPCODE_CMP, retype(dest, BRW_REGISTER_TYPE_F), src0, zero) + ->conditional_mod = BRW_CONDITIONAL_L; + v->emit(BRW_OPCODE_AND, v->reg_null_f, dest, one) + ->conditional_mod = BRW_CONDITIONAL_Z; + + /* = Before = +* 0: cmp.l.f0(8) dest:D src0:F 0F +* 1: and.z.f0(8) null:D dest:D 1D +* +* = After = +* (no changes) +*/ + + v->calculate_cfg(); + bblock_t *block0 = v->cfg->blocks[0]; + + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(1, block0->end_ip); + + EXPECT_FALSE(cmod_propagation(v)); + EXPECT_EQ(0, block0->start_ip); + EXPECT_EQ(1, block0->end_ip); + EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 0)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_L, instruction(block0, 0)->conditional_mod); + EXPECT_EQ(BRW_OPCODE_AND, instruction(block0, 1)->opcode); + EXPECT_EQ(BRW_CONDITIONAL_EQ, instruction(block0, 1)->conditional_mod); +} -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6
On SNB and IVB hw, for 1 pixel line thickness or less, the general anti-aliasing algorithm give up - garbage line is generated. Setting a Line Width of 0.0 specifies the rasterization of the “thinnest” (one-pixel-wide), non-antialiased lines. Lines rendered with zero Line Width are rasterized using Grid Intersection Quantization rules as specified by bspec section 6.3.12.1 Zero-Width (Cosmetic) Line Rasterization. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832 Signed-off-by: Marius Predut --- src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/dri/i965/gen6_sf_state.c index f9d8d27..1bed444 100644 --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw) float line_width = roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); uint32_t line_width_u3_7 = U_FIXED(line_width, 7); - /* TODO: line width of 0 is not allowed when MSAA enabled */ - if (line_width_u3_7 == 0) - line_width_u3_7 = 1; + + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { +if (ctx->Line.SmoothFlag && ctx->Line.Width <=1) + line_width_u3_7 = 0; + } else { +if (line_width_u3_7 == 0) +line_width_u3_7 = 1; + } + dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; } if (ctx->Line.SmoothFlag) { -- 1.7.9.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN7
On SNB and IVB hw, for 1 pixel line thickness or less, the general anti-aliasing algorithm give up - garbage line is generated. Setting a Line Width of 0.0 specifies the rasterization of the “thinnest” (one-pixel-wide), non-antialiased lines. Lines rendered with zero Line Width are rasterized using Grid Intersection Quantization rules as specified by bspec section 6.3.12.1 Zero-Width (Cosmetic) Line Rasterization. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832 Signed-off-by: Marius Predut Signed-off-by: Marius Predut --- src/mesa/drivers/dri/i965/gen7_sf_state.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c b/src/mesa/drivers/dri/i965/gen7_sf_state.c index c9815b0..38b4f2f 100644 --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c @@ -198,9 +198,15 @@ upload_sf_state(struct brw_context *brw) float line_width = roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); uint32_t line_width_u3_7 = U_FIXED(line_width, 7); - /* TODO: line width of 0 is not allowed when MSAA enabled */ - if (line_width_u3_7 == 0) - line_width_u3_7 = 1; + + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { + if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1) +line_width_u3_7 = 0; + } else { + if (line_width_u3_7 == 0) + line_width_u3_7 = 1; + } + dw2 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; } if (ctx->Line.SmoothFlag) { -- 1.7.9.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/4] i965/state: Create separate dirty state bits for each pipeline
On Tuesday, March 17, 2015 08:20:23 AM Kristian Høgsberg wrote: > On Tue, Mar 17, 2015 at 2:22 AM, Kenneth Graunke > wrote: > > On Monday, March 16, 2015 10:21:31 PM Kristian Høgsberg wrote: > >> On Wed, Mar 11, 2015 at 11:53 AM, Jordan Justen > >> wrote: [snip] > >> > @@ -691,8 +702,23 @@ void brw_upload_render_state(struct brw_context > >> > *brw) > >> > fprintf(stderr, "\n"); > >> >} > >> > } > >> > + > >> > + /* Save all dirty state into the other pipelines */ > >> > + for (int i = 0; i < BRW_NUM_PIPELINES; i++) { > >> > + if (i != pipeline) { > >> > + brw->state.pipelines[i].mesa |= state->mesa; > >> > + brw->state.pipelines[i].brw |= state->brw; > >> > + } > >> > + } > >> > >> When we merge this in at this point, state->mesa/brw includes dirty > >> bits from the active pipelines pipeline flags. For example, suppose we > >> do a bunch of render pipeline stuff, which queues up a lot of dirty > >> flags for the compute pipeline. Then when we go to emit state for the > >> compute pipeline we first merge all these dirty flags into > >> brw->state.dirty, then merge that into the render pipeline flags. We > >> only need to merge the new dirty bits, that is, the initial value of > >> brw->state.dirty into the render pipeline flags. If we don't modify > >> brw->state.dirty in this function and merge the flags in clear as > >> described above, this problem is easy to fix. > >> > >> Kristian OK, I see what you mean now - very good catch. 1. We do a render operation. Any new dirty bits get saved in brw->state.pipelines[COMPUTE], so they'll happen when we do the next compute workload. This completes successfully (ignore retries). 2. We do a compute operation. brw->state.pipelines[COMPUTE] gets merged into brw->state.dirty. This includes bits that happened during #1 (and were missed on the compute pipe). This all works fine. At the end, we save brw->state.dirty into brw->state.pipelines[RENDER]. 3. We do another render operation. brw->state.pipelines[RENDER] gets merged into brw->state.dirty. We now have bits from #1 again, which are not necessary. So yeah. That's bad. Sorry, I thought this was to handle the hypothetical case of pipe-switch-during-retries. But it's just switching pipelines at all! If I understand your suggestion correctly, you mean that a) We should eliminate brw->state.dirty. b) brw_upload_pipeline_state() should do: struct brw_state_flags *pipeline_state = &brw->state.pipelines[pipeline]; /* Create a local copy of the dirty state */ struct brw_state_flags dirty; dirty.mesa = brw->NewGLState | pipeline_state->mesa; dirty.brw = ctx->NewDriverState | pipeline_state->brw; Use this for uploads. c) The existing brw_clear_dirty_bits() should do: /* Save the dirty flags into the other pipelines */ for (int i = 0; i < BRW_NUM_PIPELINES; i++) { if (i != pipeline) { brw->state.pipelines[i].mesa |= brw->NewGLState; brw->state.pipelines[i].brw |= ctx->NewDriverState; } } /* We've handled any bits passed to us by other pipeline uploads */ brw->state.pipelines[pipe] = 0; /* We've handled the flags passed to us by Mesa */ brw->NewGLState = 0; ctx->NewDriverState = 0; Does that sound like a correct interpretation? This does seem to solve the problem, and I think it's a bit cleaner too. Re-evaluating my example to verify that this approach works... 1. We do a render operation. The local dirty flags are | . Nothing's saved yet, so just When done, we save to pipelines[COMPUTE]. 2. We do a compute operation. The local dirty flags are | . We use those for upload. When done, brw_clear_dirty_bits() saves only to pipelines[COMPUTE]. Notably, the bits from op #1 are /not/ saved. 3. We do a render operation. We get any new bits from Mesa, plus the bits during #2. No bits from #1. So...you're right - there's definitely a problem, and your suggestion totally fixes it. I just had to work through an example to see what you meant. Thanks again! Jordan, does this make sense to you? signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Fwd: [PATCH 12/16] main: Added entry point for glCreateProgramPipelines
-- Forwarded message -- From: Laura Ekstrand Date: Thu, Feb 26, 2015 at 3:43 PM Subject: Re: [Mesa-dev] [PATCH 12/16] main: Added entry point for glCreateProgramPipelines To: Martin Peres On Mon, Feb 16, 2015 at 6:14 AM, Martin Peres wrote: > Signed-off-by: Martin Peres > --- > src/mapi/glapi/gen/ARB_direct_state_access.xml | 7 ++ > src/mesa/main/pipelineobj.c| 35 > +- > src/mesa/main/pipelineobj.h| 3 +++ > src/mesa/main/tests/dispatch_sanity.cpp| 1 + > 4 files changed, 40 insertions(+), 6 deletions(-) > > diff --git a/src/mapi/glapi/gen/ARB_direct_state_access.xml > b/src/mapi/glapi/gen/ARB_direct_state_access.xml > index 99d2422..2102e82 100644 > --- a/src/mapi/glapi/gen/ARB_direct_state_access.xml > +++ b/src/mapi/glapi/gen/ARB_direct_state_access.xml > @@ -308,6 +308,13 @@ > > > > + > + > + > + > + > + > + > > > > diff --git a/src/mesa/main/pipelineobj.c b/src/mesa/main/pipelineobj.c > index b713d95..96bf086 100644 > --- a/src/mesa/main/pipelineobj.c > +++ b/src/mesa/main/pipelineobj.c > @@ -498,16 +498,18 @@ _mesa_DeleteProgramPipelines(GLsizei n, const GLuint > *pipelines) > * \param n Number of IDs to generate. > * \param pipelines pipeline of \c n locations to store the IDs. > */ > -void GLAPIENTRY > -_mesa_GenProgramPipelines(GLsizei n, GLuint *pipelines) > +static void > +create_program_pipelines(struct gl_context *ctx, GLsizei n, GLuint > *pipelines, > + bool dsa) > { > - GET_CURRENT_CONTEXT(ctx); > - > + const char *func; > GLuint first; > GLint i; > > + func = dsa ? "glCreateProgramPipelines" : "glGenProgramPipelines"; > + > if (n < 0) { > - _mesa_error(ctx, GL_INVALID_VALUE, "glGenProgramPipelines(n<0)"); > + _mesa_error(ctx, GL_INVALID_VALUE, "%s (n<0)", func); > Minor nit: There's an extra space here ^ >return; > } > > @@ -523,16 +525,37 @@ _mesa_GenProgramPipelines(GLsizei n, GLuint > *pipelines) > >obj = _mesa_new_pipeline_object(ctx, name); >if (!obj) { > - _mesa_error(ctx, GL_OUT_OF_MEMORY, "glGenProgramPipelines"); > + _mesa_error(ctx, GL_OUT_OF_MEMORY, "%s", func); > return; >} > > + if (dsa) { > + /* make dsa-allocated objects behave like program objects */ > + obj->EverBound = GL_TRUE; > + } > + >save_pipeline_object(ctx, obj); >pipelines[i] = first + i; > } > > } > > +void GLAPIENTRY > +_mesa_GenProgramPipelines(GLsizei n, GLuint *pipelines) > +{ > + GET_CURRENT_CONTEXT(ctx); > + > + create_program_pipelines(ctx, n, pipelines, false); > +} > + > +void GLAPIENTRY > +_mesa_CreateProgramPipelines(GLsizei n, GLuint *pipelines) > +{ > + GET_CURRENT_CONTEXT(ctx); > + > + create_program_pipelines(ctx, n, pipelines, true); > +} > + > /** > * Determine if ID is the name of an pipeline object. > * > diff --git a/src/mesa/main/pipelineobj.h b/src/mesa/main/pipelineobj.h > index 7285a78..b57bcb9 100644 > --- a/src/mesa/main/pipelineobj.h > +++ b/src/mesa/main/pipelineobj.h > @@ -82,6 +82,9 @@ _mesa_DeleteProgramPipelines(GLsizei n, const GLuint > *pipelines); > extern void GLAPIENTRY > _mesa_GenProgramPipelines(GLsizei n, GLuint *pipelines); > > +void GLAPIENTRY > +_mesa_CreateProgramPipelines(GLsizei n, GLuint *pipelines); > + > extern GLboolean GLAPIENTRY > _mesa_IsProgramPipeline(GLuint pipeline); > > diff --git a/src/mesa/main/tests/dispatch_sanity.cpp > b/src/mesa/main/tests/dispatch_sanity.cpp > index b65080e..cc2b267 100644 > --- a/src/mesa/main/tests/dispatch_sanity.cpp > +++ b/src/mesa/main/tests/dispatch_sanity.cpp > @@ -993,6 +993,7 @@ const struct function gl_core_functions_possible[] = { > { "glTextureStorage2DMultisample", 45, -1 }, > { "glTextureStorage3DMultisample", 45, -1 }, > { "glTextureBuffer", 45, -1 }, > + { "glCreateProgramPipelines", 45, -1 }, > { "glCreateQueries", 45, -1 }, > { "glGetQueryBufferObjectiv", 45, -1 }, > { "glGetQueryBufferObjectuiv", 45, -1 }, > -- > 2.3.0 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > With that fixed, Reviewed-by: Laura Ekstrand ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines.
> -Original Message- > From: ibmir...@gmail.com [mailto:ibmir...@gmail.com] On Behalf Of Ilia Mirkin > Sent: Tuesday, March 17, 2015 6:48 PM > To: Predut, Marius > Cc: mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for > thinnest width lines. > > Can you provide the output of > > git var -l marius@marius-pc:~/mesa/src$ git var -l user.email=Marius Predut user.email=marius.pre...@intel.com user.name=Marius Predut color.ui=auto sendemail.smtpserver=smtp.intel.com sendemail.signedoffbycc=no sendemail.from=marius.pre...@intel.com sendemail.to=marius.pre...@intel.com sendemail.chainreplyto=false sendemail.review_mesa.email=Marius Predut sendemail.review_mesa.name=Marius Predut sendemail.review_mesa.smtpserver=smtp.intel.com sendemail.review_mesa.signedoffbycc=no sendemail.review_mesa.to=mesa-dev@lists.freedesktop.org sendemail.review_mesa.chainreplyto=false core.repositoryformatversion=0 core.filemode=true core.bare=false core.logallrefupdates=true remote.origin.fetch=+refs/heads/*:refs/remotes/origin/* remote.origin.url=git://anongit.freedesktop.org/git/mesa/mesa branch.master.remote=origin branch.master.merge=refs/heads/master GIT_COMMITTER_IDENT=Marius Predut 1426618655 +0200 GIT_AUTHOR_IDENT=Marius Predut 1426618655 +0200 GIT_EDITOR=editor GIT_PAGER=pager > > And the headers of the patch file you're sending (or git show > --format=raw for the commit in question if you're not using patch > files as intermediates) From b84cf899ddf3b8b93b251ffcf9a082cbfe372f18 Mon Sep 17 00:00:00 2001 From: Marius Predut Date: Tue, 17 Mar 2015 19:33:21 +0200 Subject: [Mesa-dev][PATCH v1] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On SNB and IVB hw, for 1 pixel line thickness or less, > > I suspect that GIT_AUTHOR_IDENT will differ from whatever's in the > "From:" header of the patch being sent. > I fix this trouble by using --from "Marius Predut " And without space between name and the character "<"(if not it will not work :-)) > > On Tue, Mar 17, 2015 at 12:36 PM, Predut, Marius > wrote: > > -Original Message- > > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On Behalf Of > Ilia Mirkin > > Sent: Wednesday, March 11, 2015 11:09 PM > > To: Predut, Marius > > Cc: mesa-dev@lists.freedesktop.org > > Subject: Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for > thinnest width lines. > > > > On Wed, Mar 11, 2015 at 5:57 PM, wrote: > >> From: Marius Predut > > > > Set your email from name correctly in git and then you won't have this line > in your git send-email results. > > > > I have this config : > > email = marius.pre...@intel.com > > name = Marius Predut > > > > but still the first line appear > > > >> > >> On SNB and IVB hw, for 1 pixel line thickness or less, the general > >> anti-aliasing algorithm give up - garbage line is generated. > >> Setting a Line Width of 0.0 specifies the rasterization of the > >> “thinnest” (one-pixel-wide), non-antialiased lines. > >> Lines rendered with zero Line Width are rasterized using Grid > >> Intersection Quantization rules as specified by bspec section 6.3.12.1 > >> Zero-Width (Cosmetic) Line Rasterization. > >> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82668 > > > > This seems like the wrong bug reference... > > > >> Signed-off-by: Marius Predut > >> --- > >> src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +--- > >> 1 file changed, 9 insertions(+), 3 deletions(-) > >> > >> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c > >> b/src/mesa/drivers/dri/i965/gen6_sf_state.c > >> index f9d8d27..1bed444 100644 > >> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c > >> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c > >> @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw) > >>float line_width = > >> roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); > >>uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > >> - /* TODO: line width of 0 is not allowed when MSAA enabled */ > >> - if (line_width_u3_7 == 0) > >> - line_width_u3_7 = 1; > >> + > >> + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { > >> +if (ctx->Line.SmoothFlag && ctx->Line.Width <=1) > >> + line_width_u3_7 = 0; > >> + } else { > >> +if (line_width_u3_7 == 0) > >> +line_width_u3_7 = 1; > >> + } > >> + > >>dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; > >> } > >> if (ctx->Line.SmoothFlag) { > >> -- > >> 1.7.9.5 > >> > >> ___ > >> mesa-dev mailing list > >> mesa-dev@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > h
Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for thinnest width lines.
On Tue, Mar 17, 2015 at 2:11 PM, Predut, Marius wrote: >> -Original Message- >> From: ibmir...@gmail.com [mailto:ibmir...@gmail.com] On Behalf Of Ilia Mirkin >> Sent: Tuesday, March 17, 2015 6:48 PM >> To: Predut, Marius >> Cc: mesa-dev@lists.freedesktop.org >> Subject: Re: [Mesa-dev] [PATCH ] i965/aa: fixing anti-aliasing bug for >> thinnest width lines. >> >> Can you provide the output of >> >> git var -l > marius@marius-pc:~/mesa/src$ git var -l > user.email=Marius Predut > user.email=marius.pre...@intel.com > user.name=Marius Predut > color.ui=auto > sendemail.smtpserver=smtp.intel.com > sendemail.signedoffbycc=no > sendemail.from=marius.pre...@intel.com > sendemail.to=marius.pre...@intel.com These are the problem. Remove the .from setting. It's overriding the defaults. Or set it to the correct thing, i.e. "Marius Predut " >> >> I suspect that GIT_AUTHOR_IDENT will differ from whatever's in the >> "From:" header of the patch being sent. >> > > I fix this trouble by using --from "Marius Predut " > And without space between name and the character "<"(if not it will not work > :-)) That's not a great thing to do... just remove sendmail.from and it should all work. [And yeah, space between name and < required by RFC822 as I recall... been a while since I've read over it though.] -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Statically linking libstdc++ and libgcc
On 16/03/15 23:44, Ian Romanick wrote: > On 03/13/2015 02:32 PM, Emil Velikov wrote: >> * Allow people to static link against libgcc/libstdc++. >> >> Imho this should be option, disabled by default provided at configure >> time. This way builders/distributions can op-in if they choose to do so. > > I'm very strongly opposed to this. > > We already have too many build ABI options. Each and every one of them > has caused some level of support pain (TLS vs. non-TLS, shared glapi vs > non-shared, etc.). I have absolutely no interest in seeing another knob > added. > Perhaps I'm missing something but how is this yet another ABI ? I'm working on nuking the static glapi, although got distracted by other things *cough*this thread*cough*. As one of the few people working on the build system (props for Matt for helping imensely), I am not at _all_ happy with us having so many configure switches. Although, as you've mentioned before (in a slightly different wording) this is an "evil" solution, but it's a _necessary_ "evil". Clearly people are unhappy if we make this the default for obvious reasons, plus it has a change of breaking someone's workflow. So another configure which it is. If your concern is about supporting such setup, that decision is for you guys to make. Plus the check for static libstdc++/gcc_s is quite trivial so it won't increase the time to triage bugs. Cheers, Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6
On 03/17/2015 11:29 AM, Marius Predut wrote: > On SNB and IVB hw, for 1 pixel line thickness or less, the general > anti-aliasing algorithm give up - garbage line is generated. > Setting a Line Width of 0.0 specifies the rasterization of the “thinnest” > (one-pixel-wide), non-antialiased lines. > Lines rendered with zero Line Width are rasterized using Grid Intersection > Quantization rules as specified by bspec section 6.3.12.1 Zero-Width > (Cosmetic) Line Rasterization. You'll need to re-word wrap the commit message. You'll get the commit message right one of these days. :) Also... when you send out new versions of a patch, please change the patch subject to be something like "[PATCH v3] ...". This makes it easier for people to know which is the latest version to review. You should also add notes to the commit message that explain what changed from version to version. For example, this commit message should have something like: v3: Fix = used instead of == in an if-statement. Noticed by Daniel Stone. This is helps people know that their review comments have been applied. It is also important to do this when the review changes are applied and the patch committed without re-sending to the list. Maintaining history like this in commit messages helps us understand code in the future. > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832 There are a number of bugs that have been closed as duplicates of this bug. Two of these, bug #27007 and bug #60797, have test cases. Does this fix also fix those? I also have a more general question: How are you testing this? Daniel noticed a bug in an earlier version of the patch that piglit should have caught. If you're doing a full piglit run and that didn't catch the previous assignment-instead-of-comparison bug, it would be helpful if you could craft a test that would detect that bug. That may be difficult, so don't spend a huge amount of time on it. > Signed-off-by: Marius Predut > --- > src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c > b/src/mesa/drivers/dri/i965/gen6_sf_state.c > index f9d8d27..1bed444 100644 > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c > @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw) >float line_width = > roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); >uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > - /* TODO: line width of 0 is not allowed when MSAA enabled */ > - if (line_width_u3_7 == 0) > - line_width_u3_7 = 1; > + > + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { In Mesa there are often Enabled and _Enabled fields. The Enabled field is the setting made by the application via the OpenGL API. The _Enabled field means that feature in question is actually enabled, and this may depend on other state. In this case, the application may enable multisampling, but multisampling may not occur of there is not a multisample buffer. This means gl_multisample_attrib::Enabled would be set but gl_multisample_attrib::_Enabled would not. Instead of open-coding that check, just check gl_multisample_attrib::_Enabled directly: if (!ctx->Multisample._Enabled) { I actually think it's more clear if you leave the original comment and implement this as: /* Line width of 0 is not allowed when MSAA enabled */ if (ctx->Multisample._Enabled) { if (line_width_u3_7 == 0) line_width_u3_7 = 1; } else if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1.0) { /* For lines less than 1 pixel thick, the general * anti-aliasing algorithm gives up, and a garbage line is * generated. Setting a Line Width of 0.0 specifies the * rasterization of the "thinnest" (one-pixel-wide), * non-antialiased lines. * * Lines rendered with zero Line Width are rasterized using * Grid Intersection Quantization rules as specified by * bspec section 6.3.12.1 Zero-Width (Cosmetic) Line * Rasterization. */ line_width_u3_7 = 0; } I reworded the comment a bit (from the commit message), and I changed the Line.Width comparison to compare with 1.0. One final question. Does it produce better results to use 0 or 1.0? It sounds like using 1.0 would still enable line antialiasing, and the resulting line shouldn't be appreciably thicker. > +if (ctx->Line.SmoothFlag && ctx->Line.Width <=1) > + line_width_u3_7 = 0; > + } else { > +if (line_width_u3_7 == 0) > +line_width_u3_7 = 1; > + } > + >dw3 |= line_width_u3_7 << GEN6_SF_LINE_WIDTH_SHIFT; > } > if (ctx->Line.SmoothFlag) { ___ mesa-dev mailing list mesa-dev@lists.freed
Re: [Mesa-dev] [PATCH 3.5/5] i965/fs: Add unit tests for "Handle CMP.nz ... 0 and AND.nz ... 1 similarly in cmod propagation"
On Tue, Mar 17, 2015 at 10:30 AM, Ian Romanick wrote: > From: Ian Romanick > > This change should get squashed with the afore mentioned change. Tests > suggested by Matt. > > Signed-off-by: Ian Romanick > Cc: Matt Turner > --- > .../drivers/dri/i965/test_fs_cmod_propagation.cpp | 105 > + > 1 files changed, 105 insertions(+), 0 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp > b/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp > index cb92abf..8c6ff09 100644 > --- a/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp > +++ b/src/mesa/drivers/dri/i965/test_fs_cmod_propagation.cpp > @@ -449,3 +449,108 @@ TEST_F(cmod_propagation_test, > different_types_cmod_with_zero) > EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 1)->opcode); > EXPECT_EQ(BRW_CONDITIONAL_GE, instruction(block0, 1)->conditional_mod); > } > + > +TEST_F(cmod_propagation_test, andnz_one) > +{ > + fs_reg dest = v->vgrf(glsl_type::int_type); > + fs_reg src0 = v->vgrf(glsl_type::float_type); > + fs_reg zero(0.0f); > + fs_reg one(int(1)); > + > + v->emit(BRW_OPCODE_CMP, retype(dest, BRW_REGISTER_TYPE_F), src0, zero) > + ->conditional_mod = BRW_CONDITIONAL_L; > + v->emit(BRW_OPCODE_AND, v->reg_null_f, dest, one) > + ->conditional_mod = BRW_CONDITIONAL_NZ; > + > + /* = Before = > +* 0: cmp.l.f0(8) dest:D src0:F 0F We won't be emitting compares with different source and destination types since commit 6b3a301f611c9aabc090522951eda589e8302562 Author: Matt Turner Date: Wed Jan 7 11:52:05 2015 -0800 i965: Set CMP's destination type to src0's type. so let's change this comment (I see that the actual code is already retype()ing the dest). Same applies to the two below. > +* 1: and.nz.f0(8)null:D dest:D 1D > +* > +* = After = > +* 0: cmp.l.f0(8) dest:D src0:F 0F and here. > +*/ > + > + v->calculate_cfg(); > + bblock_t *block0 = v->cfg->blocks[0]; > + > + EXPECT_EQ(0, block0->start_ip); > + EXPECT_EQ(1, block0->end_ip); > + > + EXPECT_TRUE(cmod_propagation(v)); > + EXPECT_EQ(0, block0->start_ip); > + EXPECT_EQ(0, block0->end_ip); > + EXPECT_EQ(BRW_OPCODE_CMP, instruction(block0, 0)->opcode); > + EXPECT_EQ(BRW_CONDITIONAL_L, instruction(block0, 0)->conditional_mod); > + EXPECT_TRUE(retype(dest, BRW_REGISTER_TYPE_F) > + .equals(instruction(block0, 0)->dst)); > +} > + > +TEST_F(cmod_propagation_test, andnz_non_one) > +{ > + fs_reg dest = v->vgrf(glsl_type::int_type); > + fs_reg src0 = v->vgrf(glsl_type::float_type); > + fs_reg zero(0.0f); > + fs_reg nonone(int(38)); Can just pass 38 here if you want. > + > + v->emit(BRW_OPCODE_CMP, retype(dest, BRW_REGISTER_TYPE_F), src0, zero) > + ->conditional_mod = BRW_CONDITIONAL_L; > + v->emit(BRW_OPCODE_AND, v->reg_null_f, dest, nonone) > + ->conditional_mod = BRW_CONDITIONAL_NZ; > + > + /* = Before = > +* 0: cmp.l.f0(8) dest:D src0:F 0F > +* 1: and.nz.f0(8)null:D dest:D 1D s/1D/38D/ with the comments fixed, Reviewed-by: Matt Turner Thanks Ian! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Spec bug? Transform feedback after tessellation control shader
Hi, The GL 4.5 Core spec is inconsistent. The "11.1.2.1 Output Variables" section says that transform feedback is allowed after a tessellation control shader if a tessellation evaluation shader isn't present: "Each program object can specify a set of output variables from one shader to be recorded in transform feedback mode (see section 13.2). The variables that can be recorded are those emitted by the first active shader, in order, from the following list: * geometry shader * tessellation evaluation shader * tessellation control shader * vertex shader" Even glTransformFeedbackVaryings allows it: "If the set of output variables to record in transform feedback mode is specified by TransformFeedbackVaryings, a program will fail to link if: * the count specified by TransformFeedbackVaryings is non-zero, but the program object has no vertex, tessellation control, tessellation evaluation, or geometry shader;" However, the "13.2 Transform Feedback" section doesn't mention any tessellation control shader: "The data captured in transform feedback mode depends on the active programs on each of the shader stages. If a program is active for the geometry shader stage, transform feedback captures the vertices of each primitive emitted by the geometry shader. Otherwise, if a program is active for the tessellation evaluation shader stage, transform feedback captures each primitive produced by the tessellation primitive generator, whose vertices are processed by the tessellation evaluation shader. Otherwise, transform feedback captures each primitive processed by the vertex shader." Also, glBeginTransformFeedback doesn't allow GL_PATCHES, which is the only output primitive type of the tessellation control shader: "primitiveMode must be TRIANGLES, LINES, or POINTS, and specifies the output type of primitives that will be recorded into the buffer objects bound for transform feedback" The question is: Is transform feedback really allowed after the tessellation control shader? I hope not, because our hardware can't do it. Thanks, Marek PS: The spec mentions "tesellation shader" twice. There is no "tesellation shader" in OpenGL even if you ignore the bad spelling. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Spec bug? Transform feedback after tessellation control shader
AFAIK you can't have TCS without TES. But you can have TES without TCS. On Tue, Mar 17, 2015 at 2:33 PM, Marek Olšák wrote: > Hi, > > The GL 4.5 Core spec is inconsistent. The "11.1.2.1 Output Variables" > section says that transform feedback is allowed after a tessellation > control shader if a tessellation evaluation shader isn't present: > > "Each program object can specify a set of output variables from one > shader to be recorded in transform feedback mode (see section 13.2). > The variables that can be recorded are those emitted by the first > active shader, in order, from the following list: > * geometry shader > * tessellation evaluation shader > * tessellation control shader > * vertex shader" > > Even glTransformFeedbackVaryings allows it: > > "If the set of output variables to record in transform feedback mode > is specified by TransformFeedbackVaryings, a program will fail to link > if: > * the count specified by TransformFeedbackVaryings is non-zero, but the > program object has no vertex, tessellation control, tessellation evaluation, > or > geometry shader;" > > However, the "13.2 Transform Feedback" section doesn't mention any > tessellation control shader: > > "The data captured in transform feedback mode depends on the active > programs on each of the shader stages. If a program is active for the > geometry shader stage, transform feedback captures the vertices of > each primitive emitted by the geometry shader. Otherwise, if a program > is active for the tessellation evaluation shader stage, transform > feedback captures each primitive produced by the tessellation > primitive generator, whose vertices are processed by the tessellation > evaluation shader. Otherwise, transform feedback captures each > primitive processed by the vertex shader." > > Also, glBeginTransformFeedback doesn't allow GL_PATCHES, which is the > only output primitive type of the tessellation control shader: > > "primitiveMode must be TRIANGLES, LINES, or POINTS, and specifies the > output type of primitives that will be recorded into the buffer > objects bound for transform feedback" > > The question is: Is transform feedback really allowed after the > tessellation control shader? I hope not, because our hardware can't do > it. > > Thanks, > > Marek > > PS: The spec mentions "tesellation shader" twice. There is no > "tesellation shader" in OpenGL even if you ignore the bad spelling. > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/nir: Re-emit instructions instead of doing mov-to-flag when possible
On Mon, Mar 16, 2015 at 9:21 PM, Jason Ekstrand wrote: > Because of the way that NIR does conditionals, we get them in any old SSA > value. The actual boolean value used in the select or if is x != 0. > Previously, we handled this by emitting a mov.nz to move the value to the > flag register. However, this almost always adds at least one if not two > instructions because we have to go through the VGRF when we could be > comparing directly to the flag and then using the flag. > > With this commit, we simply re-emit the instruction that produces the value > when we can. By doing so, we can use the flag directly and we trust in CSE > to clean up for us if it can. > > Shader-db results: > > total instructions in shared programs: 4164120 -> 4110511 (-1.29%) > instructions in affected programs: 2397042 -> 2343433 (-2.24%) > helped:13167 > HURT: 31 > GAINED:4 > LOST: 4 > --- A small change to the cmod_propagation branch accomplishes most of this -- total instructions in shared programs: 7850607 -> 7793875 (-0.72%) instructions in affected programs: 2425140 -> 2368408 (-2.34%) helped:13502 HURT: 0 GAINED:1 LOST: 5 We always emit the MOV.NZ with a D-typed destination, which cmod_propagate won't combine with a CMP with an F-typed destination. I don't think we ever emit MOV.NZ without NIR, so no changes without it. Let me play with this some more. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Spec bug? Transform feedback after tessellation control shader
On 03/17/2015 11:35 AM, Ilia Mirkin wrote: > AFAIK you can't have TCS without TES. But you can have TES without TCS. That much is for sure. The ARB_tessellation_shader spec says the following in the overview: "In this extension, patches may not be passed beyond the tessellation evaluation shader, and an error is generated if an application provides patches and the current program object contains no tessellation evaluation shader." > On Tue, Mar 17, 2015 at 2:33 PM, Marek Olšák wrote: >> Hi, >> >> The GL 4.5 Core spec is inconsistent. The "11.1.2.1 Output Variables" >> section says that transform feedback is allowed after a tessellation >> control shader if a tessellation evaluation shader isn't present: >> >> "Each program object can specify a set of output variables from one >> shader to be recorded in transform feedback mode (see section 13.2). >> The variables that can be recorded are those emitted by the first >> active shader, in order, from the following list: >> * geometry shader >> * tessellation evaluation shader >> * tessellation control shader >> * vertex shader" >> >> Even glTransformFeedbackVaryings allows it: >> >> "If the set of output variables to record in transform feedback mode >> is specified by TransformFeedbackVaryings, a program will fail to link >> if: >> * the count specified by TransformFeedbackVaryings is non-zero, but the >> program object has no vertex, tessellation control, tessellation evaluation, >> or >> geometry shader;" >> >> However, the "13.2 Transform Feedback" section doesn't mention any >> tessellation control shader: >> >> "The data captured in transform feedback mode depends on the active >> programs on each of the shader stages. If a program is active for the >> geometry shader stage, transform feedback captures the vertices of >> each primitive emitted by the geometry shader. Otherwise, if a program >> is active for the tessellation evaluation shader stage, transform >> feedback captures each primitive produced by the tessellation >> primitive generator, whose vertices are processed by the tessellation >> evaluation shader. Otherwise, transform feedback captures each >> primitive processed by the vertex shader." >> >> Also, glBeginTransformFeedback doesn't allow GL_PATCHES, which is the >> only output primitive type of the tessellation control shader: >> >> "primitiveMode must be TRIANGLES, LINES, or POINTS, and specifies the >> output type of primitives that will be recorded into the buffer >> objects bound for transform feedback" >> >> The question is: Is transform feedback really allowed after the >> tessellation control shader? I hope not, because our hardware can't do >> it. >> >> Thanks, >> >> Marek >> >> PS: The spec mentions "tesellation shader" twice. There is no >> "tesellation shader" in OpenGL even if you ignore the bad spelling. >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Spec bug? Transform feedback after tessellation control shader
On 03/17/2015 11:33 AM, Marek Olšák wrote: > Hi, > > The GL 4.5 Core spec is inconsistent. The "11.1.2.1 Output Variables" > section says that transform feedback is allowed after a tessellation > control shader if a tessellation evaluation shader isn't present: > > "Each program object can specify a set of output variables from one > shader to be recorded in transform feedback mode (see section 13.2). > The variables that can be recorded are those emitted by the first > active shader, in order, from the following list: > * geometry shader > * tessellation evaluation shader > * tessellation control shader > * vertex shader" > > Even glTransformFeedbackVaryings allows it: > > "If the set of output variables to record in transform feedback mode > is specified by TransformFeedbackVaryings, a program will fail to link > if: > * the count specified by TransformFeedbackVaryings is non-zero, but the > program object has no vertex, tessellation control, tessellation evaluation, > or > geometry shader;" > > However, the "13.2 Transform Feedback" section doesn't mention any > tessellation control shader: > > "The data captured in transform feedback mode depends on the active > programs on each of the shader stages. If a program is active for the > geometry shader stage, transform feedback captures the vertices of > each primitive emitted by the geometry shader. Otherwise, if a program > is active for the tessellation evaluation shader stage, transform > feedback captures each primitive produced by the tessellation > primitive generator, whose vertices are processed by the tessellation > evaluation shader. Otherwise, transform feedback captures each > primitive processed by the vertex shader." > > Also, glBeginTransformFeedback doesn't allow GL_PATCHES, which is the > only output primitive type of the tessellation control shader: > > "primitiveMode must be TRIANGLES, LINES, or POINTS, and specifies the > output type of primitives that will be recorded into the buffer > objects bound for transform feedback" And to make matters worse, the ARB_tessellation_shader spec has the following issue. The resolution of this issue seems to contradict the rest of the extension spec. (8) Should coupling of tessellation control and evaluation shaders be required? RESOLVED: No. A tessellation control shader without an evaluation shader might be used in conjunction with transform feedback to generate regular transformed patches. Also, if the set of patches provided by the application is already in a form usable by the tessellator, the tessellation control shader may be bypassed. In this use case, the application would be required to provide default tessellation levels via the PatchParameterfv API, since no shader would be available to compute them. It may be useful to have a patch produced by a tessellation control shader to be fed directly to a geometry shader that performs some operation on full patches, rather than individual triangles of a tessellated patch. However, such capability is not provided in this extension. This definitely smells like a spec bug. You should submit a bug in the internal Khronos bugzilla and CC my @intel address. > The question is: Is transform feedback really allowed after the > tessellation control shader? I hope not, because our hardware can't do > it. > > Thanks, > > Marek > > PS: The spec mentions "tesellation shader" twice. There is no > "tesellation shader" in OpenGL even if you ignore the bad spelling. > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/nir: Re-emit instructions instead of doing mov-to-flag when possible
On Tue, Mar 17, 2015 at 11:38 AM, Matt Turner wrote: > On Mon, Mar 16, 2015 at 9:21 PM, Jason Ekstrand wrote: >> Because of the way that NIR does conditionals, we get them in any old SSA >> value. The actual boolean value used in the select or if is x != 0. >> Previously, we handled this by emitting a mov.nz to move the value to the >> flag register. However, this almost always adds at least one if not two >> instructions because we have to go through the VGRF when we could be >> comparing directly to the flag and then using the flag. >> >> With this commit, we simply re-emit the instruction that produces the value >> when we can. By doing so, we can use the flag directly and we trust in CSE >> to clean up for us if it can. >> >> Shader-db results: >> >> total instructions in shared programs: 4164120 -> 4110511 (-1.29%) >> instructions in affected programs: 2397042 -> 2343433 (-2.24%) >> helped:13167 >> HURT: 31 >> GAINED:4 >> LOST: 4 >> --- > > A small change to the cmod_propagation branch accomplishes most of this -- > > total instructions in shared programs: 7850607 -> 7793875 (-0.72%) > instructions in affected programs: 2425140 -> 2368408 (-2.34%) > helped:13502 > HURT: 0 > GAINED:1 > LOST: 5 > > We always emit the MOV.NZ with a D-typed destination, which > cmod_propagate won't combine with a CMP with an F-typed destination. > > I don't think we ever emit MOV.NZ without NIR, so no changes without it. > > Let me play with this some more. Another aspect here is how this is going to interact with boolean resolves. I've got patches in my wip/nir-booleans branch that I'm testing right now. I think the general approach is solid, I just need to test and work out the bugs. It could take some time as I am testing on ILK after all. --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 3/4] i965/state: Create separate dirty state bits for each pipeline
On Tue, Mar 17, 2015 at 10:46 AM, Kenneth Graunke wrote: > On Tuesday, March 17, 2015 08:20:23 AM Kristian Høgsberg wrote: >> On Tue, Mar 17, 2015 at 2:22 AM, Kenneth Graunke >> wrote: >> > On Monday, March 16, 2015 10:21:31 PM Kristian Høgsberg wrote: >> >> On Wed, Mar 11, 2015 at 11:53 AM, Jordan Justen >> >> wrote: > [snip] >> >> > @@ -691,8 +702,23 @@ void brw_upload_render_state(struct brw_context >> >> > *brw) >> >> > fprintf(stderr, "\n"); >> >> >} >> >> > } >> >> > + >> >> > + /* Save all dirty state into the other pipelines */ >> >> > + for (int i = 0; i < BRW_NUM_PIPELINES; i++) { >> >> > + if (i != pipeline) { >> >> > + brw->state.pipelines[i].mesa |= state->mesa; >> >> > + brw->state.pipelines[i].brw |= state->brw; >> >> > + } >> >> > + } >> >> >> >> When we merge this in at this point, state->mesa/brw includes dirty >> >> bits from the active pipelines pipeline flags. For example, suppose we >> >> do a bunch of render pipeline stuff, which queues up a lot of dirty >> >> flags for the compute pipeline. Then when we go to emit state for the >> >> compute pipeline we first merge all these dirty flags into >> >> brw->state.dirty, then merge that into the render pipeline flags. We >> >> only need to merge the new dirty bits, that is, the initial value of >> >> brw->state.dirty into the render pipeline flags. If we don't modify >> >> brw->state.dirty in this function and merge the flags in clear as >> >> described above, this problem is easy to fix. >> >> >> >> Kristian > > OK, I see what you mean now - very good catch. > > 1. We do a render operation. Any new dirty bits get saved in >brw->state.pipelines[COMPUTE], so they'll happen when we >do the next compute workload. > >This completes successfully (ignore retries). > > 2. We do a compute operation. > >brw->state.pipelines[COMPUTE] gets merged into brw->state.dirty. >This includes bits that happened during #1 (and were missed on >the compute pipe). > >This all works fine. At the end, we save brw->state.dirty into >brw->state.pipelines[RENDER]. > > 3. We do another render operation. > >brw->state.pipelines[RENDER] gets merged into brw->state.dirty. >We now have bits from #1 again, which are not necessary. > > So yeah. That's bad. Sorry, I thought this was to handle the > hypothetical case of pipe-switch-during-retries. But it's just > switching pipelines at all! I still think both points are valid. Just because we don't hit the retry problem doesn't mean we shouldn't fix it. Yes, there's precedence for moving the flag bits around in brw_upload_state() even in case it fails, but that just propagates the bits from where higher level mesa sets them to where we emit state from them. Conceptually the state remains the same after brw_upload_state() fails. There is also precedence for *not* changing state destructively in brw_upload_state(), that's the reason brw_clear_dirty_bits() is there in the first place. When we clear brw->state.pipelines[pipline] and merge the flags into brw->state.dirty, we change the state in a way that makes it a requirement that we call again with the same pipeline to get correct behavior. Given that we have the brw_clear_dirty_bits() in place already and that this is all new code (as opposed to changing behavior of a piece of battle-tested, stable logic), I really don't see why we would make it work this way. > If I understand your suggestion correctly, you mean that > a) We should eliminate brw->state.dirty. > b) brw_upload_pipeline_state() should do: > >struct brw_state_flags *pipeline_state = > &brw->state.pipelines[pipeline]; > >/* Create a local copy of the dirty state */ >struct brw_state_flags dirty; >dirty.mesa = brw->NewGLState | pipeline_state->mesa; >dirty.brw = ctx->NewDriverState | pipeline_state->brw; > >Use this for uploads. > > c) The existing brw_clear_dirty_bits() should do: > >/* Save the dirty flags into the other pipelines */ >for (int i = 0; i < BRW_NUM_PIPELINES; i++) { > if (i != pipeline) { > brw->state.pipelines[i].mesa |= brw->NewGLState; > brw->state.pipelines[i].brw |= ctx->NewDriverState; > } >} > >/* We've handled any bits passed to us by other pipeline uploads */ >brw->state.pipelines[pipe] = 0; > >/* We've handled the flags passed to us by Mesa */ >brw->NewGLState = 0; >ctx->NewDriverState = 0; > > Does that sound like a correct interpretation? This does seem to solve > the problem, and I think it's a bit cleaner too. Yes, pretty much. I wasn't considering eliminating brw->state.dirty, but that would be nice. We still have atoms that set dirty bits there as they're emitted, but we can use ctx->NewDriverState for that and or it into dirty.brw after each atom emit call. That's a bigger change, I see around 90 cases of setting bits in brw->state.dirty.brw, but getting rid of brw->state.dirty may be
[Mesa-dev] [Bug 89616] bufferobj.c:1639:7: error: format not a string literal and no format arguments [-Werror=format-security]
https://bugs.freedesktop.org/show_bug.cgi?id=89616 Bug ID: 89616 Summary: bufferobj.c:1639:7: error: format not a string literal and no format arguments [-Werror=format-security] Product: Mesa Version: git Hardware: All OS: Linux (All) Status: NEW Severity: normal Priority: medium Component: Mesa core Assignee: mesa-dev@lists.freedesktop.org Reporter: fabio@libero.it QA Contact: mesa-dev@lists.freedesktop.org CC: la...@jlekstrand.net Compile error on current mesa git: https://launchpadlibrarian.net/200474799/buildlog_ubuntu-vivid-amd64.mesa_10.6~git1503171930.579297~gd~v_BUILDING.txt.gz -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: Remove eglQueryString virtual dispatch.
On 03/13/2015 05:59 PM, Matt Turner wrote: Patch is Reviewed-by: Chad Versace > --- > Chad, you suggested it would be nice to remove the locking from > eglQueryString, but I don't see a way to do it. eglQueryString has > to generate EGL_NOT_INITIALIZED if the display is valid but not > initialized, and without locking it seems that there would be a > race between eglInitialize and eglQueryString. Is that the case, > or have I misunderstood something? I still think this can be done without a lock. Currently the function (like most other EGL functions) holds the display lock for the duration of the function call. But eglQueryString really only needs to ensure that (1) the display is initialized and (2) the display remains initialized until the call returns. (#2 is important because the user could possibly call eglTerminate in another thread, and eglTerminate has deferred destruction semantics). I think code like this could achieve that: // A new function that atomically increments the display's internal refcount only // if the display is initialized. Return true if the increment succeeds. bool _eglDisplayReference(_EGLDisplay *disp); // Another new function that pairs with _EGLDisplayReference. Returns // the number of remaining references. uint32_t _eglDisplayRelease(_EGLDisplay *disp); const char * eglQueryString(EGLDisplay dpy, ...) { const char *string = NULL; _EGLDisplay *disp = _eglLookupDisplay(dpy); if (!_eglDisplayReference(disp)) { // emit EGL_NOT_INITIALIZED return NULL; } // Set 'string' in the switch statement. _eglDisplayRelease(disp); return string; } To remove any races with eglTerminate, you would also need to move the destruction code from egTerminate into _eglDisplayRelease and rewrite eglTerminate to essentially just call _eglDisplayRelease. In this scheme, lockless EGL functions would follow this pattern: 1. Try to take a reference on the display. The reference will be held for the duration of the function call. 2. Do the action. 3. Release the reference and return. I admit it's a non-trivial amount of work. But doing that work would lay some groundwork for removing the display lock in other places too, even in functions that do real work like eglSwapBuffers. The bigger question is: Do we care enough to do all that work? And I think the answer remains "no" until we find real-word uses of multithreaded EGL where lock contention hurts the app. I don't know of any multithreaded EGL apps at the moment. Even so, I believe we should take care when writing new EGL code to not dig ourselves into a deeper hole. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest width lines - GEN6
> -Original Message- > From: Ian Romanick [mailto:i...@freedesktop.org] > Sent: Tuesday, March 17, 2015 8:27 PM > To: Predut, Marius; mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] [PATCH] i965/aa: fixing anti-aliasing bug for thinnest > width lines - GEN6 > > On 03/17/2015 11:29 AM, Marius Predut wrote: > > On SNB and IVB hw, for 1 pixel line thickness or less, the general anti- > aliasing algorithm give up - garbage line is generated. > > Setting a Line Width of 0.0 specifies the rasterization of the “thinnest” > (one-pixel-wide), non-antialiased lines. > > Lines rendered with zero Line Width are rasterized using Grid Intersection > Quantization rules as specified by bspec section 6.3.12.1 Zero-Width > (Cosmetic) Line Rasterization. > > You'll need to re-word wrap the commit message. You'll get the commit message > right one of these days. :) Yes right, also Brian Paul notice it. > > Also... when you send out new versions of a patch, please change the patch > subject to be something like "[PATCH v3] ...". This makes it easier for > people to know which is the latest version to review. > > You should also add notes to the commit message that explain what changed from > version to version. For example, this commit message should have something > like: > > v3: Fix = used instead of == in an if-statement. Noticed by Daniel Stone. > > This is helps people know that their review comments have been applied. > It is also important to do this when the review changes are applied and the > patch committed without re-sending to the list. Maintaining history like this > in commit messages helps us understand code in the future. > Yes indeed. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=28832 > > There are a number of bugs that have been closed as duplicates of this bug. > Two of these, bug #27007 and bug #60797, have test cases. Does this fix also > fix those? Yes , was fixed. Ok I try put all defects here. > > I also have a more general question: How are you testing this? Daniel > noticed a bug in an earlier version of the patch that piglit should have > caught. If you're doing a full piglit run and that didn't catch the previous > assignment-instead-of-comparison bug, it would be helpful if you could craft a > test that would detect that bug. That may be difficult, so don't spend a huge > amount of time on it. > I used the piglit test /bin/line-aa-width -auto Also I don’t observer pilgit test regressions when I run /piglit-run.py tests/quick.py. I used this https://bugs.freedesktop.org/attachment.cgi?id=33930 test case and checked visually. (an interrupted line rendered) I used this https://bugs.freedesktop.org/attachment.cgi?id=8675 test case and checked visually. ( a triangle for witch a line is missing) I don't know more about assignment-instead-of-comparison and Daniel noticed. > > Signed-off-by: Marius Predut > > --- > > src/mesa/drivers/dri/i965/gen6_sf_state.c | 12 +--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c > > b/src/mesa/drivers/dri/i965/gen6_sf_state.c > > index f9d8d27..1bed444 100644 > > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c > > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c > > @@ -367,9 +367,15 @@ upload_sf_state(struct brw_context *brw) > >float line_width = > > roundf(CLAMP(ctx->Line.Width, 0.0, ctx->Const.MaxLineWidth)); > >uint32_t line_width_u3_7 = U_FIXED(line_width, 7); > > - /* TODO: line width of 0 is not allowed when MSAA enabled */ > > - if (line_width_u3_7 == 0) > > - line_width_u3_7 = 1; > > + > > + if (!(multisampled_fbo && ctx->Multisample.Enabled)) { > > In Mesa there are often Enabled and _Enabled fields. The Enabled field is the > setting made by the application via the OpenGL API. The _Enabled field means > that feature in question is actually enabled, and this may depend on other > state. > > In this case, the application may enable multisampling, but multisampling may > not occur of there is not a multisample buffer. This means > gl_multisample_attrib::Enabled would be set but > gl_multisample_attrib::_Enabled would not. Instead of open-coding that check, > just check gl_multisample_attrib::_Enabled directly: > > if (!ctx->Multisample._Enabled) { > > I actually think it's more clear if you leave the original comment and > implement this as: > > /* Line width of 0 is not allowed when MSAA enabled */ > if (ctx->Multisample._Enabled) { > if (line_width_u3_7 == 0) > line_width_u3_7 = 1; > } else if (ctx->Line.SmoothFlag && ctx->Line.Width <= 1.0) { > /* For lines less than 1 pixel thick, the general > * anti-aliasing algorithm gives up, and a garbage line is > * generated. Setting a Line Width of 0.0 specifies the > * rasterization of the "thinnest" (one-pixel-wide), >
Re: [Mesa-dev] Spec bug? Transform feedback after tessellation control shader
Did you mean "Khronos Public Bugzilla"? Marek On Tue, Mar 17, 2015 at 7:58 PM, Ian Romanick wrote: > On 03/17/2015 11:33 AM, Marek Olšák wrote: >> Hi, >> >> The GL 4.5 Core spec is inconsistent. The "11.1.2.1 Output Variables" >> section says that transform feedback is allowed after a tessellation >> control shader if a tessellation evaluation shader isn't present: >> >> "Each program object can specify a set of output variables from one >> shader to be recorded in transform feedback mode (see section 13.2). >> The variables that can be recorded are those emitted by the first >> active shader, in order, from the following list: >> * geometry shader >> * tessellation evaluation shader >> * tessellation control shader >> * vertex shader" >> >> Even glTransformFeedbackVaryings allows it: >> >> "If the set of output variables to record in transform feedback mode >> is specified by TransformFeedbackVaryings, a program will fail to link >> if: >> * the count specified by TransformFeedbackVaryings is non-zero, but the >> program object has no vertex, tessellation control, tessellation evaluation, >> or >> geometry shader;" >> >> However, the "13.2 Transform Feedback" section doesn't mention any >> tessellation control shader: >> >> "The data captured in transform feedback mode depends on the active >> programs on each of the shader stages. If a program is active for the >> geometry shader stage, transform feedback captures the vertices of >> each primitive emitted by the geometry shader. Otherwise, if a program >> is active for the tessellation evaluation shader stage, transform >> feedback captures each primitive produced by the tessellation >> primitive generator, whose vertices are processed by the tessellation >> evaluation shader. Otherwise, transform feedback captures each >> primitive processed by the vertex shader." >> >> Also, glBeginTransformFeedback doesn't allow GL_PATCHES, which is the >> only output primitive type of the tessellation control shader: >> >> "primitiveMode must be TRIANGLES, LINES, or POINTS, and specifies the >> output type of primitives that will be recorded into the buffer >> objects bound for transform feedback" > > And to make matters worse, the ARB_tessellation_shader spec has the > following issue. The resolution of this issue seems to contradict the > rest of the extension spec. > > (8) Should coupling of tessellation control and evaluation shaders be > required? > > RESOLVED: No. A tessellation control shader without an evaluation > shader might be used in conjunction with transform feedback to generate > regular transformed patches. Also, if the set of patches provided by > the application is already in a form usable by the tessellator, the > tessellation control shader may be bypassed. In this use case, the > application would be required to provide default tessellation levels via > the PatchParameterfv API, since no shader would be available to compute > them. > > It may be useful to have a patch produced by a tessellation control > shader to be fed directly to a geometry shader that performs some > operation on full patches, rather than individual triangles of a > tessellated patch. However, such capability is not provided in this > extension. > > This definitely smells like a spec bug. You should submit a bug in the > internal Khronos bugzilla and CC my @intel address. > >> The question is: Is transform feedback really allowed after the >> tessellation control shader? I hope not, because our hardware can't do >> it. >> >> Thanks, >> >> Marek >> >> PS: The spec mentions "tesellation shader" twice. There is no >> "tesellation shader" in OpenGL even if you ignore the bad spelling. >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89616] bufferobj.c:1639:7: error: format not a string literal and no format arguments [-Werror=format-security]
https://bugs.freedesktop.org/show_bug.cgi?id=89616 --- Comment #1 from Laura Ekstrand --- Just pushed a commit to Mesa master that fixes this. I looked for a while and didn't find any other instances of the same error in the rest of my code. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89433] GCC 4.2 does not support -Wvla
https://bugs.freedesktop.org/show_bug.cgi?id=89433 --- Comment #4 from José Fonseca --- (In reply to Jonathan Gray from comment #3) > Created attachment 114359 [details] [review] > check if compiler supports -Werror=vla > > new patch that does a compile check for the flag configure.ac is basically m4 on top of shell/bash, right? If so the check must be done MSVC2013_COMPAT_* vars are referred in the MSVC2008_COMPAT_* assignment. Otherwise looks good. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89433] GCC 4.2 does not support -Wvla
https://bugs.freedesktop.org/show_bug.cgi?id=89433 --- Comment #5 from José Fonseca --- "... check must be done _before_ MSVC2013_COMPAT_* ..." -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Summer of Code ideas (maybe just an idea wishlist?)
Hi, Last year, I participated in GSoC, (yeah, I still read the mailing list every day :) ) so I will give my 2 cents to the topic. My background isn't at all related to graphics or computer science, I am a physicist, although my main interest is in simulation. So that took me to OpenCL, which in turn took me to clover and finally landed in Mesa. I started looking at the code to see how things worked in the backstage (you know, we physicists usually like to know how things work :) ), and I spotted a couple of things that I thought could be done better. At first, I was mostly afraid of doing anything, mostly because I am not a programmer (I still don't think of myself as one) and I didn't know if what I was doing and how I was doing it was ok. But I ended sending some patches to the mailing list, and one thing followed another and I ended doing a GSoC. And here I am still, mostly without free time to try to continue where I left everything, but mostly willing to help again with whatever I can. My points would be more or less two: First, not to just think about graphics and cs people as possible help, it is true that they may be the best for the job, but other backgrounds may also help. And second, be welcoming to new people, for some of us, the step of sending something to the list, specially the first time, may be very stressfull, and receiving an answer with a plain 'do this' may be a bit discouraging. As said, just my 2 cents. Bruno On Mon, 2015-03-16 at 09:32 -0700, Laura Ekstrand wrote: > That was basically my background (mechanical engineering + lots of > OpenGL) when I started six months ago, but I have found the lack of > mentoring to be a large roadblock. At that time, I wrote tests, but > there were few people willing to review them and give timely feedback. > I was advised to go ahead and push the tests after a month, but then > others came back weeks later with lots of late reviews after the fact. > They were highly critical and made me feel unwelcome in the community. > I've had more success working directly on the Mesa driver. > > > So I'm not sure we can attract and retain these types of students. > > > On Mon, Mar 16, 2015 at 6:23 AM, Marek Olšák wrote: > On Fri, Mar 13, 2015 at 6:24 PM, Laura Ekstrand > wrote: > > We should try to steer people away from just writing Piglit > tests for GSoC, > > unless they have a specific mentor in mind and have already > talked to him or > > her. In my experience, Piglit tests are difficult to do > well because each > > one is drastically different from the others and involves > cultivating a > > fairly deep understanding of the the OpenGL function in > question. > > > > A project pairing a specific extension with relevant tests > (like Martin and > > I have done with ARB_dsa) would be better as long as it's a > fairly specific > > domain of the OpenGL spec. That way, the student can study > the spec for one > > specific set of objects or entry points and cultivate the > necessary > > understanding they need to write the related tests. > > > > A lot of the emails we've gotten from students saying "I > want to write 4.x > > Piglit tests" have been too broad/generic and would be > difficult for a > > student to master in a summer without lots of > mentoring/direction from the > > community. > > We should also take into account that there are people having > a degree > in or studying computer science with specialization in > computer > graphics or having strong knowledge of OpenGL already. Such > people are > difficult to find, but they would be very effective with very > little > (if any) mentoring. Gamedev-related forums (gamedev.net, > opengl.org, > etc.) should have a lot of talented people suited for this > job, but > none of them are probably aware of the Mesa/Piglit GSoC. > > Marek > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965/fs: Print spills:fills and number of promoted constants.
--- src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp | 2 +- src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +- src/mesa/drivers/dri/i965/brw_fs.h | 4 .../drivers/dri/i965/brw_fs_combine_constants.cpp| 1 + src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 20 +--- src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 4 ++-- src/mesa/drivers/dri/i965/brw_vec4.cpp | 2 +- 7 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp index f9b1737..32919b1 100644 --- a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp @@ -31,7 +31,7 @@ brw_blorp_eu_emitter::brw_blorp_eu_emitter(struct brw_context *brw, : mem_ctx(ralloc_context(NULL)), generator(brw, mem_ctx, (void *) rzalloc(mem_ctx, struct brw_wm_prog_key), (struct brw_stage_prog_data *) rzalloc(mem_ctx, struct brw_wm_prog_data), - NULL, false, "BLORP") + NULL, 0, false, "BLORP") { if (debug_flag) generator.enable_debug("blorp"); diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 8702ea8..7f671dc 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -4039,7 +4039,7 @@ brw_wm_fs_emit(struct brw_context *brw, } fs_generator g(brw, mem_ctx, (void *) key, &prog_data->base, - &fp->Base, v.runtime_check_aads_emit, "FS"); + &fp->Base, v.promoted_constants, v.runtime_check_aads_emit, "FS"); if (unlikely(INTEL_DEBUG & DEBUG_WM)) { char *name; diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h index 7716529..ed0bb8f 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.h +++ b/src/mesa/drivers/dri/i965/brw_fs.h @@ -512,6 +512,8 @@ public: bool spilled_any_registers; const unsigned dispatch_width; /**< 8 or 16 */ + + unsigned promoted_constants; }; /** @@ -527,6 +529,7 @@ public: const void *key, struct brw_stage_prog_data *prog_data, struct gl_program *fp, +unsigned promoted_constants, bool runtime_check_aads_emit, const char *stage_abbrev); ~fs_generator(); @@ -638,6 +641,7 @@ private: unsigned dispatch_width; /**< 8 or 16 */ exec_list discard_halt_patches; + unsigned promoted_constants; bool runtime_check_aads_emit; bool debug_flag; const char *shader_name; diff --git a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp index 7ddb253..ebde8df 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp @@ -286,6 +286,7 @@ fs_visitor::opt_combine_constants() reg.subreg_offset = 0; } } + promoted_constants = table.len; /* Rewrite the immediate sources to refer to the new GRFs. */ for (int i = 0; i < table.len; i++) { diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp index e086266..e61a9d3 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp @@ -126,13 +126,15 @@ fs_generator::fs_generator(struct brw_context *brw, const void *key, struct brw_stage_prog_data *prog_data, struct gl_program *prog, + unsigned promoted_constants, bool runtime_check_aads_emit, const char *stage_abbrev) : brw(brw), key(key), prog_data(prog_data), prog(prog), runtime_check_aads_emit(runtime_check_aads_emit), - debug_flag(false), stage_abbrev(stage_abbrev), mem_ctx(mem_ctx) + debug_flag(false), stage_abbrev(stage_abbrev), mem_ctx(mem_ctx), + promoted_constants(promoted_constants) { ctx = &brw->ctx; @@ -1563,6 +1565,7 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width) brw_set_default_compression_control(p, BRW_COMPRESSION_COMPRESSED); int start_offset = p->next_insn_offset; + int spill_count = 0, fill_count = 0; int loop_count = 0; struct annotation_info annotation; @@ -1959,14 +1962,17 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width) case SHADER_OPCODE_GEN4_SCRATCH_WRITE: generate_scratch_write(inst, src[0]); + spill_count++; break; case SHADER_OPCODE_GEN4_SCRATCH_READ: generate_scratch_read(inst, dst); + fill_count++; break; case SHADER_OPCODE_GEN7_SCRATCH_READ: generate_scratch_read_gen7(inst, dst); + fill_count++; break; case
Re: [Mesa-dev] [PATCH] i965/fs: Print spills:fills and number of promoted constants.
On Tue, Mar 17, 2015 at 2:09 PM, Matt Turner wrote: > --- > src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp | 2 +- > src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +- > src/mesa/drivers/dri/i965/brw_fs.h | 4 > .../drivers/dri/i965/brw_fs_combine_constants.cpp| 1 + > src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 20 > +--- > src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 4 ++-- > src/mesa/drivers/dri/i965/brw_vec4.cpp | 2 +- > 7 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp > b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp > index f9b1737..32919b1 100644 > --- a/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp > +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit_eu.cpp > @@ -31,7 +31,7 @@ brw_blorp_eu_emitter::brw_blorp_eu_emitter(struct > brw_context *brw, > : mem_ctx(ralloc_context(NULL)), > generator(brw, mem_ctx, (void *) rzalloc(mem_ctx, struct > brw_wm_prog_key), > (struct brw_stage_prog_data *) rzalloc(mem_ctx, struct > brw_wm_prog_data), > - NULL, false, "BLORP") > + NULL, 0, false, "BLORP") > { > if (debug_flag) >generator.enable_debug("blorp"); > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp > b/src/mesa/drivers/dri/i965/brw_fs.cpp > index 8702ea8..7f671dc 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > @@ -4039,7 +4039,7 @@ brw_wm_fs_emit(struct brw_context *brw, > } > > fs_generator g(brw, mem_ctx, (void *) key, &prog_data->base, > - &fp->Base, v.runtime_check_aads_emit, "FS"); > + &fp->Base, v.promoted_constants, > v.runtime_check_aads_emit, "FS"); > > if (unlikely(INTEL_DEBUG & DEBUG_WM)) { >char *name; > diff --git a/src/mesa/drivers/dri/i965/brw_fs.h > b/src/mesa/drivers/dri/i965/brw_fs.h > index 7716529..ed0bb8f 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.h > +++ b/src/mesa/drivers/dri/i965/brw_fs.h > @@ -512,6 +512,8 @@ public: > bool spilled_any_registers; > > const unsigned dispatch_width; /**< 8 or 16 */ > + > + unsigned promoted_constants; > }; > > /** > @@ -527,6 +529,7 @@ public: > const void *key, > struct brw_stage_prog_data *prog_data, > struct gl_program *fp, > +unsigned promoted_constants, > bool runtime_check_aads_emit, > const char *stage_abbrev); > ~fs_generator(); > @@ -638,6 +641,7 @@ private: > unsigned dispatch_width; /**< 8 or 16 */ > > exec_list discard_halt_patches; > + unsigned promoted_constants; > bool runtime_check_aads_emit; > bool debug_flag; > const char *shader_name; > diff --git a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp > b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp > index 7ddb253..ebde8df 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp > @@ -286,6 +286,7 @@ fs_visitor::opt_combine_constants() > reg.subreg_offset = 0; >} > } > + promoted_constants = table.len; > > /* Rewrite the immediate sources to refer to the new GRFs. */ > for (int i = 0; i < table.len; i++) { > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > index e086266..e61a9d3 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp > @@ -126,13 +126,15 @@ fs_generator::fs_generator(struct brw_context *brw, > const void *key, > struct brw_stage_prog_data *prog_data, > struct gl_program *prog, > + unsigned promoted_constants, > bool runtime_check_aads_emit, > const char *stage_abbrev) > > : brw(brw), key(key), > prog_data(prog_data), > prog(prog), runtime_check_aads_emit(runtime_check_aads_emit), > - debug_flag(false), stage_abbrev(stage_abbrev), mem_ctx(mem_ctx) > + debug_flag(false), stage_abbrev(stage_abbrev), mem_ctx(mem_ctx), > + promoted_constants(promoted_constants) > { > ctx = &brw->ctx; > > @@ -1563,6 +1565,7 @@ fs_generator::generate_code(const cfg_t *cfg, int > dispatch_width) >brw_set_default_compression_control(p, BRW_COMPRESSION_COMPRESSED); > > int start_offset = p->next_insn_offset; > + int spill_count = 0, fill_count = 0; > int loop_count = 0; > > struct annotation_info annotation; > @@ -1959,14 +1962,17 @@ fs_generator::generate_code(const cfg_t *cfg, int > dispatch_width) > >case SHADER_OPCODE_GEN4_SCRATCH_WRITE: > generate_scratch_write(inst, src[0]); > + spill_count++; > break;
Re: [Mesa-dev] [PATCH] i965/fs: Print spills:fills and number of promoted constants.
On Tue, Mar 17, 2015 at 2:15 PM, Jason Ekstrand wrote: > On Tue, Mar 17, 2015 at 2:09 PM, Matt Turner wrote: >> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp >> b/src/mesa/drivers/dri/i965/brw_vec4.cpp >> index 8edb4d0..63dedae 100644 >> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp >> @@ -1969,7 +1969,7 @@ brw_vs_emit(struct brw_context *brw, >>} >> >>fs_generator g(brw, mem_ctx, (void *) &c->key, &prog_data->base.base, >> - &c->vp->program.Base, v.runtime_check_aads_emit, "VS"); >> + &c->vp->program.Base, v.runtime_check_aads_emit, >> v.promoted_constants, "VS"); > > Promoted constants and aads_emit need to be flipped around. You got > it right for FS. Thanks! > Also, does this require any adaptations to shader-db or does it work as-is? Works as is, but shader-db report.py doesn't know about the new things. I'd like to add some switches to it like --spills or --compaction and have it print stats based on those. > Other than that, > Reviewed-by: Jason Ekstrand Thanks! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/fs: Print spills:fills and number of promoted constants.
With the fix Jason mentioned: Reviewed-by: Chris Forbes On Wed, Mar 18, 2015 at 10:19 AM, Matt Turner wrote: > On Tue, Mar 17, 2015 at 2:15 PM, Jason Ekstrand wrote: >> On Tue, Mar 17, 2015 at 2:09 PM, Matt Turner wrote: >>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp >>> b/src/mesa/drivers/dri/i965/brw_vec4.cpp >>> index 8edb4d0..63dedae 100644 >>> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp >>> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp >>> @@ -1969,7 +1969,7 @@ brw_vs_emit(struct brw_context *brw, >>>} >>> >>>fs_generator g(brw, mem_ctx, (void *) &c->key, &prog_data->base.base, >>> - &c->vp->program.Base, v.runtime_check_aads_emit, >>> "VS"); >>> + &c->vp->program.Base, v.runtime_check_aads_emit, >>> v.promoted_constants, "VS"); >> >> Promoted constants and aads_emit need to be flipped around. You got >> it right for FS. > > Thanks! > >> Also, does this require any adaptations to shader-db or does it work as-is? > > Works as is, but shader-db report.py doesn't know about the new > things. I'd like to add some switches to it like --spills or > --compaction and have it print stats based on those. > >> Other than that, >> Reviewed-by: Jason Ekstrand > > Thanks! > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 1/2] glsl: Transform pow(x, 4) into (x*x)*(x*x).
Without NIR: total instructions in shared programs: 6190374 -> 6190153 (-0.00%) instructions in affected programs: 61126 -> 60905 (-0.36%) helped:156 With NIR: total instructions in shared programs: 6271584 -> 6271471 (-0.00%) instructions in affected programs: 47656 -> 47543 (-0.24%) helped:113 GAINED:4 LOST: 41 --- The NIR gained/lost numbers look bad, but they're better than the next patch alone. I wanted to send these out because Jason was doing something that involved this. The two shaders I poked at seemed to be doing saturate(pow(x, 4.0f) + 1e-7f) Just want to ensure the result is >0.0f I guess? After changing the pow(x, 4.0f) into a pair of multiplies, we're able to combine the second multiply with an add. Not sure why this means we lose SIMD16, but the shaders that we lose we couldn't get SIMD16 in the non-NIR case anyway. src/glsl/opt_algebraic.cpp | 20 1 file changed, 20 insertions(+) diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp index 69c03ea..7742f82 100644 --- a/src/glsl/opt_algebraic.cpp +++ b/src/glsl/opt_algebraic.cpp @@ -99,6 +99,12 @@ is_vec_two(ir_constant *ir) } static inline bool +is_vec_four(ir_constant *ir) +{ + return (ir == NULL) ? false : ir->is_value(4.0, 4); +} + +static inline bool is_vec_negative_one(ir_constant *ir) { return (ir == NULL) ? false : ir->is_negative_one(); @@ -742,6 +748,20 @@ ir_algebraic_visitor::handle_expression(ir_expression *ir) return mul(x, x); } + if (is_vec_four(op_const[1])) { + ir_variable *x = new(ir) ir_variable(ir->operands[1]->type, "x", + ir_var_temporary); + base_ir->insert_before(x); + base_ir->insert_before(assign(x, ir->operands[0])); + + ir_variable *squared = new(ir) ir_variable(ir->operands[1]->type, +"squared", +ir_var_temporary); + base_ir->insert_before(squared); + base_ir->insert_before(assign(squared, mul(x, x))); + return mul(squared, squared); + } + break; case ir_binop_min: -- 2.0.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [RFC PATCH 2/2] i965/fs: Allow 2-src math instructions to have immediate src1.
Without NIR: total instructions in shared programs: 6190153 -> 6185918 (-0.07%) instructions in affected programs: 185156 -> 180921 (-2.29%) helped:918 With NIR: total instructions in shared programs: 6273347 -> 6268409 (-0.08%) instructions in affected programs: 205735 -> 200797 (-2.40%) helped:1171 GAINED:1 --- src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp | 12 src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp | 6 +++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp index 7ddb253..714d1c2 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_combine_constants.cpp @@ -62,9 +62,13 @@ could_coissue(const struct brw_context *brw, const fs_inst *inst) * Returns true for instructions that don't support immediate sources. */ static bool -must_promote_imm(const fs_inst *inst) +must_promote_imm(struct brw_context *brw, const fs_inst *inst) { switch (inst->opcode) { + case SHADER_OPCODE_POW: + case SHADER_OPCODE_INT_QUOTIENT: + case SHADER_OPCODE_INT_REMAINDER: + return brw->gen < 8; case BRW_OPCODE_MAD: case BRW_OPCODE_LRP: return true; @@ -207,7 +211,7 @@ fs_visitor::opt_combine_constants() foreach_block_and_inst(block, fs_inst, inst, cfg) { ip++; - if (!could_coissue(brw, inst) && !must_promote_imm(inst)) + if (!could_coissue(brw, inst) && !must_promote_imm(brw, inst)) continue; for (int i = 0; i < inst->sources; i++) { @@ -225,7 +229,7 @@ fs_visitor::opt_combine_constants() imm->block = intersection; imm->uses->push_tail(link(const_ctx, &inst->src[i])); imm->uses_by_coissue += could_coissue(brw, inst); -imm->must_promote = imm->must_promote || must_promote_imm(inst); +imm->must_promote = imm->must_promote || must_promote_imm(brw, inst); imm->last_use_ip = ip; } else { imm = new_imm(&table, const_ctx); @@ -235,7 +239,7 @@ fs_visitor::opt_combine_constants() imm->uses->push_tail(link(const_ctx, &inst->src[i])); imm->val = val; imm->uses_by_coissue = could_coissue(brw, inst); -imm->must_promote = must_promote_imm(inst); +imm->must_promote = must_promote_imm(brw, inst); imm->first_use_ip = ip; imm->last_use_ip = ip; } diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp index 764741d..20b1976 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp @@ -476,9 +476,9 @@ fs_visitor::try_constant_propagate(fs_inst *inst, acp_entry *entry) case SHADER_OPCODE_POW: case SHADER_OPCODE_INT_QUOTIENT: case SHADER_OPCODE_INT_REMAINDER: - if (brw->gen < 8) -break; - /* fallthrough */ + /* Allow constant propagation into src1 regardless of generation, and + * let constant combining promote the constant on Gen < 8. + */ case BRW_OPCODE_BFI1: case BRW_OPCODE_ASR: case BRW_OPCODE_SHL: -- 2.0.5 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] gallium/util: Define ffsll on OpenBSD.
On 17/03/15 01:25, Jonathan Gray wrote: > On Mon, Mar 16, 2015 at 08:37:28PM +, Emil Velikov wrote: >> On 26/02/15 13:49, Jose Fonseca wrote: >>> On 26/02/15 13:42, Jose Fonseca wrote: On 26/02/15 03:55, Jonathan Gray wrote: > On Wed, Feb 25, 2015 at 07:09:26PM -0800, Matt Turner wrote: >> On Wed, Feb 25, 2015 at 7:03 PM, Jonathan Gray wrote: >>> On Wed, Feb 25, 2015 at 06:53:14PM -0800, Matt Turner wrote: On Wed, Feb 25, 2015 at 5:37 PM, Jonathan Gray wrote: > If it isn't going to be configure checks could someone merge the > original patch in this thread? I committed commit 3492e88090d2d0c0bfbc934963b8772b45fc8880 Author: Matt Turner Date: Fri Feb 20 18:46:43 2015 -0800 gallium/util: Use HAVE___BUILTIN_* macros. Reviewed-by: Eric Anholt Reviewed-by: Jose Fonseca which switched over a bunch of preprocessor checks around __builtin* calls to use the macros defined by autotools. So I think cleaning it up to use __builtin_ffs* first #ifdef HAVE___BUILTIN_* can go forward now. >>> >>> Yes but there is no HAVE_FFSLL for constructs like >>> >>> #if !defined(HAVE_FFSLL) && defined(HAVE___BUILTIN_FFSLL) >>> >>> or is it ok to always use the builtin? >> >> I think the question is whether it's okay to always use the builtin if >> it's available (as opposed to libc functions). I think the answer to >> that is yes. > > So in that case how about the following? Or is it going to break > the android scons build? > > From cba39ba72115e57d262cb4b099c4e72106f01812 Mon Sep 17 00:00:00 2001 > From: Jonathan Gray > Date: Thu, 26 Feb 2015 14:46:45 +1100 > Subject: [PATCH] gallium/util: use ffs* builtins if available > > Required to build on OpenBSD which doesn't have ffsll in libc. > > Signed-off-by: Jonathan Gray > --- > src/gallium/auxiliary/util/u_math.h | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/src/gallium/auxiliary/util/u_math.h > b/src/gallium/auxiliary/util/u_math.h > index b4a65e4..5bc9b97 100644 > --- a/src/gallium/auxiliary/util/u_math.h > +++ b/src/gallium/auxiliary/util/u_math.h > @@ -384,9 +384,6 @@ unsigned ffs( unsigned u ) > > return i; > } > -#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) > -#define ffs __builtin_ffs > -#define ffsll __builtin_ffsll Scons does define HAVE___BUILTIN_FFS for mingw. However `git grep '\` shows ffs is used directly in many other places. So I suspect this change will break them. > #endif > > #endif /* FFS_DEFINED */ > @@ -435,7 +432,11 @@ util_last_bit_signed(int i) > static INLINE int > u_bit_scan(unsigned *mask) > { > +#if defined(HAVE___BUILTIN_FFS) > + int i = __builtin_ffs(*mask) - 1; > +#else > int i = ffs(*mask) - 1; > +#endif > *mask &= ~(1 << i); > return i; > } > @@ -444,7 +445,11 @@ u_bit_scan(unsigned *mask) > static INLINE int > u_bit_scan64(uint64_t *mask) > { > +#if defined(HAVE___BUILTIN_FFSLL) > + int i = __builtin_ffsll(*mask) - 1; > +#else > int i = ffsll(*mask) - 1; > +#endif > *mask &= ~(1llu << i); > return i; > } > I think the right thing long term is to provide ffs and ffsll in c99_compat.h or c99_math.h for all platforms. And let the rest of the code just always assume it's available somehow. Otherwise, let's just '#define ffs __builtin_ffs' on OpenBSD too. >>> >>> In other words, the original patch on this thread >>> >>> http://lists.freedesktop.org/archives/mesa-dev/2015-February/076071.html >>> >>> is the only patch I've seen so far that doesn't break Mingw. >>> >>> If you rather use HAVE___BUILTIN_FFSLL, then just do >>> >>> diff --git a/src/gallium/auxiliary/util/u_math.h >>> b/src/gallium/auxiliary/util/u_math.h >>> index 959f76e..d372cfd 100644 >>> --- a/src/gallium/auxiliary/util/u_math.h >>> +++ b/src/gallium/auxiliary/util/u_math.h >>> @@ -384,7 +384,7 @@ unsigned ffs( unsigned u ) >>> >>> return i; >>> } >>> -#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) >>> +#elif defined(__MINGW32__) || defined(PIPE_OS_ANDROID) || >>> defined(HAVE___BUILTIN_FFSLL) >>> #define ffs __builtin_ffs >>> #define ffsll __builtin_ffsll >>> #endif >>> >> Jonathan >> >> Seems like this has ended up a longer discussion that anticipated :\ >> Can you please confirm if the above works for you ? >> >> Thanks >> Emil > > It looks like that diff was mangled by the mail client and doesn't have > the newline escaped. It also assumes a ffsll builtin implies a ffs > builtin is present. So how about
[Mesa-dev] [Bug 89238] "nir/nir.h", line 643: Error: In this declaration "src" is of an incomplete type "nir_alu_src[]".
https://bugs.freedesktop.org/show_bug.cgi?id=89238 Vinson Lee changed: What|Removed |Added Status|NEEDINFO|NEW --- Comment #4 from Vinson Lee --- (In reply to Jason Ekstrand from comment #3) > Created attachment 114310 [details] > Patch to fix the unsized array issue > > I just attached a patch that *should* fix the bug. Let me know how it works. "nir/nir.h", line 664: Error: An array cannot have zero size unless you use the option -features=zla. "nir/nir.h", line 722: Warning: Identifier expected instead of "}". "nir/nir.h", line 813: Error: An array cannot have zero size unless you use the option -features=zla. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] u_primconvert: add primitive restart support
This add primitive restart support to the prim conversion. This involves changing the API for the translate functions as we need to pass the prim restart index and the original number of indices into the translate functions. primitive restart is support for quads, quad strips and polygons. This deal with the case where the actual number of output primitives is less than the initially calculated number, by filling the rest of the output primitives with the restart index, the other option is to reduce the output prim number, but that will make the generator code a bit messier. Signed-off-by: Dave Airlie --- src/gallium/auxiliary/indices/u_indices.c | 36 ++-- src/gallium/auxiliary/indices/u_indices.h | 8 +- src/gallium/auxiliary/indices/u_indices_gen.py | 198 +++-- src/gallium/auxiliary/indices/u_primconvert.c | 6 +- src/gallium/auxiliary/indices/u_unfilled_gen.py| 20 ++- src/gallium/auxiliary/indices/u_unfilled_indices.c | 18 +- src/gallium/drivers/svga/svga_draw_elements.c | 3 +- 7 files changed, 202 insertions(+), 87 deletions(-) diff --git a/src/gallium/auxiliary/indices/u_indices.c b/src/gallium/auxiliary/indices/u_indices.c index 1b33f41..b20337f 100644 --- a/src/gallium/auxiliary/indices/u_indices.c +++ b/src/gallium/auxiliary/indices/u_indices.c @@ -27,18 +27,22 @@ static void translate_memcpy_ushort( const void *in, unsigned start, - unsigned nr, + unsigned in_nr, + unsigned out_nr, + unsigned prim_restart_idx, void *out ) { - memcpy(out, &((short *)in)[start], nr*sizeof(short)); + memcpy(out, &((short *)in)[start], out_nr*sizeof(short)); } static void translate_memcpy_uint( const void *in, unsigned start, - unsigned nr, + unsigned in_nr, + unsigned out_nr, + unsigned prim_restart_idx, void *out ) { - memcpy(out, &((int *)in)[start], nr*sizeof(int)); + memcpy(out, &((int *)in)[start], out_nr*sizeof(int)); } @@ -58,6 +62,7 @@ static void translate_memcpy_uint( const void *in, * \param nr number of incoming vertices * \param in_pv incoming provoking vertex convention (PV_FIRST or PV_LAST) * \param out_pv desired provoking vertex convention (PV_FIRST or PV_LAST) + * \param prim_restart whether primitive restart is disable or enabled * \param out_prim returns new PIPE_PRIM_x we'll translate to * \param out_index_size returns bytes per new index value (2 or 4) * \param out_nr returns number of new vertices @@ -69,6 +74,7 @@ int u_index_translator( unsigned hw_mask, unsigned nr, unsigned in_pv, unsigned out_pv, +unsigned prim_restart, unsigned *out_prim, unsigned *out_index_size, unsigned *out_nr, @@ -106,68 +112,68 @@ int u_index_translator( unsigned hw_mask, else { switch (prim) { case PIPE_PRIM_POINTS: - *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim]; + *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim_restart][prim]; *out_prim = PIPE_PRIM_POINTS; *out_nr = nr; break; case PIPE_PRIM_LINES: - *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim]; + *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim_restart][prim]; *out_prim = PIPE_PRIM_LINES; *out_nr = nr; break; case PIPE_PRIM_LINE_STRIP: - *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim]; + *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim_restart][prim]; *out_prim = PIPE_PRIM_LINES; *out_nr = (nr - 1) * 2; break; case PIPE_PRIM_LINE_LOOP: - *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim]; + *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim_restart][prim]; *out_prim = PIPE_PRIM_LINES; *out_nr = nr * 2; break; case PIPE_PRIM_TRIANGLES: - *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim]; + *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim_restart][prim]; *out_prim = PIPE_PRIM_TRIANGLES; *out_nr = nr; break; case PIPE_PRIM_TRIANGLE_STRIP: - *out_translate = translate[in_idx][out_idx][in_pv][out_pv][prim]; + *out_translate = translate[in_i
[Mesa-dev] [PATCH 8/9] i965/nir: Do boolean resolves on GEN <= 5
--- src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 21 + 1 file changed, 21 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp index 91a3f65..1ef4602 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp @@ -25,6 +25,7 @@ #include "glsl/ir_optimization.h" #include "glsl/nir/glsl_to_nir.h" #include "brw_fs.h" +#include "brw_nir.h" static void nir_optimize(nir_shader *nir) @@ -149,6 +150,14 @@ fs_visitor::emit_nir_code() nir_convert_from_ssa(nir); nir_validate_shader(nir); + /* This is the last pass we run before we start emitting stuff. It +* determines when we need to insert boolean resolves on GEN <= 5. We +* run it last because it stashes data in instr->pass_flags and we don't +* want that to be squashed by other NIR passes. +*/ + if (brw->gen <= 5) + brw_nir_analize_boolean_resolves(nir); + /* emit the arrays used for inputs and outputs - load/store intrinsics will * be converted to reads/writes of these arrays */ @@ -1263,6 +1272,18 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) default: unreachable("unhandled instruction"); } + + /* If we need to do a boolean resolve, replace the result with -(x & 1) +* to convert it from junk in the top 31 bits and the actual boolean in +* the bottom bit to the NIR standard of 0/~0. +*/ + if (brw->gen <= 5 && + (instr->instr.pass_flags & BRW_NIR_BOOLEAN_MASK) == BRW_NIR_BOOLEAN_NEEDS_RESOLVE) { + fs_reg masked = vgrf(glsl_type::int_type); + emit(AND(masked, result, fs_reg(1))); + masked.negate = true; + emit(MOV(result, masked)); + } } fs_reg -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/9] Various NIR fixes
This series contains a variety of fixes for NIR. One of them (by Matt) is a substantial shader-db fix. Thanks, Matt, for figuring out a much easier way to do that. A bunch of the rest are to get NIR working on older gens. With this series, NIR looks good on Jenkins. Patches 2 and 9 are to the way that we determine whether or not to use NIR. The first makes it substantially smarter so that we can use INTEL_USE_NIR as either a force-enable or force-disable. The last patch (number 9) enables NIR by default for fragment shaders on SNB+. I'm not sure that we actually want to do this now, but the shader-db numbers are looking pretty good on SNB+ so I figured I'd send the patch. On ILK-, the numbers aren't quite so good. I'm going to look that tonight or tomorrow. Jason Ekstrand (8): i965/nir: Make our environment variable checking smarter i965/fs: Make emit_lrp return an fs_inst i965/nir: Use emit_lrp for emitting flrp i965/nir: Emit MUL + ADD for MAD on GEN <= 5 i965/nir: Properly set the predicate on the SEL used in min/max i965: Add a NIR analysis pass for determining when a boolean resolve is needed i965/nir: Do boolean resolves on GEN <= 5 i965: Use NIR by default for Fragment shaders on SNB+ Matt Turner (1): i965/cmod_propagation: Ignore type in cmod prop if scan_inst is cmp src/mesa/drivers/dri/i965/Makefile.sources | 2 + src/mesa/drivers/dri/i965/brw_fs.cpp | 24 ++- src/mesa/drivers/dri/i965/brw_fs.h | 4 +- .../drivers/dri/i965/brw_fs_cmod_propagation.cpp | 9 + src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 34 ++- src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 6 +- src/mesa/drivers/dri/i965/brw_nir.h| 45 .../dri/i965/brw_nir_analize_boolean_resolves.c| 228 + 8 files changed, 342 insertions(+), 10 deletions(-) create mode 100644 src/mesa/drivers/dri/i965/brw_nir.h create mode 100644 src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 6/9] i965/nir: Properly set the predicate on the SEL used in min/max
--- src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp index 41f9ae2..91a3f65 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp @@ -1132,6 +1132,7 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) } else { emit(CMP(reg_null_d, op[0], op[1], BRW_CONDITIONAL_L)); inst = emit(SEL(result, op[0], op[1])); + inst->predicate = BRW_PREDICATE_NORMAL; } inst->saturate = instr->dest.saturate; break; @@ -1145,6 +1146,7 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) } else { emit(CMP(reg_null_d, op[0], op[1], BRW_CONDITIONAL_GE)); inst = emit(SEL(result, op[0], op[1])); + inst->predicate = BRW_PREDICATE_NORMAL; } inst->saturate = instr->dest.saturate; break; -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/9] i965/cmod_propagation: Ignore type in cmod prop if scan_inst is cmp
From: Matt Turner Shader-db results for FS instructions with NIR on HSW: total instructions in shared programs: 4186747 -> 4129871 (-1.36%) instructions in affected programs: 2438094 -> 2381218 (-2.33%) helped:13525 HURT: 0 GAINED:1 LOST: 5 Revewed-by: Jason Ekstrand --- src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp | 9 + 1 file changed, 9 insertions(+) diff --git a/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp index 1935f06..1b24358 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_cmod_propagation.cpp @@ -94,6 +94,15 @@ opt_cmod_propagation_local(bblock_t *block) scan_inst->dst.reg_offset != inst->src[0].reg_offset) break; +if (inst->conditional_mod == BRW_CONDITIONAL_NZ && +scan_inst->opcode == BRW_OPCODE_CMP && +(inst->dst.type == BRW_REGISTER_TYPE_D || + inst->dst.type == BRW_REGISTER_TYPE_UD)) { + inst->remove(block); + progress = true; + break; +} + /* This must be done before the dst.type check because the result * type of the AND will always be D, but the result of the CMP * could be anything. The assumption is that the AND is just -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 7/9] i965: Add a NIR analysis pass for determining when a boolean resolve is needed
--- src/mesa/drivers/dri/i965/Makefile.sources | 2 + src/mesa/drivers/dri/i965/brw_nir.h| 45 .../dri/i965/brw_nir_analize_boolean_resolves.c| 228 + 3 files changed, 275 insertions(+) create mode 100644 src/mesa/drivers/dri/i965/brw_nir.h create mode 100644 src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources index c69441b..05ebbe9 100644 --- a/src/mesa/drivers/dri/i965/Makefile.sources +++ b/src/mesa/drivers/dri/i965/Makefile.sources @@ -73,6 +73,8 @@ i965_FILES = \ brw_meta_util.h \ brw_misc_state.c \ brw_multisample_state.h \ + brw_nir.h \ + brw_nir_analize_boolean_resolves.c \ brw_object_purgeable.c \ brw_packed_float.c \ brw_performance_monitor.c \ diff --git a/src/mesa/drivers/dri/i965/brw_nir.h b/src/mesa/drivers/dri/i965/brw_nir.h new file mode 100644 index 000..f79c5f1 --- /dev/null +++ b/src/mesa/drivers/dri/i965/brw_nir.h @@ -0,0 +1,45 @@ +/* + * Copyright © 2015 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. + */ + +#pragma once + +#include "glsl/nir/nir.h" + +#ifdef __cplusplus +extern "C" { +#endif + +/* Flags set in the instr->pass_flags field by i965 analysis passes */ +enum { + BRW_NIR_NON_BOOLEAN = 0x0, + BRW_NIR_BOOLEAN_NEEDS_RESOLVE = 0x1, + BRW_NIR_BOOLEAN_UNRESOLVED= 0x2, + BRW_NIR_BOOLEAN_NO_RESOLVE= 0x3, + BRW_NIR_BOOLEAN_MASK = 0x3, +}; + +void brw_nir_analize_boolean_resolves(nir_shader *nir); + +#ifdef __cplusplus +} +#endif diff --git a/src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c b/src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c new file mode 100644 index 000..7d93471 --- /dev/null +++ b/src/mesa/drivers/dri/i965/brw_nir_analize_boolean_resolves.c @@ -0,0 +1,228 @@ +/* + * Copyright © 2015 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. + * + * Authors: + *Jason Ekstrand + */ + +#include "brw_nir.h" + +/* + * This file implements an analysis pass that determines when we have to do + * a boolean resolve on GEN <= 5. Instructions that need a boolean resolve + * will have the booleans portion of the instr->pass_flags field set to + * BRW_NIR_BOOLEAN_NEEDS_RESOLVE. + */ + +static uint8_t +get_resolve_state_for_src(nir_alu_instr *alu, unsigned src_idx) +{ + nir_instr *src_instr; + if (alu->src[src_idx].src.is_ssa) { + src_instr = alu->src[src_idx].src.ssa->parent_instr; + } else { + src_instr = alu->src[src_idx].src.reg.reg->parent_instr; + } + + if (src_instr) { + uint8_t state = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK; + + /* If the source instruction nees resolve then, from the per
[Mesa-dev] [PATCH 4/9] i965/nir: Use emit_lrp for emitting flrp
--- src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp index a9e75ab..5da8423 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp @@ -1239,8 +1239,7 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) break; case nir_op_flrp: - /* TODO emulate for gen < 6 */ - inst = emit(LRP(result, op[2], op[1], op[0])); + inst = emit_lrp(result, op[0], op[1], op[2]); inst->saturate = instr->dest.saturate; break; -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/9] i965/nir: Make our environment variable checking smarter
Before, we enabled NIR if you set INTEL_USE_NIR to anything which mean that INTEL_USE_NIR=false would actually turn on NIR. In preparation for turning NIR on by default, this commit makes it smarter by allowing the INTEL_USE_NIR variable to work as either a force-enable or a force-disable. --- src/mesa/drivers/dri/i965/brw_fs.cpp | 24 ++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 53ceb29..3d4d31a 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3838,6 +3838,26 @@ fs_visitor::allocate_registers() prog_data->total_scratch = brw_get_scratch_size(last_scratch); } +static bool +env_var_as_boolean(const char *var_name, bool default_value) +{ + const char *str = getenv(var_name); + if (str == NULL) + return default_value; + + if (strcmp(str, "1") == 0 || + strcasecmp(str, "true") == 0 || + strcasecmp(str, "yes") == 0) { + return true; + } else if (strcmp(str, "0") == 0 || + strcasecmp(str, "false") == 0 || + strcasecmp(str, "no") == 0) { + return false; + } else { + return default_value; + } +} + bool fs_visitor::run_vs() { @@ -3849,7 +3869,7 @@ fs_visitor::run_vs() if (INTEL_DEBUG & DEBUG_SHADER_TIME) emit_shader_time_begin(); - if (getenv("INTEL_USE_NIR") != NULL) { + if (env_var_as_boolean("INTEL_USE_NIR", false)) { emit_nir_code(); } else { foreach_in_list(ir_instruction, ir, shader->base.ir) { @@ -3923,7 +3943,7 @@ fs_visitor::run_fs() * functions called "main"). */ if (shader) { - if (getenv("INTEL_USE_NIR") != NULL) { + if (env_var_as_boolean("INTEL_USE_NIR", false)) { emit_nir_code(); } else { foreach_in_list(ir_instruction, ir, shader->base.ir) { -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/9] i965/fs: Make emit_lrp return an fs_inst
--- src/mesa/drivers/dri/i965/brw_fs.h | 4 ++-- src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h index 7716529..a520fd4 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.h +++ b/src/mesa/drivers/dri/i965/brw_fs.h @@ -303,8 +303,8 @@ public: fs_reg fix_math_operand(fs_reg src); fs_inst *emit_math(enum opcode op, fs_reg dst, fs_reg src0); fs_inst *emit_math(enum opcode op, fs_reg dst, fs_reg src0, fs_reg src1); - void emit_lrp(const fs_reg &dst, const fs_reg &x, const fs_reg &y, - const fs_reg &a); + fs_inst *emit_lrp(const fs_reg &dst, const fs_reg &x, const fs_reg &y, + const fs_reg &a); void emit_minmax(enum brw_conditional_mod conditionalmod, const fs_reg &dst, const fs_reg &src0, const fs_reg &src1); bool try_emit_saturate(ir_expression *ir); diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp index 6d56115..2076695 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp @@ -288,7 +288,7 @@ fs_visitor::visit(ir_dereference_array *ir) this->result = src; } -void +fs_inst * fs_visitor::emit_lrp(const fs_reg &dst, const fs_reg &x, const fs_reg &y, const fs_reg &a) { @@ -305,12 +305,12 @@ fs_visitor::emit_lrp(const fs_reg &dst, const fs_reg &x, const fs_reg &y, emit(ADD(one_minus_a, negative_a, fs_reg(1.0f))); emit(MUL(x_times_one_minus_a, x, one_minus_a)); - emit(ADD(dst, x_times_one_minus_a, y_times_a)); + return emit(ADD(dst, x_times_one_minus_a, y_times_a)); } else { /* The LRP instruction actually does op1 * op0 + op2 * (1 - op0), so * we need to reorder the operands. */ - emit(LRP(dst, a, y, x)); + return emit(LRP(dst, a, y, x)); } } -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 5/9] i965/nir: Emit MUL + ADD for MAD on GEN <= 5
--- src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp index 5da8423..41f9ae2 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp @@ -1234,7 +1234,13 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) break; case nir_op_ffma: - inst = emit(MAD(result, op[2], op[1], op[0])); + if (brw->gen >= 6) { + inst = emit(MAD(result, op[2], op[1], op[0])); + } else { + fs_reg temp = vgrf(glsl_type::float_type); + emit(MUL(temp, op[0], op[1])); + inst = emit(ADD(result, temp, op[2])); + } inst->saturate = instr->dest.saturate; break; -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 9/9] i965: Use NIR by default for Fragment shaders on SNB+
GLSL IR vs. NIR FS instructions on snb: total instructions in shared programs: 4976784 -> 4973072 (-0.07%) instructions in affected programs: 3309521 -> 3305809 (-0.11%) helped:7161 HURT: 9839 GAINED:55 LOST: 23 GLSL IR vs. NIR FS instructions on ivb: total instructions in shared programs: 4593938 -> 4588496 (-0.12%) instructions in affected programs: 2948822 -> 2943380 (-0.18%) helped:6878 HURT: 8801 GAINED:69 LOST: 44 GLSL IR vs. NIR FS instructions on hsw: total instructions in shared programs: 4089459 -> 4081368 (-0.20%) instructions in affected programs: 2592474 -> 2584383 (-0.31%) helped:6492 HURT: 9403 GAINED:69 LOST: 32 GLSL IR vs. NIR FS instructions on bdw: total instructions in shared programs: 4065050 -> 4058653 (-0.16%) instructions in affected programs: 255 -> 2549158 (-0.25%) helped:5910 HURT: 9765 GAINED:45 LOST: 53 --- src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp index 3d4d31a..42f6604 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp @@ -3943,7 +3943,7 @@ fs_visitor::run_fs() * functions called "main"). */ if (shader) { - if (env_var_as_boolean("INTEL_USE_NIR", false)) { + if (env_var_as_boolean("INTEL_USE_NIR", brw->gen >= 6)) { emit_nir_code(); } else { foreach_in_list(ir_instruction, ir, shader->base.ir) { -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/cmod_propagation: Ignore type in cmod prop if scan_inst is cmp
On Tue, Mar 17, 2015 at 7:17 PM, Jason Ekstrand wrote: > From: Matt Turner > > Shader-db results for FS instructions with NIR on HSW: > > total instructions in shared programs: 4186747 -> 4129871 (-1.36%) > instructions in affected programs: 2438094 -> 2381218 (-2.33%) > helped:13525 > HURT: 0 > GAINED:1 > LOST: 5 > > Revewed-by: Jason Ekstrand > --- Thanks for sending this out. I think we should just make the subject prefix i965/fs. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/cmod_propagation: Ignore type in cmod prop if scan_inst is cmp
On Tue, Mar 17, 2015 at 7:21 PM, Matt Turner wrote: > On Tue, Mar 17, 2015 at 7:17 PM, Jason Ekstrand wrote: >> From: Matt Turner >> >> Shader-db results for FS instructions with NIR on HSW: >> >> total instructions in shared programs: 4186747 -> 4129871 (-1.36%) >> instructions in affected programs: 2438094 -> 2381218 (-2.33%) >> helped:13525 >> HURT: 0 >> GAINED:1 >> LOST: 5 >> >> Revewed-by: Jason Ekstrand >> --- > > Thanks for sending this out. > > I think we should just make the subject prefix i965/fs. How about "i965/fs: Ignore type in cmod prop if scan_inst is cmp" --Jason ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 Bug ID: 89622 Summary: Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE Product: Mesa Version: git Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Other Assignee: mesa-dev@lists.freedesktop.org Reporter: daniel.seb...@ieee.org QA Contact: mesa-dev@lists.freedesktop.org First, I want to point out that building Mesa from scratch *without* the options --with-dri-drivers=nouveau --with-gallium-drivers=nouveau produces no nouveau_dri.so library file. There is a nouveau_vieux_dri.so, but no nouveau_dri.so. When I then run an app that accesses this newly built mesa library files I see the warning message: libGL error: unable to load driver: nouveau_dri.so libGL error: driver pointer missing libGL error: failed to load driver: nouveau It's no harm of course, but if this should not be happening--given nourveau_dri.so--wasn't built, I'll file a bug report. OK, I filed a bug report about vertical white lines in the legacy swrast_dri.so associated with intervals of GL_MAX_TEXTURE_SIZE in the x dimension of the input image: https://bugs.freedesktop.org/show_bug.cgi?id=89586 I then went to try the newer Gallium version of the software implementation and found that a limitation is placed on the size of the input image to GL_MAX_TEXTURE_SIZE. This can be seen in frame buffer images I will attach, Screenshot-incomplete_image_x-dimension.png and Screenshot-incomplete_image_y-dimension.png where only a portion of the gradient image appears. The source of this limitation is in src/mesa/state_tracker/st_cb_drawpixels.c. Out of curiousity, I removed the limitation as follows: --- a/src/mesa/state_tracker/st_cb_drawpixels.c +++ b/src/mesa/state_tracker/st_cb_drawpixels.c @@ -1110,14 +1110,6 @@ st_DrawPixels(struct gl_context *ctx, GLint x, GLint y, st_validate_state(st); - /* Limit the size of the glDrawPixels to the max texture size. -* Strictly speaking, that's not correct but since we don't handle -* larger images yet, this is better than crashing. -*/ - clippedUnpack = *unpack; - unpack = &clippedUnpack; - clamp_size(st->pipe, &width, &height, &clippedUnpack); - if (format == GL_DEPTH_STENCIL) write_stencil = write_depth = GL_TRUE; else if (format == GL_STENCIL_INDEX) and the result is that the image comes out fine. So, what is the problem here? Is it that not all systems are ensured to have non-paged memory, and my system just happens to have it? Can this be tested and adjusted accordingly? More generally, it seems that the Gallium version of pixel zoom/draw can be done in a more straightforward way than legacy swrast is doing. Notice that Gallium is treating the image as blocks (i.e., the clip affects both width and height). It shouldn't be difficult to write a double loop to write to the frame buffer in blocks (unless there is a problem of not being able to select the memory page in the st_DrawPixels() routine because that is done prior to the function call). What role does the st->pipe play in affecting size of writes? -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 Dan Sebald changed: What|Removed |Added CC||daniel.seb...@ieee.org --- Comment #1 from Dan Sebald --- Created attachment 114408 --> https://bugs.freedesktop.org/attachment.cgi?id=114408&action=edit Clipping in the x-dimension -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #2 from Dan Sebald --- Created attachment 114409 --> https://bugs.freedesktop.org/attachment.cgi?id=114409&action=edit Clipping in the y-dimension -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #3 from Dan Sebald --- Created attachment 114410 --> https://bugs.freedesktop.org/attachment.cgi?id=114410&action=edit Fixed image in x-dimension -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #4 from Dan Sebald --- Created attachment 114411 --> https://bugs.freedesktop.org/attachment.cgi?id=114411&action=edit Fixed image in y-dimension -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/cmod_propagation: Ignore type in cmod prop if scan_inst is cmp
On Tue, Mar 17, 2015 at 7:22 PM, Jason Ekstrand wrote: > On Tue, Mar 17, 2015 at 7:21 PM, Matt Turner wrote: >> On Tue, Mar 17, 2015 at 7:17 PM, Jason Ekstrand wrote: >>> From: Matt Turner >>> >>> Shader-db results for FS instructions with NIR on HSW: >>> >>> total instructions in shared programs: 4186747 -> 4129871 (-1.36%) >>> instructions in affected programs: 2438094 -> 2381218 (-2.33%) >>> helped:13525 >>> HURT: 0 >>> GAINED:1 >>> LOST: 5 >>> >>> Revewed-by: Jason Ekstrand >>> --- >> >> Thanks for sending this out. >> >> I think we should just make the subject prefix i965/fs. > > How about "i965/fs: Ignore type in cmod prop if scan_inst is cmp" Sounds good. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/9] i965/cmod_propagation: Ignore type in cmod prop if scan_inst is cmp
On Tue, Mar 17, 2015 at 7:28 PM, Matt Turner wrote: > On Tue, Mar 17, 2015 at 7:22 PM, Jason Ekstrand wrote: >> On Tue, Mar 17, 2015 at 7:21 PM, Matt Turner wrote: >>> On Tue, Mar 17, 2015 at 7:17 PM, Jason Ekstrand >>> wrote: From: Matt Turner Shader-db results for FS instructions with NIR on HSW: total instructions in shared programs: 4186747 -> 4129871 (-1.36%) instructions in affected programs: 2438094 -> 2381218 (-2.33%) helped:13525 HURT: 0 GAINED:1 LOST: 5 Revewed-by: Jason Ekstrand --- >>> >>> Thanks for sending this out. >>> >>> I think we should just make the subject prefix i965/fs. >> >> How about "i965/fs: Ignore type in cmod prop if scan_inst is cmp" > > Sounds good. Done ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/2] mesa: remove MSVC warning pragmas
Removing this block of pragmas doesn't seem to increase the number of warning generated by MSVC. Other than signed/unsigned comparison warnings there's very few other warnings nowadays. --- src/mesa/main/compiler.h | 20 1 file changed, 20 deletions(-) diff --git a/src/mesa/main/compiler.h b/src/mesa/main/compiler.h index 5c60391..55152fd 100644 --- a/src/mesa/main/compiler.h +++ b/src/mesa/main/compiler.h @@ -60,26 +60,6 @@ extern "C" { #endif -/** - * Disable assorted warnings - */ -#if defined(_WIN32) && !defined(__CYGWIN__) -# if !defined(__GNUC__) /* mingw environment */ -#pragma warning( disable : 4068 ) /* unknown pragma */ -#pragma warning( disable : 4710 ) /* function 'foo' not inlined */ -#pragma warning( disable : 4711 ) /* function 'foo' selected for automatic inline expansion */ -#pragma warning( disable : 4127 ) /* conditional expression is constant */ -#if defined(MESA_MINWARN) -# pragma warning( disable : 4244 ) /* '=' : conversion from 'const double ' to 'float ', possible loss of data */ -# pragma warning( disable : 4018 ) /* '<' : signed/unsigned mismatch */ -# pragma warning( disable : 4305 ) /* '=' : truncation from 'const double ' to 'float ' */ -# pragma warning( disable : 4550 ) /* 'function' undefined; assuming extern returning int */ -# pragma warning( disable : 4761 ) /* integral size mismatch in argument; conversion supplied */ -#endif -# endif -#endif - - /* XXX: Use standard `__func__` instead */ #ifndef __FUNCTION__ # define __FUNCTION__ __func__ -- 1.9.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] mesa: add void to format_array_format_table_init() declaration
Silences an MSVC warning where it's called from call_once(). --- src/mesa/main/formats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c index 422c9dc..2bc8bca 100644 --- a/src/mesa/main/formats.c +++ b/src/mesa/main/formats.c @@ -388,7 +388,7 @@ array_formats_equal(const void *a, const void *b) } static void -format_array_format_table_init() +format_array_format_table_init(void) { const struct gl_format_info *info; mesa_array_format array_format; -- 1.9.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] mapi: add new _glapi_new_nop_table() and _glapi_set_nop_handler()
Ping - on this series. -Brian On 03/13/2015 01:22 PM, Brian Paul wrote: _glapi_new_nop_table() creates a new dispatch table populated with pointers to no-op functions. _glapi_set_nop_handler() is used to register a callback function which will be called from each of the no-op functions. Now we always generate a separate no-op function for each GL entrypoint. This allows us to do proper stack clean-up for Windows __stdcall and lets us report the actual function name in error messages. Before this change, for non-Windows release builds we used a single no-op function for all entrypoints. --- src/mapi/glapi/glapi.h | 11 ++ src/mapi/glapi/glapi_nop.c | 84 ++ src/mapi/mapi_glapi.c | 23 + src/mapi/table.c | 23 ++--- src/mapi/table.h | 8 + 5 files changed, 108 insertions(+), 41 deletions(-) diff --git a/src/mapi/glapi/glapi.h b/src/mapi/glapi/glapi.h index 8d991fb..673295b 100644 --- a/src/mapi/glapi/glapi.h +++ b/src/mapi/glapi/glapi.h @@ -80,6 +80,9 @@ extern "C" { #endif typedef void (*_glapi_proc)(void); + +typedef void (*_glapi_nop_handler_proc)(const char *name); + struct _glapi_table; @@ -159,6 +162,14 @@ _GLAPI_EXPORT struct _glapi_table * _glapi_create_table_from_handle(void *handle, const char *symbol_prefix); +_GLAPI_EXPORT void +_glapi_set_nop_handler(_glapi_nop_handler_proc func); + +/** Return pointer to new dispatch table filled with no-op functions */ +_GLAPI_EXPORT struct _glapi_table * +_glapi_new_nop_table(unsigned num_entries); + + /** Deprecated function */ _GLAPI_EXPORT unsigned long _glthread_GetID(void); diff --git a/src/mapi/glapi/glapi_nop.c b/src/mapi/glapi/glapi_nop.c index 628276e..0041ed5 100644 --- a/src/mapi/glapi/glapi_nop.c +++ b/src/mapi/glapi/glapi_nop.c @@ -30,14 +30,21 @@ * This file defines a special dispatch table which is loaded with no-op * functions. * - * When there's no current rendering context, calling a GL function like - * glBegin() is a no-op. Apps should never normally do this. So as a - * debugging aid, each of the no-op functions will emit a warning to - * stderr if the MESA_DEBUG or LIBGL_DEBUG env var is set. + * Mesa can register a "no-op handler function" which will be called in + * the event that a no-op function is called. + * + * In the past, the dispatch table was loaded with pointers to a single + * no-op function. But that broke on Windows because the GL entrypoints + * use __stdcall convention. __stdcall means the callee cleans up the + * stack. So one no-op function can't properly clean up the stack. This + * would lead to crashes. + * + * Another benefit of unique no-op functions is we can accurately report + * the function's name in an error message. */ - +#include #include "glapi/glapi_priv.h" @@ -51,25 +58,32 @@ _glapi_set_warning_func(_glapi_proc func) { } -/* - * When GLAPIENTRY is __stdcall (i.e. Windows), the stack is popped by the - * callee making the number/type of arguments significant. + +/** + * We'll jump though this function pointer whenever a no-op function + * is called. */ -#if defined(_WIN32) || defined(DEBUG) +static _glapi_nop_handler_proc nop_handler = NULL; + + +/** + * Register the no-op handler call-back function. + */ +void +_glapi_set_nop_handler(_glapi_nop_handler_proc func) +{ + nop_handler = func; +} + /** * Called by each of the no-op GL entrypoints. */ -static int -Warn(const char *func) +static void +nop(const char *func) { -#if defined(DEBUG) - if (getenv("MESA_DEBUG") || getenv("LIBGL_DEBUG")) { - fprintf(stderr, "GL User Error: gl%s called without a rendering context\n", - func); - } -#endif - return 0; + if (nop_handler) + nop_handler(func); } @@ -79,7 +93,8 @@ Warn(const char *func) static GLint NoOpUnused(void) { - return Warn(" function"); + nop("unused GL entry point"); + return 0; } /* @@ -89,31 +104,28 @@ NoOpUnused(void) #define KEYWORD1_ALT static #define KEYWORD2 GLAPIENTRY #define NAME(func) NoOp##func -#define DISPATCH(func, args, msg) Warn(#func); -#define RETURN_DISPATCH(func, args, msg) Warn(#func); return 0 +#define DISPATCH(func, args, msg) nop(#func); +#define RETURN_DISPATCH(func, args, msg) nop(#func); return 0 /* * Defines for the table of no-op entry points. */ #define TABLE_ENTRY(name) (_glapi_proc) NoOp##name +#define DISPATCH_TABLE_NAME __glapi_noop_table +#define UNUSED_TABLE_NAME __unused_noop_functions + +#include "glapi/glapitemp.h" -#else -static int -NoOpGeneric(void) +/** Return pointer to new dispatch table filled with no-op functions */ +struct _glapi_table * +_glapi_new_nop_table(unsigned num_entries) { - if (getenv("MESA_DEBUG") || getenv("LIBGL_DEBUG")) { - fprintf(stderr, "GL User Error: calling GL function without a rendering context\n"); + struct _glapi_table *table = malloc(num_entries * sizeof(_glapi
Re: [Mesa-dev] [PATCH 5/9] i965/nir: Emit MUL + ADD for MAD on GEN <= 5
Instead of doing this, I think it would be better to add something to nir_shader_compiler_options and do it in nir_opt_algebraic, similar to what Eric has already done for other things. I've seen some shaders where we transform a mul + a series of adds into a series of mad's, which is good on gen6+ (at the very least, we reduce sched dependencies, and we may be able to eliminate the mul) but if we do that and then just naively lower it back we'll get worse results than we started with. Similar thing with the previous lrp patch. On Tue, Mar 17, 2015 at 10:17 PM, Jason Ekstrand wrote: > --- > src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > index 5da8423..41f9ae2 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp > @@ -1234,7 +1234,13 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) >break; > > case nir_op_ffma: > - inst = emit(MAD(result, op[2], op[1], op[0])); > + if (brw->gen >= 6) { > + inst = emit(MAD(result, op[2], op[1], op[0])); > + } else { > + fs_reg temp = vgrf(glsl_type::float_type); > + emit(MUL(temp, op[0], op[1])); > + inst = emit(ADD(result, temp, op[2])); > + } >inst->saturate = instr->dest.saturate; >break; > > -- > 2.3.2 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] radeonsi: increase coords array for radeon_llvm_emit_prepare_cube_coords
On 18.03.2015 01:53, Marek Olšák wrote: > From: Marek Olšák > > radeon_llvm_emit_prepare_cube_coords uses coords[4] in some cases (TXB2 etc.) > > Discovered by Coverity. Reported by Ilia Mirkin. > > Cc: > --- > src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c | 2 +- > src/gallium/drivers/radeonsi/si_shader.c| 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > index d89e2b4..1690194 100644 > --- a/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > +++ b/src/gallium/drivers/radeon/radeon_setup_tgsi_llvm.c > @@ -748,7 +748,7 @@ static void txp_fetch_args( > const struct tgsi_full_instruction * inst = emit_data->inst; > LLVMValueRef src_w; > unsigned chan; > - LLVMValueRef coords[4]; > + LLVMValueRef coords[5]; > > emit_data->dst_type = LLVMVectorType(bld_base->base.elem_type, 4); > src_w = lp_build_emit_fetch(bld_base, emit_data->inst, 0, TGSI_CHAN_W); > diff --git a/src/gallium/drivers/radeonsi/si_shader.c > b/src/gallium/drivers/radeonsi/si_shader.c > index de889ed..4dcf756 100644 > --- a/src/gallium/drivers/radeonsi/si_shader.c > +++ b/src/gallium/drivers/radeonsi/si_shader.c > @@ -1572,7 +1572,7 @@ static void tex_fetch_args( > const struct tgsi_full_instruction * inst = emit_data->inst; > unsigned opcode = inst->Instruction.Opcode; > unsigned target = inst->Texture.Texture; > - LLVMValueRef coords[4]; > + LLVMValueRef coords[5]; > LLVMValueRef address[16]; > int ref_pos; > unsigned num_coords = tgsi_util_get_texture_coord_dim(target, &ref_pos); > Reviewed-by: Michel Dänzer -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] mesa: remove MSVC warning pragmas
Acked-by: Matt Turner ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] mesa: add void to format_array_format_table_init() declaration
Reviewed-by: Matt Turner ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #5 from Ilia Mirkin --- (In reply to Dan Sebald from comment #0) > First, I want to point out that building Mesa from scratch *without* the > options > > --with-dri-drivers=nouveau --with-gallium-drivers=nouveau > > produces no nouveau_dri.so library file. There is a nouveau_vieux_dri.so, > but no nouveau_dri.so. When I then run an app that accesses this newly > built mesa library files I see the warning message: > > libGL error: unable to load driver: nouveau_dri.so > libGL error: driver pointer missing > libGL error: failed to load driver: nouveau > > It's no harm of course, but if this should not be happening--given > nourveau_dri.so--wasn't built, I'll file a bug report. libGL has no idea what's built. It's told by your X server (via DRI2) that it should look for a DRI driver called "nouveau", so it looks for "nouveau_dri.so". > OK, I filed a bug report about vertical white lines in the legacy > swrast_dri.so associated with intervals of GL_MAX_TEXTURE_SIZE in the x > dimension of the input image: > > https://bugs.freedesktop.org/show_bug.cgi?id=89586 > > I then went to try the newer Gallium version of the software implementation > and found that a limitation is placed on the size of the input image to > GL_MAX_TEXTURE_SIZE. This can be seen in frame buffer images I will attach, > Screenshot-incomplete_image_x-dimension.png and > Screenshot-incomplete_image_y-dimension.png where only a portion of the > gradient image appears. The source of this limitation is in > src/mesa/state_tracker/st_cb_drawpixels.c. Out of curiousity, I removed the > limitation as follows: > > --- a/src/mesa/state_tracker/st_cb_drawpixels.c > +++ b/src/mesa/state_tracker/st_cb_drawpixels.c > @@ -1110,14 +1110,6 @@ st_DrawPixels(struct gl_context *ctx, GLint x, GLint > y, > > st_validate_state(st); > > - /* Limit the size of the glDrawPixels to the max texture size. > -* Strictly speaking, that's not correct but since we don't handle > -* larger images yet, this is better than crashing. > -*/ > - clippedUnpack = *unpack; > - unpack = &clippedUnpack; > - clamp_size(st->pipe, &width, &height, &clippedUnpack); > - > if (format == GL_DEPTH_STENCIL) >write_stencil = write_depth = GL_TRUE; > else if (format == GL_STENCIL_INDEX) > > and the result is that the image comes out fine. > > So, what is the problem here? Is it that not all systems are ensured to > have non-paged memory, and my system just happens to have it? Can this be > tested and adjusted accordingly? More generally, it seems that the Gallium > version of pixel zoom/draw can be done in a more straightforward way than > legacy swrast is doing. Notice that Gallium is treating the image as blocks > (i.e., the clip affects both width and height). It shouldn't be difficult > to write a double loop to write to the frame buffer in blocks (unless there > is a problem of not being able to select the memory page in the > st_DrawPixels() routine because that is done prior to the function call). > What role does the st->pipe play in affecting size of writes? For those of us still getting with the program... let's say that max_texture_size = 128 (for simplicity). Is the situation that you have a (say) 256-pixel wide image that you're feeding to glDrawPixels to be put onto a 128-wide texture, and you're using glPixelZoom(0.5) to achieve this? -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89624] Drivers, Gallium/legacy swrast glDrawPixels differences
https://bugs.freedesktop.org/show_bug.cgi?id=89624 Bug ID: 89624 Summary: Drivers, Gallium/legacy swrast glDrawPixels differences Product: Mesa Version: git Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Other Assignee: mesa-dev@lists.freedesktop.org Reporter: daniel.seb...@ieee.org QA Contact: mesa-dev@lists.freedesktop.org In the following two bug reports: https://bugs.freedesktop.org/show_bug.cgi?id=89586 https://bugs.freedesktop.org/show_bug.cgi?id=89622 I discussed some artifacts concerning pixels at or beyond GL_MAX_TEXTURE_SIZE in the input image. In the first bug report I attached a patch that fixes the vertical lines in legacy swrast. In the second I simply noted that Gallium swrast limits the image, and I think it wouldn't be too difficult to correct that. Here I want to point out the subtle difference between legacy swrast (post patch in bug #89586) and the Gallium swrast using images with only 20 x 20 input pixels. For legacy swrast, the attached image Screenshot-differences_legacy_swrast-annotated.png shows how the scaled image can extend past the edge of the boundary in the x-dimension, or not (and it never falls short of the boundary). On the other hand, the image will fall short of boundary in the y-dimension, or not (and it never extends past). I'm guessing this behavior has to do with xfactor > 0 and yfactor < 0. For Gallium swrast, the attached image Screenshot-differences_gallium-annotated.png illustrates that scaled image aligns with the x-dimension edge (an it always seems to do so). However, the image can extend past the bottom in the y-dimension, or not (and it seems to never fall short). Since these two behaviors are different, as subtle as it may be, at least one of them has a bug. I suspect that both of them might have a flaw as far as precisely following OpenGl standard. It's interesting that the x-dimension for the Gallium driver seems most consistent. Granted, the axis box itself may not be exact, so to say exactly what the bug is at this stage is premature, but at least the behavior of the axis box should be the same when all that is varied is the graphics driver and not the application binary. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89624] Drivers, Gallium/legacy swrast glDrawPixels differences
https://bugs.freedesktop.org/show_bug.cgi?id=89624 Dan Sebald changed: What|Removed |Added CC||daniel.seb...@ieee.org --- Comment #1 from Dan Sebald --- Created attachment 114412 --> https://bugs.freedesktop.org/attachment.cgi?id=114412&action=edit Illustration of legacy driver behavior for glDrawPixels -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89624] Drivers, Gallium/legacy swrast glDrawPixels differences
https://bugs.freedesktop.org/show_bug.cgi?id=89624 --- Comment #2 from Dan Sebald --- Created attachment 114413 --> https://bugs.freedesktop.org/attachment.cgi?id=114413&action=edit Illustration of Gallium driver behavior for glDrawPixels -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #6 from Dan Sebald --- Correct. -1 < xfactor < 1, excluding 0. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #7 from Dan Sebald --- Oh, and attempting to load suitable driver first is fair. -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 8.5/9] i965/nir: Use signed integer type for booleans
FS instructions with NIR on i965: total instructions in shared programs: 2663561 -> 2619051 (-1.67%) instructions in affected programs: 1612965 -> 1568455 (-2.76%) helped:5455 HURT: 12 FS instructions with NIR on g4x: total instructions in shared programs: 2352633 -> 2307908 (-1.90%) instructions in affected programs: 1441842 -> 1397117 (-3.10%) helped:5463 HURT: 11 FS instructions with NIR on ilk: total instructions in shared programs: 3997305 -> 3934278 (-1.58%) instructions in affected programs: 2189409 -> 2126382 (-2.88%) helped:8969 HURT: 22 FS instructions with NIR on hsw (snb and ivb were similar): total instructions in shared programs: 4109389 -> 4109242 (-0.00%) instructions in affected programs: 109869 -> 109722 (-0.13%) helped:339 HURT: 190 No SIMD16 programs were gained or lost on any platform More signed integers --- src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp index 1ef4602..69c5680 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp @@ -518,7 +518,7 @@ fs_visitor::nir_emit_if(nir_if *if_stmt) /* first, put the condition into f0 */ fs_inst *inst = emit(MOV(reg_null_d, retype(get_nir_src(if_stmt->condition), - BRW_REGISTER_TYPE_UD))); + BRW_REGISTER_TYPE_D))); inst->conditional_mod = BRW_CONDITIONAL_NZ; emit(IF(BRW_PREDICATE_NORMAL)); @@ -594,9 +594,9 @@ static brw_reg_type brw_type_for_nir_type(nir_alu_type type) { switch (type) { - case nir_type_bool: case nir_type_unsigned: return BRW_REGISTER_TYPE_UD; + case nir_type_bool: case nir_type_int: return BRW_REGISTER_TYPE_D; case nir_type_float: @@ -1282,7 +1282,7 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) fs_reg masked = vgrf(glsl_type::int_type); emit(AND(masked, result, fs_reg(1))); masked.negate = true; - emit(MOV(result, masked)); + emit(MOV(retype(result, BRW_REGISTER_TYPE_D), masked)); } } -- 2.3.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 89622] Drivers/Gallium/swrast: Pixel image limited to GL_MAX_TEXTURE_SIZE
https://bugs.freedesktop.org/show_bug.cgi?id=89622 --- Comment #8 from Ilia Mirkin --- (In reply to Dan Sebald from comment #0) > So, what is the problem here? Is it that not all systems are ensured to > have non-paged memory, and my system just happens to have it? Can this be > tested and adjusted accordingly? More generally, it seems that the Gallium > version of pixel zoom/draw can be done in a more straightforward way than > legacy swrast is doing. Notice that Gallium is treating the image as blocks > (i.e., the clip affects both width and height). It shouldn't be difficult > to write a double loop to write to the frame buffer in blocks (unless there > is a problem of not being able to select the memory page in the > st_DrawPixels() routine because that is done prior to the function call). > What role does the st->pipe play in affecting size of writes? Worrying about mmu-less (or non-page-on-demand) issues is not something we do. Basically st_DrawPixels takes the pixel data, sticks it into a texture, and then uses shaders to texture that onto the framebuffer. Obviously the texture can't be larger than MAX_TEXTURE_SIZE. However no harm done if you cut up the source data into MAX_TEXTURE_SIZE-sized blocks and discarded fragments that were not going to be covered by that texture slice (or even better, set the viewport or scissors appropriately). If you were going to be sneaky, you could even concoct some scheme to use a 2d array texture, although ultimately even that has limits. I don't immediately see a way to transfer a non-0,0-origin block of pixels from memory into a texture, but I'm sure it's reasonably easy to do, and a helper probably exists for some other functions. (Maybe it can be specified as a gl_pixelstore_attrib? It has a SkipRows/SkipPixels attribute.) -- 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 http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/9] i965/nir: Emit MUL + ADD for MAD on GEN <= 5
On Tue, Mar 17, 2015 at 7:35 PM, Connor Abbott wrote: > Instead of doing this, I think it would be better to add something to > nir_shader_compiler_options and do it in nir_opt_algebraic, similar to > what Eric has already done for other things. I've seen some shaders > where we transform a mul + a series of adds into a series of mad's, > which is good on gen6+ (at the very least, we reduce sched > dependencies, and we may be able to eliminate the mul) but if we do > that and then just naively lower it back we'll get worse results than > we started with. Similar thing with the previous lrp patch. Yes, that may be better in some cases. I'm still not sure I like the whole pile of booleans thing... > On Tue, Mar 17, 2015 at 10:17 PM, Jason Ekstrand wrote: >> --- >> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 8 +++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> index 5da8423..41f9ae2 100644 >> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp >> @@ -1234,7 +1234,13 @@ fs_visitor::nir_emit_alu(nir_alu_instr *instr) >>break; >> >> case nir_op_ffma: >> - inst = emit(MAD(result, op[2], op[1], op[0])); >> + if (brw->gen >= 6) { >> + inst = emit(MAD(result, op[2], op[1], op[0])); >> + } else { >> + fs_reg temp = vgrf(glsl_type::float_type); >> + emit(MUL(temp, op[0], op[1])); >> + inst = emit(ADD(result, temp, op[2])); >> + } >>inst->saturate = instr->dest.saturate; >>break; >> >> -- >> 2.3.2 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 9/9] i965: Use NIR by default for Fragment shaders on SNB+
I did this for SNB+ only because we did have some shader-db regressions. However, with the patch 8.5 I just sent out, we're down to about +0.2% on ILK-. Arguably, that's probably close enough for the old platforms especially if it lets us move forward. The majority of hurt shaders there are planeshift shaders which are hurt because they use indirect uniforms which causes NIR to do pull constants for everything and that's bad. We need to get our uniform story figured out better for NIR soon. Also, I didn't recommend it for SIMD8 FS on BDW because the shader-db result there are not great. I don't know that I'd say dire, but it needs some work/looking into. --Jason On Tue, Mar 17, 2015 at 7:17 PM, Jason Ekstrand wrote: > GLSL IR vs. NIR FS instructions on snb: > total instructions in shared programs: 4976784 -> 4973072 (-0.07%) > instructions in affected programs: 3309521 -> 3305809 (-0.11%) > helped:7161 > HURT: 9839 > GAINED:55 > LOST: 23 > > GLSL IR vs. NIR FS instructions on ivb: > total instructions in shared programs: 4593938 -> 4588496 (-0.12%) > instructions in affected programs: 2948822 -> 2943380 (-0.18%) > helped:6878 > HURT: 8801 > GAINED:69 > LOST: 44 > > GLSL IR vs. NIR FS instructions on hsw: > total instructions in shared programs: 4089459 -> 4081368 (-0.20%) > instructions in affected programs: 2592474 -> 2584383 (-0.31%) > helped:6492 > HURT: 9403 > GAINED:69 > LOST: 32 > > GLSL IR vs. NIR FS instructions on bdw: > total instructions in shared programs: 4065050 -> 4058653 (-0.16%) > instructions in affected programs: 255 -> 2549158 (-0.25%) > helped:5910 > HURT: 9765 > GAINED:45 > LOST: 53 > --- > src/mesa/drivers/dri/i965/brw_fs.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp > b/src/mesa/drivers/dri/i965/brw_fs.cpp > index 3d4d31a..42f6604 100644 > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp > @@ -3943,7 +3943,7 @@ fs_visitor::run_fs() > * functions called "main"). > */ >if (shader) { > - if (env_var_as_boolean("INTEL_USE_NIR", false)) { > + if (env_var_as_boolean("INTEL_USE_NIR", brw->gen >= 6)) { > emit_nir_code(); > } else { > foreach_in_list(ir_instruction, ir, shader->base.ir) { > -- > 2.3.2 > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/23] mesa: glGetProgramResourceLocationIndex
On 03/17/2015 04:20 PM, Ilia Mirkin wrote: On Tue, Mar 17, 2015 at 5:13 AM, Tapani Pälli wrote: On 03/16/2015 08:08 PM, Ilia Mirkin wrote: On Fri, Mar 13, 2015 at 4:37 AM, Tapani Pälli wrote: +/** + * Returns output index for dual source blending. + */ GLint GLAPIENTRY _mesa_GetProgramResourceLocationIndex(GLuint program, GLenum programInterface, const GLchar *name) { - return -1; + GET_CURRENT_CONTEXT(ctx); + struct gl_shader_program *shProg = + lookup_linked_program(program, "glGetProgramResourceLocationIndex"); + + if (!shProg || invalid_array_element_syntax(name)) + return -1; + + /* From the GL_ARB_program_interface_query spec: +* +* "For GetProgramResourceLocationIndex, must be +* PROGRAM_OUTPUT." +*/ And presumably it must be a program with a fragment shader (which might not be there for a no-rast or compute pipeline). spec says that -1 is returned: "If has been successfully linked but contains no fragment shader, no error will be generated but -1 will be returned." this is what happens with the implementation. Can you explain how the current implementation takes care of that? My earlier suggestion for this was that _mesa_program_resource_location_index returns -1 because resource cannot be found. But now I see that one could call this for any output of any stage and then it would likely return 0 or whatever is the uninitialized state of ir_variable::data.index. Will have to fix this, thanks for spotting this! // Tapani ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 9/9] i965: Use NIR by default for Fragment shaders on SNB+
Thanks. Looked through stats and at some of the regressions. Some of the areas I noticed we were doing worse: We generate two CMPs for discard_if; only one without NIR. I think you had an idea about solving this. SEL peephole interference -- we knew about this in general, but I found an interesting failure mode: NIR generates (+f0) if(8) JIP: 4 UIP: 5 else(8) JIP: 3 mov(8) g124<1>F-g124<8,8,1>F endif(8)JIP: 6 We should of course improve dead-control-flow-elimination to remove the else and set the if's predicate_inverse, but non-NIR generates (+f0) sel(8)g124<1>Fg25<8,8,1>F -g25<8,8,1>F which is probably better even than a predicated MOV since it's not a partial write and subject to optimization restrictions. The assembly matches the NIR exactly, so I'm assuming NIR is removing a self-assignment in the if-then block, preventing it from turning it into a conditional select. Also, since the fs's SEL peephole only recognizes MOVs to the same destination in order, it doesn't handle tropico-5/387 properly, which already improved from 110 to 79 instructions. I also noticed an interesting pattern in defense-grid/537 (and some others, I think). NIR generates mul(8) g4<1>Fg2<8,8,1>F g3<8,8,1>F mul(8) g6<1>Fg5<8,8,1>F g4<8,8,1>F mad(8) g127<1>F g6<4,4,1>F g3<4,4,1>F g2<4,4,1>F whereas without NIR we get: mul(8) g5<1>Fg2<8,8,1>F g4<8,8,1>F mad(8) g127<1>F g5<4,4,1>F g3<4,4,1>F g5<4,4,1>F Again, the assembly matches the NIR, so NIR is doing something bad. The changes to sun-temple/209 (and a lot of other sun temple shaders) look wrong. I'm not sure if the translation from NIR is doing something bad or what, but we should have 8 texture fetches and we only end up with 2. Some highlights -- Some shaders are improved because NIR recognizes exp2(log(x) * y) as pow(x, y) when the GLSL optimizations couldn't, because its dead code elimination is much too stupid to allow tree grafting to put the IR in the form we expect. This accounts for a low of the high percentage gains. It cuts some Kerbal Space Program shaders in half (~1000 -> ~500 instructions) and removes ~100 spills and ~100 fills from each. Not sure how it actually accomplished this. Too hard to read the assembly with all of the spills. Code generated by tropico-5/252 is really stupid. We MOVs both before and in an if statement that do the same thing. NIR cleans that up easily as a side-effect of SSA. In strike-suit-zero/156 we emit more MADs that we couldn't recognize from the GLSL IR, since again, its DCE is terrible and we can't combine trees. In the-binding-of-isaac-rebirth/18, it's able to CSE some duplicate rcp expressions in different basic blocks. I'm not sure why the backend isn't doing this. At this point I've looked through all of the shaders helped by >15% or more. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev