[Mesa-dev] [PATCH 02/13] i965: remove ARB_compute_shader extension override

[Jordan Justen]

On 2017-11-14 07:37:27, Emil Velikov wrote:
> On 10 November 2017 at 23:39, Jordan Justen <jordan.l.justen at intel.com> wrote:
> > On 2017-11-10 08:19:38, Emil Velikov wrote:
> >> On 7 November 2017 at 11:54, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >> > From: Emil Velikov <emil.velikov at collabora.com>
> >> >
> >> > Checking the override was useful in the early stages of developing the
> >> > extension.
> >> >
> >> > Now that everything is wired, where possible, we can drop the check.
> >> > Doing so allows us to simplify some of the related code.
> >> >
> >> > Cc: Jordan Justen <jordan.l.justen at intel.com>
> >> > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> >> > ---
> >> >  src/mesa/drivers/dri/i965/brw_context.c | 3 +--
> >> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >> >
> >> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> >> > index 1bf39b07382..3947a71a46a 100644
> >> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> >> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> >> > @@ -348,8 +348,7 @@ brw_initialize_context_constants(struct brw_context *brw)
> >> >           (_mesa_is_desktop_gl(ctx) &&
> >> >            ctx->Const.MaxComputeWorkGroupSize[0] >= 1024) ||
> >> >           (ctx->API == API_OPENGLES2 &&
> >> > -          ctx->Const.MaxComputeWorkGroupSize[0] >= 128) ||
> >> > -         _mesa_extension_override_enables.ARB_compute_shader,
> >> > +          ctx->Const.MaxComputeWorkGroupSize[0] >= 128),
> >>
> >> Jordan can you throw an Ack on this patch if it makes sense?
> >> Brian already reviewed the series, but I'd appreciate input from
> >> someone familiar with the Intel specifics.
> >
> > Regarding patches 2 & 8, the point of those being non-static was so
> > the driver could take some action if the user requested an extension
> > override.
> >
> > I can't remember, but maybe with SIMD32 being supported, this might no
> > longer affect any i965 devices.
> >
> > I still think it could be good to allow the driver to easily know what
> > extensions were overridden, but I'll concede that it is not too
> > important. So, go ahead with the change if you want.
> >
> Agreed allowing the driver to query the extension status is useful,
> esp. during development.
> 
> Since they're not used I made them static for now, but can add a
> comment as below to make it clearer.
> What do you think? Can I get your official blessing (ack/r-b) with that?

For this patch, Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

I'm not sure what you are proposing for patch 8. I would prefer to
just drop 8, but I'm not sure how big of an impact that makes to your
series.

If you are proposing to drop patch 8, and instead add the comment
below on the variables... What about this small tweak?

/* Sometimes the driver wants to query the extension override status before
 * a context is created. These variables are filled with extension override
 * information before context creation.
 *
 * This can be useful during extension bring-up when an extension is
 * partially implemented, but cannot yet be advertised as supported.
 *
 * Use it with care and keep access read-only.
 */


> Thanks
> Emil
> 
> 
> /* Sometimes the driver wants to query the extensions status before a context
>  * is created. To achieve that one should use the following two variables. This
>  * is very useful during extension bring-up, but can be a double-edged sword.
>  *
>  * Use it with care and keep access read-only.
>  */