[Mesa-dev] [PATCH 3/8] mesa: Add interpolation override bitfields.

[Paul Berry]

On 25 October 2011 10:21, Brian Paul <brianp at vmware.com> wrote:

> On 10/24/2011 05:37 PM, Paul Berry wrote:
>
>> On 24 October 2011 15:58, Brian Paul <brianp at vmware.com
>> <mailto:brianp at vmware.com>> wrote:
>>
>>    On 10/24/2011 03:38 PM, Paul Berry wrote:
>>
>>        This patch adds the bitfields InterpOverridesFlat,
>>        InterpOverridesSmooth, and InterpOverridesNoperspective to
>>        gl_fragment_program.  These bitfields keep track of which fragment
>>        shader inputs are overridden with the GLSL "flat", "smooth", and
>>        "noperspective" interpolation qualifiers.
>>
>>
>>    The names of those fields seems a little confusing to me.  For
>>    example, "InterpOverridesFlat" sounds like a field that overrides
>>    flat shading with something else.
>>
>>    How about just "InterpFlat", "InterpSmooth", etc?
>>
>>
>> The meaning I was trying to convey with "overrides" is that bits are
>> only set in these bitfields if the interpolation mode is overridden in
>> GLSL.  For fragment shader inputs that don't have their interpolation
>> mode overridden, all of the bitfields have a zero.  (I had to do this
>> in order to avoid adding a bunch of code to the handling of fixed
>> function fragment shading and ARB fragment programs).
>>
>>
>>    Or, how about an enum that indicates the interpolation mode for
>>    each fragment shader input?  Something like this:
>>
>>    enum interp_mode
>>    {
>>       INTERP_DEFAULT,  /* I _think_ we need a default mode, right? */
>>       INTERP_FLAT,
>>       INTERP_SMOOTH,
>>       INTERP_NO_PERSPECTIVE
>>    };
>>
>>    struct gl_fragment_program
>>    {
>>       ...
>>       enum interp_mode InterpMode[FRAG_ATTRIB_MAX];
>>       ...
>>    };
>>
>>
>>    This would also prevent a non-sensical state where a particular
>>    bit is accidentally set in more than one of the masks.
>>
>>
>> Yeah, I prefer this too, and that's essentially what I did in patch 2
>> of the series.  To be honest I find all the bitfield stuff rather
>> ugly, and I would rather drop this patch entirely, but a lot of the
>> i965 code still relies on things being bitfields.  Is it the only
>> driver that does?
>>
>
> Which bitfields are you referring to specifically?  I think the ones in
> gl_program are used in the gallium state tracker, but I'd have to
> double-check.


>
>  If so, I suppose I might could be convinced to move
>> all the bitfield handling of interpolation into i965 where it won't
>> contaminate the other implementations.
>>
>
> I don't know what works best for i965 but using an enumerated type for the
> interpolation modes seems cleaner for core Mesa.


Ok, after rereading your comments and discussing this with Eric, Ian, and
Ken, I reconsidered and actually tried implementing this as an array of
enums, as you suggested.  It pushes a little bit of complication into i965,
but it's not bad, and core mesa gets a lot cleaner.  On balance, I think
it's an improvement.  So now that I've seen your perspective, you can color
me convinced.

I'll rebase and send out a revised patch series.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20111025/862df94f/attachment.htm>