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.

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.

(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.

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

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

(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.

(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.

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

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

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

(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.

(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.

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.

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);

Still have some indentation problem. Will fix after review.

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!

(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.

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

Comment on attachment 8450166 [details] [diff] [review] Enable vsync in hwc. v2 Review of attachment 8450166 [details] [diff] [review]: ----------------------------------------------------------------- changing reviewer

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 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.

(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!

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 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.

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?

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.

(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

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

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.

(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.

(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 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 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.

(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.

(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.

transferring it sushil who has better idea about MDP than me

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 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)

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 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 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.

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.

(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.

(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 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.

(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 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.

(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 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.

(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.

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()

(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 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.

(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.

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