[PATCH 0/9] per-device idle counters
Jeremy Huddleston
jeremyhu at apple.com
Wed Mar 14 17:38:25 PDT 2012
For the original series and Peter's update, with or without Jamey's change to linked lists,
Reviewed-by: Jeremy Huddleston <jeremyhu at apple.com>
On Mar 14, 2012, at 5:26 PM, Jamey Sharp <jamey at minilop.net> wrote:
> On Wed, Mar 14, 2012 at 4:30 PM, Peter Hutterer
> <peter.hutterer at who-t.net> 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;
>
> This seems like increasing the fragility of the code...
>
> I ran out of time to actually test, but the attached alternative patch
> compiles, at least, and the diffstat makes me happy. What say you?
>
>> in configure.ac:1312, it's always defined, so no conditionals are necessary.
>> AC_DEFINE(XSYNC, 1, [Support XSync extension])
>
> Good. I assume disabling it at runtime doesn't hurt anything either?
> If that's possible...
>
>>> 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.
>
> Looks perfect, except I'm a bit baffled at the NULL check: I'm hoping
> the pCounters can't actually ever be null, but you're making me
> wonder...?
>
>> From cc02b58e95822a50658ef7fe13e862c3f569ee22 Mon Sep 17 00:00:00 2001
>> From: Peter Hutterer <peter.hutterer at 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.hutterer at 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;
>> --
>> 1.7.7.6
>>
>>
>>
>>
>>
>>>
>>> The series looks fine generally; these are just minor details.
>>>
>>> Jamey
>>>
>>> On 3/13/12, Peter Hutterer <peter.hutterer at who-t.net> 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
>>>> _______________________________________________
>>>> xorg-devel at lists.x.org: X.Org development
>>>> Archives: http://lists.x.org/archives/xorg-devel
>>>> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>>>>
> <0001-sync-Use-a-linked-list-instead-of-an-array-for-SysCo.patch>_______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list