[Mesa-dev] [PATCH-RFC] i965: do not advertise MESA_FORMAT_Z_UNORM16 support

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/19/2014 12:08 PM, Kenneth Graunke wrote:
> On 02/18/2014 09:48 PM, Chia-I Wu wrote:
>> Since 73bc6061f5c3b6a3bb7a8114bb2e1ab77d23cfdb, Z16 support is
>> not advertised for OpenGL ES contexts due to the terrible
>> performance.  It is still enabled for desktop GL because it was
>> believed GL 3.0+ requires Z16.
>> 
>> It turns out only GL 3.0 requires Z16, and that is corrected in
>> later GL versions.  In light of that, and per Ian's suggestion,
>> stop advertising Z16 support by default, and add a drirc option,
>> gl30_sized_format_rules, so that users can override.
> 
> I actually don't think that GL 3.0 requires Z16, either.
> 
> In glspec30.20080923.pdf, page 180, it says: "[...] memory
> allocation per texture component is assigned by the GL to match the
> allocations listed in tables 3.16-3.18 as closely as possible. 
> [...]
> 
> Required Texture Formats [...] In addition, implementations are
> required to support the following sized internal formats.
> Requesting one of these internal formats for any texture type will
> allocate exactly the internal component sizes and types shown for
> that format in tables 3.16-3.17:"
> 
> Notably, however, GL_DEPTH_COMPONENT16 does /not/ appear in table
> 3.16 or table 3.17.  It appears in table 3.18, where the "exact"
> rule doesn't apply, and thus we fall back to the "closely as
> possible" rule.
> 
> The confusing part is that the ordering of the tables in the PDF
> is:
> 
> Table 3.16 (pages 182-184) Table 3.18 (bottom of page 184 to top of
> 185) Table 3.17 (page 185)
> 
> I'm guessing that people saw table 3.16, then saw the one after
> with DEPTH_COMPONENT* formats, and assumed it was 3.17.  But it's
> not.

Yay latex!  Thank you for putting things in random order because it
fit better. :(

> I think we should just drop Z16 support entirely, and I think we
> should remove the requirement from the Piglit test.

If the test is wrong, and it sounds like it is, then I'm definitely in
favor of changing it.

The reason to have Z16 is low-bandwidth GPUs in resource constrained
environments.  If an app specifically asks for Z16, then there's a
non-zero (though possibly infinitesimal) probability they're doing it
for a reason.  For at least some platforms, isn't there "just" a
work-around to implement to fix the performance issue?  Doesn't the
performance issue only affect some platforms to begin with?

Maybe just change the check to

   ctx->TextureFormatSupported[MESA_FORMAT_Z_UNORM16] =
      ! platform has z16 performance issues;

>> This regresses required-sized-texture-formats on GL 3.0.
>> 
>> Signed-off-by: Chia-I Wu <olv at lunarg.com> Cc: Ian Romanick
>> <ian.d.romanick at intel.com> --- 
>> src/mesa/drivers/dri/i965/brw_context.c         | 3 +++ 
>> src/mesa/drivers/dri/i965/brw_context.h         | 1 + 
>> src/mesa/drivers/dri/i965/brw_surface_formats.c | 7 ++++--- 
>> src/mesa/drivers/dri/i965/intel_screen.c        | 4 ++++ 4 files
>> changed, 12 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>> b/src/mesa/drivers/dri/i965/brw_context.c index ffbdb94..8ecf80b
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_context.c +++
>> b/src/mesa/drivers/dri/i965/brw_context.c @@ -553,6 +553,9 @@
>> brw_process_driconf_options(struct brw_context *brw) 
>> brw->disable_derivative_optimization = 
>> driQueryOptionb(&brw->optionCache,
>> "disable_derivative_optimization");
>> 
>> +   brw->enable_z16 = +      driQueryOptionb(&brw->optionCache,
>> "gl30_sized_format_rules"); + brw->precompile =
>> driQueryOptionb(&brw->optionCache, "shader_precompile");
>> 
>> ctx->Const.ForceGLSLExtensionsWarn = diff --git
>> a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h index 98e90e2..fd10884
>> 100644 --- a/src/mesa/drivers/dri/i965/brw_context.h +++
>> b/src/mesa/drivers/dri/i965/brw_context.h @@ -1093,6 +1093,7 @@
>> struct brw_context bool disable_throttling; bool precompile; bool
>> disable_derivative_optimization; +   bool enable_z16;
>> 
>> driOptionCache optionCache; /** @} */ diff --git
>> a/src/mesa/drivers/dri/i965/brw_surface_formats.c
>> b/src/mesa/drivers/dri/i965/brw_surface_formats.c index
>> 6a7e00a..1d5f044 100644 ---
>> a/src/mesa/drivers/dri/i965/brw_surface_formats.c +++
>> b/src/mesa/drivers/dri/i965/brw_surface_formats.c @@ -623,10
>> +623,11 @@ brw_init_surface_formats(struct brw_context *brw) *
>> increased depth stalls from a cacheline-based heuristic for
>> detecting * depth stalls. * -    * However, desktop GL 3.0+
>> require that you get exactly 16 bits when -    * asking for
>> DEPTH_COMPONENT16, so we have to respect that. +    * However,
>> desktop GL 3.0, and no other version, requires that you get +
>> * exactly 16 bits when asking for DEPTH_COMPONENT16, so we have
>> an drirc +    * option to decide whether to respect that or not. 
>> */ -   if (_mesa_is_desktop_gl(ctx)) +   if (brw->enable_z16) 
>> ctx->TextureFormatSupported[MESA_FORMAT_Z_UNORM16] = true;
>> 
>> /* On hardware that lacks support for ETC1, we map ETC1 to RGBX 
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>> b/src/mesa/drivers/dri/i965/intel_screen.c index ba22971..087fc3c
>> 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++
>> b/src/mesa/drivers/dri/i965/intel_screen.c @@ -64,6 +64,10 @@
>> DRI_CONF_BEGIN 
>> DRI_CONF_OPT_BEGIN_B(disable_derivative_optimization, "false") 
>> DRI_CONF_DESC(en, "Derivatives with finer granularity by
>> default") DRI_CONF_OPT_END + +
>> DRI_CONF_OPT_BEGIN_B(gl30_sized_format_rules, "false") +
>> DRI_CONF_DESC(en, "Honor GL 3.0 specific rules for sized
>> formats") +      DRI_CONF_OPT_END DRI_CONF_SECTION_END
>> 
>> DRI_CONF_SECTION_QUALITY

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)

iEYEARECAAYFAlMFL7YACgkQX1gOwKyEAw/u5wCfdqN1ln6GvnMuxZkiQISJQ8ln
6fIAn2Df7+l81YjGXfILU6nIseIEGs7O
=0FbD
-----END PGP SIGNATURE-----