Re: [Mesa-dev] [PATCH 2/2] util/u_helpers: return correct number of bound buffers

[Ilia Mirkin]

I'm probably just being dense... can you provide an exact sequence of
calls that would cause this logic to fail? Seems like it should work
as-is...

On Sun, Dec 6, 2015 at 4:12 AM, Patrick Rudolph  wrote:
> In case a state tracker unbinds every slot by a seperate
> pipe->set_vertex_buffers() call, starting from slot zero, the number
> of bound buffers would not reach zero at all. Unbinding all buffers
> at once or starting at the top-most slot results in correct behaviour.
>
> Calculating the correct number of bound buffers fixes a NULL pointer
> dereference in nvc0_validate_vertex_buffers_shared().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93004
>
> Signed-off-by: Patrick Rudolph 
> ---
>  src/gallium/auxiliary/util/u_helpers.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/util/u_helpers.c 
> b/src/gallium/auxiliary/util/u_helpers.c
> index 09619c1..09020b0 100644
> --- a/src/gallium/auxiliary/util/u_helpers.c
> +++ b/src/gallium/auxiliary/util/u_helpers.c
> @@ -81,7 +81,13 @@ void util_set_vertex_buffers_count(struct 
> pipe_vertex_buffer *dst,
> const struct pipe_vertex_buffer *src,
> unsigned start_slot, unsigned count)
>  {
> -   uint32_t enabled_buffers = (1ull << *dst_count) - 1;
> +   unsigned i;
> +   uint32_t enabled_buffers = 0;
> +
> +   for (i = 0; i < *dst_count; i++) {
> +  if (dst[i].buffer || dst[i].user_buffer)
> + enabled_buffers |= (1ull << i);
> +   }
>
> util_set_vertex_buffers_mask(dst, _buffers, src, start_slot,
>  count);
> --
> 2.4.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] util/u_helpers: return correct number of bound buffers

[Ilia Mirkin]

On Wed, Dec 9, 2015 at 2:01 PM, Patrick Rudolph  wrote:
> Ok, first of all bind some buffers:
>
> pipe->set_vertex_buffers(pipe, 0, 1, );
> pipe->set_vertex_buffers(pipe, 1, 1, );
> pipe->set_vertex_buffers(pipe, 2, 1, );
>
> num_vtxbufs is now 3 as it should be.
>
> Now you are unbinding buffers, one after another starting at slot 0:
> pipe->set_vertex_buffers(pipe, 0, 1, NULL);
>
> enabled_buffers = (1ull << num_vtxbufs) - 1 = 0x7;
> util_set_vertex_buffers_mask(...);
> enabled_buffers = 0x6;
> num_vtxbufs = util_last_bit(enabled_buffers) = 3
>
>
> pipe->set_vertex_buffers(pipe, 1, 1, NULL);
>
> enabled_buffers = (1ull << num_vtxbufs) - 1 = 0x7;
> util_set_vertex_buffers_mask(...);
> enabled_buffers = 0x5;
> num_vtxbufs = util_last_bit(enabled_buffers) = 3
>
>
> pipe->set_vertex_buffers(pipe, 2, 1, NULL);
>
> enabled_buffers = (1ull << num_vtxbufs) - 1 = 0x7;
> util_set_vertex_buffers_mask(...);
> enabled_buffers = 0x3;
> num_vtxbufs = util_last_bit(enabled_buffers) = 2
>
> There are no buffers bound any more, but num_vtxbufs is now 2 instead of
> 0.
>
> There would be no problem if you start at slot 2 going down to 0.
> There would be no problem if you unbind all buffers at once.
>
> I hope this clarifies the problem.

Right, thank you, yes it does. With a slightly fixed commit log
including a bit more justification, this is also

Reviewed-by: Ilia Mirkin 

Basically something mentioning that the current algorithm does not
account for pre-existing holes in the buffer list.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] util/u_helpers: return correct number of bound buffers

[Patrick Rudolph]

Ok, first of all bind some buffers:

pipe->set_vertex_buffers(pipe, 0, 1, );
pipe->set_vertex_buffers(pipe, 1, 1, );
pipe->set_vertex_buffers(pipe, 2, 1, );

num_vtxbufs is now 3 as it should be.

Now you are unbinding buffers, one after another starting at slot 0:
pipe->set_vertex_buffers(pipe, 0, 1, NULL);

enabled_buffers = (1ull << num_vtxbufs) - 1 = 0x7;
util_set_vertex_buffers_mask(...);
enabled_buffers = 0x6;
num_vtxbufs = util_last_bit(enabled_buffers) = 3


pipe->set_vertex_buffers(pipe, 1, 1, NULL);

enabled_buffers = (1ull << num_vtxbufs) - 1 = 0x7;
util_set_vertex_buffers_mask(...);
enabled_buffers = 0x5;
num_vtxbufs = util_last_bit(enabled_buffers) = 3


pipe->set_vertex_buffers(pipe, 2, 1, NULL);

enabled_buffers = (1ull << num_vtxbufs) - 1 = 0x7;
util_set_vertex_buffers_mask(...);
enabled_buffers = 0x3;
num_vtxbufs = util_last_bit(enabled_buffers) = 2

There are no buffers bound any more, but num_vtxbufs is now 2 instead of
0.

There would be no problem if you start at slot 2 going down to 0.
There would be no problem if you unbind all buffers at once.

I hope this clarifies the problem.

Kind Regards,
Patrick

On 2015-12-09 07:10 PM, Ilia Mirkin wrote:
> I'm probably just being dense... can you provide an exact sequence of
> calls that would cause this logic to fail? Seems like it should work
> as-is...
> 
> On Sun, Dec 6, 2015 at 4:12 AM, Patrick Rudolph  wrote:
>> In case a state tracker unbinds every slot by a seperate
>> pipe->set_vertex_buffers() call, starting from slot zero, the number
>> of bound buffers would not reach zero at all. Unbinding all buffers
>> at once or starting at the top-most slot results in correct behaviour.
>>
>> Calculating the correct number of bound buffers fixes a NULL pointer
>> dereference in nvc0_validate_vertex_buffers_shared().
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93004
>>
>> Signed-off-by: Patrick Rudolph 
>> ---
>>  src/gallium/auxiliary/util/u_helpers.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_helpers.c 
>> b/src/gallium/auxiliary/util/u_helpers.c
>> index 09619c1..09020b0 100644
>> --- a/src/gallium/auxiliary/util/u_helpers.c
>> +++ b/src/gallium/auxiliary/util/u_helpers.c
>> @@ -81,7 +81,13 @@ void util_set_vertex_buffers_count(struct 
>> pipe_vertex_buffer *dst,
>> const struct pipe_vertex_buffer *src,
>> unsigned start_slot, unsigned count)
>>  {
>> -   uint32_t enabled_buffers = (1ull << *dst_count) - 1;
>> +   unsigned i;
>> +   uint32_t enabled_buffers = 0;
>> +
>> +   for (i = 0; i < *dst_count; i++) {
>> +  if (dst[i].buffer || dst[i].user_buffer)
>> + enabled_buffers |= (1ull << i);
>> +   }
>>
>> util_set_vertex_buffers_mask(dst, _buffers, src, start_slot,
>>  count);
>> --
>> 2.4.3
>>
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/2] util/u_helpers: return correct number of bound buffers

[Patrick Rudolph]

In case a state tracker unbinds every slot by a seperate
pipe->set_vertex_buffers() call, starting from slot zero, the number
of bound buffers would not reach zero at all. Unbinding all buffers
at once or starting at the top-most slot results in correct behaviour.

Calculating the correct number of bound buffers fixes a NULL pointer
dereference in nvc0_validate_vertex_buffers_shared().

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93004

Signed-off-by: Patrick Rudolph 
---
 src/gallium/auxiliary/util/u_helpers.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/gallium/auxiliary/util/u_helpers.c 
b/src/gallium/auxiliary/util/u_helpers.c
index 09619c1..09020b0 100644
--- a/src/gallium/auxiliary/util/u_helpers.c
+++ b/src/gallium/auxiliary/util/u_helpers.c
@@ -81,7 +81,13 @@ void util_set_vertex_buffers_count(struct pipe_vertex_buffer 
*dst,
const struct pipe_vertex_buffer *src,
unsigned start_slot, unsigned count)
 {
-   uint32_t enabled_buffers = (1ull << *dst_count) - 1;
+   unsigned i;
+   uint32_t enabled_buffers = 0;
+
+   for (i = 0; i < *dst_count; i++) {
+  if (dst[i].buffer || dst[i].user_buffer)
+ enabled_buffers |= (1ull << i);
+   }
 
util_set_vertex_buffers_mask(dst, _buffers, src, start_slot,
 count);
-- 
2.4.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] util/u_helpers: return correct number of bound buffers

[Marek Olšák]

Reviewed-by: Marek Olšák 

Marek

On Sun, Dec 6, 2015 at 10:12 AM, Patrick Rudolph  wrote:
> In case a state tracker unbinds every slot by a seperate
> pipe->set_vertex_buffers() call, starting from slot zero, the number
> of bound buffers would not reach zero at all. Unbinding all buffers
> at once or starting at the top-most slot results in correct behaviour.
>
> Calculating the correct number of bound buffers fixes a NULL pointer
> dereference in nvc0_validate_vertex_buffers_shared().
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93004
>
> Signed-off-by: Patrick Rudolph 
> ---
>  src/gallium/auxiliary/util/u_helpers.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/util/u_helpers.c 
> b/src/gallium/auxiliary/util/u_helpers.c
> index 09619c1..09020b0 100644
> --- a/src/gallium/auxiliary/util/u_helpers.c
> +++ b/src/gallium/auxiliary/util/u_helpers.c
> @@ -81,7 +81,13 @@ void util_set_vertex_buffers_count(struct 
> pipe_vertex_buffer *dst,
> const struct pipe_vertex_buffer *src,
> unsigned start_slot, unsigned count)
>  {
> -   uint32_t enabled_buffers = (1ull << *dst_count) - 1;
> +   unsigned i;
> +   uint32_t enabled_buffers = 0;
> +
> +   for (i = 0; i < *dst_count; i++) {
> +  if (dst[i].buffer || dst[i].user_buffer)
> + enabled_buffers |= (1ull << i);
> +   }
>
> util_set_vertex_buffers_mask(dst, _buffers, src, start_slot,
>  count);
> --
> 2.4.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev