[Mesa-dev] [PATCH] aubinator: implement a rolling window of programs

[Lionel Landwerlin]

On 01/09/17 15:37, Eric Engestrom wrote:
> On Friday, 2017-09-01 10:38:33 +0100, Lionel Landwerlin wrote:
>> If we have more programs than what we can store,
>> aubinator_error_decode will assert. Instead let's have a rolling
>> window of programs.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>>   src/intel/tools/aubinator_error_decode.c | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
>> index 636f56a3365..42cc6994353 100644
>> --- a/src/intel/tools/aubinator_error_decode.c
>> +++ b/src/intel/tools/aubinator_error_decode.c
>> @@ -47,6 +47,8 @@
>>   #define GREEN_HEADER CSI "1;42m"
>>   #define NORMAL       CSI "0m"
>>   
>> +#define MIN(a, b) ((a) < (b) ? (a) : (b))
>> +
>>   /* options */
>>   
>>   static bool option_full_decode = true;
>> @@ -300,7 +302,7 @@ static void decode(struct gen_spec *spec,
>>                                  enabled[1] ? "SIMD16 fragment shader" :
>>                                  enabled[2] ? "SIMD32 fragment shader" : NULL;
>>   
>> -            programs[num_programs++] = (struct program) {
>> +            programs[num_programs++ % ARRAY_SIZE(programs)] = (struct program) {
> num_programs++ will eventually overflow, you should wrap it around on
> your own term. How about:
>
> ----8<----
> diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
> index 636f56a336..173e0e97fe 100644
> --- a/src/intel/tools/aubinator_error_decode.c
> +++ b/src/intel/tools/aubinator_error_decode.c
> @@ -222,6 +222,11 @@ struct program {
>   static struct program programs[MAX_NUM_PROGRAMS];
>   static int num_programs = 0;
>   
> +static int next_program()
> +{
> +   return num_programs = (num_programs + 1) % ARRAY_SIZE(programs);
> +}

I guess we need to take some care with the programs[0] then, as this 
might leave it empty/uninitialized.

> +
>   static void decode(struct gen_spec *spec,
>                      const char *buffer_name,
>                      const char *ring_name,
> @@ -300,7 +305,7 @@ static void decode(struct gen_spec *spec,
>                                  enabled[1] ? "SIMD16 fragment shader" :
>                                  enabled[2] ? "SIMD32 fragment shader" : NULL;
>   
> -            programs[num_programs++] = (struct program) {
> +            programs[next_program()] = (struct program) {
>                  .type = type,
>                  .command = inst->name,
>                  .command_offset = offset,
> ---->8----
>
> With that, you can keep the assert (although it would only fire it
> someone were to manually increment `num_programs`) and the MIN() becomes
> unnecessary.
>
>>                  .type = type,
>>                  .command = inst->name,
>>                  .command_offset = offset,
>> @@ -309,7 +311,7 @@ static void decode(struct gen_spec *spec,
>>               };
>>            } else {
>>               if (enabled[0]) /* SIMD8 */ {
>> -               programs[num_programs++] = (struct program) {
>> +               programs[num_programs++ % ARRAY_SIZE(programs)] = (struct program) {
>>                     .type = "SIMD8 fragment shader",
>>                     .command = inst->name,
>>                     .command_offset = offset,
>> @@ -319,7 +321,7 @@ static void decode(struct gen_spec *spec,
>>                  };
>>               }
>>               if (enabled[1]) /* SIMD16 */ {
>> -               programs[num_programs++] = (struct program) {
>> +               programs[num_programs++ % ARRAY_SIZE(programs)] = (struct program) {
>>                     .type = "SIMD16 fragment shader",
>>                     .command = inst->name,
>>                     .command_offset = offset,
>> @@ -328,7 +330,7 @@ static void decode(struct gen_spec *spec,
>>                  };
>>               }
>>               if (enabled[2]) /* SIMD32 */ {
>> -               programs[num_programs++] = (struct program) {
>> +               programs[num_programs++ % ARRAY_SIZE(programs)] = (struct program) {
>>                     .type = "SIMD32 fragment shader",
>>                     .command = inst->name,
>>                     .command_offset = offset,
>> @@ -375,7 +377,7 @@ static void decode(struct gen_spec *spec,
>>               NULL;
>>   
>>            if (is_enabled) {
>> -            programs[num_programs++] = (struct program) {
>> +            programs[num_programs++ % ARRAY_SIZE(programs)] = (struct program) {
>>                  .type = type,
>>                  .command = inst->name,
>>                  .command_offset = offset,
>> @@ -384,8 +386,6 @@ static void decode(struct gen_spec *spec,
>>               };
>>            }
>>         }
>> -
>> -      assert(num_programs < MAX_NUM_PROGRAMS);
>>      }
>>   }
>>   
>> @@ -593,7 +593,7 @@ read_data_file(FILE *file)
>>            if (strcmp(buffer_name, "user") == 0) {
>>               printf("Disassembly of programs in instruction buffer at "
>>                      "0x%08"PRIx64":\n", gtt_offset);
>> -            for (int i = 0; i < num_programs; i++) {
>> +            for (int i = 0; i < MIN(ARRAY_SIZE(programs), num_programs); i++) {
>>                  if (programs[i].instruction_base_address == gtt_offset) {
>>                       printf("\n%s (specified by %s at batch offset "
>>                              "0x%08"PRIx64") at offset 0x%08"PRIx64"\n",
>> -- 
>> 2.14.1
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev