On 25 October 2011 10:21, Brian Paul <span dir="ltr"><<a href="mailto:brianp@vmware.com">brianp@vmware.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div class="im">On 10/24/2011 05:37 PM, Paul Berry wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
On 24 October 2011 15:58, Brian Paul <<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a><br></div><div><div></div><div class="h5">
<mailto:<a href="mailto:brianp@vmware.com" target="_blank">brianp@vmware.com</a>>> wrote:<br>
<br>
On 10/24/2011 03:38 PM, Paul Berry wrote:<br>
<br>
This patch adds the bitfields InterpOverridesFlat,<br>
InterpOverridesSmooth, and InterpOverridesNoperspective to<br>
gl_fragment_program. These bitfields keep track of which fragment<br>
shader inputs are overridden with the GLSL "flat", "smooth", and<br>
"noperspective" interpolation qualifiers.<br>
<br>
<br>
The names of those fields seems a little confusing to me. For<br>
example, "InterpOverridesFlat" sounds like a field that overrides<br>
flat shading with something else.<br>
<br>
How about just "InterpFlat", "InterpSmooth", etc?<br>
<br>
<br>
The meaning I was trying to convey with "overrides" is that bits are<br>
only set in these bitfields if the interpolation mode is overridden in<br>
GLSL. For fragment shader inputs that don't have their interpolation<br>
mode overridden, all of the bitfields have a zero. (I had to do this<br>
in order to avoid adding a bunch of code to the handling of fixed<br>
function fragment shading and ARB fragment programs).<br>
<br>
<br>
Or, how about an enum that indicates the interpolation mode for<br>
each fragment shader input? Something like this:<br>
<br>
enum interp_mode<br>
{<br>
INTERP_DEFAULT, /* I _think_ we need a default mode, right? */<br>
INTERP_FLAT,<br>
INTERP_SMOOTH,<br>
INTERP_NO_PERSPECTIVE<br>
};<br>
<br>
struct gl_fragment_program<br>
{<br>
...<br>
enum interp_mode InterpMode[FRAG_ATTRIB_MAX];<br>
...<br>
};<br>
<br>
<br>
This would also prevent a non-sensical state where a particular<br>
bit is accidentally set in more than one of the masks.<br>
<br>
<br>
Yeah, I prefer this too, and that's essentially what I did in patch 2<br>
of the series. To be honest I find all the bitfield stuff rather<br>
ugly, and I would rather drop this patch entirely, but a lot of the<br>
i965 code still relies on things being bitfields. Is it the only<br>
driver that does?<br>
</div></div></blockquote>
<br>
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.</blockquote><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<div class="im">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If so, I suppose I might could be convinced to move<br>
all the bitfield handling of interpolation into i965 where it won't<br>
contaminate the other implementations.<br>
</blockquote>
<br></div>
I don't know what works best for i965 but using an enumerated type for the interpolation modes seems cleaner for core Mesa.</blockquote><div><br>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.<br>
<br>I'll rebase and send out a revised patch series.<br></div></div>