[Mesa-dev] [PATCH 18.0 v2] i965, gallium: Disable ARB_get_program_binary for compat profiles

[Ilia Mirkin]

Jordan - terribly sorry I got you into this mess :( Just drop the
st/mesa hunk and push the i965 change - I believe that's all reviewed
and tested.

Marek - feel free to rewrite it the way you suggest. This is something
that'll get reverted later, so I didn't want to mess about with
function signatures, and adding new parameters which will later become
unused.

On Fri, Feb 23, 2018 at 7:44 AM, Marek Olšák <maraeo at gmail.com> wrote:
> Please keep the extension exposed in GL compat. Eventually we want to
> expose all extensions in GL compat.
>
> st_init_limits is the proper place - please use that. While
> st_init_limits doesn't have the API parameter, both of its call sites
> have it, so it's just a matter of passing it to the function.
>
> Thanks,
> Marek
>
> On Fri, Feb 23, 2018 at 1:48 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Thu, Feb 22, 2018 at 7:41 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> On Thu, Feb 22, 2018 at 7:39 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 23 February 2018 at 00:17, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>> On Thu, Feb 22, 2018 at 7:11 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>> diff --git a/src/mesa/state_tracker/st_context.c b/src/mesa/state_tracker/st_context.c
>>>>>>> index d3e7d3fb7fa..4c14245a38f 100644
>>>>>>> --- a/src/mesa/state_tracker/st_context.c
>>>>>>> +++ b/src/mesa/state_tracker/st_context.c
>>>>>>> @@ -518,6 +518,15 @@ st_create_context_priv(struct gl_context *ctx, struct pipe_context *pipe,
>>>>>>>           ctx->Const.ShaderCompilerOptions[i].EmitNoIndirectSampler = true;
>>>>>>>     }
>>>>>>>
>>>>>>> +   /* The QT framework has a bug in their shader program cache, which is built
>>>>>>> +    * on GL_ARB_get_program_binary. In an effort to allow them to fix the bug
>>>>>>> +    * we don't enable binary formats for compatibility profiles.
>>>>>>> +    * This is only being done on the 18.0 release branch.
>>>>>>> +    */
>>>>>>> +   if (ctx->API == API_OPENGL_COMPAT) {
>>>>>>> +      ctx->Const.NumProgramBinaryFormats = 0;
>>>>>>> +   }
>>>>>>> +
>>>>>> Please move this alike the i965 instance -> adjust the constant within
>>>>>> st_init_limits.
>>>>>
>>>>> Nope. It's in the right place where it is. (I put it there, Jordan
>>>>> just added it to his patch.)
>>>>>
>>>> Silly question incoming - Why? It seems misleading to set the const in
>>>> one place, only to change it elsewhere?
>>>
>>> Doing what you suggest won't compile. Limits (and extensions) are used
>>> to infer ctx->API. So you can't set them based on ctx->API, it's
>>> circular. So this, like a handful of other things, are
>>> post-auto-everything fixups.
>>
>> Actually not strictly correct -- st_init_limits is used as part of the
>> GLX_MESA_query_renderer dance. It does not currently receive a ctx
>> because one doesn't necessarily exist, but it definitely knows what
>> API it is in all cases, so that could be passed in as an argument.
>>
>> However that would be more change for what seems like no reason. Other
>> dependencies can be more complicated (the circular dependencies start
>> hitting when you start depending on versions of things), so the
>> post-fixup approach is still going to be needed. And this will all get
>> backed out, so no need to modify a bunch of surrounding code.
>>
>>   -ilia
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev