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

[Emil Velikov]

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

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.

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

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

Thanks
Emil