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;
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
