GPM unresponsive for two minutes after startup on Macbooks

The patch has been uploaded to the mactel PPA.

Repackaged the patch a bit, hopefully fits together with the ubuntu patches a bit better. The two following attachments replace the old patch.

I don't understand why the patch has to change the type of loop being used. Why can't it just use the new function as the increment or decrement functions in the for loops? It would make the patch much smaller.

I presume you are talking about patch 91.

The current code is executing the body of the for loop for both end points, cur and shared_value_abs, Besides being unnecessary, it means the loop exits for a value outside of the valid range. This makes it impossible to use the guarded_up/down functions, which never leaves the valid range.

It *is* possible to make the patch smaller, by 18 lines, by leaving the if statements untouched, like this:

@@ -331,11 +322,12 @@
                return TRUE;
        }

- /* step the correct way */
+ /* step the correct way - by using guarded and scaled stepping */
        if (cur < shared_value_abs) {
                /* going up */
- for (i=cur; i<=shared_value_abs; i++) {
- ret = gpm_brightness_xrandr_output_set_internal (brightness, output, i);
+ while (cur < shared_value_abs) {
+ cur = gpm_brightness_get_guarded_up (min, cur, shared_value_abs);
+ ret = gpm_brightness_xrandr_output_set_internal (brightness, output, cur);
                        if (!ret) {
                                break;
                        }
@@ -344,9 +336,10 @@
                        }
                }
        } else {
- /* going down */
- for (i=cur; i>=shared_value_abs; i--) {
- ret = gpm_brightness_xrandr_output_set_internal (brightness, output, i);
+ while (cur > shared_value_abs) {
+ /* going down */
+ cur = gpm_brightness_get_guarded_down (shared_value_abs, cur, max);
+ ret = gpm_brightness_xrandr_output_set_internal (brightness, output, cur);
                        if (!ret) {
                                break;
                        }

However, the resulting code looks rather artifical. Would you prefer a patch base on the above?

... make that 20 lines; the "going down" comment had been strangely misplaced.

On Mon, 2008-10-27 at 21:40 +0000, rydberg wrote:
> It *is* possible to make the patch smaller, by 18 lines, by leaving the
> if statements untouched, like this:
<snip>
> However, the resulting code looks rather artifical. Would you prefer a
> patch base on the above?

Yes, I think that's better. Not because the code is better, but because
the patch is smaller and it's more obvious what you're changing. I find
that in general, when submitting patches upstream, it is better to be
small and try your best to show exactly what you're changing. Those
patches tend to get accepted faster. (and sometimes at all)

True. I removed the old attachments and added new ones accordingly. I also took the liberty of changing patch 90 to use non-linear stepping; it makes every step roughly equal in perceived brightness change, and allows for much finer tuning at low brightness.

This bug was fixed in the package gnome-power-manager - 2.24.2-2ubuntu1

---------------
gnome-power-manager (2.24.2-2ubuntu1) jaunty; urgency=low

  [ Ted Gould ]
  * Merge from Debian experimental.
  * Remove 20_fix_precentage_closures.patch as already applied upstream.
  * Merge in the mactel changes. See mactel changelog entries below.
  * Remove 02_cast_align.patch as it only hides the problem and doesn't fix
  * Remove 13_bugreport_bin_sh.patch as it conflicts with the Debian added
    07-bugreport-shebang.patch patch.
  * Remove 26_check_for_active_session.patch as it should no longer be
    required with the upstream changes.
  * debian/rules: don't disable policy-kit

  [ Steve Langasek ]
  * debian/control: point Vcs-Bzr at current branch.
  * Drop debian/patches/70_relibtoolize.patch in favor of
    76_autoreconf.patch
  * debian/patches/14_inhibit_tool.patch,
    debian/patches/17_icon_cache.patch: split out the changes to
    Makefile.in, so we can handle these as a batch in 76_autoreconf.patch
  * Refresh 76_autoreconf.patch for the above changes
  * 95_dont_listen_for_suspend_X_key.patch: handle 'suspend' via hal only,
    the same as we already do for 'hibernate', eliminating a double-suspend
    on ThinkPads. LP: #306310.

gnome-power-manager (2.24.2-2) experimental; urgency=low

  * Switch to quilt to manage patches; build-depend on quilt.
  * 02_cast_align.patch: new patch. Use memcpy instead of casting
    pointers with incompatible alignments. Closes: #510011.
  * 70_relibtoolize.patch: re-run the autotools on top of the source to
    avoid the rpath issue.
  * Pass -O1 -z defs --as-needed to the linker.

gnome-power-manager (2.24.2-1) experimental; urgency=low

  * New upstream release
  * debian/patches/02-ups-invalid-free.patch
    - Removed, fixed upstream
  * debian/patches/03-system-policy.patch
    - Updated
  * debian/patches/04-graph-labels.patch
    - Removed, fixed upstream
  * debian/patches/05_translation_crash.patch
    - Removed, fixed upstream
  * debian/patches/06-bugreport-debian.patch
    - Updated
  * debian/patches/08-desktop-bugreport-path.patch
    - Updated

gnome-power-manager (2.24.0-1mactel4) unstable; urgency=low

  * Fix light sensor scaling

gnome-power-manager (2.24.0-1mactel3) unstable; urgency=low

  * Nonlinear stepping for smooth dimming
  * Patch trimming for upstream

gnome-power-manager (2.24.0-1mactel2) unstable; urgency=low

  * Mixed signed/unsigned problem in gpm-brightness-xrandr fixed.
    LP: #289520

gnome-power-manager (2.24.0-1mactel1) unstable; urgency=low

  * The gpm-brightness-xrandr smooth brightness function does not take
    the number of brightness levels into account. On recent Macbooks,
    this leads to some 20000 brightness-set calls on startup, an
    activity that takes about one and a half minute. During this time,
    gpm is not responsive. This patch introduces scaled stepping,
    which fixes the problem. LP: #289520

 -- Steve Langasek <email address hidden> Sat, 07 Feb 2009 09:17:32 -0500