[Mesa-dev] [PATCH 4/4] mesa: fix validate for secondary interpolator

[Miklós Máté]

On 21/11/17 02:09, Ian Romanick wrote:
> 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.
Should I add spec quotations to these two error checks or to all of them?

>
>> - made the DOT4 check fatal, and improved the error msg
>>
>> Piglit: spec/ati_fragment_shader/error08-secondary
>>
>> Signed-off-by: Miklós Máté <mtmkls at 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 ().
There are lots of unnecessary () in this file. Should I attempt to 
remove them all? Should I do it in this patch or in a separate one?

>
>> +         _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)) {
>>