<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 23, 2015 at 1:46 PM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 23/01/15 20:51, Jason Ekstrand wrote:<br>
><br>
><br>
> On Thu, Jan 22, 2015 at 9:27 AM, Emil Velikov <<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a><br>
</span><div><div class="h5">> <mailto:<a href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>>> wrote:<br>
><br>
>     On 05/01/15 17:45, Laura Ekstrand wrote:<br>
>     > This comment is vague.  Do you have a specific recommendation for the<br>
>     > code here?<br>
>     ><br>
>     Seems like I'm way too subtle - yes I have a few.<br>
><br>
><br>
>     1. Add ARB_direct_state_access to struct gl_extension<br>
>     --- a/src/mesa/main/mtypes.h<br>
>     +++ b/src/mesa/main/mtypes.h<br>
>     @@ -3731,6 +3731,7 @@ struct gl_extensions<br>
>         GLboolean ARB_depth_clamp;<br>
>         GLboolean ARB_depth_texture;<br>
>         GLboolean ARB_derivative_control;<br>
>     +   GLboolean ARB_direct_state_access<br>
>         GLboolean ARB_draw_buffers_blend;<br>
>         GLboolean ARB_draw_elements_base_vertex;<br>
><br>
><br>
>     2. Use it in the extensions table.<br>
>     --- a/src/mesa/main/extensions.c<br>
>     +++ b/src/mesa/main/extensions.c<br>
>     @@ -103,6 +103,7 @@ static const struct extension extension_table[] = {<br>
>         { "GL_ARB_depth_clamp",                         o(ARB_depth_clamp),<br>
>                             GL,             2003 },<br>
>         { "GL_ARB_depth_texture",<br>
>     o(ARB_depth_texture),                       GLL,            2001 },<br>
>         { "GL_ARB_derivative_control",<br>
>     o(ARB_derivative_control),                  GL,             2014 },<br>
>     +   { "GL_ARB_direct_state_access",<br>
>     o(ARB_direct_state_access),                 GL,             2014 },<br>
><br>
><br>
>     3. Make use of if when the spec amends existing behaviour - most of the<br>
>     spec text as of section "New Tokens" onwards. Clearly with this series<br>
>     you're adding the new entry points(functions) so it does not apply<br>
>     here :)<br>
><br>
><br>
>     if (foo->Extensions.ARB_direct_state_access) {<br>
>      ....<br>
>     }<br>
><br>
><br>
>     Pretty much every extension that was added to mesa follows this approach<br>
>     so keeping up with traditions is always nice.<br>
><br>
><br>
> Yes, and no...  We have the table of booleans in gl_extensions so that<br>
> we can expose different extensions/behavior on different drivers.<br>
> However, ARB_direct_state_access doesn't actually add new functionality,<br>
> just new ways of getting at old functionality.  We *should* be able to<br>
> implement it in a driver-agnostic way entirely within core mesa.<br>
> Therefore, there's no reason to be able to shut it off on a per-driver<br>
> basis and no reason for the flag in gl_extensions.  If we find that, for<br>
> some reason, we only want to support it in core contexts or that it adds<br>
> something some drivers can't handle it, then we'll need the flag.<br>
</div></div>True, yet the usual approach so far had been:<br>
1. add the flag<br>
2. enable when/where possible<br>
3. evaluate if things can be enabled for everyone<br>
4. drop it (replace with dummy_true).<br>
Why bother ? See below.<br></blockquote><div><br></div><div>The "usual approach" is for extensions that add functionality and require per-driver implementation.  This extension is kind of unique in that *nothing* it adds is per-driver (as far as I know).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
There will be a point where the extension will still be dummy_false, yet<br>
the amendments to the spec will be applied.<br></blockquote><div><br></div><div>What "ammendments to the spec"?  Once it gets implemented, we'll turn it on.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
At that point there will be a "few" reports from your QA team and other<br>
people, that piglit (other) has regressed. Going the usual route will<br>
save you that, at the cost of having one extra commit worth<br>
(presumingly) ~50loc.<br>
<br>
Hope with ^^ things make (a bit more) sense :)<br></blockquote><div><br></div><div>Not really.  Right now it's not even 100% implemented, so it needs to be off for everyone.  As far as anyone can tell, it will go directly from dummy_false to dummy_true.  If we do find something in the way of implementing it that can't be done on some drivers, we can add the flag and then turn it on per-driver instead of turning it on for everyone.  I'm really not seeing how a per-driver flag will do any good.<br></div><div>--Jason<br></div></div></div></div>