[Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags
Jose Fonseca
jfonseca at vmware.com
Mon Jun 13 10:19:55 UTC 2016
On 13/06/16 09:56, Roland Scheidegger wrote:
> 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.
I think I'd prefer that.
Foremost, it seems sensible to for for drivers to expose more flags than
requested (the opposite would bad of course.)
And concerning niceness, if the state trackers sticked with the
contract, this wouldn't be necessary in the first place.
Honestly, state trackers really should be using the union of all
possible bind flags, instead of expecting things to stick... Why
exactly can't we get that?
Jose
More information about the mesa-dev
mailing list