[Mesa-dev] [PATCH v2] i965: Disable auxiliary buffers when there are self-dependencies.
Jason Ekstrand
jason at jlekstrand.net
Sat Oct 7 03:09:33 UTC 2017
On October 6, 2017 8:00:04 PM Kenneth Graunke <kenneth at whitecape.org> wrote:
> Jason and I investigated several OpenGL CTS failures where the tests
> bind the same texture for rendering and texturing, at the same time.
> This has defined results as long as the reads happen before writes,
> or the regions are non-overlapping. Normally, this just works out.
>
> However, CCS can cause problems. If the shader is reading one set of
> pixels, and writing to different pixels that are adjacent, they may end
> up being covered by the same CCS block. So rendering may be writing a
> CCS block, while the sampler is trying to read it. Corruption ensues.
>
> Disabling CCS is unfortunate, but safe.
>
> Fixes several KHR-GL45.texture_barrier.* subtests.
>
> Cc: nanleychery at gmail.com
> Cc: jason at jlekstrand.net
> ---
> src/mesa/drivers/dri/i965/brw_draw.c | 44 ++++++++++++++++-----------
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 12 +++-----
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
> 3 files changed, 33 insertions(+), 25 deletions(-)
>
> Only one patch this time around. This is a lot nicer.
>
> diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> index c7ed7284501..fcb194dbe86 100644
> --- a/src/mesa/drivers/dri/i965/brw_draw.c
> +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> @@ -336,21 +336,39 @@ brw_merge_inputs(struct brw_context *brw,
> }
> }
>
> +/* Disable auxiliary buffers if a renderbuffer is also bound as a texture
> + * or shader image. This causes a self-dependency, where both rendering
> + * and sampling may concurrently read or write the CCS buffer, causing
> + * incorrect pixels.
> + *
> + * We don't support sampling from CCS_D, so this only matters for CCS_E.
> + */
> static bool
> -intel_disable_rb_aux_buffer(struct brw_context *brw, const struct brw_bo *bo)
> +intel_disable_rb_aux_buffer(struct brw_context *brw,
> + struct intel_mipmap_tree *tex_mt,
> + const char *usage)
> {
> const struct gl_framebuffer *fb = brw->ctx.DrawBuffer;
> bool found = false;
>
> + /* Nothing to disable, don't bother looking */
> + if (tex_mt->aux_usage != ISL_AUX_USAGE_CCS_E)
> + return false;
As I mentioned in the office today, I'm not convinced this is actually
needed. When it isn't CCS_E, we'll resolve anyway so passing disable_aux =
true won't hurt anything.
> +
> for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) {
> const struct intel_renderbuffer *irb =
> intel_renderbuffer(fb->_ColorDrawBuffers[i]);
>
> - if (irb && irb->mt->bo == bo) {
> + if (irb && irb->mt->bo == tex_mt->bo) {
> found = brw->draw_aux_buffer_disabled[i] = true;
> }
> }
>
> + if (found) {
> + perf_debug("Disabling CCS because a renderbuffer is also bound %s.\n",
> + usage);
> + }
> +
> return found;
> }
>
> @@ -363,7 +381,6 @@ intel_disable_rb_aux_buffer(struct brw_context *brw,
> const struct brw_bo *bo)
> void
> brw_predraw_resolve_inputs(struct brw_context *brw)
> {
> - const struct gen_device_info *devinfo = &brw->screen->devinfo;
> struct gl_context *ctx = &brw->ctx;
> struct intel_texture_object *tex_obj;
>
> @@ -383,15 +400,11 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
> enum isl_format view_format =
> translate_tex_format(brw, tex_obj->_Format, sampler->sRGBDecode);
>
> - bool aux_supported;
> - intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
> - &aux_supported);
> + const bool disable_aux =
> + intel_disable_rb_aux_buffer(brw, tex_obj->mt, "for sampling");
>
> - if (!aux_supported && devinfo->gen >= 9 &&
> - intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
> - perf_debug("Sampling renderbuffer with non-compressible format - "
> - "turning off compression\n");
> - }
> + intel_miptree_prepare_texture(brw, tex_obj->mt, view_format,
> + disable_aux);
>
> brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
>
> @@ -412,13 +425,10 @@ brw_predraw_resolve_inputs(struct brw_context *brw)
> tex_obj = intel_texture_object(u->TexObj);
>
> if (tex_obj && tex_obj->mt) {
> - intel_miptree_prepare_image(brw, tex_obj->mt);
> + intel_disable_rb_aux_buffer(brw, tex_obj->mt,
> + "as a shader image");
>
> - if (tex_obj->mt->aux_usage == ISL_AUX_USAGE_CCS_E &&
> - intel_disable_rb_aux_buffer(brw, tex_obj->mt->bo)) {
> - perf_debug("Using renderbuffer as shader image - turning "
> - "off lossless compression\n");
> - }
> + intel_miptree_prepare_image(brw, tex_obj->mt);
>
> brw_render_cache_set_check_flush(brw, tex_obj->mt->bo);
> }
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 670a92c1168..48392e7494a 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -2630,9 +2630,9 @@ intel_miptree_prepare_texture_slices(struct
> brw_context *brw,
> enum isl_format view_format,
> uint32_t start_level, uint32_t num_levels,
> uint32_t start_layer, uint32_t num_layers,
> - bool *aux_supported_out)
> + bool disable_aux)
> {
> - enum isl_aux_usage aux_usage =
> + enum isl_aux_usage aux_usage = disable_aux ? ISL_AUX_USAGE_NONE :
> intel_miptree_texture_aux_usage(brw, mt, view_format);
> bool clear_supported = aux_usage != ISL_AUX_USAGE_NONE;
>
> @@ -2646,20 +2646,18 @@ intel_miptree_prepare_texture_slices(struct
> brw_context *brw,
> intel_miptree_prepare_access(brw, mt, start_level, num_levels,
> start_layer, num_layers,
> aux_usage, clear_supported);
> - if (aux_supported_out)
> - *aux_supported_out = aux_usage != ISL_AUX_USAGE_NONE;
> }
>
> void
> intel_miptree_prepare_texture(struct brw_context *brw,
> struct intel_mipmap_tree *mt,
> enum isl_format view_format,
> - bool *aux_supported_out)
> + bool disable_aux)
> {
> intel_miptree_prepare_texture_slices(brw, mt, view_format,
> 0, INTEL_REMAINING_LEVELS,
> 0, INTEL_REMAINING_LAYERS,
> - aux_supported_out);
> + disable_aux);
> }
>
> void
> @@ -2678,7 +2676,7 @@ intel_miptree_prepare_fb_fetch(struct brw_context *brw,
> uint32_t start_layer, uint32_t num_layers)
> {
> intel_miptree_prepare_texture_slices(brw, mt, mt->surf.format, level, 1,
> - start_layer, num_layers, NULL);
> + start_layer, num_layers, false);
I think we probably want true here. It is for fb_fetch after all. :-)
Also, we probably want to do disable_rb_aux_buffer for framebuffer fetch as
well.
> }
>
> enum isl_aux_usage
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index 439b0f66aeb..5ab929bb40b 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -640,7 +640,7 @@ void
> intel_miptree_prepare_texture(struct brw_context *brw,
> struct intel_mipmap_tree *mt,
> enum isl_format view_format,
> - bool *aux_supported_out);
> + bool disable_aux);
> void
> intel_miptree_prepare_image(struct brw_context *brw,
> struct intel_mipmap_tree *mt);
> --
> 2.14.2
>
More information about the mesa-dev
mailing list