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 <[email protected]>
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 <[email protected]>
---
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;
--
1.7.7.6
>
> The series looks fine generally; these are just minor details.
>
> Jamey
>
> On 3/13/12, Peter Hutterer <[email protected]> wrote:
> >
> > We currently have one global idlecounter. This patch series adds additional
> > counters named "DEVICEIDLETIME n" (where n is the device id) that apply to
> > that device only.
> >
> > One use-case here is as syndaemon replacement. A client can simply put an
> > idlecounter on the keyboard above the touchpad. When that keyboard goes out
> > of idle, disable the touchpad, then re-enable when the keyboard goes idle
> > again.
> >
> > Cheers,
> > Peter
> > _______________________________________________
> > [email protected]: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
> >
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
