[Mesa-dev] [PATCH 2/3 v2] r600g: take into account offset to system inputs at tgsi_interp_egcm()

[Constantine Kharlamov]

On 26.06.2017 16:58, Emil Velikov wrote:
> Hi Constantine,
> 
> Thanks for giving r600 some much needed love.
> 
> While the patch has landed, just going to share some generic comments.
> Most of which are documented here https://www.mesa3d.org/submittingpatches.html.

Thanks for the feedback!

> On 24 June 2017 at 15:06, Constantine Kharlamov <Hi-Angel at yandex.ru> wrote:
>> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=100785
> s/Fixes/Bugzilla/ and keep this above the s-o-b/r-b/other tags.
> 
> Should this fix land in the stable releases - see
> https://www.mesa3d.org/submittingpatches.html#nominations for the
> specifics.

This is a good question. AFAIK it's working in stable, even though by pure coincidence.
The bug been revealed due to recent TGSI changes.

Tbh ofc there could've been real world situations to reveal the bug in some other way.
I'd recommend it into stable after I got it piglit-tested just for the safe case. I
mean, I'm sure it should be okay, but due to not very good familiarity with the code
yet I'm a little paranoid. I've just tried building stable, but it fails due to some
llvm changes. And if it built, I'm not sure I won't fall into the hang
https://bugs.freedesktop.org/show_bug.cgi?id=101575

Additionally: α) the bug is specific to interpolateAt* which are supported since 4.00,
whilst the core version of half the r600g cards stuck at 3.3 due to missing hw support
for some 64 operations, β) from a discussion a month ago on IRC I got that the function
is rarely used, usually when app doing something funky, and γ) out of curiosity I grepped
through Wine sources for "interpolateAt", and indeed it's not used anywhere.

All in all, I think it's just not worth the hassle.

>>
>> v2: I was too much twiddling whether to initialize nsys_inputs at the beginning of shader initialization or for allocation of system values, and by the time I decided to go with the first one, I forgot to change it back.
>>
> Mentioning in the commit message why you opted for the current
> location might be a good idea?
> 
> In either way, please keep the text within ~72 columns.

You're right. I was thinking that if allocate_system_value_inputs() wasn't called for
some reason, the nsys_inputs could have a junk value. AFAIK nothing bad can happen ATM,
but it might change in the future.

>> Signed-off-by: Constantine Kharlamov <Hi-Angel at yandex.ru>
>> ---
>>  src/gallium/drivers/r600/r600_shader.c | 8 ++++----
>>  src/gallium/drivers/r600/r600_shader.h | 5 +++--
>>  2 files changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/gallium/drivers/r600/r600_shader.c b/src/gallium/drivers/r600/r600_shader.c
>> index 156dba085d..2eb8187341 100644
>> --- a/src/gallium/drivers/r600/r600_shader.c
>> +++ b/src/gallium/drivers/r600/r600_shader.c
>> @@ -1134,9 +1134,10 @@ static int allocate_system_value_inputs(struct r600_shader_ctx *ctx, int gpr_off
> 
>> -                       k = ctx->shader->ninput ++;
>> +                       k = ctx->shader->ninput++;
> 
> [snip]
> 
>> -       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;
> 
> These seem like unrelated white space changes. Please try to keep
> those separate patches since they only distract from the important
> parts of the commit.
> 
> -Emil
>