[Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags
Jose Fonseca
jfonseca at vmware.com
Tue Jun 14 14:22:57 UTC 2016
On 13/06/16 16:50, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
>
> The gallium contract would be that bind flags must indicate all possible
> bindings a resource might get used, but fact is the mesa state tracker does
> not set bind flags correctly, and this is more or less unfixable due to GL.
>
> This caused a bug with piglit arb_uniform_buffer_object-rendering-dsa
> since 6e6fd911da8a1d9cd62fe0a8a4cc0fb7bdccfe02 - the commit is correct,
> but it caused us to miss updates to fs UBOs completely, since the
> corresponding buffer didn't have the appropriate bind flag set (thus we
> wouldn't check if it is indeed currently bound).
> See the discussion about this starting here:
> https://lists.freedesktop.org/archives/mesa-dev/2016-June/119829.html
>
> So, update the bind flags when we detect such usage.
> Note we update this value for now only in places which matter for us - that
> is creating sampler/surface view, or binding constant buffer. There's plenty
> more places (setting streamout buffers, vertex/index buffers, ...) where
> things can be set with the wrong bind flags, but the bind flags there never
> matter.
>
> While here also make sure we only set dirty constant bit when it's a fs
> constant buffer - totally doesn't matter if it's vs/gs.
> ---
> src/gallium/drivers/llvmpipe/lp_state.h | 2 +-
> src/gallium/drivers/llvmpipe/lp_state_derived.c | 2 +-
> src/gallium/drivers/llvmpipe/lp_state_fs.c | 12 ++++++++++--
> src/gallium/drivers/llvmpipe/lp_state_sampler.c | 8 +++++---
> src/gallium/drivers/llvmpipe/lp_surface.c | 9 ++++++++-
> src/gallium/drivers/llvmpipe/lp_texture.c | 12 +++---------
> 6 files changed, 28 insertions(+), 17 deletions(-)
>
> diff --git a/src/gallium/drivers/llvmpipe/lp_state.h b/src/gallium/drivers/llvmpipe/lp_state.h
> index 78918cf..f15d70d 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state.h
> +++ b/src/gallium/drivers/llvmpipe/lp_state.h
> @@ -46,7 +46,7 @@
> #define LP_NEW_STIPPLE 0x40
> #define LP_NEW_FRAMEBUFFER 0x80
> #define LP_NEW_DEPTH_STENCIL_ALPHA 0x100
> -#define LP_NEW_CONSTANTS 0x200
> +#define LP_NEW_FS_CONSTANTS 0x200
> #define LP_NEW_SAMPLER 0x400
> #define LP_NEW_SAMPLER_VIEW 0x800
> #define LP_NEW_VERTEX 0x1000
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_derived.c b/src/gallium/drivers/llvmpipe/lp_state_derived.c
> index 9e29902..f76de6b 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_derived.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_derived.c
> @@ -235,7 +235,7 @@ void llvmpipe_update_derived( struct llvmpipe_context *llvmpipe )
> llvmpipe->stencil_ref.ref_value);
> }
>
> - if (llvmpipe->dirty & LP_NEW_CONSTANTS)
> + if (llvmpipe->dirty & LP_NEW_FS_CONSTANTS)
> lp_setup_set_fs_constants(llvmpipe->setup,
> ARRAY_SIZE(llvmpipe->constants[PIPE_SHADER_FRAGMENT]),
> llvmpipe->constants[PIPE_SHADER_FRAGMENT]);
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> index 7dceff7..3a678e3 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
> @@ -2847,6 +2847,13 @@ llvmpipe_set_constant_buffer(struct pipe_context *pipe,
> /* note: reference counting */
> util_copy_constant_buffer(&llvmpipe->constants[shader][index], cb);
>
> + if (constants) {
> + if (!(constants->bind & PIPE_BIND_CONSTANT_BUFFER)) {
> + debug_printf("Illegal set constant without bind flag\n");
> + constants->bind |= PIPE_BIND_CONSTANT_BUFFER;
> + }
> + }
> +
> if (shader == PIPE_SHADER_VERTEX ||
> shader == PIPE_SHADER_GEOMETRY) {
> /* Pass the constants to the 'draw' module */
> @@ -2869,8 +2876,9 @@ llvmpipe_set_constant_buffer(struct pipe_context *pipe,
> draw_set_mapped_constant_buffer(llvmpipe->draw, shader,
> index, data, size);
> }
> -
> - llvmpipe->dirty |= LP_NEW_CONSTANTS;
> + else {
> + llvmpipe->dirty |= LP_NEW_FS_CONSTANTS;
> + }
>
> if (cb && cb->user_buffer) {
> pipe_resource_reference(&constants, NULL);
> diff --git a/src/gallium/drivers/llvmpipe/lp_state_sampler.c b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> index 81b998a..4441f2a 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> @@ -169,11 +169,13 @@ llvmpipe_create_sampler_view(struct pipe_context *pipe,
> {
> struct pipe_sampler_view *view = CALLOC_STRUCT(pipe_sampler_view);
> /*
> - * XXX we REALLY want to see the correct bind flag here but the OpenGL
> - * state tracker can't guarantee that at least for texture buffer objects.
> + * XXX: bind flags from OpenGL state tracker are notoriously unreliable.
> + * This looks unfixable, so fix the bind flags instead when it happens.
> */
> - if (!(texture->bind & PIPE_BIND_SAMPLER_VIEW))
> + if (!(texture->bind & PIPE_BIND_SAMPLER_VIEW)) {
> debug_printf("Illegal sampler view creation without bind flag\n");
> + texture->bind |= PIPE_BIND_SAMPLER_VIEW;
> + }
>
> if (view) {
> *view = *templ;
> diff --git a/src/gallium/drivers/llvmpipe/lp_surface.c b/src/gallium/drivers/llvmpipe/lp_surface.c
> index 96f8ed8..dd1c446 100644
> --- a/src/gallium/drivers/llvmpipe/lp_surface.c
> +++ b/src/gallium/drivers/llvmpipe/lp_surface.c
> @@ -131,8 +131,15 @@ llvmpipe_create_surface(struct pipe_context *pipe,
> {
> struct pipe_surface *ps;
>
> - if (!(pt->bind & (PIPE_BIND_DEPTH_STENCIL | PIPE_BIND_RENDER_TARGET)))
> + if (!(pt->bind & (PIPE_BIND_DEPTH_STENCIL | PIPE_BIND_RENDER_TARGET))) {
> debug_printf("Illegal surface creation without bind flag\n");
> + if (util_format_is_depth_or_stencil(surf_tmpl->format)) {
> + pt->bind |= PIPE_BIND_DEPTH_STENCIL;
> + }
> + else {
> + pt->bind |= PIPE_BIND_RENDER_TARGET;
> + }
> + }
>
> ps = CALLOC_STRUCT(pipe_surface);
> if (ps) {
> diff --git a/src/gallium/drivers/llvmpipe/lp_texture.c b/src/gallium/drivers/llvmpipe/lp_texture.c
> index ee41948..36f1c6b 100644
> --- a/src/gallium/drivers/llvmpipe/lp_texture.c
> +++ b/src/gallium/drivers/llvmpipe/lp_texture.c
> @@ -542,14 +542,14 @@ llvmpipe_transfer_map( struct pipe_context *pipe,
> }
> }
>
> - /* Check if we're mapping the current constant buffer */
> + /* Check if we're mapping a current constant buffer */
> if ((usage & PIPE_TRANSFER_WRITE) &&
> (resource->bind & PIPE_BIND_CONSTANT_BUFFER)) {
> unsigned i;
> for (i = 0; i < ARRAY_SIZE(llvmpipe->constants[PIPE_SHADER_FRAGMENT]); ++i) {
> if (resource == llvmpipe->constants[PIPE_SHADER_FRAGMENT][i].buffer) {
> /* constants may have changed */
> - llvmpipe->dirty |= LP_NEW_CONSTANTS;
> + llvmpipe->dirty |= LP_NEW_FS_CONSTANTS;
> break;
> }
> }
> @@ -640,13 +640,6 @@ llvmpipe_is_resource_referenced( struct pipe_context *pipe,
> unsigned level)
> {
> struct llvmpipe_context *llvmpipe = llvmpipe_context( pipe );
> -
> - /*
> - * XXX checking only resources with the right bind flags
> - * is unsafe since with opengl state tracker we can end up
> - * with resources bound to places they weren't supposed to be
> - * (buffers bound as sampler views is one possibility here).
> - */
> if (!(presource->bind & (PIPE_BIND_DEPTH_STENCIL |
> PIPE_BIND_RENDER_TARGET |
> PIPE_BIND_SAMPLER_VIEW)))
> @@ -687,6 +680,7 @@ llvmpipe_get_format_alignment( enum pipe_format format )
>
> /**
> * Create buffer which wraps user-space data.
> + * XXX unreachable.
> */
> struct pipe_resource *
> llvmpipe_user_buffer_create(struct pipe_screen *screen,
>
LGTM. Thanks.
Reviewed-by: Jose Fonseca <jfonseca at vmware.com>
More information about the mesa-dev
mailing list