[Mesa-dev] [PATCH] [RFC] r600g: do not pick TGSI input registers at random

[Constantine Kharlamov]

20.06.2017, 08:34, "Constantine Kharlamov" <hi-angel at yandex.ru>:
> 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 <Hi-Angel at yandex.ru>
>>  ---
>>   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 {