Sotaro Ikeda [:sotaro] Assignee
	Description • 10 years ago

+++ This bug was initially created as a clone of Bug #1024025 +++

gecko does not deliver fence to HwComposer. Need to deliver it to HwComposer.

The patch will be moved from Bug 1024025.

	Sushil
	Comment 2 • 10 years ago

Does it mean, the acquire fence on Gecko layers has been enabled and tested on v1.4 as well ? Until now, we were setting hwcLayer.acquireFenceFd = -1, for each HWC layer as content was promised to be ready on layers when they come for Composition.

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

(In reply to Sushil from comment #2)
> Does it mean, the acquire fence on Gecko layers has been enabled and tested
> on v1.4 as well ? 

Suhil, what do you mean tested? Can't we set acuqire fence? After it is enabled, all WebGL content is going to use this path. It say that we are going to test on flame device.

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

In gecko, acquire fence is already enabled by Bug 1001417.

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

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> Created attachment 8438736 [details] [diff] [review]
> patch - Deliver acquire fence to HwComposer

attachment 8438736 [details] [diff] [review] changes a place of Wait fence of OpenGL. It improve also OpenGL Composition's performance.

	Sushil
	Comment 6 • 10 years ago

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> (In reply to Sushil from comment #2)
> > Does it mean, the acquire fence on Gecko layers has been enabled and tested
> > on v1.4 as well ? 
> 
> Suhil, what do you mean tested? Can't we set acuqire fence? After it is
> enabled, all WebGL content is going to use this path. It say that we are
> going to test on flame device.

I mean, have you tested this patch for any potential side-effect in use cases like Video, Camera, etc. where HWC is used (on overlay & non H/W overlay devices) ? Can I pull this patch on v1.4 and test ?

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

(In reply to Sushil from comment #6)
> (In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > (In reply to Sushil from comment #2)
> > > Does it mean, the acquire fence on Gecko layers has been enabled and tested
> > > on v1.4 as well ? 
> > 
> > Suhil, what do you mean tested? Can't we set acuqire fence? After it is
> > enabled, all WebGL content is going to use this path. It say that we are
> > going to test on flame device.
> 
> I mean, have you tested this patch for any potential side-effect in use
> cases like Video, Camera, etc. where HWC is used (on overlay & non H/W
> overlay devices) ? Can I pull this patch on v1.4 and test ?

Which effect? Camera and video does not provide Acquire fence. The patch is for master. But you can apply it to v1.4.

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

(In reply to Sotaro Ikeda [:sotaro] from comment #7)
> (In reply to Sushil from comment #6)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #3)
> > > (In reply to Sushil from comment #2)
> > > > Does it mean, the acquire fence on Gecko layers has been enabled and tested
> > > > on v1.4 as well ? 
> > > 
> > > Suhil, what do you mean tested? Can't we set acuqire fence? After it is
> > > enabled, all WebGL content is going to use this path. It say that we are
> > > going to test on flame device.
> > 
> > I mean, have you tested this patch for any potential side-effect in use
> > cases like Video, Camera, etc. where HWC is used (on overlay & non H/W
> > overlay devices) ? Can I pull this patch on v1.4 and test ?
> 
> Which effect? Camera and video does not provide Acquire fence. The patch is
> for master. But you can apply it to v1.4.

Sorry, video does not provide acquire fence. But camera could provide acquire fence. But until Bug 1001417, acquire fence is not handled at all. It seems more problematic.

	Sushil
	Comment 9 • 10 years ago

In Video playback use-case, when UI seek-bar is present, I believe we should be getting acquire fences now (with your patch). I will test it on v1.4.

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

Thanks for checking! But iirc, OMXCodec does not provide acquire fence. Who provides AcquireFence during playback?

	Sushil
	Comment 11 • 10 years ago

In Comment 9, I was pointing to acquire fences of UI layers in frame, when video controls are present.

	Sushil
	Comment 12 • 10 years ago

Comment on attachment 8438736 [details] [diff] [review]
patch - Deliver acquire fence to HwComposer

On a good v1.4 build, I pulled patch from Bug 1001417 and this patch, but now the device crashes. Please check. Reason: "Compositor: unhandled page fault".

Please list if there are any dependencies which I should make sure, I must have.

	Sushil
	Comment 13 • 10 years ago

Reason for crash. You need to skip or fix the loop in Commit(), in case of GPU Composition frame.
Number of layers = 2.
HWC layer[0] = Bogus app layer
HWC layer[1] = FB layer

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

Add more checks.

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

Fix nits.

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

Add HWC_FRAMEBUFFER check.

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

Sushi, does the patch attachment 8439368 [details] [diff] [review] work on your device? On my side, I did not saw the crash on flame and on qrd device with old patch.

	Sushil
	Comment 18 • 10 years ago

(mList->hwLayers[j].flags & HWC_SKIP_LAYER) check is not needed, HWC_FRAMEBUFFER check takes care of it.

I am little busy currently. Later today, I will test your patch on my device and let you know.

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

Apply the comment.

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

Nominate to "b2g-v1.4". This bug affect to WebGL game's performance and blocking 1.4+ bug.

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/161cad49ab77

	Erin Lancaster [:elan]
	Comment 22 • 10 years ago

triage team: see comment 20.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 23 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/161cad49ab77

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

NI :elan, if we still need given we passed partner certification?

	Preeti Raghunath(:Preeti)
	Comment 25 • 10 years ago

Sotaro,

Please provide risk analysis here. Also provide information on improvement in perf with taking this fix in 1.4.

	Preeti Raghunath(:Preeti)
	Comment 26 • 10 years ago

Moving to backlog please re-nom if necessary for partner cert.

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

(In reply to Preeti Raghunath(:Preeti) from comment #25)
> Sotaro,
> 
> Please provide risk analysis here. Also provide information on improvement
> in perf with taking this fix in 1.4.

This patch change as to deliver Fence to HwComposer. HwComposer's fence handling might cause the crash like Bug 1025824. By applying Bug 1025824 fix, the risk becomes low.

I checked performance down on master flame. At the time performance was degraded from 46 fps(OpenGL) to 33fps(HwComposer). But on latest v1.4 flame, the fps was like the following. It is good. 

- b2g-v1.4, OpenGL composition: 41fps
- b2g-v1.4, HwComposer composition: 42fps

The performance difference might comes gfx layer's difference.

	Sushil
	Comment 28 • 10 years ago

Sotaro,

Please land this patch (and Bug 1025824) on b2g v2.0 as well.

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

(In reply to Sushil from comment #28)
> Sotaro,
> 
> Please land this patch (and Bug 1025824) on b2g v2.0 as well.

Bug 1024144 is not landed on v2.0. Therefore, Bug 1025824 is not necessary.

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

(In reply to Sushil from comment #28)
> Sotaro,
> 
> Please land this patch (and Bug 1025824) on b2g v2.0 as well.

If this bug get "b2g-v2.0+", this bug's patch is automatically uplifted.

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

(In reply to Sotaro Ikeda [:sotaro] from comment #30)
> (In reply to Sushil from comment #28)
> > Sotaro,
> > 
> > Please land this patch (and Bug 1025824) on b2g v2.0 as well.
> 
> If this bug get "b2g-v2.0+", this bug's patch is automatically uplifted.

I am missing to understand the impact on 2.0 here to make this a blocker. Sushil/Sotoro can you help with here? What do the fps numbers look like currently on 2.0 and how much will the landing here improve?

	Sushil
	Comment 32 • 10 years ago

HwcComposer2D used to set acquire fence of layers as -1 which means that whenever TryRender() is called, the content on all Gecko layers is ready to be composed, i.e. the painting is complete. 

Since Bug 1001417 has landed on v2.0 so it makes sense to land this patch to make HwcComposer2D and below modules (HAL and driver) to be aware of acquire fences of each HWC layer. If we do not land this patch, we might hit issues like tearing or corruption, i.e. when driver is reading the content (since acquire fence is -1) but the acquire fence has not been signaled (layer is still being painted).

Sotaro can give details on FPS improvement.

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

(In reply to Sushil from comment #32)
> HwcComposer2D used to set acquire fence of layers as -1 which means that
> whenever TryRender() is called, the content on all Gecko layers is ready to
> be composed, i.e. the painting is complete. 
> 
> Since Bug 1001417 has landed on v2.0 so it makes sense to land this patch to
> make HwcComposer2D and below modules (HAL and driver) to be aware of acquire
> fences of each HWC layer. If we do not land this patch, we might hit issues
> like tearing or corruption, i.e. when driver is reading the content (since
> acquire fence is -1) but the acquire fence has not been signaled (layer is
> still being painted).
> 
> Sotaro can give details on FPS improvement.

Sotaro can you help here?

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

> 
> Since Bug 1001417 has landed on v2.0 so it makes sense to land this patch to
> make HwcComposer2D and below modules (HAL and driver) to be aware of acquire
> fences of each HWC layer. If we do not land this patch, we might hit issues
> like tearing or corruption, i.e. when driver is reading the content (since
> acquire fence is -1) but the acquire fence has not been signaled (layer is
> still being painted).

The tearing/corruption problem should not happen. Acquire fence wait happens at GrallocTextureSourceOGL::BindEGLImage(). Bind EGL Image happen even when on full hw composition to correctly unbind previous gralloc buffer.

https://hg.mozilla.org/releases/mozilla-aurora/file/234b0fa642ad/gfx/layers/opengl/GrallocTextureHost.cpp#l444

> Sotaro can give details on FPS improvement.

I checked the improvement on b2g-v2.0 flame, with an application that I recently always use for the performance check. It was about 1 fps improvement.
- without fix: 47fps
- with fix: 48fps

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

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