On 11/27/2017 01:27 PM, Roland Scheidegger wrote: > 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).
I can believe what you're saying about 3 being the maximum... I was going from memory of last week rather than looking at the code again. :) > 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