Omg, nvm, it was a copy-paste error (the "input[i]"), I should've written a follow up mail. The real problem remains to be seen.
Alright, later then. 20.06.2017, 10:04, "Constantine Kharlamov" <[email protected]>: > 20.06.2017, 08:34, "Constantine Kharlamov" <[email protected]>: >> This was discussed on IRC, there was much confusion, and overall there was >> a belief like the bug in some place other. A few minutes ago I was writing a >> follow-up in mail because I'll have trouble replying today. >> >> And… for writing a reply I found the actual bug, and lol, it is funny! >> Here's the excerpt of gdb at tgsi_interp_egcm(): >> >> (gdb) p ctx->shader->input[0] >> $162 = {name = 23, gpr = 1, done = 0, type = TGSI_FILE_OUTPUT, sid >> = 0, spi_sid = 0, interpolate = 0, ij_index = 0, interpolate_location = 0, >> lds_pos = 0, back_color_input = 0, write_mask = 0, ring_offset = 0} >> (gdb) p ctx->shader->input[1] >> $163 = {name = 5, gpr = 2, done = 0, type = TGSI_FILE_INPUT, sid = >> 9, spi_sid = 10, interpolate = 2, ij_index = 0, interpolate_location = 0, >> lds_pos = 0, back_color_input = 0, write_mask = 0, ring_offset = 0} >> (gdb) p ctx->shader->input[2] >> $164 = {name = 5, gpr = 3, done = 0, type = TGSI_FILE_INPUT, sid = >> 10, spi_sid = 11, interpolate = 2, ij_index = 0, interpolate_location = 0, >> lds_pos = 1, back_color_input = 0, write_mask = 0, ring_offset = 0} >> (gdb) >> >> Notice the "type" field I added, at the "0" index. Numerically >> TGSI_FILE_OUTPUT is "3", so it's not a default value, it was deliberately >> assigned. >> >> Pure accident :D > > Sry for the noise, it was just really bogging me that I missed the important > bit why it's funny. The thing is, I wasn't supposed to add the > "ctx->shader->input[i].type = TGSI_FILE_OUTPUT" for the RFC, I did it just > because it was obvious to do. So "type" at the "0" index should've been a > uninformative zero. However I did it, and here we are. > >> On 19.06.2017 12:57, Constantine Kharlamov wrote: >>> tgsi_declaration() configures inputs. Then tgsi_interp_egcm() uses one >>> of them for interpolation. Unfortunately it was choosing registers by >>> using >>> Src[0].Register.Index of the interpolation instruction as an index into >>> shader >>> inputs. Of course it was working by pure coincidence. E.g. for pidglit >>> test >>> "interpolateAtSample" the order of inputs happened to be IMM[1], then >>> IN[0], >>> then IN[1]. So instead of indexing into IN[1] it was indexing into IN[0]. >>> >>> The workaround is saving tgsi_file_type in inputs at tgsi_declaration(), >>> and >>> later at tgsi_interp_egcm() (possibly in other places too) cycling >>> through the >>> inputs searching for the appropriate one. >>> >>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=100785 >>> >>> ------ >>> >>> Because of unfamilarity with the code architecture I am unsure how to >>> handle >>> some things, i.e.: >>> >>> α) For some reason tgsi_declaration() never sees "ctx->shader->input[0]" >>> (i.e. >>> IMM[1]). It's configured at the end of "allocate_system_value_inputs()" >>> which >>> is fine, but I can't snoop around for places other than tgsi_declaration() >>> where inputs could be configured — it would be unreliable and fragile. I >>> tried >>> forcing to start parse from the beginning just before the cycle where >>> tgsi_declaration() is called, but it didn't help, for some reason the "0" >>> input >>> does not appear in the cycle. >>> >>> I thought at this point it would be better to just ask. >>> >>> β) I put an assert at tgsi_interp_egcm() in case some bug left the input >>> register unconfigured. In release version it'd return -ECANCELED — the >>> other >>> possible fail I found in the function is -ENOMEM, so I don't know if >>> there's a >>> better value. That said, I don't think it matters much either — there's a >>> unique print+assert. >>> >>> Signed-off-by: Constantine Kharlamov <[email protected]> >>> --- >>> src/gallium/drivers/r600/r600_shader.c | 23 +++++++++++++++++++++-- >>> src/gallium/drivers/r600/r600_shader.h | 5 +++-- >>> 2 files changed, 24 insertions(+), 4 deletions(-) >>> >>> diff --git a/src/gallium/drivers/r600/r600_shader.c >>> b/src/gallium/drivers/r600/r600_shader.c >>> index 156dba085d..b373a70bca 100644 >>> --- a/src/gallium/drivers/r600/r600_shader.c >>> +++ b/src/gallium/drivers/r600/r600_shader.c >>> @@ -861,6 +861,7 @@ static int tgsi_declaration(struct r600_shader_ctx >>> *ctx) >>> ctx->shader->input[i].interpolate = >>> d->Interp.Interpolate; >>> ctx->shader->input[i].interpolate_location = >>> d->Interp.Location; >>> ctx->shader->input[i].gpr = >>> ctx->file_offset[TGSI_FILE_INPUT] + d->Range.First + j; >>> + ctx->shader->input[i].type = TGSI_FILE_INPUT; >>> if (ctx->type == PIPE_SHADER_FRAGMENT) { >>> ctx->shader->input[i].spi_sid = >>> r600_spi_sid(&ctx->shader->input[i]); >>> switch (ctx->shader->input[i].name) { >>> @@ -905,6 +906,7 @@ static int tgsi_declaration(struct r600_shader_ctx >>> *ctx) >>> ctx->shader->output[i].gpr = >>> ctx->file_offset[TGSI_FILE_OUTPUT] + d->Range.First + j; >>> ctx->shader->output[i].interpolate = >>> d->Interp.Interpolate; >>> ctx->shader->output[i].write_mask = >>> d->Declaration.UsageMask; >>> + ctx->shader->input[i].type = TGSI_FILE_OUTPUT; >>> if (ctx->type == PIPE_SHADER_VERTEX || >>> ctx->type == PIPE_SHADER_GEOMETRY || >>> ctx->type == PIPE_SHADER_TESS_EVAL) { >>> @@ -6316,17 +6318,34 @@ static int tgsi_msb(struct r600_shader_ctx *ctx) >>> return 0; >>> } >>> >>> +static int tgsi_index_reg_in_inputs(const struct r600_shader *in, const >>> struct tgsi_src_register *reg) >>> +{ >>> + for (int i = 0, same_type_i = 0; i < in->ninput; ++i) { >>> + if (in->input[i].type == reg->File) { >>> + if (same_type_i == reg->Index) >>> + return i; >>> + else >>> + ++same_type_i; >>> + } >>> + } >>> + return -1; >>> +} >>> + >>> static int tgsi_interp_egcm(struct r600_shader_ctx *ctx) >>> { >>> struct tgsi_full_instruction *inst = >>> &ctx->parse.FullToken.FullInstruction; >>> struct r600_bytecode_alu alu; >>> int r, i = 0, k, interp_gpr, interp_base_chan, tmp, lasti; >>> unsigned location; >>> - int input; >>> + const int input = tgsi_index_reg_in_inputs(ctx->shader, >>> &inst->Src[0].Register); >>> >>> assert(inst->Src[0].Register.File == TGSI_FILE_INPUT); >>> >>> - input = inst->Src[0].Register.Index; >>> + if (input == -1) { >>> + R600_ERR("The register not found in inputs!"); >>> + assert(false); >>> + return -ECANCELED; >>> + } >>> >>> /* Interpolators have been marked for use already by >>> allocate_system_value_inputs */ >>> if (inst->Instruction.Opcode == TGSI_OPCODE_INTERP_OFFSET || >>> diff --git a/src/gallium/drivers/r600/r600_shader.h >>> b/src/gallium/drivers/r600/r600_shader.h >>> index cfdb020033..ea141e43b5 100644 >>> --- a/src/gallium/drivers/r600/r600_shader.h >>> +++ b/src/gallium/drivers/r600/r600_shader.h >>> @@ -45,15 +45,16 @@ struct r600_shader_io { >>> unsigned name; >>> unsigned gpr; >>> unsigned done; >>> + enum tgsi_file_type type; >>> int sid; >>> int spi_sid; >>> unsigned interpolate; >>> unsigned ij_index; >>> - unsigned interpolate_location; // TGSI_INTERPOLATE_LOC_CENTER, >>> CENTROID, SAMPLE >>> + unsigned interpolate_location; // TGSI_INTERPOLATE_LOC_CENTER, >>> CENTROID, SAMPLE >>> unsigned lds_pos; /* for evergreen */ >>> unsigned back_color_input; >>> unsigned write_mask; >>> - int ring_offset; >>> + int ring_offset; >>> }; >>> >>> struct r600_shader { > > _______________________________________________ > mesa-dev mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
