[Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags
Jose Fonseca
jfonseca at vmware.com
Sat Jun 11 16:41:48 UTC 2016
On 11/06/16 00:19, 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, instead use a new bind_actual value in llvmpipe_resource instead of
> the bind one in the pipe_resource, and update it accordingly whenever
> a resource is bound "illegally".
> 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 | 9 ++++++---
> src/gallium/drivers/llvmpipe/lp_surface.c | 10 +++++++++-
> src/gallium/drivers/llvmpipe/lp_texture.c | 24 +++++++++++-------------
> src/gallium/drivers/llvmpipe/lp_texture.h | 2 ++
> 7 files changed, 40 insertions(+), 21 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..a30a051 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) {
> + struct llvmpipe_resource *lpr = llvmpipe_resource(constants);
> + if (!(lpr->bind_actual & PIPE_BIND_CONSTANT_BUFFER)) {
Let's add a warning like the one we already for sampler views.
> + lpr->bind_actual |= 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..2d61e24 100644
> --- a/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> +++ b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
> @@ -168,12 +168,15 @@ llvmpipe_create_sampler_view(struct pipe_context *pipe,
> const struct pipe_sampler_view *templ)
> {
> struct pipe_sampler_view *view = CALLOC_STRUCT(pipe_sampler_view);
> + struct llvmpipe_resource *lpr = llvmpipe_resource(texture);
> /*
> - * 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 update internal actual_bind flags instead.
> */
> - if (!(texture->bind & PIPE_BIND_SAMPLER_VIEW))
> + if (!(lpr->bind_actual & PIPE_BIND_SAMPLER_VIEW)) {
> debug_printf("Illegal sampler view creation without bind flag\n");
> + lpr->bind_actual |= 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..f33cede 100644
> --- a/src/gallium/drivers/llvmpipe/lp_surface.c
> +++ b/src/gallium/drivers/llvmpipe/lp_surface.c
> @@ -130,9 +130,17 @@ llvmpipe_create_surface(struct pipe_context *pipe,
> const struct pipe_surface *surf_tmpl)
> {
> struct pipe_surface *ps;
> + struct llvmpipe_resource *lpr = llvmpipe_resource(pt);
>
> - if (!(pt->bind & (PIPE_BIND_DEPTH_STENCIL | PIPE_BIND_RENDER_TARGET)))
> + if (!(lpr->bind_actual & (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)) {
> + lpr->bind_actual |= PIPE_BIND_DEPTH_STENCIL;
> + }
> + else {
> + lpr->bind_actual |= 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..218d445 100644
> --- a/src/gallium/drivers/llvmpipe/lp_texture.c
> +++ b/src/gallium/drivers/llvmpipe/lp_texture.c
> @@ -247,6 +247,7 @@ llvmpipe_resource_create_front(struct pipe_screen *_screen,
> return NULL;
>
> lpr->base = *templat;
> + lpr->bind_actual = lpr->base.bind;
> pipe_reference_init(&lpr->base.reference, 1);
> lpr->base.screen = &screen->base;
>
> @@ -448,6 +449,7 @@ llvmpipe_resource_from_handle(struct pipe_screen *screen,
> }
>
> lpr->base = *template;
> + lpr->bind_actual = lpr->base.bind;
> pipe_reference_init(&lpr->base.reference, 1);
> lpr->base.screen = screen;
>
> @@ -542,14 +544,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)) {
> + (lpr->bind_actual & 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,16 +642,10 @@ 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)))
> + struct llvmpipe_resource *lpr = llvmpipe_resource(presource);
> + if (!(lpr->bind_actual & (PIPE_BIND_DEPTH_STENCIL |
> + PIPE_BIND_RENDER_TARGET |
> + PIPE_BIND_SAMPLER_VIEW)))
> return LP_UNREFERENCED;
>
> return lp_setup_is_resource_referenced(llvmpipe->setup, presource);
> @@ -687,6 +683,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,
> @@ -712,6 +709,7 @@ llvmpipe_user_buffer_create(struct pipe_screen *screen,
> buffer->base.array_size = 1;
> buffer->userBuffer = TRUE;
> buffer->data = ptr;
> + buffer->bind_actual = bind_flags;
>
> return &buffer->base;
> }
> diff --git a/src/gallium/drivers/llvmpipe/lp_texture.h b/src/gallium/drivers/llvmpipe/lp_texture.h
> index 3d315bb..3d48d1a 100644
> --- a/src/gallium/drivers/llvmpipe/lp_texture.h
> +++ b/src/gallium/drivers/llvmpipe/lp_texture.h
> @@ -59,6 +59,8 @@ struct sw_displaytarget;
> struct llvmpipe_resource
> {
> struct pipe_resource base;
> + /* the bind flags which actually correspond to reality */
> + unsigned bind_actual;
Is this really necessary? Why not simply modify the base bind flags?
>
> /** Row stride in bytes */
> unsigned row_stride[LP_MAX_TEXTURE_LEVELS];
Jose
More information about the mesa-dev
mailing list