[Mesa-dev] [PATCH] [RFC] r600g: do not pick TGSI input registers at random
Constantine Kharlamov
Hi-Angel at yandex.ru
Tue Jun 20 05:34:47 UTC 2017
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
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 {
>
More information about the mesa-dev
mailing list