Am 27.11.2017 um 21:29 schrieb Ian Romanick: > On 11/20/2017 06:04 PM, Roland Scheidegger wrote: >> Am 21.11.2017 um 01:40 schrieb Ian Romanick: >>> On 11/20/2017 04:07 PM, Miklós Máté wrote: >>>> This fixes crash when: >>>> - first pass begins with alpha inst >>>> - first pass ends with color inst, second pass begins with alpha inst >>>> >>>> Also, use the symbolic name instead of a number. >>>> Piglit: spec/ati_fragment_shader/api-alphafirst >>>> >>>> Signed-off-by: Miklós Máté <mtm...@gmail.com> >>>> --- >>>> src/mesa/main/atifragshader.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/src/mesa/main/atifragshader.c b/src/mesa/main/atifragshader.c >>>> index 49ddb6e5af..d6fc37ac8f 100644 >>>> --- a/src/mesa/main/atifragshader.c >>>> +++ b/src/mesa/main/atifragshader.c >>>> @@ -598,8 +598,10 @@ _mesa_FragmentOpXATI(GLint optype, GLuint arg_count, >>>> GLenum op, GLuint dst, >>>> curProg->cur_pass=3; >>>> >>>> /* decide whether this is a new instruction or not ... all color >>>> instructions are new, >>>> - and alpha instructions might also be new if there was no preceding >>>> color inst */ >>>> - if ((optype == 0) || (curProg->last_optype == optype)) { >>>> + and alpha instructions might also be new if there was no preceding >>>> color inst, >>>> + and this may be the first inst of the pass */ >>> >>> I know the code previously used this same formatting, but the Mesa style >>> is for the */ of a multi-line comment to be on its own line. Each other >>> line should also start with a *. And line-wrap around 78 characters. >>> Like: >>> >>> /* Decide whether this is a new instruction or not. All color >>> instructions >>> * are new, and alpha instructions might also be new if there was no >>> * preceding color inst. This may also be the first inst of the pass. >>> */ >>> >>>> + if ((optype == ATI_FRAGMENT_SHADER_COLOR_OP) || (curProg->last_optype >>>> == optype) >>>> + || (curProg->numArithInstr[curProg->cur_pass >> 1] == 0)) { >>> >>> I lost the debate about where the || (or &&) should go... it should be >>> on the previous line. Most of the parenthesis are unnecessary, and the >>> second line should line up with the opening (. >>> >>> On a side topic... if anyone understands how >>> ati_fragment_shader::cur_pass works, it would be really great if they >>> could document it in mtypes.h. >> >> This just indicates which pass is currently being specified. Some >> instructions will trigger a new pass, some instructions are only valid >> in the first or second pass and so on, and you can have a maximum of 2 >> passes. > > Which is the confusing part. ATI fragment shaders can have two passes, > but, as far as I can tell by reading the code, > ati_fragment_shader::cur_pass can have a maximum value of... 5? At > least 4 for sure. Ah yes I wasn't very accurate there. unlike NumPasses, cur_pass distinguishes between the texture and arithmetic phases. Hence cur_pass being 0 means currently texture instructions are specified for the first pass. cur_pass 1 arithmetic for the first pass. cur_pass 2/3 correspond to the second pass accordingly. I can't see though how you could get values larger than 3 (if the value is 3 and there's some instruction which would increase it, that should be an error).
Roland > >> I guess it's a bit awkward how this needs to work due to the shader >> being specified bit-by-bit rather than all at once, but the actual >> concept is very similar to the multiple phases of d3d9 and r300 (and >> didn't i915 have something similar too). Of course, if you translate it >> away (on everything but r200) then only the error checking aspect of it >> really matters in the end. >> >> FWIW the patches all look correct to me, apparently there were quite >> some errors in there (I think it was mostly verified with doom3 at that >> time...). >> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev