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

[Emil Velikov]

On 15 November 2017 at 19:30, Jordan Justen <jordan.l.justen at intel.com> wrote:
> 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?
>
I was split between dropping or keeping with the comment squashed.
Let's drop 08 - the dead trivial conflict in 12/13 is fine.

> /* 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.
>  */
>
Nice - it reads a lot easier. I'll whip it into a patch form and send
out (for posterity) tomorrow.

Thanks
Emil