<div dir="ltr">On 4 February 2014 10:03, Jordan Justen <span dir="ltr"><<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">>><br>
>> What about trying to make use of<br>
>> MESA_EXTENSION_OVERRIDE=GL_ARB_compute_shader?<br>
>><br>
>> We could add<br>
>> extensions.c:bool _mesa_is_extension_override_enabled(char *)<br>
>><br>
>> And then<br>
>>     if (_mesa_is_extension_override_enabled("GL_ARB_compute_shader"))<br>
>><br>
>> Or, similarly, get overrides shoved into ctx->Extensions to allow<br>
>> checking for overrides at this early stage.<br>
>><br>
>> -Jordan<br>
><br>
><br>
> I looked into what would be necessary to do this, and it's unfortunately<br>
> more complicated than it should be due to some flukes about initialization<br>
> order (perhaps this is what you were alluding to when you said "get<br>
> overrides shoved into ctx->Extensions to allow checking for overrides at<br>
> this early stage").<br>
<br>
</div></div>Yeah, I'll admit I looked at it a bit, and decided to keep my feedback<br>
'hand wavy'. :)<br>
<div><div class="h5"><br>
>  Currently, our initialization order is:<br>
><br>
> 1. brwCreateContext() calls brw_initialize_context_constants(), which needs<br>
> to know whether compute shaders are supported in order to set constants like<br>
> MAX_COMPUTE_TEXTURE_IMAGE_UNITS and MAX_UNIFORM_BUFFER_BINDINGS correctly.<br>
><br>
> 2. Later, brwCreateContext() calls intelInitExtensions(), which is the<br>
> function where we'll set ctx->Extensions.ARB_compute_shader to true once<br>
> compute shaders are fully supported.<br>
><br>
> 3. Much later in initialization, when the context is being made current for<br>
> the first time, core Mesa calls _mesa_make_extension_string(), which calls<br>
> get_extension_override() to modify the the ctx->Extensions table based on<br>
> the environment variable override.<br>
><br>
> To do what you suggested, I would either have to:<br>
><br>
> A. change 1 to call _mesa_is_extension_override_enabled(); that function, in<br>
> turn, would have to parse the MESA_EXTENSION_OVERRIDE string, but we<br>
> couldn't reuse the parsing code in _mesa_make_extension_string(), since that<br>
> parsing code updates ctx->Extensions as a side effect, and it's not time to<br>
> update ctx->Extensions yet.  In addition to the code duplication, we would<br>
> have a danger of bugs, since the override takes effect so late in<br>
> initialization--if we ever accidentally introduced some code in between<br>
> steps 2 and 3 that checked the value of ctx->Extensions.ARB_compute_shader,<br>
> it would see false even if compute shaders were enabled by override.<br>
><br>
> Or:<br>
><br>
> B. change the order of initialization so that 2 happens first, followed by 3<br>
> and then 1.  In the long run I think this would be a good thing, but it's a<br>
> big change (since it affects all back ends) and I'm not sure it would be a<br>
> good idea to hold up this patch series waiting for it.<br>
><br>
><br>
> How would you feel if I landed the patch series as is, and then worked on a<br>
> follow up patch to do B?<br>
<br>
</div></div>Sure, you can have my r-b for this then.<br>
<span class="HOEnZb"><font color="#888888"><br>
-Jordan<br></font></span></blockquote><div><br></div><div>Great, thanks.  I've pushed the patches.  I'll work on "B" as soon as I can.<br></div></div></div></div>