Re: [Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags

2016-06-14 Thread Jose Fonseca 

On 13/06/16 16:50, srol...@vmware.com wrote:

From: Roland Scheidegger 

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_VERTEX0x1000
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(>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(, 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

[Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags

2016-06-13 Thread sroland 
From: Roland Scheidegger 

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_VERTEX0x1000
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(>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(, 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 |=

Re: [Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags

2016-06-13 Thread Jose Fonseca 

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, srol...@vmware.com wrote:

From: Roland Scheidegger 

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_VERTEX0x1000
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(>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(, 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);
  /*
-*

Re: [Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags

2016-06-13 Thread Roland Scheidegger 
Am 11.06.2016 um 18:41 schrieb Jose Fonseca:
> On 11/06/16 00:19, srol...@vmware.com wrote:
>> From: Roland Scheidegger 
>>
>> 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_VERTEX0x1000
>> 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(>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(, 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
>> +++

Re: [Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags

2016-06-11 Thread Jose Fonseca 

On 11/06/16 00:19, srol...@vmware.com wrote:

From: Roland Scheidegger 

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_VERTEX0x1000
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(>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(, 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

[Mesa-dev] [PATCH] llvmpipe: hack-fix bugs due to bogus bind flags

2016-06-10 Thread sroland 
From: Roland Scheidegger 

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_VERTEX0x1000
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(>constants[shader][index], cb);
 
+   if (constants) {
+  struct llvmpipe_resource *lpr = llvmpipe_resource(constants);
+  if (!(lpr->bind_actual & PIPE_BIND_CONSTANT_BUFFER)) {
+ 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(, 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