[Mesa-dev] [RFC PATCH 01/14] anv/tests: Fix block_pool_no_free test.

[Jason Ekstrand]

On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <
rafael.antognolli at intel.com> wrote:

> The test was checking whether -1 was smaller than an unsigned int, which
> is always false. So it was exiting early and never running until the
> end, since it would reach the condition (thread_max == -1).
>
> However, just fixing that is not enough. The test is currently getting
> the highest block on each iteration, and then on the next one, until it
> reaches the end. But by that point, we wouldn't have looked at all
> blocks of all threads. For instance, for 3 threads and 4 blocks per
> thread, a situation like this (unlikely to happen):
>
> [Thread]: [Blocks]
>    [0]: [0, 32, 64, 96]
>    [1]: [128, 160, 192, 224]
>    [2]: [256, 288, 320, 352]
>
> Would cause the test to iterate only over the thread number 2.
>
> The fix is to always grab the lowest block on each iteration, and assert
> that it is higher than the one from the previous iteration.
> ---
>  src/intel/vulkan/tests/block_pool_no_free.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/vulkan/tests/block_pool_no_free.c
> b/src/intel/vulkan/tests/block_pool_no_free.c
> index 17006dd3bc7..730297d4e36 100644
> --- a/src/intel/vulkan/tests/block_pool_no_free.c
> +++ b/src/intel/vulkan/tests/block_pool_no_free.c
> @@ -78,16 +78,16 @@ static void validate_monotonic(uint32_t **blocks)
>     unsigned next[NUM_THREADS];
>     memset(next, 0, sizeof(next));
>
> -   int highest = -1;
> +   uint32_t lowest = UINT32_MAX;
>

I think this should still be named "highest" since it is the highest thing
we've seen.  Also, I kind-of think -1 still makes sense as a starting
value.  Maybe we just need to make the comparison below explicitly signed
with a cast.


>     while (true) {
> -      /* First, we find which thread has the highest next element */
> -      int thread_max = -1;
> +      /* First, we find which thread has the lowest next element */
> +      uint32_t thread_max = UINT32_MAX;
>

Yes, this loop needs to change.  However, I think we should rename the
variables to thread_min and min_thread_idx.


>        int max_thread_idx = -1;
>        for (unsigned i = 0; i < NUM_THREADS; i++) {
>           if (next[i] >= BLOCKS_PER_THREAD)
>              continue;
>
> -         if (thread_max < blocks[i][next[i]]) {
> +         if (thread_max > blocks[i][next[i]]) {
>              thread_max = blocks[i][next[i]];
>              max_thread_idx = i;
>           }
> @@ -96,13 +96,14 @@ static void validate_monotonic(uint32_t **blocks)
>        /* The only way this can happen is if all of the next[] values are
> at
>         * BLOCKS_PER_THREAD, in which case, we're done.
>         */
> -      if (thread_max == -1)
> +      if (thread_max == UINT32_MAX)
>           break;
>
> -      /* That next element had better be higher than the previous highest
> */
> -      assert(blocks[max_thread_idx][next[max_thread_idx]] > highest);
> +      /* That next element had better be higher than the previous lowest
> */
> +      assert(lowest == UINT32_MAX ||
> +             blocks[max_thread_idx][next[max_thread_idx]] > lowest);
>
> -      highest = blocks[max_thread_idx][next[max_thread_idx]];
> +      lowest = blocks[max_thread_idx][next[max_thread_idx]];
>        next[max_thread_idx]++;
>     }
>  }
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181210/bfc864b5/attachment.html>