On Wed, Mar 14, 2012 at 05:11:04PM -0700, James Jones wrote: > 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.
sorry, I'm having troubles parsing this sentence. So you'd prefer leaving SyncNumSystemCounters out decrement it automatically even after the container array has been removed already? or force-reset it to 0 and only decrement while the list is still present, leaving the counter at 0? (which is what the first diff below adds) Cheers, Peter > 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