[Mesa-dev] [PATCH] [RFC] r600g: do not pick TGSI input registers at random
Constantine Kharlamov
hi-angel at yandex.ru
Tue Jun 20 07:56:27 UTC 2017
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" <hi-angel at yandex.ru>:
> 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 {
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list