[Mesa-dev] [PATCH 1/2] i965/gen9: Configure rbc buffers as plain for non-rbc tex views

[Kenneth Graunke]

On Tuesday, May 31, 2016 5:16:22 PM PDT Topi Pohjolainen wrote:
> Fixes rendering in Shadow of Mordor with rbc. Application writes
> RGBA_UNORM texture filling it with values the application wants to
> later on treat as SRGB_ALPHA.
> Intel driver enables lossless compression for the buffer by the time
> of writing. However, the driver fails to make sure the buffer can be
> sampled as something else later on and unfortunately there is
> restriction in the hardware for using lossless compression for srgb
> formats which looks to extend itself to the sampling engine also.
> Requesting srgb to linear conversion on top of compressed buffer
> results the color values to be pretty much garbage.
> 
> Fortunately none of tracked benchmarks showed a regression with
> this.
> 
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> CC: Kenneth Graunke <kenneth at whitecape.org
> ---
>  src/mesa/drivers/dri/i965/brw_context.c        | 38 ++++++++++++++++++++++++--
>  src/mesa/drivers/dri/i965/gen8_surface_state.c | 13 ++++++++-
>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 2504dce..652acbf 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -70,6 +70,7 @@
>  #include "tnl/t_pipeline.h"
>  #include "util/ralloc.h"
>  #include "util/debug.h"
> +#include "isl/isl.h"
>  
>  /***************************************
>   * Mesa's Driver Functions
> @@ -166,6 +167,38 @@ intel_update_framebuffer(struct gl_context *ctx,
>                                   fb->DefaultGeometry.NumSamples);
>  }
>  
> +/* On Gen9 color buffers may be compressed by the hardware (lossless
> + * compression). There are, however, format restrictions and care needs to be
> + * taken that the sampler engine is capable for re-interpreting a buffer with
> + * format different the buffer was originally written with.
> + *
> + * For example, SRGB formats are not compressible and the sampler engine isn't
> + * capable of treating RGBA_UNORM as SRGB_ALPHA. In such a case the underlying
> + * color buffer needs to be resolved so that the sampling surface can be
> + * sampled as non-compressed (i.e., without the auxiliary MCS buffer being
> + * set).
> + */
> +static bool
> +intel_texture_view_requires_resolve(struct brw_context *brw,
> +                                    struct intel_texture_object *intel_tex)
> +{
> +   if (brw->gen < 9 ||
> +      !intel_miptree_is_lossless_compressed(brw, intel_tex->mt))
> +     return false;
> +
> +   const uint32_t brw_format = brw_format_for_mesa_format(intel_tex->_Format);
> +
> +   if (isl_format_supports_lossless_compression(brw->intelScreen->devinfo,
> +                                                brw_format))
> +      return false;
> +
> +   perf_debug("Incompatible sampling format (%s) for rbc (%s)\n",
> +              _mesa_get_format_name(intel_tex->_Format),
> +              _mesa_get_format_name(intel_tex->mt->format));
> +
> +   return true;
> +}
> +
>  static void
>  intel_update_state(struct gl_context * ctx, GLuint new_state)
>  {
> @@ -198,8 +231,9 @@ intel_update_state(struct gl_context * ctx, GLuint new_state)
>        /* Sampling engine understands lossless compression and resolving
>         * those surfaces should be skipped for performance reasons.
>         */
> -      intel_miptree_resolve_color(brw, tex_obj->mt,
> -                                  INTEL_MIPTREE_IGNORE_CCS_E);
> +      const int flags = intel_texture_view_requires_resolve(brw, tex_obj) ?
> +                           0 : INTEL_MIPTREE_IGNORE_CCS_E;
> +      intel_miptree_resolve_color(brw, tex_obj->mt, flags);
>        brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
>     }
>  
> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> index a3ad108..e7d9306 100644
> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> @@ -40,6 +40,7 @@
>  #include "brw_state.h"
>  #include "brw_defines.h"
>  #include "brw_wm.h"
> +#include "isl/isl.h"
>  
>  /**
>   * Convert an swizzle enumeration (i.e. SWIZZLE_X) to one of the Gen7.5+
> @@ -254,8 +255,18 @@ gen8_emit_texture_surface_state(struct brw_context *brw,
>      * the color buffer should always have been resolved before it is used as
>      * a texture so there is no need for it. On Gen9 it will be uploaded when
>      * the surface is losslessly compressed (CCS_E).
> +    * However, sampling engine is not capable of re-interpreting the
> +    * underlying color buffer in non-compressible formats when the surface
> +    * is configured as compressed. Therefore state upload has made sure the
> +    * buffer is in resolved state allowing the surface to be configured as
> +    * as non-compressed.
>      */
> -   if (mt->num_samples <= 1 && aux_mode != GEN9_SURFACE_AUX_MODE_CCS_E) {
> +   if (mt->num_samples <= 1 &&
> +       (aux_mode != GEN9_SURFACE_AUX_MODE_CCS_E ||
> +        !isl_format_supports_lossless_compression(
> +            brw->intelScreen->devinfo, format))) {
> +      assert(!mt->mcs_mt ||
> +             mt->fast_clear_state == INTEL_FAST_CLEAR_STATE_RESOLVED);
>        aux_mt = NULL;
>        aux_mode = GEN8_SURFACE_AUX_MODE_NONE;
>     }
> 

With Matt and Grazvydas' suggestions fixed up, this series is:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
Cc: "12.0" <mesa-stable at lists.freedesktop.org>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160531/e42e6550/attachment-0001.sig>