[Mesa-dev] [Mesa-stable] [PATCH] i965: Fix PBO cache coherency issue after _mesa_meta_pbo_GetTexSubImage().

[Anuj Phogat]

On Tue, May 12, 2015 at 7:55 AM, Francisco Jerez <currojerez at riseup.net> wrote:
> This problem can easily be reproduced with a number of
> ARB_shader_image_load_store piglit tests, which use a buffer object as
> PBO for a pixel transfer operation and later on bind the same buffer
> to the pipeline as shader image -- The problem is not exclusive to
> images though, and is likely to affect other kinds of buffer objects
> that can be bound to the 3D pipeline, including vertex, index,
> uniform, atomic counter buffers, etc.
>
> CC: 10.5 <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/intel_pixel_read.c | 24 +++++++++++++++++++++++-
>  src/mesa/drivers/dri/i965/intel_tex_image.c  |  9 ++++++++-
>  2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c b/src/mesa/drivers/dri/i965/intel_pixel_read.c
> index d3ca38b..3038057 100644
> --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c
> +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c
> @@ -226,8 +226,30 @@ intelReadPixels(struct gl_context * ctx,
>
>     if (_mesa_is_bufferobj(pack->BufferObj)) {
>        if (_mesa_meta_pbo_GetTexSubImage(ctx, 2, NULL, x, y, 0, width, height, 1,
> -                                        format, type, pixels, pack))
> +                                        format, type, pixels, pack)) {
> +         /* _mesa_meta_pbo_GetTexSubImage() implements PBO transfers by
> +          * binding the user-provided BO as a fake framebuffer and rendering
> +          * to it.  This breaks the invariant of the GL that nothing is able
> +          * to render to a BO, causing nondeterministic corruption issues
> +          * because the render cache is not coherent with a number of other
> +          * caches that the BO could potentially be bound to afterwards.
> +          *
> +          * This could be solved in the same way that we guarantee texture
> +          * coherency after a texture is attached to a framebuffer and
> +          * rendered to, but that would involve checking *all* BOs bound to
> +          * the pipeline for the case we need to emit a cache flush due to
> +          * previous rendering to any of them -- Including vertex, index,
> +          * uniform, atomic counter, shader image, transform feedback,
> +          * indirect draw buffers, etc.
> +          *
> +          * That would increase the per-draw call overhead even though it's
> +          * very unlikely that any of the BOs bound to the pipeline has been
> +          * rendered to via a PBO at any point, so it seems better to just
> +          * flush here unconditionally.
> +          */
> +         intel_batchbuffer_emit_mi_flush(brw);
>           return;
> +      }
>
>        perf_debug("%s: fallback to CPU mapping in PBO case\n", __func__);
>     }
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 7952ee5..85d3d04 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -486,8 +486,15 @@ intel_get_tex_image(struct gl_context *ctx,
>        if (_mesa_meta_pbo_GetTexSubImage(ctx, 3, texImage, 0, 0, 0,
>                                          texImage->Width, texImage->Height,
>                                          texImage->Depth, format, type,
> -                                        pixels, &ctx->Pack))
> +                                        pixels, &ctx->Pack)) {
> +         /* Flush to guarantee coherency between the render cache and other
> +          * caches the PBO could potentially be bound to after this point.
> +          * See the related comment in intelReadPixels() for a more detailed
> +          * explanation.
> +          */
> +         intel_batchbuffer_emit_mi_flush(brw);
>           return;
> +      }
>
>        perf_debug("%s: fallback to CPU mapping in PBO case\n", __func__);
>     }
> --
> 2.3.5
>
> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable

Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>