[Mesa-dev] [PATCH 2/2] mesa: don't double incr/decr ActiveCounters

[Rob Clark]

On Mon, Jul 2, 2018 at 12:15 PM, Brian Paul <brianp at vmware.com> wrote:
> On 07/02/2018 09:13 AM, Rob Clark wrote:
>>
>> Frameretrace ends up w/ excess calls to SelectPerfMonitorCountersAMD()
>> which ends up re-enabling already enabled counters.  Which causes
>> ActiveCounters[group] to be double incremented for the same counter.
>> This causes BeginPerfMonitorAMD() to fail.
>>
>> The AMD_performance_monitor spec doesn't say that an error should be
>> generated in this case.  So I think the safe thing to do is just safe-
>> guard against excess increments/decrements.
>>
>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>> ---
>>   src/mesa/main/performance_monitor.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/src/mesa/main/performance_monitor.c
>> b/src/mesa/main/performance_monitor.c
>> index 359727777ff..37762e056b0 100644
>> --- a/src/mesa/main/performance_monitor.c
>> +++ b/src/mesa/main/performance_monitor.c
>> @@ -480,12 +480,16 @@ _mesa_SelectPerfMonitorCountersAMD(GLuint monitor,
>> GLboolean enable,
>>      if (enable) {
>>         /* Enable the counters */
>>         for (i = 0; i < numCounters; i++) {
>> +         if (BITSET_TEST(m->ActiveCounters[group], counterList[i]))
>> +            continue;
>>            ++m->ActiveGroups[group];
>>            BITSET_SET(m->ActiveCounters[group], counterList[i]);
>
>
> If it were me, I'd simply use a conditional w/out continue:
>
>    if (!BITSET_TEST(m->ActiveCounters[group], counterList[i])) {
>
>       ++m->ActiveGroups[group];
>       BITSET_SET(m->ActiveCounters[group], counterList[i]);
>    }


In cases like this (adding a condition to body of loop), I tend to
prefer if(cond)continue just because it doesn't change the indentation
level of the rest of the loop body, so it looks a bit nicer in
git-blame..

But it's a pretty mild preference, and in this case the loop body
isn't something so complicated with lots of history ;-)

BR,
-R

>
>>         }
>>      } else {
>>         /* Disable the counters */
>>         for (i = 0; i < numCounters; i++) {
>> +         if (!BITSET_TEST(m->ActiveCounters[group], counterList[i]))
>> +            continue;
>>            --m->ActiveGroups[group];
>>            BITSET_CLEAR(m->ActiveCounters[group], counterList[i]);
>>         }
>>
>
> -Brian