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-an...@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-an...@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 > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
