Bug 622651

Summary: Xext/sync.c IDLETIME counter sometimes fails to fire negative transition alarms
Product: [Fedora] Fedora Reporter: Tim Taiwanese Liim <tim.liim>
Component: xorg-x11-serverAssignee: Adam Jackson <ajax>
Status: CLOSED RAWHIDE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: 13CC: cbrereton1, hannes, jh.redhat-2018, richardfearn, xgl-maint, yaneti
Target Milestone: ---Keywords: Patch, Triaged
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2010-10-12 03:00:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Tim Taiwanese Liim 2010-08-10 04:06:06 UTC
Description of problem:
    This issue was first described in 
        bug612620 Fade-out to screensaver cannot be interrupted 
                  with xorg-x11-server-Xorg-1.8.2-1.fc13

    It was caused by Xext/sync.c, who should but failed to fire a negative
    transition alarm.  See bug612620 Comment #21, Item #6,#7, which is
    copied here for ease of reference.

    #6 So how is a "+/- transition" defined, exactly?
       See [6] for code listing; after stripping the anti-syntatic-sugar,
       they boil down to
            test #1 (+trans):  oldval <  test_value <= newval
            test #2 (-trans):  newval <= test_value <  oldval
       This works under all usual cases.  But here is a corner case that
       breaks this code.

      - Assume we have test_value of 60000ms, with a pair of +trans
        trigger (when the user is idle for >=60sec) and -trans (when the
        user is no longer idle, ie. idle time drops to be <60 sec from
        >=60 sec).
      - In good cases, the newval is usually 60001-60003ms when +trans
        passes test #1 
            oldval (eg.59999) <  test_value (60000) <= newval (eg. 60002)
        So when it comes to test for -trans, we have test #2
            newval (eg.  1ms) <= test_value (60000) <  oldval (eg. 60002)
        which is true, so the -trans is triggered, and g-ss is no longer idle.

      - In bad cases, the newval is always 6000ms when +trans passes test #1 
            oldval (eg.59999) <  test_value (60000) <= newval (eg. 60000)
        So when it comes to test for -trans, we have test #2
            newval (eg.  1ms) <= test_value (60000) <  oldval (eg. 60000)
        which is false, so the -trans is *not* triggered, and g-ss does not 
        receives no-longer-idle notice.

      - so I think test #2 should be
            newval (eg.  1ms) <  test_value (60000) <= oldval (eg. 60000)

    #7 Changing test #2 to be
        SyncCheckTriggerNegativeTransition(SyncTrigger *pTrigger, CARD64 oldval) 
        { 
            return (pTrigger->pCounter == NULL || 
                    (XSyncValueGreaterOrEqual(oldval, pTrigger->test_value) && 
                     XSyncValueLessThan(pTrigger->pCounter->value, 
                                           pTrigger->test_value))); 
        }
       And now my g-ss fading is interruptable, everytime.  It seems the
       proposed change in Xext/sync.c fixes this bug.


Version-Release number of selected component (if applicable):
    xorg-x11-server-Xorg-1.8.2-3.fc13.i686

How reproducible:
    60-80% of attempted g-ss fading interuption

Steps to Reproduce:
    1.login to gnome
    2.when screen starts to fade, hit a key to stop it.
  
Actual results:
    60-80% of attempted g-ss fading interuption are ignored.
    The rest of fading-interruption are successful.

Expected results:
    g-ss screen fading should always be interruptable by user activity.

Additional info:
    See bug612620 Comment #21 for root cause analysis.  Feel free to
    disagree.  I see that maintaining sync.c a tough job: people
    notice its existence only when it is broken, and there are too
    many corner cases to cover.

    So my thanks to you for keeping it working.

Comment 1 Tim Taiwanese Liim 2010-08-12 03:33:53 UTC
Found another boundary condition issue (bug612620 Comment #27); please
see the analysis there.  Here is an summary of proposed fixes.

Fix #1 (bug612620 Comment #21 item #7)
    SyncCheckTriggerNegativeTransition(SyncTrigger *pTrigger, CARD64 oldval) 
    { 
        return (pTrigger->pCounter == NULL || 
                (XSyncValueGreaterOrEqual(oldval, pTrigger->test_value) && 
                 XSyncValueLessThan(pTrigger->pCounter->value, 
                                       pTrigger->test_value))); 
    }

Fix #2 (bug612620 Comment #27)
SyncComputeBracketValues(SyncCounter *pCounter) 
  else if (pTrigger->test_type == XSyncNegativeTransition && 
             ct != XSyncCounterNeverIncreases) 
  { 
    /*if (XSyncValueGreaterThan   (pCounter->value, pTrigger->test_value) && */
      if (XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value) && 
          XSyncValueGreaterThan(pTrigger->test_value, psci->bracket_less))

Comment 2 Tim Taiwanese Liim 2010-10-12 03:00:36 UTC
I'll close this bug.  In bug612620 Comment #41 Adam said
    Fixed in 1.9.0