The Dark (K)Night

[NiLuJe]

Crappy puns aside:

• Faster SW inversion when done via invertRect (Lua/C, although the C version will be drastically faster on x64, because GCC vectorizes it properly).
• Speaking of, enforce aggressive optimizations when building the C bb, to ensure this will actually happen for real, and not just on my box ;p.
• Switch to HW PxP inversion on eInk devices where this is known to be stable (i.e., Kindles, Kobos aside from the Aura).
• When in NightMode, enforce a possibly less stupid waveform mode (fix the truly ancient FR: allow refresh flags to be changed at runtime koreader#1375). On Kindle, where supported, this attempts to use the nightmode-optimized waveform modes. Needs testers on PW4/KOA2, because there's probably two of them.

[NiLuJe]

Pinging @arooni as a potential PW4 tester ;).

Technically, I don't even need you to test this, a strace of what the default Reader does (both in UI & reader) when in nightmode would be great/better :).

The PW4 strace build is here, as the one bundled with USBNet will probably crash on mxcfb ioctls on those devices ;).

I'll need the output of
strace -fitv -e trace=ioctl -p $(pidof awesome)
after having moved around the UI and the Reader a bit ;)

[NiLuJe]

The frontlight level is still annoying with flashing updates, but this probably requires lower-level shenanigans than we have access to to do right, as far as timing is concerned.

Might be able to piggy-back on the native API on the PW4/KOA2, though, but this might be non-trivial.

[NiLuJe]

Bonus point: If the device can do HW inversion, nightmode now works with the C blitter on eInk ;).

[NiLuJe]

Mwahahaha, even on earlier devices (PW2+), as long as you're running FW >= 5.6.x, GL16_INV has a non-stupid behavior when flashing: it only flashes white once, instead of the weird double-hiccup you get on Kobo.

[NiLuJe]

Vague attempt at getting rid of the weird flashes on Kobo:

diff --git a/ffi/framebuffer_mxcfb.lua b/ffi/framebuffer_mxcfb.lua
index 6e2897a..85290da 100644
--- a/ffi/framebuffer_mxcfb.lua
+++ b/ffi/framebuffer_mxcfb.lua
@@ -249,15 +249,41 @@ local function mxc_update(fb, update_ioctl, refarea, refresh_type, waveform_mode
 
     -- Handle night mode shenanigans
     if fb.night_mode then
-        -- We're in nightmode! If the device can do HW inversion safely, do that!
-        if fb.device:canHWInvert() then
-            refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
-        end
+        if fb.device:isKindle() then
+            -- We're in nightmode! If the device can do HW inversion safely, do that!
+            if fb.device:canHWInvert() then
+                refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
+            end
 
-        -- If the waveform is not fast or GC16, enforce a nightmode-specific mode (usually, GC16), to limit ghosting
-        if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
-            waveform_mode = fb:_getNightWaveFormMode()
-            refarea[0].waveform_mode = waveform_mode
+            -- If the waveform is not fast or GC16, enforce a nightmode-specific mode (usually, GC16), to limit ghosting
+            if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
+                waveform_mode = fb:_getNightWaveFormMode()
+                refarea[0].waveform_mode = waveform_mode
+            end
+        else
+            -- On Kobo, there's no shiny nightmode-specific waveform mode. Which means flashes are awful.
+            -- So, if the device can do HW inversion, fake a flash by splitting a refresh in two:
+            -- a first one, non-inverted, which will essentially flash to white,
+            -- and then, the actual inverted one, which we mùake non-flashing.
+            if fb.device:canHWInvert() then
+                if refresh_type == C.UPDATE_MODE_FULL then
+                    -- So, first one, uninverted, non-flashing
+                    refarea[0].update_mode = C.UPDATE_MODE_PARTIAL
+                    C.ioctl(fb.fd, update_ioctl, refarea)
+                end
+
+                -- Then, the real night-modded one, still partial
+                refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
+                if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
+                    waveform_mode = fb:_getNightWaveFormMode()
+                    refarea[0].waveform_mode = waveform_mode
+                end
+            else
+                if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
+                    waveform_mode = fb:_getNightWaveFormMode()
+                    refarea[0].waveform_mode = waveform_mode
+                end
+            end
         end
     end

It's.... different. You don't get a weird double white-flash, but you get to see the page uninverted on screen for a noticeable delay.
Obviously tanks the interactivity a bit, because flashing updates are split in two non-flashing ones.

Ultimately... not a fan. (Of either behavior, really, but, eh xD).

[NiLuJe]

On the upside: ghosting is bearable on a Mk.7, while it's fairly ridiculous on my H2O ;).

[NiLuJe]

Okay, done testing on my end.

Stuff todo before it's ready for merge:

• PW4/KOA2 test
  * Kill debug prints

[NiLuJe]

Another attempt at playing with those crappy flashes... this time by enforcing a specific waveform mode in the first univerted one.

diff --git a/ffi/framebuffer_mxcfb.lua b/ffi/framebuffer_mxcfb.lua
index 6e2897a..4c1018e 100644
--- a/ffi/framebuffer_mxcfb.lua
+++ b/ffi/framebuffer_mxcfb.lua
@@ -249,15 +249,47 @@ local function mxc_update(fb, update_ioctl, refarea, refresh_type, waveform_mode
 
     -- Handle night mode shenanigans
     if fb.night_mode then
-        -- We're in nightmode! If the device can do HW inversion safely, do that!
-        if fb.device:canHWInvert() then
-            refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
-        end
+        if fb.device:isKindle() then
+            -- We're in nightmode! If the device can do HW inversion safely, do that!
+            if fb.device:canHWInvert() then
+                refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
+            end
 
-        -- If the waveform is not fast or GC16, enforce a nightmode-specific mode (usually, GC16), to limit ghosting
-        if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
-            waveform_mode = fb:_getNightWaveFormMode()
-            refarea[0].waveform_mode = waveform_mode
+            -- If the waveform is not fast or GC16, enforce a nightmode-specific mode to limit ghosting & flashing
+            if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
+                waveform_mode = fb:_getNightWaveFormMode()
+                refarea[0].waveform_mode = waveform_mode
+            end
+        else
+            -- On Kobo, there's no shiny nightmode-specific waveform mode. Which means flashes are awful.
+            -- So, if the device can do HW inversion, fake a flash by splitting a refresh in two:
+            -- a first one, non-inverted, REAGL, which will emulate a (much) slower flash to white,
+            -- and then, the actual inverted one, which we make non-flashing.
+            if fb.device:canHWInvert() then
+                if refresh_type == C.UPDATE_MODE_FULL then
+                    -- So, first one, uninverted, non-flashing, REAGL
+                    refarea[0].update_mode = C.UPDATE_MODE_PARTIAL
+                    refarea[0].waveform_mode = C.WAVEFORM_MODE_REAGL
+                    -- NOTE: When trying this with A2, don't forget to enforce FORCE_MONOCHROME...
+                    C.ioctl(fb.fd, update_ioctl, refarea)
+                    -- Next marker, and reset to original waveform mode
+                    marker = fb:_get_next_marker()
+                    refarea[0].update_marker = marker
+                    refarea[0].waveform_mode = waveform_mode or C.WAVEFORM_MODE_GC16
+                end
+
+                -- Then, the real night-modded one, still partial
+                refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
+                if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
+                    waveform_mode = fb:_getNightWaveFormMode()
+                    refarea[0].waveform_mode = waveform_mode
+                end
+            else
+                if not fb:_isFastWaveFormMode(waveform_mode) and waveform_mode ~= C.WAVEFORM_MODE_GC16 then
+                    waveform_mode = fb:_getNightWaveFormMode()
+                    refarea[0].waveform_mode = waveform_mode
+                end
+            end
         end
     end

Here, REAGL, because it's the only one that doesn't flash the previous screen content (... most of the time).

I initially started w/ A2, because that appeared to look fine in theory on a Kindle Touch, where I was confirming that GL16_INV safely falls back to AUTO (it does, and that suffers from the same double hiccup flashes), but that turned out to look way, way worse on the Forma ;).

Anyway, that still sucks. Just learned another fun quirk, though: flashing a REAGL (in this context), flashes to pure white instead of the previous's screen content.
Which might be interesting to know...

[NiLuJe]

And another crappy test, shutting down the FL during flashes...

diff --git a/ffi/framebuffer_mxcfb.lua b/ffi/framebuffer_mxcfb.lua
index 6e2897a..b8d5271 100644
--- a/ffi/framebuffer_mxcfb.lua
+++ b/ffi/framebuffer_mxcfb.lua
@@ -249,6 +249,10 @@ local function mxc_update(fb, update_ioctl, refarea, refresh_type, waveform_mode
 
     -- Handle night mode shenanigans
     if fb.night_mode then
+        -- If it's a flashing update, turn off the light...
+        if refresh_type == C.UPDATE_MODE_FULL then
+            fb.device:getPowerDevice():toggleFrontlight()
+        end
         -- We're in nightmode! If the device can do HW inversion safely, do that!
         if fb.device:canHWInvert() then
             refarea[0].flags = bor(refarea[0].flags, C.EPDC_FLAG_ENABLE_INVERSION)
@@ -294,6 +298,11 @@ local function mxc_update(fb, update_ioctl, refarea, refresh_type, waveform_mode
         --]]
         fb.debug("refresh: wait for completion of marker", marker, "with collision_test", collision_test)
         fb.mech_wait_update_complete(fb, marker, collision_test)
+
+        if fb.night_mode then
+            -- Lights back on!
+            fb.device:getPowerDevice():toggleFrontlight()
+        end
     end
 end

Not necessarily a fan either, but, it's not completely terrible.
Ought to be a fast ramp-up/down instead of a toggle, though.

[NiLuJe]

Except we can't really do a ramp-up/down, because we can't do parallel execution in Lua.

Otherwise, something as simple as

diff --git a/frontend/device/generic/powerd.lua b/frontend/device/generic/powerd.lua
index 04c0dcad..87974b5f 100644
--- a/frontend/device/generic/powerd.lua
+++ b/frontend/device/generic/powerd.lua
@@ -47,8 +47,24 @@ function BasePowerD:setDissmisBatteryStatus(status) self.battery_warning = statu
 function BasePowerD:isChargingHW() return false end
 function BasePowerD:frontlightIntensityHW() return 0 end
 function BasePowerD:isFrontlightOnHW() return self.fl_intensity > self.fl_min end
-function BasePowerD:turnOffFrontlightHW() self:_setIntensity(self.fl_min) end
-function BasePowerD:turnOnFrontlightHW() self:_setIntensity(self.fl_intensity) end
+function BasePowerD:turnOffFrontlightHW()
+    local util = require("ffi/util")
+    for i = 1,20 do
+        self:_setIntensity(self.fl_intensity - ((self.fl_intensity / 20) * i))
+        if (i < 20) then
+            util.usleep(25)
+        end
+    end
+end
+function BasePowerD:turnOnFrontlightHW()
+    local util = require("ffi/util")
+    for i = 1,20 do
+        self:_setIntensity(self.fl_min + ((self.fl_intensity / 20) * i))
+        if (i < 20) then
+            util.usleep(25)
+        end
+    end
+end
 -- Anything needs to be done before do a real hardware suspend. Such as turn off
 -- front light.
 function BasePowerD:beforeSuspend() end

Looks fairly swanky... Except we need it to be non-blocking, which it cleary isn't and can't be ;).

[NiLuJe]

And that concludes today's experiments ;).

Also, according to the kernel headers, GCK16 ought to be the right nightmode waveform on KOA2/PW4, so this should be good to go ;).

[poire-z]

because we can't do parallel execution in Lua.

In Lua not, but in KOReader we can :)

koreader-base/ffi/util.lua

Lines 222 to 283 in ed04819

	--- Run lua code (func) in a forked subprocess
	--
	-- With with_pipe=true, sets up a pipe for communication
	-- from children towards parent.
	-- func is called with the child pid as 1st argument, and,
	-- if with_pipe: a fd for writting
	-- This function returns (to parent): the child pid, and,
	-- if with_pipe: a fd for reading what the child wrote
	function util.runInSubProcess(func, with_pipe)
	local parent_read_fd, child_write_fd
	if with_pipe then
	local pipe = ffi.new('int[2]', {-1, -1})
	if C.pipe(pipe) ~= 0 then -- failed creating pipe !
	return false
	end
	parent_read_fd, child_write_fd = pipe[0], pipe[1]
	if parent_read_fd == -1 or child_write_fd == -1 then
	return false
	end
	end
	local pid = C.fork()
	if pid == 0 then -- child process
	-- We need to wrap it with pcall: otherwise, if we were in a
	-- subroutine, the error would just abort the coroutine, bypassing
	-- our os.exit(0), and this subprocess would be a working 2nd instance
	-- of KOReader (with libraries or drivers probably getting messed up).
	local ok, err = xpcall(function()
	-- Give the child its own process group, so we can kill(-pid) it
	-- to have all its own children killed too (otherwise, parent
	-- process would kill the child, the child's children would
	-- be adopted by init, but parent would still have
	-- util.isSubProcessDone() returning false until all the child's
	-- children are done.
	C.setpgid(0, 0)
	if parent_read_fd then
	-- close our duplicate of parent fd
	C.close(parent_read_fd)
	end
	-- Just run the provided lua code object in this new process,
	-- and exit immediatly (so we do not release drivers and
	-- resources still used by parent process)
	-- We pass child pid to func, which can serve as a key
	-- to communicate with parent process.
	-- We pass child_write_fd (if with_pipe) so 'func' can write to it
	pid = C.getpid()
	func(pid, child_write_fd)
	end, debug.traceback)
	if not ok then
	print("error in subprocess:", err)
	end
	os.exit(0)
	end
	-- parent/main process
	if pid == -1 then -- on failure, fork() returns -1
	return false
	end
	if child_write_fd then
	-- close our duplicate of child fd
	C.close(child_write_fd)
	end
	return pid, parent_read_fd
	end

(whether it's worth forking for that, and if the forked subprocess can use the frontlight fd, is left as a nightly exercise for the reader :)

[NiLuJe]

Why, yes, yes it does ;p.

I don't even need to sleep, the overhead is high enough that it's not necessary.
On the other hand, it's large enough that past a 10-step ramp, it's too long for the current timing in the NM case...

[NiLuJe]

diff --git a/frontend/device/generic/powerd.lua b/frontend/device/generic/powerd.lua
index 04c0dcad..95875e19 100644
--- a/frontend/device/generic/powerd.lua
+++ b/frontend/device/generic/powerd.lua
@@ -47,8 +47,33 @@ function BasePowerD:setDissmisBatteryStatus(status) self.battery_warning = statu
 function BasePowerD:isChargingHW() return false end
 function BasePowerD:frontlightIntensityHW() return 0 end
 function BasePowerD:isFrontlightOnHW() return self.fl_intensity > self.fl_min end
-function BasePowerD:turnOffFrontlightHW() self:_setIntensity(self.fl_min) end
-function BasePowerD:turnOnFrontlightHW() self:_setIntensity(self.fl_intensity) end
+function BasePowerD:turnOffFrontlightHW()
+    local util = require("ffi/util")
+    util.runInSubProcess(function()
+        for i = 1,5 do
+            self:_setIntensity(self.fl_intensity - ((self.fl_intensity / 5) * i))
+            -- NOTE: No need to sleep, the overhead of the syscalls is good enough ;).
+            --[[
+            if (i < 20) then
+                util.usleep(1)
+            end
+            --]]
+        end
+    end, false)
+end
+function BasePowerD:turnOnFrontlightHW()
+    local util = require("ffi/util")
+    util.runInSubProcess(function()
+        for i = 1,5 do
+            self:_setIntensity(self.fl_min + ((self.fl_intensity / 5) * i))
+            --[[
+            if (i < 20) then
+                util.usleep(1)
+            end
+            --]]
+        end
+    end, false)
+end
 -- Anything needs to be done before do a real hardware suspend. Such as turn off
 -- front light.
 function BasePowerD:beforeSuspend() end

This works okay w/ the NM hack above, for instance.
That said, it looks fancy, but I'm not sure it actually helps :D.

For a general purpose toggle, I prefer a slower, smoother 20 step ramp, though.

(The timing/sleep/overhead thing might be device-specific (i.e., sysfs vs. ioctl vs. lipc)).

[poire-z]

Personally I don't mind the straight on/off.
(Btw, if you go into using util.runInSubProcess(), there's the burden of collecting the subprocess by (re)scheduling (as long as needed) a call to the next function util.isSubProcessDone(pid) to get rid of all these zombies from the darkside or the light :) - too much burden for such a feature it seems to me.)

[NiLuJe]

There are a couple ways to avoid having to reap zombies ourselves (a double fork so it gets reparented to init straight away, as init will wait on zombies, or SIGCHLD signal shenanigans).

[poire-z]

(Just for info, in case you feel like going at that: don't implement that as the default in the current util.runInSubProcess(), as the current bits of code using it want to be able to poll for the subprocess being done or not. Should probably be another function, util.runAndForget() :)

[NiLuJe]

Managed to fit it in there with an extra optional arg, without that being too convoluted (I think ;p).

[NiLuJe]

Went with the double-fork method, because I'm familmiar with it from KFMon, and because signals are hell.

[NiLuJe]

Did not appear to break the thumbnail generation code, which seems to be the main other user ;).

[poire-z]

We never get the inner pid in the main process right?
So, why opid here? Except for clarity. But it's a bit confusing (reading it for the first time) that you use opid in a non double_fork if - and that at the end, we return pid. May be use just one of opid or pid everywhere after that, and mention it has no meaning when double_fork ?

Otherwise, looks fine and not too convoluted enough :)

[NiLuJe]

Unless I'm having C hangups, I can't declare a local variable inside a branch, that'd limit it to that branch's scope. So I can't call it pid if !double_fork and opid if double_fork ;).

So I have to set opid in the function scope, which means it gets to be used even when !double_fork, in which case opid == pid ;).

[NiLuJe]

And I do have to make the distinction between inner and outer fork in that final check, it has to be the outer fork fork() failure, as the inner one is handled in the double_fork branch ;).

[NiLuJe]

Oh, wait, I get what you mean. -_-".

[poire-z]

Not really following each case you mention, because it feels I don't have to, if the following is right:

you could just use the single local variable pid defined outside the branch and forks.
Then in each branch, you're a different forked process: each forked process can use the single local pid the way it wants too: it won't be seen by the other forked processes as each one gets its own copy of the memory once it writes to: there shouldn't be any automatically shared memory between all these forked processes and the main one. So, the parent will just see the one from the first C.fork().

[NiLuJe]

Yeah, making it exceedingly clear that pid != pid depending on whether you're in the child process or the parent process would make things more convoluted, and what I went with was indeed not very clear, thanks ;).

[poire-z]

May be mention that in the double_fork case, the pid we return is of no use and not valid (as it has already been wait()ed).
Dunno if we should return pid=nil, or that first fork pid anyway (so the caller can have something to wait() on, that will return the expected value of no more there.

[NiLuJe]

Yeah, good point.

I guess nil sounds good, because I'm not quite sure what waitpid returns for an already-reaped PID (ECHILD?).

[NiLuJe]

Yep, ECHILD it is.

[NiLuJe]

Okay, just documented it, since isSubprocessDone can handle it sanely. Returning nil would potentially be less friendly ;).

[poire-z]

Did not appear to break the thumbnail generation code, which seems to be the main other user ;).

There are also a few things that use it via Trapper:dismissableRunInSubprocess(): selected text translation, and the wikipedia lookup results, including the dynamic and interruptable loading of thumbnail images (at the top right of each textbox in small lookup, and of each textbox page when full wikipedia).

[NiLuJe]

Final RFC, I'll do a final check later today before merging ;).

[poire-z]

No issue for me over a weekend with this (so, lua blitting only) and the frontend buddy PR.

[poire-z]

(Unrelated: may be we could build the C libblitbuffer.so on all platforms, and have it loaded not just simply if it is present, but based on another setting prefered_blitter set per device - for easier testing/comparing performances.)

[NiLuJe]

@poire-z : That was basically the plan after this, as nightmode was essentially the only missing feature with the C blitter on eInk ;).

(Remember that toggle in the Dev menu? ^^).