[Mesa-dev] [PATCH 30/30] i965/cs: Allow ARB_compute_shader to be enabled via env var.

[Paul Berry]

On 4 February 2014 10:03, Jordan Justen <jljusten at gmail.com> wrote:

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

Great, thanks.  I've pushed the patches.  I'll work on "B" as soon as I can.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140205/01450cc2/attachment.html>