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

[Ilia Mirkin]

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).

>
> 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?

>
>> 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.

>
>> 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.)

  -ilia