890541 - (jb-gonk) Call power_module_t::setInteractive() when display is enabled

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

We should mimic the Android framework and call setInteractive(true) when the display is enabled, and setInteractive(false) when not.

Ref: https://www.codeaurora.org/cgit/quic/la/platform/hardware/libhardware/tree/include/hardware/power.h?h=jb_mr1#n92

Comment 1

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: (gonk-jb) Call setInteractive() when display is enabled/disable

Comment 2

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: (gonk-jb) Call setInteractive() when display is enabled/disable

This patch loads android jb power hal . It uses ib's powerhal->setinteractive function to disable/enable Display. It performs same sequence which android does here https://github.com/android/platform_frameworks_base/blob/master/services/java/com/android/server/power/PowerManagerService.java#L2576

Comment 3

[Michael Wu [:mwu]]

Comment on attachment 771707 [details] [diff] [review]
(gonk-jb) Call setInteractive() when display is enabled/disable

Please rebase your patch against a newer gecko.

Comment 4

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I will update another patch here soon. thanks

Comment 5

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: (gonk-jb) Call setInteractive() when display is enabled/disable

new patch . Please review it.

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 772374 [details] [diff] [review]
(gonk-jb) Call setInteractive() when display is enabled/disable

Review of attachment 772374 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks - looks fine overall. There are some nits I'd like to have fixed. Also, your patch has windows line terminators - please use unix line endings.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +127,2 @@
>          autosuspend_disable();
> +        if (mPowerModule) {

mPowerModule is not optional, so don't check for it. We should crash instead if there is no power module.

@@ +142,2 @@
>          autosuspend_enable();
> +        if (mPowerModule) {

Ditto

::: widget/gonk/libdisplay/GonkDisplayJB.h
@@ +18,5 @@
>  
>  #include "GonkDisplay.h"
>  #include "FramebufferSurface.h"
>  #include "hardware/hwcomposer.h"
> +#include <hardware/power.h>

The includes in this file use "" instead of <>.

@@ +49,5 @@
>      hw_module_t const*        mModule;
>      hw_module_t const*        mFBModule;
>      hwc_composer_device_1_t*  mHwc;
>      framebuffer_device_t*     mFBDevice;
> +    power_module_t* mPowerModule;

Align the variable name with the other variables.

Comment 7

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Attachment: (gonk-jb) Call setInteractive() when display is enabled/disable

fixed all nits. Please review it.

Comment 8

[Michael Wu [:mwu]]

Comment on attachment 772793 [details] [diff] [review]
(gonk-jb) Call setInteractive() when display is enabled/disable

Review of attachment 772793 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks.

BTW whatever you're using to generate patches is still generating windows line endings.

Comment 9

[Michael Vines [:m1] [:evilmachines]]

https://hg.mozilla.org/projects/birch/rev/0a7c9fbe5c45

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a7c9fbe5c45