[PATCH 0/9] per-device idle counters

[Peter Hutterer]

On Wed, Mar 14, 2012 at 09:01:13AM -0700, Jamey Sharp wrote:
> This series seems like a good idea to me! I have a few review comments though:
> 
> Patch 1 seems strange. Shouldn't the counters themselves get freed at
> reset? If they are, shouldn't that lead to the number of counters
> reaching 0?

you're right but the order is a tad weird. The ResetProc is called before
the counters are cleaned up, so we free the container list but the number is
still correct. Later, the number goes down to 0 when the counters are
actually freed. Technically correct, but weird. I think the diff below would
make sense to merge into patch 1/9. This way, we remove all knowledge of
system counters in the ResetProc and don't get any weird effects.

diff --git a/Xext/sync.c b/Xext/sync.c
index 6c8c564..bf47c93 100644
--- a/Xext/sync.c
+++ b/Xext/sync.c
@@ -1200,8 +1200,8 @@ FreeCounter(void *env, XID id)
                    SysCounterList[i] = SysCounterList[i+1];
                }
            }
+            SyncNumSystemCounters--;
        }
-       SyncNumSystemCounters--;
     }
     free(pCounter);
     return Success;

 
> I guess the input ABI should get bumped for the patch that stores
> per-device idle times. We decided to bump ABI more aggressively,
> right?

correct, but I need to go through my lists to check whether I've got
anything else that bumps the ABI in the near future. If so, I prefer
bundling them with one ABI bump.

> I haven't checked: is it possible to build xserver without SYNC, or to
> disable it at runtime? If so, the final patch needs some conditionals,
> I assume.

in configure.ac:1312, it's always defined, so no conditionals are necessary.
AC_DEFINE(XSYNC, 1, [Support XSync extension])

> Here's a request you can ignore if you like: Please consider making
> SYSCOUNTERPRIV a static inline instead of a macro, and removing the
> cast from inside it. That'd be different from the usual idiom in the X
> server, but the usual idiom is dumb. :-) I'd prefer to see all the
> callers assign their "pointer pCounter"s to an appropriately-typed
> local once right at the beginning of the function, which also serves
> as documentation for what type you're supposed to pass in there.

How does this one look then? I'd squash this in but it's easier to review as
a diff on top.