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

[Timothy Arceri]

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,

> 
>>   /**
>>    * 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;