[Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags
Roland Scheidegger
sroland at vmware.com
Mon Jun 13 08:56:11 UTC 2016
Am 11.06.2016 um 18:41 schrieb Jose Fonseca:
> 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.
Ok.
>
>> + 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?
It's not really necessary. The only reason I didn't modify the bind
flags directly is because I thought it isn't nice to modify the parts of
an immutable object which are visible outside the driver. But I can
easily change that.
Roland
>
>>
>> /** Row stride in bytes */
>> unsigned row_stride[LP_MAX_TEXTURE_LEVELS];
>
> Jose
More information about the mesa-dev
mailing list