Sushil Assignee
	Description • 10 years ago

I am planning to implement "invalidate" hook for HwcComposer2D. The idea is to have mechanism to queue a Render request from HwcComposer2D on current mRoot / composition layer tree, similar to LayerManagerComposite::EndTransaction() call.

Here is reference from Android:
- https://android.googlesource.com/platform/frameworks/native/+/kitkat-dev/services/surfaceflinger/DisplayHardware/HWComposer.cpp#281
- https://android.googlesource.com/platform/frameworks/native/+/kitkat-dev/services/surfaceflinger/DisplayHardware/HWComposer.cpp#261
- https://android.googlesource.com/platform/frameworks/native/+/kitkat-dev/services/surfaceflinger/DisplayHardware/HWComposer.cpp#138

	Sushil Assignee
	Comment 1 • 10 years ago

I need your inputs on this.

	Sushil Assignee
	Comment 2 • 10 years ago

Is it ok, to get hold of Layer Manager object and just call EndTransaction() on it ?

	Matt Woodrow (:mattwoodrow)
	Comment 3 • 10 years ago

Can you please explain what problem you're trying to solve with this?

	Sushil Assignee
	Comment 4 • 10 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Can you please explain what problem you're trying to solve with this?

It is needed for an optimization where HAL initiates an invalidate once via mCBContext->procs.invalidate hook. The aim is to queue a Render request from HAL with the existing layer tree. Meanwhile, if a new update comes from framework via normal LayerManagerComposite::EndTransaction() path, then that will be preferred. I think we can start supporting normal hwc hooks like invalidate, vsync and hotplug.

	Michael Wu [:mwu]
	Comment 5 • 10 years ago

(In reply to Sushil from comment #4)
> I think we can start supporting normal hwc hooks like
> invalidate, vsync and hotplug.

Bug 987527 implements some of those hooks - maybe you can build on top of that?

	Sushil Assignee
	Comment 6 • 10 years ago

(In reply to Michael Wu [:mwu] from comment #5)
> Bug 987527 implements some of those hooks - maybe you can build on top of
> that?

Bug 987527 implements Vsync hook. I will implement "invalidate" hook in this bug. Currently, looking into Layer Manager code to find a simple way of doing it.

	Sushil Assignee
	Comment 7 • 10 years ago

Attaching the prototype patch to be discussed.

	Sushil Assignee
	Comment 8 • 10 years ago

Hi MWu, Matt and Sotaro,

I need your inputs. Patch in Comment 7 is a prototype of what I am trying to do. In static display use case, I want to trigger a composition cycle from HwcComposer2D::Invalidate(). Current patch is not correct because HwcComposer2D::Invalidate() gets called on the "invalidator" thread (run from HAL) but I want to execute this new Composition cycle (BeginTransaction & EndEmptyTransaction) on the original Compositor thread. To achieve this, I need to signal the Compositor thread from Invalidate(). Basically, we need to implement it as similar to [1]. Hence, I need your inputs / few pointers here. We need HWC invalidate() hook to support a Power optimization in HAL.

I believe [2] should be the appropriate point to trigger on Compositor thread to run a Composition cycle.
Also I noticed with current patch that the layer tree with root as "mRoot" has changed when TryRender() gets called in the HwcComposer2D::Invalidate() call. For ex: In paused Video use case, layer tree of the last static frame had Color, Thebes and Image layer. But I could see only a Color layer, via Invalidate call. So it seems the layer tree of mRoot has changed after last Composition phase. Are we destroying layer tree after each Composition Phase? If Yes, then I need to look into it because I am relying on mRoot to preserve the last layer tree since no painting has happened after last frame update from framework (as it is static use case).

[1]: https://android.googlesource.com/platform/frameworks/native/+/kitkat-dev/services/surfaceflinger/DisplayHardware/HWComposer.cpp#281

[2]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#461

	Sotaro Ikeda [:sotaro]
	Comment 9 • 10 years ago

On android, the function call sequence like the following.

->HWComposer::hook_invalidate() //hw composer hal's "invalidator" thread
 ->HWComposer::invalidate()
  ->SurfaceFlinger::repaintEverything()
     // Trigger SurfaceFlinger's composition

	Sotaro Ikeda [:sotaro]
	Comment 10 • 10 years ago

(In reply to Sushil from comment #8)
> Also I noticed with current patch that the layer tree with root as "mRoot"
> has changed when TryRender() gets called in the HwcComposer2D::Invalidate()
> call. For ex: In paused Video use case, layer tree of the last static frame
> had Color, Thebes and Image layer. But I could see only a Color layer, via
> Invalidate call. So it seems the layer tree of mRoot has changed after last
> Composition phase. Are we destroying layer tree after each Composition
> Phase?

LayerTree is not destroyed for each rendering. It is updated by requests from layout(DisplayList), though it is very dynamic compared to android.

	Sotaro Ikeda [:sotaro]
	Comment 11 • 10 years ago

LayerManager and Compositor APIs are not thread safe, they basically need to be called on compositor thread.  CompositorParent::ScheduleRenderOnCompositorThread() is a function to trigger render from other thread. It might be used for it.

http://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#435

	Sushil Assignee
	Comment 12 • 10 years ago

Thanks Sotaro.

CompositorParent::ScheduleRenderOnCompositorThread() worked for me. I will upload the patch for review, after some more testing.

	Sushil Assignee
	Comment 13 • 10 years ago

This patch implements HWC invalidate hook. Please review. HWC invalidate is required to support a Power optimization in HAL for static display use cases.

	Sushil Assignee
	Comment 14 • 10 years ago

Boris,

The patch in Comment 13 implements HWC invalidate hook but it's enabling is blocked by check at [1] which was landed in Bugzilla 987527. I believe registerProcs() is required to register all HWC hooks. It should not be blocked by Vsync check at [1]. So we should remove check at [1], and instead add the "Vsync disable check" at [2] like this: 

-    return true;
+
+    if (!gfxPrefs::FrameUniformityHWVsyncEnabled()) {
+        device->eventControl(device, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC, false);
+        mHasHWVsync = false;
+    }
+
+    return mHasHWVsync;

Since it is not related to HWC invalidate implementation. So I wanted this to land in a separate patch. Please let me know, if you want me to add this code in current patch.

[1]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#199
[2]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#214

	Michael Wu [:mwu]
	Comment 15 • 10 years ago

Hmm, the way that this patch grabs a LayerManager doesn't seem ideal. Instead, can we explicitly pass the LayerManager to HwcComposer2D in nsWindow.cpp? nsWindow.cpp would also be able to unset it when it's getting destroyed.

	Boris Chiou [:boris]
	Comment 16 • 10 years ago

(In reply to Sushil from comment #14)
> Boris,
> 
> The patch in Comment 13 implements HWC invalidate hook but it's enabling is
> blocked by check at [1] which was landed in Bugzilla 987527. I believe
> registerProcs() is required to register all HWC hooks. It should not be
> blocked by Vsync check at [1]. So we should remove check at [1], and instead
> add the "Vsync disable check" at [2] like this: 
> 
> -    return true;
> +
> +    if (!gfxPrefs::FrameUniformityHWVsyncEnabled()) {
> +        device->eventControl(device, HWC_DISPLAY_PRIMARY, HWC_EVENT_VSYNC,
> false);
> +        mHasHWVsync = false;
> +    }
> +
> +    return mHasHWVsync;
> 
> Since it is not related to HWC invalidate implementation. So I wanted this
> to land in a separate patch. Please let me know, if you want me to add this
> code in current patch.
> 

Using a separate patch is OK to me. Thanks for fixing this problem.

> [1]:
> http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> cpp#199
> [2]:
> http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> cpp#214

	Sushil Assignee
	Comment 17 • 10 years ago

(In reply to Michael Wu [:mwu] from comment #15)
> Hmm, the way that this patch grabs a LayerManager doesn't seem ideal.
> Instead, can we explicitly pass the LayerManager to HwcComposer2D in
> nsWindow.cpp? nsWindow.cpp would also be able to unset it when it's getting
> destroyed.

MWu,

Yes, I also agree because current patch grabs LayerManagerComposite* in Invalidate(), which is not ideal since Layer Manager does not change for each Root layer (layer tree). But we cannot set it from nsWindow Constructor because it is too early in pipeline. HwcComposer2D and LayerManagerComposite are not initialized by that time. Here is the flow during boot-up:

E/        (  199): GonkDisplayJB::GonkDisplayJB()
E/Gonk    (  199): nsWindow::nsWindow()
E/HWComposer(  199): HwcComposer2D::HwcComposer2D()
E/lmComposite(  199): LayerManagerComposite::LayerManagerComposite()
E/lmComposite(  199): LayerManagerComposite::Initialize()
E/glContext(  199): GLContextEGL::GLContextEGL()
E/glContext(  199): GLContextEGL::GLContextEGL ... Call mHwc->Init
E/HWComposer(  199): HwcComposer2D::Init()
E/lmComposite(  199): LayerManagerComposite::Render()

I would suggest, we can set it from LayerManagerComposite::Initialize(). OR let me know, if you were thinking of setting it from another place in nsWindow.cpp ?

	Michael Wu [:mwu]
	Comment 18 • 10 years ago

I was thinking of setting it up in GetLayerManager where a lot of the layer manager and compositing code is setup. Dunno if you can get the right things there, but it seems like it might work.

	Sotaro Ikeda [:sotaro]
	Comment 19 • 10 years ago

I feel that the function calls from HwcComposer2D::Invalidate() to CompositorParent::ScheduleRenderOnCompositorThread() is a bit long. It might becomes simpler when HwcComposer2D directly calls the ScheduleRenderOnCompositorThread().

can't we register CompositorParent to HwcComposer2D in nsWindow::GetLayerManager() after calling CreateCompositor()?

	Sushil Assignee
	Comment 20 • 10 years ago

Attaching patch as per Comment 18.

	Sushil Assignee
	Comment 21 • 10 years ago

Attaching delayed render logs with patch of Comment 20.

	Sushil Assignee
	Comment 22 • 10 years ago

(In reply to Michael Wu [:mwu] from comment #18)
> I was thinking of setting it up in GetLayerManager where a lot of the layer
> manager and compositing code is setup. Dunno if you can get the right things
> there, but it seems like it might work.

Hi MWu,

There was no issue with my last patch. But when I tried with this approach (patch in Comment 20). I noticed that there is delay of ~ 18 sec in scheduling the Render on Compositor thread. But Invalidate() call finishes instantly (it has no issues). The delay occurs only in the scheduling. Please check logs in Comment 21, let me know if I have missed something. This is during static display use case.

	Sushil Assignee
	Comment 23 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> can't we register CompositorParent to HwcComposer2D in
> nsWindow::GetLayerManager() after calling CreateCompositor()?

Sotaro,

Can you please provide more details / few pointers? I am not much familiar with CompositorParent class in Gecko. As per my testing, HwcComposer2D::Invalidate() call does not take any time to finish.

	Sushil Assignee
	Comment 24 • 10 years ago

Added more logs for Comment 22, CompositorParent::ScheduleComposition() call is getting delayed.
Here are logs:

01-01 05:45:19.389   221   678 E HWComposer: HWC: HookInvalidate
01-01 05:45:19.389   221   678 E HWComposer: HwcComposer2D::Invalidate E
01-01 05:45:19.389   221   678 E lmComposite: LayerManagerComposite::ScheduleComposition E
01-01 05:45:19.389   221   678 E lmComposite: LayerManagerComposite::ScheduleComposition X
01-01 05:45:19.389   221   678 E HWComposer: HwcComposer2D::Invalidate X
....
01-01 05:46:00.409   221  1009 E CompositorParent: CompositorParent::ScheduleComposition
01-01 05:46:00.409   221  1009 E CompositorParent: CompositorParent::CompositeToTarget
01-01 05:46:00.409   221  1009 E CompositorParent: CompositeToTarget: lm->BeginTransaction
01-01 05:46:00.409   221  1009 E CompositorParent: CompositeToTarget: lm->EndEmptyTransaction
01-01 05:46:00.409   221  1009 E lmComposite: LayerManagerComposite::Render

	Sushil Assignee
	Comment 25 • 10 years ago

I found out the cause for issue in Comment 22. In LayerManagerComposite::ScheduleComposition(), "mCompositor" is NULL but if I check it at [1] in previous/next Render() call, it is not NULL. It means we have a stale LayerManager (hence LayerManagerComposite) object in HwcComposer2D::Invalidate(). Hence, nsWindow::ScheduleRender() was never called. That next delayed Render() was a normal framework update.

MWu,
I think the previous patch (attachment 8491075 [details] [diff] [review]) was fine. We are just caching mRoot* of every frame and whenever Invalidate() happens, we grab the updated LayerManager* with "mRoot->Manager()" api. If you have any other suggestions, please let me know.

[1]: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#603

	Sotaro Ikeda [:sotaro]
	Comment 27 • 10 years ago

(In reply to Sushil from comment #23)
> (In reply to Sotaro Ikeda [:sotaro] from comment #19)
> > can't we register CompositorParent to HwcComposer2D in
> > nsWindow::GetLayerManager() after calling CreateCompositor()?
> 
> Sotaro,
> 
> Can you please provide more details / few pointers? I am not much familiar
> with CompositorParent class in Gecko. As per my testing,
> HwcComposer2D::Invalidate() call does not take any time to finish.

I think a code like attachment 8493110 [details] [diff] [review] is simpler. Can you confirm if the patch work?

	Sotaro Ikeda [:sotaro]
	Comment 28 • 10 years ago

(In reply to Sushil from comment #24)
> Added more logs for Comment 22, CompositorParent::ScheduleComposition() call
> is getting delayed.
> Here are logs:

Compositor thread might be busy for other tasks. The compositor thread does LayerTransaction, APZ, Composition.

	Sushil Assignee
	Comment 29 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> (In reply to Sushil from comment #24)
> > Added more logs for Comment 22, CompositorParent::ScheduleComposition() call
> > is getting delayed.
> > Here are logs:
> 
> Compositor thread might be busy for other tasks. The compositor thread does
> LayerTransaction, APZ, Composition.

No, I mentioned the root cause in Comment 25. So, nsWindow::ScheduleRender() was never being called with attachment 8492382 [details] [diff] [review]. The next Render() was a normal framework update (time update on Status Bar).

	Sotaro Ikeda [:sotaro]
	Comment 30 • 10 years ago

Sushi, how can I test the patches on flame device? I am not sure how to intentionally trigger HookInvalidate() from hal.

	Sushil Assignee
	Comment 31 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> 
> I think a code like attachment 8493110 [details] [diff] [review] is simpler.
> Can you confirm if the patch work?

Thanks. I will check with this patch and let you know.

	Sushil Assignee
	Comment 32 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> 
> I think a code like attachment 8493110 [details] [diff] [review] is simpler.
> Can you confirm if the patch work?

Sotaro,

Yes, the patch works fine and it is simpler than mine. Do you want to upload this patch or you want me to upload ?

Regarding Comment 30, our optimization is only for H/W Overlay devices, so HookInvalidate() will not be triggered on Flame device. To test, you will also need change in Comment 14 (to register hooks), which we are planning to land in a separate patch (bugzilla).

	Sushil Assignee
	Comment 33 • 10 years ago

Or should we include that change (Comment 14) in current patch? I don't want to create a new Bugzilla. What do you say ?

	Sotaro Ikeda [:sotaro]
	Comment 34 • 10 years ago

(In reply to Sushil from comment #33)
> Or should we include that change (Comment 14) in current patch? I don't want
> to create a new Bugzilla. What do you say ?

Can you update a new patch that have Comment 14? It makes things simpler :-)

	Sushil Assignee
	Comment 35 • 10 years ago

Sotaro, I have attached the patch for Comment 14.

Registration of HWC hooks should not be blocked by gfx preference for enabling H/W Vsync.

	Sushil Assignee
	Comment 36 • 10 years ago

I have uploaded only Comment 14 patch. Or did you want me to upload the final consolidated patch ?

	Sotaro Ikeda [:sotaro]
	Comment 37 • 10 years ago

I am going to update attachment 8493110 [details] [diff] [review]. It has some parts that seems better to be updated.

	Sotaro Ikeda [:sotaro]
	Comment 38 • 10 years ago

Fix thread safety.

	Sotaro Ikeda [:sotaro]
	Comment 39 • 10 years ago

It might be better the patches reviewed individually.

	Matt Woodrow (:mattwoodrow)
	Comment 40 • 10 years ago

This approach looks good to me.

	Michael Wu [:mwu]
	Comment 41 • 10 years ago

Comment on attachment 8493758 [details] [diff] [review]
patch - Hold CompositorParent at HwcComposer2D

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

::: widget/gonk/HwcComposer2D.h
@@ +18,5 @@
>  #define mozilla_HwcComposer2D
>  
>  #include "Composer2D.h"
>  #include "Layers.h"
> +#include "mozilla/Mutex.h" 

trailing whitespace

::: widget/gonk/nsWindow.cpp
@@ +604,5 @@
>      return nullptr;
>  }
>  
> +void
> +nsWindow::DestroyCompositor()

I think we can avoid implementing our own DestroyCompositor() entirely - DestroyCompositor is only called on the destruction of this window and if CreateCompositor fails. If CreateCompositor fails, we don't call SetCompositorParent. So we only need to SetCompositorParent(nullptr) in ~nsWindow().

	Sushil Assignee
	Comment 42 • 10 years ago

Comment on attachment 8493758 [details] [diff] [review]
patch - Hold CompositorParent at HwcComposer2D

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

::: widget/gonk/HwcComposer2D.cpp
@@ +110,5 @@
>      , mPrevRetireFence(Fence::NO_FENCE)
>      , mPrevDisplayFence(Fence::NO_FENCE)
>  #endif
>      , mPrepared(false)
>      , mHasHWVsync(false)

No need to initialize "mCompositorParent" here ?

	Sushil Assignee
	Comment 43 • 10 years ago

Raised new Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1071704 for Comment 16.

	Sotaro Ikeda [:sotaro]
	Comment 44 • 10 years ago

(In reply to Sushil from comment #42)
> Comment on attachment 8493758 [details] [diff] [review]
> patch - Hold CompositorParent at HwcComposer2D
> 
> Review of attachment 8493758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +110,5 @@
> >      , mPrevRetireFence(Fence::NO_FENCE)
> >      , mPrevDisplayFence(Fence::NO_FENCE)
> >  #endif
> >      , mPrepared(false)
> >      , mHasHWVsync(false)
> 
> No need to initialize "mCompositorParent" here ?

nsRefPtr initialize the raw pointer to nullptr;

	Sotaro Ikeda [:sotaro]
	Comment 45 • 10 years ago

(In reply to Michael Wu [:mwu] from comment #41)
> Comment on attachment 8493758 [details] [diff] [review]
> patch - Hold CompositorParent at HwcComposer2D
> 
> Review of attachment 8493758 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.h
> @@ +18,5 @@
> >  #define mozilla_HwcComposer2D
> >  
> >  #include "Composer2D.h"
> >  #include "Layers.h"
> > +#include "mozilla/Mutex.h" 
> 
> trailing whitespace
> 
> ::: widget/gonk/nsWindow.cpp
> @@ +604,5 @@
> >      return nullptr;
> >  }
> >  
> > +void
> > +nsWindow::DestroyCompositor()
> 
> I think we can avoid implementing our own DestroyCompositor() entirely -
> DestroyCompositor is only called on the destruction of this window and if
> CreateCompositor fails. If CreateCompositor fails, we don't call
> SetCompositorParent. So we only need to SetCompositorParent(nullptr) in
> ~nsWindow().

Yhea, ~nsWindow() seems enough. I added DestroyCompositor() because, b2g created 3 nsWindow instances. But they are all alive until shutdown.

	Sotaro Ikeda [:sotaro]
	Comment 46 • 10 years ago

Apply the comments.

	Michael Wu [:mwu]
	Comment 47 • 10 years ago

Comment on attachment 8493914 [details] [diff] [review]
patch - Hold CompositorParent at HwcComposer2D

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

Looks good, thanks.

	Sushil Assignee
	Comment 48 • 10 years ago

Comment on attachment 8493914 [details] [diff] [review]
patch - Hold CompositorParent at HwcComposer2D

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

LGTM.

	Sotaro Ikeda [:sotaro]
	Comment 49 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=bcc450139e96

	Sotaro Ikeda [:sotaro]
	Comment 50 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #49)
> https://tbpl.mozilla.org/?tree=Try&rev=bcc450139e96

Hmm, all builds are failed by wired errors.

	Sotaro Ikeda [:sotaro]
	Comment 51 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=4c0c57a108b7

	Sotaro Ikeda [:sotaro]
	Comment 52 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/03b1abc4f5e7

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/03b1abc4f5e7

	Sushil Assignee
	Comment 54 • 10 years ago

[Blocking Requested - why for this release]: HWC invalidate hook is required for an optimization in HAL.

	bhavana bajaj [:bajaj]
	Comment 55 • 10 years ago

(In reply to Sushil from comment #54)
> [Blocking Requested - why for this release]: HWC invalidate hook is required
> for an optimization in HAL.

Please seek approval for mozilla-aurora when ready.

	Sushil Assignee
	Comment 56 • 10 years ago

Comment on attachment 8493914 [details] [diff] [review]
patch - Hold CompositorParent at HwcComposer2D

Approval Request Comment
[Feature/regressing bug #]: Power optimization in static display use cases, which use full/partial HWC Composition path.
[User impact if declined]: No user impact but power numbers will not be optimized.
[Describe test coverage new/current, TBPL]: Basic display sanity test, including Video playback, Camera/Camcorder, UI, etc.
[Risks and why]: No
[String/UUID change made/needed]: No

	Ryan VanderMeulen [:RyanVM]
	Comment 57 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/c7a463892539