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. > 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...). > > Roland > >>> if (curProg->numArithInstr[curProg->cur_pass >> 1] > 7) { >>> _mesa_error(ctx, GL_INVALID_OPERATION, "C/AFragmentOpATI(instrCount)"); >>> return; >>> >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=DwIGaQ&c=uilaK90D4TOVoH58JNXRgQ&r=_QIjpv-UJ77xEQY8fIYoQtr5qv8wKrPJc7v7_-CYAb0&m=E021TggYOgVksqpASyumbrMCSKMsRIh5__J3WER0vyw&s=ek3znIpxIGsT2v0AG80jt9petuMBJvX1nXseAg81aYA&e= _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev