From f4184d5450c12e107d3e41ae29e5927c75543259 Mon Sep 17 00:00:00 2001 From: Roland Scheidegger Date: Mon, 13 Jun 2016 17:49:01 +0200 Subject: [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags 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. Reviewed-by: Jose Fonseca --- 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 78918cf984d..f15d70df7d0 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 9e29902619a..f76de6b1ea4 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 7dceff7bc57..3a678e39da5 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 81b998ac410..4441f2a40d6 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 96f8ed82cd8..dd1c4465323 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 ee419481d5d..36f1c6b1a20 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, -- 2.30.2