On 11/20/2017 04:07 PM, Miklós Máté wrote:
> This patch fixes multiple problems:
> - the interpolator check was duplicated
> - both had arg instead of argRep
> - I split it into color and alpha for better readability and error msg
> - the DOT4 check only applies to color instruction according to the spec
For the sake of reviewers and the next person to look at this code, a
spec quotation or two in the code would be really helpful.
> - made the DOT4 check fatal, and improved the error msg
>
> Piglit: spec/ati_fragment_shader/error08-secondary
>
> Signed-off-by: Miklós Máté <mtm...@gmail.com>
> ---
> src/mesa/main/atifragshader.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/main/atifragshader.c b/src/mesa/main/atifragshader.c
> index 313ba0c66d..1bca8267b8 100644
> --- a/src/mesa/main/atifragshader.c
> +++ b/src/mesa/main/atifragshader.c
> @@ -171,15 +171,15 @@ static int check_arith_arg(struct ati_fragment_shader
> *curProg,
> _mesa_error(ctx, GL_INVALID_ENUM, "C/AFragmentOpATI(arg)");
> return 0;
> }
> - if ((arg == GL_SECONDARY_INTERPOLATOR_ATI) && (((optype == 0) && (argRep
> == GL_ALPHA)) ||
> - ((optype == 1) && ((arg == GL_ALPHA) || (argRep == GL_NONE))))) {
> - _mesa_error(ctx, GL_INVALID_OPERATION, "C/AFragmentOpATI(sec_interp)");
> - return 0;
> - }
> - if ((arg == GL_SECONDARY_INTERPOLATOR_ATI) && (((optype == 0) && (argRep
> == GL_ALPHA)) ||
> - ((optype == 1) && ((arg == GL_ALPHA) || (argRep == GL_NONE))))) {
> - _mesa_error(ctx, GL_INVALID_OPERATION, "C/AFragmentOpATI(sec_interp)");
> - return 0;
Adding this comment here makes it clear that the code is correct.
/* The ATI_fragment_shader spec says:
*
* The error INVALID_OPERATION is generated by
* ColorFragmentOp[1..3]ATI if <argN> is SECONDARY_INTERPOLATOR_ATI
* and <argNRep> is ALPHA, or by AlphaFragmentOp[1..3]ATI if <argN>
* is SECONDARY_INTERPOLATOR_ATI and <argNRep> is ALPHA or NONE, ...
*/
> + if (arg == GL_SECONDARY_INTERPOLATOR_ATI) {
> + if ((optype == ATI_FRAGMENT_SHADER_COLOR_OP) && (argRep == GL_ALPHA)) {
Like before, drop the unnecessary ().
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> "CFragmentOpATI(sec_interp)");
> + return 0;
> + } else if ((optype == ATI_FRAGMENT_SHADER_ALPHA_OP) &&
> + ((argRep == GL_ALPHA) || (argRep == GL_NONE))) {
Remove extra () and line up with the opening (.
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> "AFragmentOpATI(sec_interp)");
> + return 0;
> + }
> }
> if ((curProg->cur_pass == 1) &&
> ((arg == GL_PRIMARY_COLOR_ARB) || (arg ==
> GL_SECONDARY_INTERPOLATOR_ATI))) {
> @@ -642,10 +642,11 @@ _mesa_FragmentOpXATI(GLint optype, GLuint arg_count,
> GLenum op, GLuint dst,
> return;
> }
> }
Same here regarding the comment.
/* The ATI_fragment_shader spec says:
*
* The error INVALID_OPERATION is generated by... ColorFragmentOp2ATI
* if <op> is DOT4_ATI and <argN> is SECONDARY_INTERPOLATOR_ATI and
* <argNRep> is ALPHA or NONE.
*/
> - if ((op == GL_DOT4_ATI) &&
> + if ((optype == ATI_FRAGMENT_SHADER_COLOR_OP) && (op == GL_DOT4_ATI) &&
> (((arg1 == GL_SECONDARY_INTERPOLATOR_ATI) && ((arg1Rep == GL_ALPHA) ||
> (arg1Rep == GL_NONE))) ||
> (((arg2 == GL_SECONDARY_INTERPOLATOR_ATI) && ((arg2Rep == GL_ALPHA) ||
> (arg2Rep == GL_NONE)))))) {
> - _mesa_error(ctx, GL_INVALID_OPERATION, "C/AFragmentOpATI(sec_interp)");
> + _mesa_error(ctx, GL_INVALID_OPERATION,
> "C/AFragmentOpATI(sec_interpDOT4)");
> + return;
> }
>
> if (!check_arith_arg(curProg, optype, arg1, arg1Rep)) {
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
