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

[Emil Velikov]

On 23/01/15 21:53, Jason Ekstrand wrote:
> 
> 
> On Fri, Jan 23, 2015 at 1:46 PM, Emil Velikov <emil.l.velikov at gmail.com
> <mailto:emil.l.velikov at gmail.com>> wrote:
> 
>     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>
>     > <mailto: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.
> 
> 
> 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).
>  
There has been other similar cases, yet I cannot pick one from the top
of my head. And yes I did understand that is has *nothing* driver
specific about it :)

> 
>     There will be a point where the extension will still be dummy_false, yet
>     the amendments to the spec will be applied.
> 
> 
> What "ammendments to the spec"?  Once it gets implemented, we'll turn it on.
>  
See note 3, that I've mentioned above. Here is a rough example:

As you handle the following
"
    Accepted by the <pname> parameter of GetTextureParameter{if}v and
    GetTextureParameterI{i ui}v:

        TEXTURE_TARGET                                      0x1006
"

you will allow the pname, in a scenario when one should not.
I.e. the extension will not be advertised, yet the parameter will be
accepted and no error will be thrown.

This is a silly example, yet I hope it illustrates the point.

> 
>     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 :)
> 
> 
> Not really.  Right now it's not even 100% implemented, so it needs to be
> off for everyone.
True, I'm not against that.

> 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.
The idea behind the flag is to control/distinguish if the extension is
advertised _and_ if its functionality is enabled.

Presently you're gradually enabling the functionality without
advertising the extension. As such there are going to be cases, where
you allow/forbid X or Y (as per the spec text), yet the user will be
confused, as mesa does not advertise support for arb_dsa.

Does this shed more light on what I'm thinking ?

-Emil