I like the cleanups here. I prefer the SysCounterGetPrivate() helper over the macro, but don't particularly think the need to allow SyncNumSystemCounters to go negative by decrementing in ResetProc() is a win for clarity.
Thanks, -James On 3/14/12 4:30 PM, Peter Hutterer wrote: > 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. > > From cc02b58e95822a50658ef7fe13e862c3f569ee22 Mon Sep 17 00:00:00 2001 > From: Peter Hutterer<peter.hutte...@who-t.net> > Date: Thu, 15 Mar 2012 09:27:24 +1000 > Subject: [PATCH] Xext: replace the macro with a static inline function. > > Signed-off-by: Peter Hutterer<peter.hutte...@who-t.net> > --- > Xext/sync.c | 24 ++++++++++++++++++------ > Xext/syncsrv.h | 2 -- > 2 files changed, 18 insertions(+), 8 deletions(-) > > diff --git a/Xext/sync.c b/Xext/sync.c > index 6c8c564..5f74dab 100644 > --- a/Xext/sync.c > +++ b/Xext/sync.c > @@ -115,6 +115,15 @@ static void SyncInitServerTime(void); > > static void SyncInitIdleTime(void); > > +static inline void* > +SysCounterGetPrivate(SyncCounter *counter) > +{ > + BUG_WARN(!IsSystemCounter(counter)); > + > + return counter->pSysCounterInfo ? counter->pSysCounterInfo->private : > NULL; > +} > + > + > static Bool > SyncCheckWarnIsCounter(const SyncObject* pSync, const char *warning) > { > @@ -2747,7 +2756,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 > *pValue_return) > CARD32 idle; > > if (pCounter) { > - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); > + SyncCounter *counter = pCounter; > + IdleCounterPriv *priv = SysCounterGetPrivate(counter); > deviceid = priv->deviceid; > } else > deviceid = XIAllDevices; > @@ -2758,8 +2768,8 @@ IdleTimeQueryValue (pointer pCounter, CARD64 > *pValue_return) > static void > IdleTimeBlockHandler(pointer pCounter, struct timeval **wt, pointer > LastSelectMask) > { > - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); > SyncCounter *counter = pCounter; > + IdleCounterPriv *priv = SysCounterGetPrivate(counter); > XSyncValue *less = priv->value_less, > *greater = priv->value_greater; > XSyncValue idle, old_idle; > @@ -2835,7 +2845,8 @@ IdleTimeBlockHandler(pointer pCounter, struct timeval > **wt, pointer LastSelectMa > static void > IdleTimeWakeupHandler (pointer pCounter, int rc, pointer LastSelectMask) > { > - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); > + SyncCounter *counter = pCounter; > + IdleCounterPriv *priv = SysCounterGetPrivate(counter); > XSyncValue *less = priv->value_less, > *greater = priv->value_greater; > XSyncValue idle; > @@ -2856,7 +2867,8 @@ static void > IdleTimeBracketValues (pointer pCounter, CARD64 *pbracket_less, > CARD64 *pbracket_greater) > { > - IdleCounterPriv *priv = SYSCOUNTERPRIV(pCounter); > + SyncCounter *counter = pCounter; > + IdleCounterPriv *priv = SysCounterGetPrivate(counter); > XSyncValue *less = priv->value_less, > *greater = priv->value_greater; > Bool registered = (less || greater); > @@ -2897,7 +2909,7 @@ init_system_idle_counter(const char *name, int deviceid) > priv->deviceid = deviceid; > priv->value_less = priv->value_greater = NULL; > > - SYSCOUNTERPRIV(idle_time_counter) = priv; > + idle_time_counter->pSysCounterInfo->private = priv; > > return idle_time_counter; > } > diff --git a/Xext/syncsrv.h b/Xext/syncsrv.h > index 361b68d..6c0bf74 100644 > --- a/Xext/syncsrv.h > +++ b/Xext/syncsrv.h > @@ -71,8 +71,6 @@ typedef void (*SyncSystemCounterBracketValues)(pointer > counter, > CARD64 *pbracket_less, > CARD64 *pbracket_greater); > > -#define SYSCOUNTERPRIV(counter) > (((SyncCounter*)(counter))->pSysCounterInfo->private) > - > typedef struct _SysCounterInfo { > char *name; > CARD64 resolution; nvpublic _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel