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
