1024144 - Deliver acquire fence to HwComposer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

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

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Deliver acquire fence to HwComposer

Comment 2

[Sushil]

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.

Comment 3

[Sotaro Ikeda [:sotaro]]

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

Comment 4

[Sotaro Ikeda [:sotaro]]

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

Comment 5

[Sotaro Ikeda [:sotaro]]

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

Comment 6

[Sushil]

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

Comment 7

[Sotaro Ikeda [:sotaro]]

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

Comment 8

[Sotaro Ikeda [:sotaro]]

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

Comment 9

[Sushil]

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.

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Sushil]

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

Comment 12

[Sushil]

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.

Comment 13

[Sushil]

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

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Deliver acquire fence to HwComposer

Add more checks.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Deliver acquire fence to HwComposer

Fix nits.

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Deliver acquire fence to HwComposer

Add HWC_FRAMEBUFFER check.

Comment 17

[Sotaro Ikeda [:sotaro]]

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.

Comment 18

[Sushil]

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

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: patch v5 - Deliver acquire fence to HwComposer

Apply the comment.

Comment 20

[Sotaro Ikeda [:sotaro]]

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

Comment 21

[Sotaro Ikeda [:sotaro]]

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

Comment 22

[Erin Lancaster [:elan]]

triage team: see comment 20.

Comment 23

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

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

Comment 24

[bhavana bajaj [:bajaj]]

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

Comment 25

[Preeti Raghunath(:Preeti)]

Sotaro,

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

Comment 26

[Preeti Raghunath(:Preeti)]

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

Comment 27

[Sotaro Ikeda [:sotaro]]

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

Comment 28

[Sushil]

Sotaro,

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

Comment 29

[Sotaro Ikeda [:sotaro]]

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

Comment 30

[Sotaro Ikeda [:sotaro]]

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

Comment 31

[bhavana bajaj [:bajaj]]

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

Comment 32

[Sushil]

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.

Comment 33

[bhavana bajaj [:bajaj]]

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

Comment 34

[Sotaro Ikeda [:sotaro]]

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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