[Mesa-dev] [PATCH] st/atifs: merge gl_program and ati_fragment_shader

[Ian Romanick]

On 11/13/2017 10:49 PM, Timothy Arceri wrote:
> 
> 
> On 14/11/17 13:30, Ian Romanick wrote:
>> On 11/10/2017 02:35 AM, Timothy Arceri wrote:
>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>> index 6b5c5bbb36..7c357b07ee 100644
>>> --- a/src/mesa/main/mtypes.h
>>> +++ b/src/mesa/main/mtypes.h
>>> @@ -2352,58 +2376,29 @@ struct gl_fragment_program_state
>>>    */
>>>   struct gl_compute_program_state
>>>   {
>>>      /** Currently enabled and valid program (including internal
>>> programs
>>>       * and compiled shader programs).
>>>       */
>>>      struct gl_program *_Current;
>>>   };
>>>     -/**
>>> - * ATI_fragment_shader runtime state
>>> - */
>>> -
>>> -struct atifs_instruction;
>>> -struct atifs_setupinst;
>>> -
>>> -/**
>>> - * ATI fragment shader
>>> - */
>>> -struct ati_fragment_shader
>>> -{
>>> -   GLuint Id;
>>> -   GLint RefCount;
>>> -   struct atifs_instruction *Instructions[2];
>>> -   struct atifs_setupinst *SetupInst[2];
>>> -   GLfloat Constants[8][4];
>>> -   GLbitfield LocalConstDef;  /**< Indicates which constants have
>>> been set */
>>> -   GLubyte numArithInstr[2];
>>> -   GLubyte regsAssigned[2];
>>> -   GLubyte NumPasses;         /**< 1 or 2 */
>>> -   GLubyte cur_pass;
>>> -   GLubyte last_optype;
>>> -   GLboolean interpinp1;
>>> -   GLboolean isValid;
>>> -   GLuint swizzlerq;
>>> -   struct gl_program *Program;
>>> -};
>>> -
>>
>> Other places in Mesa would do this by C-style subclassing of gl_program:
>>
>> struct ati_fragment_shader {
>>     struct gl_program Base;
>>     struct atifs_instruction *Instructions[2];
>>     struct atifs_setupinst *SetupInst[2];
>>     GLfloat Constants[8][4];
>>     GLbitfield LocalConstDef;  /**< Indicates which constants have
>> been set */
>>     GLubyte numArithInstr[2];
>>     GLubyte regsAssigned[2];
>>     GLubyte NumPasses;         /**< 1 or 2 */
>>     GLubyte cur_pass;
>>     GLubyte last_optype;
>>     GLboolean interpinp1;
>>     GLboolean isValid;
>>     GLuint swizzlerq;
>> };
>>
>> Is there a reason to not do that here?
> 
> The st already does a C-style subclassing of gl_program, so it would
> start getting messy (which is what this patch tries to avoid in the
> first place). Also that type of subclassing is generally used for adding
> driver specific additional fields, not for specifying the basics of the
> program as we are doing here.
> 
> Having these fields inside gl_program simplifies things a bunch. Is
> there something you don't like? IMO this makes things more consistent as
> its how we handle arb programs. The final step is to make this all a
> union but that requires a bit of work on i915 and swrast,

There wasn't anything in particular that I didn't like, I was just
trying to understand the design choice.  What I mostly didn't like was
the old way ati_fragment_shader was implemented. :) I had forgotten that
ARB programs were also in gl_program, so this makes a bit more sense now.

>>>   /**
>>>    * Context state for GL_ATI_fragment_shader
>>>    */
>>>   struct gl_ati_fragment_shader_state
>>>   {
>>>      GLboolean Enabled;
>>>      GLboolean Compiling;
>>>      GLfloat GlobalConstants[8][4];
>>> -   struct ati_fragment_shader *Current;
>>> +   struct gl_program *Current;
>>>   };
>>>     /**
>>>    *  Shader subroutine function definition
>>>    */
>>>   struct gl_subroutine_function
>>>   {
>>>      char *name;
>>>      int index;
>>>      int num_compat_types;
>