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

[Ian Romanick]

On 11/21/2017 04:22 PM, Miklós Máté wrote:
> 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?

Usually people will only use newer coding style in the code that they
add or directly change.  So I would only ask that you don't add more
extra () and add the spec quotes to the code you are modifying.  If you
feel like sending additional patches to modernize the rest of the file,
I'll be happy to review them.  Don't feel compelled to make those extra
changes, though... especially if you have more work in progress.

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