[Mesa-dev] [PATCH 01/41] glapi: Added ARB_direct_state_access.xml file.

[Emil Velikov]

On 23/01/15 20:51, Jason Ekstrand wrote:
> 
> 
> On Thu, Jan 22, 2015 at 9:27 AM, Emil Velikov <emil.l.velikov at gmail.com
> <mailto:emil.l.velikov at gmail.com>> wrote:
> 
>     On 05/01/15 17:45, Laura Ekstrand wrote:
>     > This comment is vague.  Do you have a specific recommendation for the
>     > code here?
>     >
>     Seems like I'm way too subtle - yes I have a few.
> 
> 
>     1. Add ARB_direct_state_access to struct gl_extension
>     --- a/src/mesa/main/mtypes.h
>     +++ b/src/mesa/main/mtypes.h
>     @@ -3731,6 +3731,7 @@ struct gl_extensions
>         GLboolean ARB_depth_clamp;
>         GLboolean ARB_depth_texture;
>         GLboolean ARB_derivative_control;
>     +   GLboolean ARB_direct_state_access
>         GLboolean ARB_draw_buffers_blend;
>         GLboolean ARB_draw_elements_base_vertex;
> 
> 
>     2. Use it in the extensions table.
>     --- a/src/mesa/main/extensions.c
>     +++ b/src/mesa/main/extensions.c
>     @@ -103,6 +103,7 @@ static const struct extension extension_table[] = {
>         { "GL_ARB_depth_clamp",                         o(ARB_depth_clamp),
>                             GL,             2003 },
>         { "GL_ARB_depth_texture",
>     o(ARB_depth_texture),                       GLL,            2001 },
>         { "GL_ARB_derivative_control",
>     o(ARB_derivative_control),                  GL,             2014 },
>     +   { "GL_ARB_direct_state_access",
>     o(ARB_direct_state_access),                 GL,             2014 },
> 
> 
>     3. Make use of if when the spec amends existing behaviour - most of the
>     spec text as of section "New Tokens" onwards. Clearly with this series
>     you're adding the new entry points(functions) so it does not apply
>     here :)
> 
> 
>     if (foo->Extensions.ARB_direct_state_access) {
>      ....
>     }
> 
> 
>     Pretty much every extension that was added to mesa follows this approach
>     so keeping up with traditions is always nice.
> 
> 
> Yes, and no...  We have the table of booleans in gl_extensions so that
> we can expose different extensions/behavior on different drivers. 
> However, ARB_direct_state_access doesn't actually add new functionality,
> just new ways of getting at old functionality.  We *should* be able to
> implement it in a driver-agnostic way entirely within core mesa. 
> Therefore, there's no reason to be able to shut it off on a per-driver
> basis and no reason for the flag in gl_extensions.  If we find that, for
> some reason, we only want to support it in core contexts or that it adds
> something some drivers can't handle it, then we'll need the flag.
True, yet the usual approach so far had been:
1. add the flag
2. enable when/where possible
3. evaluate if things can be enabled for everyone
4. drop it (replace with dummy_true).
Why bother ? See below.

There will be a point where the extension will still be dummy_false, yet
the amendments to the spec will be applied.
At that point there will be a "few" reports from your QA team and other
people, that piglit (other) has regressed. Going the usual route will
save you that, at the cost of having one extra commit worth
(presumingly) ~50loc.

Hope with ^^ things make (a bit more) sense :)

Thanks
Emil