987527 - Register Vsync monitor to HWComposer

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect, P1)

Description

[Vincent Lin[:vilin]]

A part of Project Butter. We need to register a callback function to HWComposer(display HAL) so that as vsync is received in gonk it just dispatch a runnable to main thread for notification to gecko/hal.

Comment 1

[Vincent Lin[:vilin]]

Attachment: bug-987527-fix.patch

Comment 2

[Sushil]

Vincent,

Registering callback functions and hooks for vsync in HwcComposer2D is fine. I also thought of adding it but where is it needed ? I think I am not getting the complete picture, how it is being used with your patch. For composition, h/w vsync is not needed as the retire fence of composition gets signalled at the Vsync boundary. So h/w vsync gets signalled (by driver) at the same time when Gecko receives signal on retire fence of last composition set.

Comment 3

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #2)
> Vincent,
> 
> Registering callback functions and hooks for vsync in HwcComposer2D is fine.
> I also thought of adding it but where is it needed ? I think I am not
> getting the complete picture, how it is being used with your patch. For
> composition, h/w vsync is not needed as the retire fence of composition gets
> signalled at the Vsync boundary. So h/w vsync gets signalled (by driver) at
> the same time when Gecko receives signal on retire fence of last composition
> set.

The meta bug is Bug 987532. RefreshDriver, Compositor and InputDispatcher will need it. IMHO, I think Vsync trigger time and fence signal time are not always the same(it might be platform-dependent). And what if we plan to have triple-buffering FrameBufferSurface(Bug 988160)? Then compositor won't have to wait for the release fence of front buffer(just lock the intermedia buffer). Anyway, Android SurfaceFlinger is also invalidated by Vsync.

Comment 4

[Vincent Lin[:vilin]]

Comment on attachment 8398389 [details] [diff] [review]
bug-987527-fix.patch

WIP

Comment 5

[Vincent Lin[:vilin]]

We got the E on nexus 5 on B2G while it worked well on nexus 4.

E/qdhwcomposer(183): hwc_vsync_control: vsync control failed. Dpy=0, enable=1 : Operation not supported on transport endpoint

Comment 6

[Vincent Lin[:vilin]]

It turns out eventControl() was called before registerProcs().

Comment 7

[Sushil]

(In reply to Vincent Lin[:vilin] from comment #3)
> (In reply to Sushil from comment #2)
> > Vincent,
> > 
> > Registering callback functions and hooks for vsync in HwcComposer2D is fine.
> > I also thought of adding it but where is it needed ? I think I am not
> > getting the complete picture, how it is being used with your patch. For
> > composition, h/w vsync is not needed as the retire fence of composition gets
> > signalled at the Vsync boundary. So h/w vsync gets signalled (by driver) at
> > the same time when Gecko receives signal on retire fence of last composition
> > set.
> 
> IMHO, I think Vsync trigger time and fence signal time are not
> always the same(it might be platform-dependent). 

Yes, on Video mode panel, release fences and retire fence get signaled at the same time (at vsync). But on Command mode panel, release fences get signaled before the retire fence.

> And what if we plan to have
> triple-buffering FrameBufferSurface(Bug 988160)? Then compositor won't have
> to wait for the release fence of front buffer(just lock the intermedia
> buffer). 

This should already been taken care by FrameBuffer Surface release fence when FB surface buffers are 3. 

> Anyway, Android SurfaceFlinger is also invalidated by Vsync.
The only issue with invalidation with vsync is that if frame miss a vsync signal then it has to wait until next vsync signal. BTW, your patch is not blocking composition with h/w vsync signal like SF.

Comment 8

[Vincent Lin[:vilin]]

(In reply to Sushil from comment #7)
> (In reply to Vincent Lin[:vilin] from comment #3)
> > (In reply to Sushil from comment #2)
> > > Vincent,
> > > 
> > > Registering callback functions and hooks for vsync in HwcComposer2D is fine.
> > > I also thought of adding it but where is it needed ? I think I am not
> > > getting the complete picture, how it is being used with your patch. For
> > > composition, h/w vsync is not needed as the retire fence of composition gets
> > > signalled at the Vsync boundary. So h/w vsync gets signalled (by driver) at
> > > the same time when Gecko receives signal on retire fence of last composition
> > > set.
> > 
> > IMHO, I think Vsync trigger time and fence signal time are not
> > always the same(it might be platform-dependent). 
> 
> Yes, on Video mode panel, release fences and retire fence get signaled at
> the same time (at vsync). But on Command mode panel, release fences get
> signaled before the retire fence.

BTW. By observing compositor thread with systrace.
I guess N4 is video mode and N5 is command mode, right ?
I found in N4 if compositor miss a vsync then it always has to waitForever until next vsyc.
While in N5, I found it was also blocked, but released before next vsync.

> 
> > And what if we plan to have
> > triple-buffering FrameBufferSurface(Bug 988160)? Then compositor won't have
> > to wait for the release fence of front buffer(just lock the intermedia
> > buffer). 
> 
> This should already been taken care by FrameBuffer Surface release fence
> when FB surface buffers are 3. 

yap~
I mean that.

> 
> > Anyway, Android SurfaceFlinger is also invalidated by Vsync.
> The only issue with invalidation with vsync is that if frame miss a vsync
> signal then it has to wait until next vsync signal. BTW, your patch is not
> blocking composition with h/w vsync signal like SF.

You're right.
1. triple-buffering can recover one single overtime.
2. Did you say WIP on 987523 ? I would like to block and I've ever tried this way, but we can't block composite thread because it will also block the IPC message loop in our current implementation. Then main thread will also blocked.

Comment 9

[Vincent Lin[:vilin]]

Attachment: bug-987527-fix.patch

WIP

Comment 10

[Vincent Lin[:vilin]]

Attachment: bug-987527-fix.patch

WIP.
Main thread is kind of busy so makes direct notification to hal in Vsync thread.

Comment 11

[u459114]

Hi Vincent,
Do you think HwcComposer2D.cpp is a right place to support SW vsync? Or, we should construct SW sync in Hal.cpp?

Comment 12

[u459114]

compile error
./mozilla-central/widget/gonk/HwcComposer2D.cpp:214:40: error: 'ATRACE_INT' was not declared in this scope

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to C.J. Ku[:cjku] from comment #11)
> Hi Vincent,
> Do you think HwcComposer2D.cpp is a right place to support SW vsync? Or, we
> should construct SW sync in Hal.cpp?

Should we need to consider the desktop browser case? Desktop build would not include the HwcComposer2D.cpp.

Comment 14

[Vincent Lin[:vilin]]

(In reply to Jerry Shih[:jerry] from comment #13)
> (In reply to C.J. Ku[:cjku] from comment #11)
> > Hi Vincent,
> > Do you think HwcComposer2D.cpp is a right place to support SW vsync? Or, we
> > should construct SW sync in Hal.cpp?
> 
> Should we need to consider the desktop browser case? Desktop build would not
> include the HwcComposer2D.cpp.

Since we focus on the touch events, only b2g will enable this feature.

(In reply to C.J. Ku[:cjku] from comment #11)
> Hi Vincent,
> Do you think HwcComposer2D.cpp is a right place to support SW vsync? Or, we
> should construct SW sync in Hal.cpp?

In the upcoming WIP, it has a SW vsync in HwcComposer2D.cpp(gonk). I suggest that only b2g supports "Silk", so it's ok to put SW vsync in gonk.

Refresh and Composite are in gecko, but input is in gonk. I mean "input" is a platform dependent module. Before attempting to apply "Silk" to other platforms, we have to know the details about how the input system works first. Future works ?

I will also fix the build error in the next WIP.

Comment 15

[Vincent Lin[:vilin]]

Attachment: bug-987527-fix.patch

Comment 16

[Vincent Lin[:vilin]]

Attachment: bug-987527-fix.patch

Comment 17

[Sushil]

Vlin,

Let's open a new bug to just add the hwc hooks in HwcComposer2D first. Then, we can implement them as needed. For ex: I am planning to implement invalidate hook in Bugzilla: 1005322.

Comment 18

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: hwc_vsync.patch

enable hw vsync event in hwc

Comment 19

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Enable vsync in hwc

Bug 987532 project silk need a hw vsync event.
These patch enable vsync in hwc for project silk infrastructure.

--
Hi Tapas,
Should we care about the sequence of these code? I think we should set inactive before enable auto suspend.

  autosuspend_enable();
  mPowerModule->setInteractive(mPowerModule, false);

Comment 20

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: Enable vsync in hwc. v2

fix indentation

Comment 21

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Still have some indentation problem.
Will fix after review.

Comment 22

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8450166 [details] [diff] [review]
Enable vsync in hwc. v2

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

Just a minor bug while testing. Thanks for doing this Jerry!

::: widget/gonk/HwcComposer2D.cpp
@@ +129,5 @@
> +            }
> +            mHwc->registerProcs(mHwc, &mHWCProcs);
> +            mHasHWVsync = true;
> +
> +            EnableVsync(false);

Just tested, this line should be true if we have vsync enabled. Thanks!

Comment 23

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Mason Chang [:mchang] from comment #22)
> > +            mHwc->registerProcs(mHwc, &mHWCProcs);
> > +            mHasHWVsync = true;
> > +
> > +            EnableVsync(false);
> 
> Just tested, this line should be true if we have vsync enabled. Thanks!

No, we disable the vsync event by default.
We will call EnableVsync(true) to enable vsync in the "VsyncDispatcher" module.

Comment 24

[Jerry Shih[:jerry] (UTC+8) (inactive)]

After this bug, I will create another patch to create a "vsync" thread, and enable hwc vsync event.

Comment 25

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

Comment on attachment 8450166 [details] [diff] [review]
Enable vsync in hwc. v2

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

changing reviewer

Comment 26

[Sushil]

Comment on attachment 8450166 [details] [diff] [review]
Enable vsync in hwc. v2

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

::: widget/gonk/HwcComposer2D.cpp
@@ +158,5 @@
>  }
>  
> +#if ANDROID_VERSION >= 17
> +void
> +HwcComposer2D::EnableVsync(bool aEnable)

Where are you calling EnableVsync(true)? Please share the other patch which calls it.

@@ +195,5 @@
> +void
> +HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> +{
> +    //process vsync here
> +    printf_stderr("vsync event timestamp:%lld", aTimestamp);

I hope you will remove this printf.

Comment 27

[Michael Wu [:mwu]]

Comment on attachment 8450166 [details] [diff] [review]
Enable vsync in hwc. v2

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

::: widget/gonk/HwcComposer2D.cpp
@@ +118,5 @@
>          mRBSwapSupport = false;
>      }
> +
> +    if (Preferences::GetBool("gfx.hw-vsync", false)) {
> +        if (mHwc->registerProcs) {

combine the if checks.

::: widget/gonk/HwcComposer2D.h
@@ +102,5 @@
>      void setCrop(HwcLayer* layer, hwc_rect_t srcCrop);
>      void setHwcGeometry(bool aGeometryChanged);
>  
> +#if ANDROID_VERSION >= 17
> +    //hwc event callback

nit: space after //

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +166,3 @@
>          mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, !enabled);
> +    }
> +    else if (mFBDevice->enableScreen) {

else on the same line as }, if you want to do this.

Comment 28

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Sushil from comment #26)
> Comment on attachment 8450166 [details] [diff] [review]
> Enable vsync in hwc. v2
> 
> Review of attachment 8450166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +158,5 @@
> >  }
> >  
> > +#if ANDROID_VERSION >= 17
> > +void
> > +HwcComposer2D::EnableVsync(bool aEnable)
> 
> Where are you calling EnableVsync(true)? Please share the other patch which
> calls it.
> 

It will be called at Bug 987529. I will update that bug using this patch.
Thanks!

Comment 29

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v3)

In order to enable/disable the vsync from mHwc, we should make sure
the order of each command is correct. In this patch, we also add
a callback function for enabling/disabling vsync in GonkDisplay to
handle vsync events. We will finish the the TODO part in other Vsync patches.

Comment 30

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v4)

In order to enable/disable the vsync from mHwc, we should make sure
the order of each command is correct. In this patch, we also add
a callback function for enabling/disabling vsync in GonkDisplay to
handle vsync events.

We will finish the the TODO part in other Vsync patches.

Comment 31

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer

In order to enable/disable the vsync from mHwc, we should make sure
the order of each command is correct. In this patch, we also add
a callback function for enabling/disabling vsync in GonkDisplay to
handle vsync events.

We will implement the TODO part (vsync related code) in Bug 987529.

Comment 32

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8462351 [details] [diff] [review]
Register Vsync monitor to HWComposer

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +168,5 @@
> +        /**
> +         * Only mHwc support Vsync callback, so we only handle vsync here.
> +         * Note: The order is important. we should make sure each
> +         *       enable/Disable can work properly.
> +         */

if(enable){
  if(mHwc){
    blank(false)
  }
  if(mEnabledCallback){
    mEnabledCallback(true)
  }
}else{
  if(mEnabledCallback){
    mEnabledCallback(false)
  }
  if(mHwc){
    blank(true)
  }
}

---
Maybe the vsync event implementation is not in mHwc, so let it out of mHwc scope.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

if (enabled) {
autosuspend_disable();
mPowerModule->setInteractive(mPowerModule, true);
}
....
if (!enabled) {
autosuspend_enable();
mPowerModule->setInteractive(mPowerModule, false);
}

--
Should we need to take care of the calling sequence of autosuspend and the powerModule::setInteractive() for enable/disable? Does it need a reverse sequence between enable and disable?

Comment 34

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v6)

In order to enable/disable the vsync from mHwc, we should make sure
the order of each command is correct. In this patch, we also add
a callback function for enabling/disabling vsync in GonkDisplay to
handle vsync events.

Refactor SetEnabled() in GonkDisplay.cpp.

Comment 35

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #34)
> Created attachment 8463203 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v6)
> 
> In order to enable/disable the vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, we also add
> a callback function for enabling/disabling vsync in GonkDisplay to
> handle vsync events.
> 
> Refactor SetEnabled() in GonkDisplay.cpp.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=9de067267093

Comment 36

[Sushil]

I will review it tomorrow. Sorry, I was busy in something else so could not review it today.

Comment 37

[Michael Wu [:mwu]]

Comment on attachment 8463203 [details] [diff] [review]
Register Vsync monitor to HWComposer (v6)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +32,5 @@
>  #include "gfx2DGlue.h"
>  
>  #if ANDROID_VERSION >= 17
>  #include "libdisplay/FramebufferSurface.h"
> +#include "libdisplay/GonkDisplayJB.h"

GonkDisplay.h is the only include that you can use.

::: widget/gonk/libdisplay/GonkDisplay.h
@@ +44,3 @@
>      virtual void OnEnabled(OnEnabledCallbackType callback) = 0;
>  
> +    virtual void OnVsyncEnabled(OnVsyncEnabledCallbackType callback) = 0;

AFAICT there's no reason to add a new hook here. Use the current callback in nsWindow.cpp (displayEnabledCallback) to do what you need.

Comment 38

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #37)
> Comment on attachment 8463203 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v6)
> 
> Review of attachment 8463203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +32,5 @@
> >  #include "gfx2DGlue.h"
> >  
> >  #if ANDROID_VERSION >= 17
> >  #include "libdisplay/FramebufferSurface.h"
> > +#include "libdisplay/GonkDisplayJB.h"
> 
> GonkDisplay.h is the only include that you can use.
OK.
> 
> ::: widget/gonk/libdisplay/GonkDisplay.h
> @@ +44,3 @@
> >      virtual void OnEnabled(OnEnabledCallbackType callback) = 0;
> >  
> > +    virtual void OnVsyncEnabled(OnVsyncEnabledCallbackType callback) = 0;
> 
> AFAICT there's no reason to add a new hook here. Use the current callback in
> nsWindow.cpp (displayEnabledCallback) to do what you need.

Hi, Michael,

Thanks for your review.
In GonkDisplayJB::SetEnabled(bool enabled), we should make sure the execution order is correct:
ex.
if(enabled) {
  mHwc->blank(false)
  enable Vsync
} else {
  disable Vsync
  mHwc->blank(true)
}
If the execution order is not correct (ex. enable/disable Vsync always after mHwc->blank()), the HW Vsync may not work.
Actually, I'm not sure whether displayEnabledCallback() should always be called after mHwc->blank() or not (according to the original design), so I added a new callback function for Vsync to make sure the order is correct. Could you please provide me more information about this? If displayEnabledCallback() could be called before mHwc->blank(), I think I can use the current callback function to do what I want. Thanks.

Comment 39

[Michael Wu [:mwu]]

(In reply to Boris Chiou [:boris] from comment #38)
> If the execution order is not correct (ex. enable/disable Vsync always after
> mHwc->blank()), the HW Vsync may not work.
> Actually, I'm not sure whether displayEnabledCallback() should always be
> called after mHwc->blank() or not (according to the original design), so I
> added a new callback function for Vsync to make sure the order is correct.
> Could you please provide me more information about this? If
> displayEnabledCallback() could be called before mHwc->blank(), I think I can
> use the current callback function to do what I want. Thanks.

I think you can move the callback where ever you need it. It posts a runnable to the main thread, so it shouldn't be sensitive to the location where it is called.

Comment 40

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #39)
> (In reply to Boris Chiou [:boris] from comment #38)
> > If the execution order is not correct (ex. enable/disable Vsync always after
> > mHwc->blank()), the HW Vsync may not work.
> > Actually, I'm not sure whether displayEnabledCallback() should always be
> > called after mHwc->blank() or not (according to the original design), so I
> > added a new callback function for Vsync to make sure the order is correct.
> > Could you please provide me more information about this? If
> > displayEnabledCallback() could be called before mHwc->blank(), I think I can
> > use the current callback function to do what I want. Thanks.
> 
> I think you can move the callback where ever you need it. It posts a
> runnable to the main thread, so it shouldn't be sensitive to the location
> where it is called.

Great. Thanks for your clarification. Let me revise it in next patch.

Comment 41

[Sushil]

Comment on attachment 8463203 [details] [diff] [review]
Register Vsync monitor to HWComposer (v6)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +205,5 @@
> +
> +void
> +HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> +{
> +    // TODO: Handle Vsync event here

Are you planning to block composition, until H/W vsync received? I think I should take a look at meta bug, to get bigger picture.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +185,4 @@
>  
> +        // Enable Vsync
> +        if (mVsyncEnabledCallback) {
> +            mVsyncEnabledCallback(true);

Will it call "HwcComposer2D::EnableVsync(true)" ? I am not getting why this callback is needed? How about calling mHwc->eventControl() here ?

@@ +189,5 @@
> +        }
> +    } else {
> +        // Disable Vsync, first
> +        if (mVsyncEnabledCallback) {
> +            mVsyncEnabledCallback(false);

Same as above comment.

Comment 42

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v7)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct.

Remove redundant callback functions.

Comment 43

[Boris Chiou [:boris]]

Comment on attachment 8464637 [details] [diff] [review]
Register Vsync monitor to HWComposer (v7)

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

::: gfx/layers/opengl/Composer2D.h
@@ +36,5 @@
>  
>  class Layer;
>  
>  class Composer2D {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Composer2D)

Change to threadsafe recounting so that I can dispatch it to main thread.

::: widget/gonk/HwcComposer2D.cpp
@@ +144,5 @@
>  }
>  
> +#if ANDROID_VERSION >= 17
> +bool
> +HwcComposer2D::InitHWVsync()

Add this InitHWVsync() API for Vsync Dispatcher. Vsync Dispatcher will decide whether we should initialize HW Vsync or not,

@@ +173,5 @@
> +HwcComposer2D::EnableVsync(bool aEnable)
> +{
> +    nsCOMPtr<nsIRunnable> event =
> +        NS_NewRunnableMethodWithArg<bool>(this, &HwcComposer2D::RunVsyncEventControl, aEnable);
> +    NS_DispatchToMainThread(event);

Actually, I should make sure that only main thread can enable/disable the HW Vsync.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +167,5 @@
>  
> +        if (mHwc) {
> +            // Disable blank, and then enable Vsync
> +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);
> +            mHwc->eventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, true);

I just added mHwc->eventControl() here. If we do not register this Vsync callback function, this mHwc->eventControl will have no effect.

Besides, I can not use HwcComposer2D in GonkDisplayJB.cpp because GonkDisplay is an independent module and it should not see HwcComposer2D.

Comment 44

[Boris Chiou [:boris]]

(In reply to Sushil from comment #41)
> Comment on attachment 8463203 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v6)
> 
> Review of attachment 8463203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +205,5 @@
> > +
> > +void
> > +HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> > +{
> > +    // TODO: Handle Vsync event here
> 
> Are you planning to block composition, until H/W vsync received? I think I
> should take a look at meta bug, to get bigger picture.

Thanks for your review, Sushil. Yes, we want to follow the 60Hz HW Vsync event. Actually, Jerry will implement this part in another, bug 987527. This function will notify Vsync Dispatcher and tell it the time stamp, so the composition can follow this HW Vsync event.
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +185,4 @@
> >  
> > +        // Enable Vsync
> > +        if (mVsyncEnabledCallback) {
> > +            mVsyncEnabledCallback(true);
> 
> Will it call "HwcComposer2D::EnableVsync(true)" ? I am not getting why this
> callback is needed? How about calling mHwc->eventControl() here ?
OK, in my new patch, I just call mHwc->eventControl() directly.

> 
> @@ +189,5 @@
> > +        }
> > +    } else {
> > +        // Disable Vsync, first
> > +        if (mVsyncEnabledCallback) {
> > +            mVsyncEnabledCallback(false);
> 
> Same as above comment.

Comment 45

[Boris Chiou [:boris]]

(In reply to Sushil from comment #41)
> Comment on attachment 8463203 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v6)
> 
> Review of attachment 8463203 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +205,5 @@
> > +
> > +void
> > +HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> > +{
> > +    // TODO: Handle Vsync event here
> 
> Are you planning to block composition, until H/W vsync received? I think I
> should take a look at meta bug, to get bigger picture.
> 

Thanks for your review, Sushil. Yes, we want to follow the 60Hz HW Vsync. Actually, Jerry will implement this part in the meta bug, Bug 987527. This function will notify Vsync Dispatcher and tell it the time stamp, so the compositor can follow this HW Vsync event.

> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +185,4 @@
> >  
> > +        // Enable Vsync
> > +        if (mVsyncEnabledCallback) {
> > +            mVsyncEnabledCallback(true);
> 
> Will it call "HwcComposer2D::EnableVsync(true)" ? I am not getting why this
> callback is needed? How about calling mHwc->eventControl() here ?

OK, in my new patch, I just call mHwc->eventControl() directly.

> 
> @@ +189,5 @@
> > +        }
> > +    } else {
> > +        // Disable Vsync, first
> > +        if (mVsyncEnabledCallback) {
> > +            mVsyncEnabledCallback(false);
> 
> Same as above comment.

Comment 46

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #39)
> (In reply to Boris Chiou [:boris] from comment #38)
> > If the execution order is not correct (ex. enable/disable Vsync always after
> > mHwc->blank()), the HW Vsync may not work.
> > Actually, I'm not sure whether displayEnabledCallback() should always be
> > called after mHwc->blank() or not (according to the original design), so I
> > added a new callback function for Vsync to make sure the order is correct.
> > Could you please provide me more information about this? If
> > displayEnabledCallback() could be called before mHwc->blank(), I think I can
> > use the current callback function to do what I want. Thanks.
> 
> I think you can move the callback where ever you need it. It posts a
> runnable to the main thread, so it shouldn't be sensitive to the location
> where it is called.

In my new patch, I removed the callback function and tried to call mHwc->eventControl() directly. Therefore, I don't need to put my code into this display callback function in nsWindow.cpp. 

By the way, unfortunately, I can not call HwcComposer2D::getInstance() in this display callback function because it might cause some problems. After powering up the device, GonkDisplayJB::SetEnabled(true) will be called immediately, and HwcComposer2D is not initialized yet. I think it is better to use HwcComposer2D after it has been initialized, so I decided to call mHwc->eventControl() in GonkDisplayJB::SetEnabled() directly.

Comment 47

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct.

Remove redundant include

Comment 48

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

transferring it sushil who has better idea about MDP than me

Comment 49

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v9)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Comment 50

[Boris Chiou [:boris]]

Comment on attachment 8467686 [details] [diff] [review]
Register Vsync monitor to HWComposer (v9)

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

::: gfx/layers/opengl/Composer2D.h
@@ +36,5 @@
>  
>  class Layer;
>  
>  class Composer2D {
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Composer2D)

Change to threadsafe recounting so that I can dispatch it to main thread.

::: widget/gonk/HwcComposer2D.cpp
@@ +145,5 @@
>  }
>  
> +#if ANDROID_VERSION >= 17
> +bool
> +HwcComposer2D::RegisterHwcEventCallback()

We will call this function in GonkVsyncDispatcher::StartUpVsyncEvent() (Bug 1048667).

@@ +179,5 @@
> +    return mHasHWVsync;
> +}
> +
> +void
> +HwcComposer2D::EnableVsync(bool aEnable)

We will call this function in GonkVsyncDispatcher::EnableVsyncEvent(bool aEnable). (Bug 1048667)

@@ +219,5 @@
> +
> +void
> +HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> +{
> +    // TODO: Handle Vsync event here

Ex. We will call VsyncDispatcher::NotifyVsync(int64_t aTimestamp) here. (Bug 1048667)

Comment 51

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #50)
> Comment on attachment 8467686 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v9)
> 
> Review of attachment 8467686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/opengl/Composer2D.h
> @@ +36,5 @@
> >  
> >  class Layer;
> >  
> >  class Composer2D {
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(Composer2D)
> 
> Change to threadsafe recounting so that I can dispatch it to main thread.
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +145,5 @@
> >  }
> >  
> > +#if ANDROID_VERSION >= 17
> > +bool
> > +HwcComposer2D::RegisterHwcEventCallback()
> 
> We will call this function in GonkVsyncDispatcher::StartUpVsyncEvent() (Bug
> 1048667).
> 
> @@ +179,5 @@
> > +    return mHasHWVsync;
> > +}
> > +
> > +void
> > +HwcComposer2D::EnableVsync(bool aEnable)
> 
> We will call this function in GonkVsyncDispatcher::EnableVsyncEvent(bool
> aEnable). (Bug 1048667)
> 
> @@ +219,5 @@
> > +
> > +void
> > +HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)
> > +{
> > +    // TODO: Handle Vsync event here
> 
> Ex. We will call VsyncDispatcher::NotifyVsync(int64_t aTimestamp) here. (Bug
> 1048667)

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=2c9ed75ae2dd

Comment 52

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #49)
> Created attachment 8467686 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v9)
> 
> In order to enable/disable the Vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, I added an
> interface for registering these mHwc callback functions and some
> accessor/helper functions for manipulating Hwc Vsync.

Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2c9ed75ae2dd

Comment 53

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #52)
> (In reply to Boris Chiou [:boris] from comment #49)
> > Created attachment 8467686 [details] [diff] [review]
> > Register Vsync monitor to HWComposer (v9)
> > 
> > In order to enable/disable the Vsync from mHwc, we should make sure
> > the order of each command is correct. In this patch, I added an
> > interface for registering these mHwc callback functions and some
> > accessor/helper functions for manipulating Hwc Vsync.
> 
> Treeherder:
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2c9ed75ae2dd

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=2c9ed75ae2dd

Comment 54

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Sushil and Michael,

Please check. This is our proposed project silk diagram.
https://wiki.mozilla.org/Project_Silk

We want to do refresh driver tick and compose aligned with vsync. We need a precise 60fps vsync event, so we enable the hw vsync event in hwc here.

--
We use hwc vsync event at Bug 1048667 attachment 8467741 [details] [diff] [review].
VsyncDispatcher will register the hwc event at:
https://bugzilla.mozilla.org/attachment.cgi?id=8467741&action=diff#a/widget/gonk/GonkVsyncDispatcher.cpp_sec3

Hwc will dispatch the vsync event to VsyncDispatcher at:
https://bugzilla.mozilla.org/attachment.cgi?id=8467741&action=diff#a/widget/gonk/HwcComposer2D.cpp_sec4
please check the "HwcComposer2D::Vsync(int aDisplay, int64_t aTimestamp)" function.

VsyncDispatcher will call "HwcComposer2D::EnableVsync(bool aEnable)" to enable/disable hw vsync event.

Comment 55

[Michael Wu [:mwu]]

Comment on attachment 8467686 [details] [diff] [review]
Register Vsync monitor to HWComposer (v9)

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

Seems mostly fine, but I don't think I have enough context here to really tell. I'll need to also look at the users..

::: widget/gonk/HwcComposer2D.cpp
@@ +148,5 @@
> +bool
> +HwcComposer2D::RegisterHwcEventCallback()
> +{
> +    bool ret = false;
> +    if (gfxPrefs::SilkHWVsyncEnabled()) {

Invert this and return early.

@@ +150,5 @@
> +{
> +    bool ret = false;
> +    if (gfxPrefs::SilkHWVsyncEnabled()) {
> +        HwcDevice* device = (HwcDevice*)GetGonkDisplay()->GetHWCDevice();
> +        if (!device) {

Return early. Can also check for registerProcs here.

@@ +154,5 @@
> +        if (!device) {
> +            // Can not get device, so do nothing and return false
> +            LOGE("Failed to get hwc");
> +        } else if (device->registerProcs) {
> +            memset(&mHWCProcs, 0, sizeof(mHWCProcs));

I recommend statically initializing this table and also taking the callbacks out of the HwcComposer2D class. Putting those callbacks in HwcComposer2D doesn't give you much.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +155,5 @@
>  }
>  
>  void
>  GonkDisplayJB::SetEnabled(bool enabled)
>  {

The restructuring in this function doesn't seem to make things any better. Please minimize your changes to this function.

@@ +167,5 @@
>  
> +        if (mHwc) {
> +            // Disable blank, and then enable Vsync
> +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);
> +            mHwc->eventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, true);

Is it safe to enable vsync without vsync callbacks registered?

Comment 56

[Sushil]

Comment on attachment 8467686 [details] [diff] [review]
Register Vsync monitor to HWComposer (v9)

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +167,3 @@
>  
> +        if (mHwc) {
> +            // Disable blank, and then enable Vsync

You can check for "enabled" to decide the order of calls. It will minimize code.

@@ +167,4 @@
>  
> +        if (mHwc) {
> +            // Disable blank, and then enable Vsync
> +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);

Use "!enabled" here, instead of false.

@@ +167,5 @@
>  
> +        if (mHwc) {
> +            // Disable blank, and then enable Vsync
> +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);
> +            mHwc->eventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, true);

UDid you test it? Shouldn't enable Vsync be called only when "mHasHWVsync" is true?
Use "enabled" here, instead of true.

Comment 57

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v10)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Revise GonkDisplayJB:SetEnabled() to minimize code. Statically initialize the
hwcomposer callback table and move these callback functions out of the
HwcComposer2D class.

Comment 58

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v11)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Revise GonkDisplayJB:SetEnabled() to minimize code. Statically initialize the
hwcomposer callback table and move these callback functions out of the
HwcComposer2D class.

Comment 59

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #55)
> Comment on attachment 8467686 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v9)
> 
> Review of attachment 8467686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Seems mostly fine, but I don't think I have enough context here to really
> tell. I'll need to also look at the users..
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +148,5 @@
> > +bool
> > +HwcComposer2D::RegisterHwcEventCallback()
> > +{
> > +    bool ret = false;
> > +    if (gfxPrefs::SilkHWVsyncEnabled()) {
> 
> Invert this and return early.
OK

> 
> @@ +150,5 @@
> > +{
> > +    bool ret = false;
> > +    if (gfxPrefs::SilkHWVsyncEnabled()) {
> > +        HwcDevice* device = (HwcDevice*)GetGonkDisplay()->GetHWCDevice();
> > +        if (!device) {
> 
> Return early. Can also check for registerProcs here.
OK

> 
> @@ +154,5 @@
> > +        if (!device) {
> > +            // Can not get device, so do nothing and return false
> > +            LOGE("Failed to get hwc");
> > +        } else if (device->registerProcs) {
> > +            memset(&mHWCProcs, 0, sizeof(mHWCProcs));
> 
> I recommend statically initializing this table and also taking the callbacks
> out of the HwcComposer2D class. Putting those callbacks in HwcComposer2D
> doesn't give you much.
Of course. I can remove these the static callback function out of the class.
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +155,5 @@
> >  }
> >  
> >  void
> >  GonkDisplayJB::SetEnabled(bool enabled)
> >  {
> 
> The restructuring in this function doesn't seem to make things any better.
> Please minimize your changes to this function.
OK. Thanks.

> 
> @@ +167,5 @@
> >  
> > +        if (mHwc) {
> > +            // Disable blank, and then enable Vsync
> > +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);
> > +            mHwc->eventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, true);
> 
> Is it safe to enable vsync without vsync callbacks registered?
According to my tests on flame, it's safe without vsync callbacks registered. However, I am unsure whether it is safe on other platforms, so I can add a flag to check if vsync callback is registered.

Comment 60

[Boris Chiou [:boris]]

(In reply to Sushil from comment #56)
> Comment on attachment 8467686 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v9)
> 
> Review of attachment 8467686 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +167,3 @@
> >  
> > +        if (mHwc) {
> > +            // Disable blank, and then enable Vsync
> 
> You can check for "enabled" to decide the order of calls. It will minimize
> code.
OK, thanks for your suggestion.
> 
> @@ +167,4 @@
> >  
> > +        if (mHwc) {
> > +            // Disable blank, and then enable Vsync
> > +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);
> 
> Use "!enabled" here, instead of false.
OK.
> 
> @@ +167,5 @@
> >  
> > +        if (mHwc) {
> > +            // Disable blank, and then enable Vsync
> > +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, false);
> > +            mHwc->eventControl(mHwc, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, true);
> 
> UDid you test it? Shouldn't enable Vsync be called only when "mHasHWVsync"
> is true?
> Use "enabled" here, instead of true.
Actually, I tested this many times and it can work on flame. However, I'm not sure whether it is safe on other platforms, so added a new flag to check this in my new patch. BTW, I cannot include/use HwcComposer2D here(GonkDisplay shouldn't know HwcComposer2D), so mHasHWVsync cannot be used in GonkDisplay.

Comment 61

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #58)
> Created attachment 8468233 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v11)
> 
> In order to enable/disable the Vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, I added an
> interface for registering these mHwc callback functions and some
> accessor/helper functions for manipulating Hwc Vsync.
> 
> Revise GonkDisplayJB:SetEnabled() to minimize code. Statically initialize the
> hwcomposer callback table and move these callback functions out of the
> HwcComposer2D class.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=13217cd5f6e1
https://tbpl.mozilla.org/?tree=Try&rev=13217cd5f6e1

Comment 62

[Michael Wu [:mwu]]

Comment on attachment 8468233 [details] [diff] [review]
Register Vsync monitor to HWComposer (v11)

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

Getting close. I want to check the code that calls this and make sure that also makes sense, but this patch itself seems ok.

::: widget/gonk/HwcComposer2D.cpp
@@ +88,5 @@
> +    // no op
> +}
> +
> +// Android hwccomposer callback table
> +static hwc_procs_t sHWCProcs = {

I think you can make this const. (see my comment in RegisterHwcEventCallback)

@@ +178,5 @@
> +HwcComposer2D::RegisterHwcEventCallback()
> +{
> +    if (!gfxPrefs::SilkHWVsyncEnabled()) {
> +        return false;
> +    } else {

This is a early return, so you don't need an else and can avoid an indentation level. The same applies for the other early returns in this function.

@@ +187,5 @@
> +            return false;
> +        } else {
> +            if (device->common.version >= HWC_DEVICE_API_VERSION_1_1) {
> +                // This callback will be NULL if the h/w composer is using
> +                // HWC_DEVICE_API_VERSION_1_0.

HWC_DEVICE_API_VERSION_1_0 is explicitly not supported (see GonkDisplayJB.cpp) so you don't need to worry about this.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +166,5 @@
> +    if (mHwc) {
> +        if (enabled) {
> +            // Disable blank, and then enable Vsync
> +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, !enabled);
> +            if (mVsyncInitialized) {

I think this will be simpler if you moved the call to mEnabledCallback here and let the callback handle it, so GonkDisplay doesn't need to know what vsync is.

Comment 63

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v12)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Fix according to Michael's comments.

Comment 64

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #62)
> Comment on attachment 8468233 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v11)
> 
> Review of attachment 8468233 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Getting close. I want to check the code that calls this and make sure that
> also makes sense, but this patch itself seems ok.
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +88,5 @@
> > +    // no op
> > +}
> > +
> > +// Android hwccomposer callback table
> > +static hwc_procs_t sHWCProcs = {
> 
> I think you can make this const. (see my comment in RegisterHwcEventCallback)
OK. Thanks.
> 
> @@ +178,5 @@
> > +HwcComposer2D::RegisterHwcEventCallback()
> > +{
> > +    if (!gfxPrefs::SilkHWVsyncEnabled()) {
> > +        return false;
> > +    } else {
> 
> This is a early return, so you don't need an else and can avoid an
> indentation level. The same applies for the other early returns in this
> function.
> 
OK.
> @@ +187,5 @@
> > +            return false;
> > +        } else {
> > +            if (device->common.version >= HWC_DEVICE_API_VERSION_1_1) {
> > +                // This callback will be NULL if the h/w composer is using
> > +                // HWC_DEVICE_API_VERSION_1_0.
> 
> HWC_DEVICE_API_VERSION_1_0 is explicitly not supported (see
> GonkDisplayJB.cpp) so you don't need to worry about this.

Great!
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +166,5 @@
> > +    if (mHwc) {
> > +        if (enabled) {
> > +            // Disable blank, and then enable Vsync
> > +            mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, !enabled);
> > +            if (mVsyncInitialized) {
> 
> I think this will be simpler if you moved the call to mEnabledCallback here
> and let the callback handle it, so GonkDisplay doesn't need to know what
> vsync is.
OK, I moved the call to displayEnabledCallback() in nsWindow.cpp, so I can use mHasHWVsync to check it.

Comment 65

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #63)
> Created attachment 8468290 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v12)
> 
> In order to enable/disable the Vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, I added an
> interface for registering these mHwc callback functions and some
> accessor/helper functions for manipulating Hwc Vsync.
> 
> Fix according to Michael's comments.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1cddc6dafb37
https://tbpl.mozilla.org/?tree=Try&rev=1cddc6dafb37

Comment 66

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v13)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Fix according to Michael's comments.

Comment 67

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #66)
> Created attachment 8468339 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v13)
> 
> In order to enable/disable the Vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, I added an
> interface for registering these mHwc callback functions and some
> accessor/helper functions for manipulating Hwc Vsync.
> 
> Fix according to Michael's comments.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=59ecc6c5fc41
https://tbpl.mozilla.org/?tree=Try&rev=59ecc6c5fc41

Comment 68

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v14)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Fix some build errors in ICS.

Comment 69

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #68)
> Created attachment 8468972 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v14)
> 
> In order to enable/disable the Vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, I added an
> interface for registering these mHwc callback functions and some
> accessor/helper functions for manipulating Hwc Vsync.
> 
> Fix some build errors in ICS.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b4b333ef761
https://tbpl.mozilla.org/?tree=Try&rev=8b4b333ef761

Comment 70

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v15)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Minimize code.

try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=baf492ac441d
https://tbpl.mozilla.org/?tree=Try&rev=baf492ac441d

Comment 71

[Michael Wu [:mwu]]

Comment on attachment 8469053 [details] [diff] [review]
Register Vsync monitor to HWComposer (v15)

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

::: widget/gonk/HwcComposer2D.cpp
@@ +182,5 @@
> +void
> +HwcComposer2D::EnableVsync(bool aEnable)
> +{
> +#if ANDROID_VERSION >= 17
> +    if (NS_IsMainThread()) {

Are we really expecting this to be called from on main thread and off main thread? If so.. that might not be a great idea. But if not, just pick one.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +172,2 @@
>          mEnabledCallback(enabled);
> +    }

I think this change would be smaller if you duplicated the mEnabledCallback code instead of the blank/enableScreen code.

Comment 72

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #71)
> Comment on attachment 8469053 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v15)
> 
> Review of attachment 8469053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +182,5 @@
> > +void
> > +HwcComposer2D::EnableVsync(bool aEnable)
> > +{
> > +#if ANDROID_VERSION >= 17
> > +    if (NS_IsMainThread()) {
> 
> Are we really expecting this to be called from on main thread and off main
> thread? If so.. that might not be a great idea. But if not, just pick one.

Actually, we only expect that mHwc->eventControl() is always called by one specific thread.
GonkDisplayJB::SetEnabled() is called by main thread, and other threads may also call EnableVsync() (ex. VsyncDispatcher thread, Bug 1048667). We want to make sure mHwc->eventControl() and mHwc->blink() are called together by GonkDisplayJB::SetEnabled() and don't break the order. (ex. If we want to blink the screen, we have to call "EnableVsync(false)" first, and then "mHwc->blink(true)". There should not be any "EnableVsync(true)" between these two commands.) Therefore, as you mentioned, this function could be called from on main thread(ex. GonkDisplay) and off main thread(ex. VsyncDispatcher). It might not be a great idea, but we want to handle this case. Other people don't need to know which thread should handle EnableVsync(), so it can make our code more concise.

> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +172,2 @@
> >          mEnabledCallback(enabled);
> > +    }
> 
> I think this change would be smaller if you duplicated the mEnabledCallback
> code instead of the blank/enableScreen code.
OK, please check my new patch. Thanks for your review.

Comment 73

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v16)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Refactor GonkDisplayJB::SetEnabled()

Comment 74

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v17)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Fix the preference name according to Milan's suggestion (Bug 1048667)

Comment 75

[Michael Wu [:mwu]]

Comment on attachment 8470738 [details] [diff] [review]
Register Vsync monitor to HWComposer (v17)

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

I think everything is mostly done here. I'm not reviewing the gfxPrefs.h changes, but otherwise things make sense within this patch. However, I want to see how GeckoVsyncDispatcher fits in before stamping this.

::: widget/gonk/HwcComposer2D.cpp
@@ +86,5 @@
> +{
> +    // no op
> +}
> +
> +// Android hwccomposer callback table

Unnecessary comment.

@@ +186,5 @@
> +    if (NS_IsMainThread()) {
> +        RunVsyncEventControl(aEnable);
> +    } else {
> +        // Post Vsync event control to main thread because we need to make sure
> +        // the execution order for blank and event control.

I don't think we need a comment here.

@@ +204,5 @@
> +    }
> +
> +    HwcDevice* device = (HwcDevice*)GetGonkDisplay()->GetHWCDevice();
> +    if (!device || !device->registerProcs) {
> +        // Can not get device, so do nothing and return false

This comment doesn't help.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +170,2 @@
>          mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, !enabled);
> +    } else {

Android no longer calls enableScreen, so we can't assume enableScreen is implemented.

Comment 76

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v18)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Remove redundant comments.

try result:
https://tbpl.mozilla.org/?tree=Try&rev=165a044a6a1a

Comment 77

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #75)
> Comment on attachment 8470738 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v17)
> 
> Review of attachment 8470738 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think everything is mostly done here. I'm not reviewing the gfxPrefs.h
> changes, but otherwise things make sense within this patch. However, I want
> to see how GeckoVsyncDispatcher fits in before stamping this.
> 
OK, Thanks. I uploaded a new patch to remove these redundant comments. After checking GeckoVsyncDispatcher, could you please review my new patch again? Thank you.

> ::: widget/gonk/HwcComposer2D.cpp
> @@ +86,5 @@
> > +{
> > +    // no op
> > +}
> > +
> > +// Android hwccomposer callback table
> 
> Unnecessary comment.
OK
> 
> @@ +186,5 @@
> > +    if (NS_IsMainThread()) {
> > +        RunVsyncEventControl(aEnable);
> > +    } else {
> > +        // Post Vsync event control to main thread because we need to make sure
> > +        // the execution order for blank and event control.
> 
> I don't think we need a comment here.
OK
> 
> @@ +204,5 @@
> > +    }
> > +
> > +    HwcDevice* device = (HwcDevice*)GetGonkDisplay()->GetHWCDevice();
> > +    if (!device || !device->registerProcs) {
> > +        // Can not get device, so do nothing and return false
> 
> This comment doesn't help.
OK
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +170,2 @@
> >          mHwc->blank(mHwc, HWC_DISPLAY_PRIMARY, !enabled);
> > +    } else {
> 
> Android no longer calls enableScreen, so we can't assume enableScreen is
> implemented.

Oops, I should add back this check. Thanks.

Comment 78

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v19)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Remove a redundant interface and not to use mHwc in RunVsyncEventControl
to avoid dependance on HwcComposer2D::Init()

Comment 79

[Boris Chiou [:boris]]

(In reply to Boris Chiou [:boris] from comment #78)
> Created attachment 8472132 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v19)
> 
> In order to enable/disable the Vsync from mHwc, we should make sure
> the order of each command is correct. In this patch, I added an
> interface for registering these mHwc callback functions and some
> accessor/helper functions for manipulating Hwc Vsync.
> 
> Remove a redundant interface and not to use mHwc in RunVsyncEventControl
> to avoid dependance on HwcComposer2D::Init()

Try build:
https://tbpl.mozilla.org/?tree=Try&rev=58014c6cec6d

Comment 80

[Michael Wu [:mwu]]

Comment on attachment 8472132 [details] [diff] [review]
Register Vsync monitor to HWComposer (v19)

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

This is blocking a different bug now, so let's get it landed. Thanks!

::: gfx/thebes/gfxPrefs.h
@@ +194,5 @@
>    DECL_GFX_PREF(Once, "gfx.work-around-driver-bugs",           WorkAroundDriverBugs, bool, true);
>  
>    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
>  
> +  // Generate vsync event by hardware

Nit: Maybe word it like: Use vsync events generated by hardware

::: widget/gonk/HwcComposer2D.cpp
@@ +175,5 @@
> +void
> +HwcComposer2D::EnableVsync(bool aEnable)
> +{
> +#if ANDROID_VERSION >= 17
> +    if (NS_IsMainThread()) {

I am still skeptical about this needing to run on any thread, but we can come back to that later.

Comment 81

[Boris Chiou [:boris]]

(In reply to Michael Wu [:mwu] from comment #80)
> Comment on attachment 8472132 [details] [diff] [review]
> Register Vsync monitor to HWComposer (v19)
> 
> Review of attachment 8472132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is blocking a different bug now, so let's get it landed. Thanks!
Thank you :)
> 
> ::: gfx/thebes/gfxPrefs.h
> @@ +194,5 @@
> >    DECL_GFX_PREF(Once, "gfx.work-around-driver-bugs",           WorkAroundDriverBugs, bool, true);
> >  
> >    DECL_GFX_PREF(Live, "gfx.draw-color-bars",                   CompositorDrawColorBars, bool, false);
> >  
> > +  // Generate vsync event by hardware
> 
> Nit: Maybe word it like: Use vsync events generated by hardware
OK.
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +175,5 @@
> > +void
> > +HwcComposer2D::EnableVsync(bool aEnable)
> > +{
> > +#if ANDROID_VERSION >= 17
> > +    if (NS_IsMainThread()) {
> 
> I am still skeptical about this needing to run on any thread, but we can
> come back to that later.

We will call this function from VsyncDispather thread (in Silk project) and main thread. However, we still can discuss this later.

Comment 82

[Boris Chiou [:boris]]

Attachment: Register Vsync monitor to HWComposer (v20, carry mwu's r+)

In order to enable/disable the Vsync from mHwc, we should make sure
the order of each command is correct. In this patch, I added an
interface for registering these mHwc callback functions and some
accessor/helper functions for manipulating Hwc Vsync.

Rebased.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=5970a36f5b1a

Comment 83

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ab766a8e39c

Comment 84

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ab766a8e39c