Ticket #7981 (assigned defect)

Opened 6 years ago

Last modified 5 years ago

EC mask setting is inefficient

Reported by: cjb Owned by: dsaxena
Priority: normal Milestone: 8.2.0 (was Update.2)
Component: kernel Version: not specified
Keywords: polish:8.2.0 cjbfor8.2 csafor8.2 Cc: rsmith, mstone, cscott
Action Needed: design Verified: no
Deployments affected: Blocked By:
Blocking:

Description

From rsmith:

So looking my EC logs from the newer joyrides I see that there are like 6 repeat calls for setting up the mask. This is very non-optimal. Without the fastpath EC code you are talking about 100ms or so of EC command time with fastpath its still going to be 25 or 30ms.

Just seems silly to me. Why can we not also have a control that echos a single hex value? Just like the SCSI debugging code.

==

(Note that this isn't so urgent, because it only affects entering "sleep", and not leaving it. It's still a waste, though.

Attachments

olpc-all-events.patch (2.3 kB) - added by dsaxena 6 years ago.
Patch to implement "all" interface

Change History

follow-up: ↓ 3   Changed 6 years ago by dsaxena

This might be due to OHM disabling events via the /sys/power/wakeup_events interface, where we write one file per bit and on every write I do a RMW on the EC mask. I should do some sort of caching in the kernel and only write back the bits when we trigger a suspend.

  Changed 6 years ago by cjb

This might be due to OHM disabling events via the /sys/power/wakeup_events interface

Yes, certainly is.

I should do some sort of caching in the kernel and only write back the bits when we trigger a suspend.

Oh, that's a fine idea, I hadn't thought of that. Thanks!

in reply to: ↑ 1 ; follow-up: ↓ 6   Changed 6 years ago by cjb

Replying to dsaxena:

This might be due to OHM disabling events via the /sys/power/wakeup_events interface, where we write one file per bit and on every write I do a RMW on the EC mask. I should do some sort of caching in the kernel and only write back the bits when we trigger a suspend.

This won't quite work, because I want to turn off the BAT SOC events before going into idle suspend, and I want to re-enable them on resume. Perhaps instead have the sysfs files modify the kernel's internal idea of the mask, and then "echo 1 > commit" to commit them to the EC? (Or something similar.)

  Changed 6 years ago by cjb

  • keywords polish:8.2.0 cjbfor8.2 added
  • milestone changed from 8.2.1 to 8.2.0 (was Update.2)

Now that OHM is handling the EC mask, suspend and resume are being delayed hundreds of milliseconds by this inefficiency. I'd like a system as proposed here for 8.2, if possible.

  Changed 6 years ago by cjb

(Though any solution that stops the delay and allows reprogramming at resume would suffice if the above is difficult.)

in reply to: ↑ 3   Changed 6 years ago by dsaxena

  • status changed from new to assigned

Replying to cjb:

Replying to dsaxena:

This might be due to OHM disabling events via the /sys/power/wakeup_events interface, where we write one file per bit and on every write I do a RMW on the EC mask. I should do some sort of caching in the kernel and only write back the bits when we trigger a suspend.

This won't quite work, because I want to turn off the BAT SOC events before going into idle suspend, and I want to re-enable them on resume. Perhaps instead have the sysfs files modify the kernel's internal idea of the mask, and then "echo 1 > commit" to commit them to the EC? (Or something similar.)

Hmm...that does add a complication. I don't really like the idea of a separate "commit" operation as sysfs ops are meant to be atomically completed and this seems to go counter to that. We may just have to go back to the original idea of one single file that takes the whole bitmask. Let me mull this over a bit...

  Changed 6 years ago by cjb

Thanks. Don't mull long enough that we miss 8.2. :)

The other, least invasive, option here is an "all" entry that corresponds with 0xFF. Then OHM does echo 0 > all on the way down, and echo 1 > all on the way up, and I'm happy about performance again.

Changed 6 years ago by dsaxena

Patch to implement "all" interface

  Changed 6 years ago by dsaxena

Chris, please see attached patch. If it looks good to you, I'll commit to testing.

  Changed 6 years ago by cjb

ACK, looks good to me.

  Changed 6 years ago by dsaxena

The patch had a major bug but I've fixed and RPM is @ http://dev.laptop.org/~dilinger/testing/kernel-devel-2.6.25-20080908.1.olpc.63579b21df15ab7.i586.rpm. New joyride with it should appear soon if all the autobuild magic is working.

  Changed 6 years ago by cscott

  • next_action changed from never set to approve for release

Presumably this is a candidate for 8.2? Has it been tested in joyride yet?

  Changed 6 years ago by cjb

  • next_action changed from approve for release to test in build

Ready for testing in joyride.

  Changed 6 years ago by cjb

OHM changes that take advantage of this are also on their way to Joyride, as ohm-0_1_1-6_19_20080910git_olpc3.

  Changed 6 years ago by cscott

ohm.i386 0:0.1.1-6.19.20080910git.olpc3
kernel.i586 0:2.6.25-20080909.2.olpc.2dfd32b70c58803

Please test in joyride and assign action to 'approve for release' ASAP; we're trying to do a new stable build today.

  Changed 6 years ago by cscott

  • cc mstone, cscott added

  Changed 6 years ago by cscott

  • keywords csafor8.2 added

  Changed 6 years ago by cjb

Will have this tested in Joyride in an hour or so, thanks.

  Changed 6 years ago by cjb

  • next_action changed from test in build to add to release

Tested working, please pull. There aren't any user-visible changes.

  Changed 6 years ago by cscott

  • next_action changed from add to release to test in release

ohm-0_1_1-6_20_20080911git_olpc3 added to stable repo: http://mock.laptop.org/gitweb/gitweb.cgi?p=repos;a=commitdiff;h=d11b42529c97a1c9030b51a20ca9d1e5931eb79b

kernel-2.6.25-20080909.3.olpc.850b087f7daf1b0 added to stable repo: http://mock.laptop.org/gitweb/gitweb.cgi?p=repos;a=commitdiff;h=10fbd74d6603239e05f057028d7b77e0974c1128

Should be in 8.2 build 760. Please test.

  Changed 5 years ago by cjb

  • next_action changed from test in release to design

So, this is still very slow. Richard, do you have a faster way for us to set the EC mask yet?

If not, I'll paper around the problem by setting it before we start suspending in idle suspend (and setting it back if we end up not suspending after all) and then waiting until the UI is responsive again before resetting it when we come out of idle suspend, to avoid blocking on setting the mask. This won't affect the slowness when someone hits the power button, though, so I'd much rather make it faster in all cases..

  Changed 5 years ago by pgf

can you remind me (i'm sure richard remembers) what the "faster" way for the EC mask to be set is? i thought the kernel changes made it so there's just once command transaction to the EC. no?

  Changed 5 years ago by cjb

what the "faster" way for the EC mask to be set is?

I think Richard mentioned having a faster roundtrip for EC commands in the works (about six months ago), so that's all I'm referring to. I don't remember any more details. :)

Note: See TracTickets for help on using tickets.