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

[Emil Velikov]

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:
>> Hi Jordan,
>>
>> On 22 February 2018 at 19:59, Jordan Justen <jordan.l.justen at intel.com> wrote:
>>> 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 more than 1
>>> binary format for compatibility profiles.
>>>
>> Doing NumProgramBinaryFormats = 0; does not disable the extension.
>> So either the commit summary is off, or you'd want to actually disable
>> the extension.
>
> Desire is to leave the ext enabled, just go back to not supporting any
> binary formats. Summary could be updated to say "Disable binary
> formats for compat profiles" or something.
>
>> Latter of which could be achieved by dropping the GLC
>> instance in src/mesa/main/extensions_table.h.
>> Note: i'm not 100% sure on that last one.
>
> I looked at doing that, but it's a lot more of a pain than it's worth.
> (You also have to teach the functions to not do anything useful, or
> figure out how to get them out of the dispatch table for compat.)
> There could also be software that really wants those entrypoints but
> deals with the lack of formats -- going back to the "old thing" seemed
> safer (i.e. ext enabled, zero formats).
>
Indeed disabling the extension might lead to some hasty side-effects.

>>
>> Note that NumProgramBinaryFormats also impacts the ES2 version of the
>> extension OES_get_program_binary.
>> Hence, I'd use explicit if (ctx->API == API_OPENGL_COMPAT) checks.
>
> That's precisely what the patch does... am I missing something?
>
Nope, you're not. I've got confused by the i965/gallium asymmetry.

>>
>>> This is only being done on the 18.0 release branch.
>>>
>> Perhaps tad silly question: why do we want this only for the 18.0
>> series and not in master?
>
> Reasonable question, one I don't have a perfect answer to. It's a QT
> bug, so I guess the hope is that it'll be fixed by the time next
> release rolls around? FWIW I don't have a huge amount of preference on
> this.
>
Certainly hope so. Yet anyone testing Mesa from git will get the
lovely issue. Perhaps they also use patched QT?

>>
>>> Ref: https://bugreports.qt.io/browse/QTBUG-66420
>>> Ref: https://bugs.freedesktop.org/show_bug.cgi?id=105065
>>> Cc: "18.0" <mesa-stable at lists.freedesktop.org>
>>> Cc: Mark Janes <mark.a.janes at intel.com>
>>> Cc: Kenneth Graunke <kenneth at whitecape.org>
>>> Cc: Scott D Phillips <scott.d.phillips at intel.com>
>>> Cc: Ilia Mirkin <imirkin at alum.mit.edu>
>>> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>>> [imirkin at alum.mit.edu: Added st_context.c (gallium) change]
>>> ---
>>>  docs/relnotes/17.4.0.html               | 2 +-
>>>  src/mesa/drivers/dri/i965/brw_context.c | 9 ++++++++-
>>>  src/mesa/state_tracker/st_context.c     | 9 +++++++++
>>>  3 files changed, 18 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/docs/relnotes/17.4.0.html b/docs/relnotes/17.4.0.html
>>> index 412c0fc455e..fecdfe77969 100644
>>> --- a/docs/relnotes/17.4.0.html
>>> +++ b/docs/relnotes/17.4.0.html
>>> @@ -53,7 +53,7 @@ Note: some of the new features are only available with certain drivers.
>>>  <li>GL_ARB_enhanced_layouts on r600/evergreen+</li>
>>>  <li>GL_ARB_bindless_texture on nvc0/kepler</li>
>>>  <li>OpenGL 4.3 on r600/evergreen with hw fp64 support</li>
>>> -<li>Support 1 binary format for GL_ARB_get_program_binary on i965</li>
>>> +<li>Support 1 binary format for GL_ARB_get_program_binary on i965 (except in GL compatibility profiles)</li>
>>>  </ul>
>>>
>>>  <h2>Bug fixes</h2>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
>>> index e9358b7bc9c..58527d77263 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_context.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_context.c
>>> @@ -704,7 +704,14 @@ brw_initialize_context_constants(struct brw_context *brw)
>>>        ctx->Const.AllowMappedBuffersDuringExecution = true;
>>>
>>>     /* GL_ARB_get_program_binary */
>>> -   ctx->Const.NumProgramBinaryFormats = 1;
>>> +   /* 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 more than 1 binary format for compatibility profiles.
>>> +    * This is only being done on the 18.0 release branch.
>>> +    */
>>> +   if (ctx->API != API_OPENGL_COMPAT) {
>>> +      ctx->Const.NumProgramBinaryFormats = 1;
>>> +   }
>>>  }
>>>
>>>  static void
>>> 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?

Either way - just pointing out some stuff that looks odd.
I'm not trying to block the patch or anything.

-Emil