919676 - Fix multiple H/W composer prepare calls with different HWC layer lists.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Sushil]

Attachment: Fix multiple HWC prepare calls with different layer lists.patch

1. In current architecture, when composition falls back from full HWC to GPU Composition, Gecko ends-up in calling H/W Composer prepare twice with different HWC layer lists. This approach is not efficient as the required resources (like MDP pipe) for FB layer get configured in the 1st prepare call itself. Moreover, it disturbs the resource clean-up logic through-out the display HAL and driver.

2. This approach cannot support partial HWC Composition as the OVERLAY layers need to be drawn along with FB layer so HWC layer list must not change in the subsequent set call.

This change makes sure that there will be a single HWC prepare call followed by set with same HWC list, whenever composition falls back from full HWC to GPU or partial HWC Composition.

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Comment on attachment 808732 [details] [diff] [review]
Fix multiple HWC prepare calls with different layer lists.patch

(1 reviewer is usually enough. /me takes a step back)

Comment 2

[Michael Wu [:mwu]]

Dwilson, want to do the first review pass on this? I think you had some ideas about what direction this should be going.

Comment 3

[Sushil]

Attachment: Fix multiple hwc prepare calls with different layer lists.

Added a null check to previous patch.

Comment 4

[Sushil]

Diego, similar to what you had mentioned in Bug # 915729, here is overview of this patch.

Draw cycle has 2 parts:
1. TryRender()
(a). It does basic checks to make sure if H/W composition is possible in this frame. If yes, [b] otherwise [2].
(b). TryHwComposition(): Calls mHwc->prepare() to prepare HWC in HAL and check if full HWC Composition is possible.
In prepare, HAL marks GPU and OVERLAY layers and configure all the resources. If all layers have been marked as OVERLAY, call mHwc->set() to draw all layers using OVERLAY, hence draw cycle ends and GPU is not used at all. If any layer is marked for GPU, all layers are composed using GPU and [2] gets called.

2. Render(): All GPU layers have been rendered on FrameBuffer surface.
If coming from (a), call mHwc->prepare() and mHwc->set() to draw the FB layer.
If coming from (b), call mHwc->set() to draw the FB layer (along with OVERLAY layers once we will add support for partial HWC Composition in Bug # 915729).

Render() makes sure that prepare is not called twice in same draw cycle.

Comment 5

[Diego Wilson [:diego]]

Comment on attachment 808992 [details] [diff] [review]
Fix multiple hwc prepare calls with different layer lists.

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

::: gfx/gl/GLContextProviderEGL.cpp
@@ +597,3 @@
>          if (mSurface && !mPlatformContext) {
>  #ifdef MOZ_WIDGET_GONK
>              if (!mIsOffscreen)

Please enclose the contents of the this if-block in brackets

::: widget/gonk/HwcComposer2D.cpp
@@ +221,4 @@
>      // OK!  We can compose this layer with hwc.
>  
>      int current = mList ? mList->numHwLayers : 0;
> +    if (!mList || current >= (mMaxLayerCount - FB_LAYER_COUNT)) { // FB layer is last

Instead of this please do a size check/realloc just before adding the fb layer to the list.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +264,5 @@
>  }
>  
> +void
> +GonkDisplayJB::UpdateFBSurface(EGLDisplay dpy, EGLSurface sur)
> +{

Out of all this, I believe HwcComposer2D only needs to stop the boot animation and swap the buffers. I'm thinking we can:

1. Add a GonkDisplay::StopBootAnimation() and call it from HwcComposer2D::Render(). In fact this was probably a bug in ICS too.
2. Call eglSwapBuffers() either directly from HwcComposer2D::Render() or before calling render in GLContextProviderEGL.

@@ +279,5 @@
> +}
> +
> +void
> +GonkDisplayJB::SetFBReleaseFence(int fence)
> +{

fbsurface is already exposed so you can try a GetGonkDisplay()->GetFBSurface()->setReleaseFenceFd().

Comment 6

[Sushil]

Attachment: Fix multiple hwc prepare calls with different layer lists - v1.

Addressed review comments, except for calling eglSwapBuffers() from HwcComposer2D.
I think we should keep GonkDisplay (JB/ICS) as the common interface for calling eglSwapBuffers (egl related stuff), instead of adding EGL headers and linking EGL libs everywhere just to make a call. What do you say ?

Please review.

Comment 7

[Sushil]

Attachment: Fix multiple hwc prepare calls with different layer lists.

These are updates in the latest patch:
1. Addressed last review comments.
2. Added Prepare() and Commit() in HwcComposer2D for code clean-up.
3. Discussed with Diego, we are keeping UpdateFBSurface() and SetFBReleaseFd() in GonkDisplay.

Comment 8

[Diego Wilson [:diego]]

Comment on attachment 812903 [details] [diff] [review]
Fix multiple hwc prepare calls with different layer lists.

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

::: widget/gonk/HwcComposer2D.cpp
@@ +469,5 @@
> +    }
> +
> +    // Full HWC Composition
> +    Commit();
> +

Why is the below only done on full hwc composition but not on partial composition? Could it be done at the end of Commit() ?

@@ +624,5 @@
>          return false;
>      }
>  
>      if (!TryHwComposition()) {
> +        LOGE("H/W Composition failed");

Please change this to LOGD. It currently spams a lot!

Comment 9

[Diego Wilson [:diego]]

Comment on attachment 812903 [details] [diff] [review]
Fix multiple hwc prepare calls with different layer lists.

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

mwu: this has some GonkDisplay changes that could use your feedback.

Comment 10

[Sushil]

(In reply to Diego Wilson [:diego] from comment #8)
> Comment on attachment 812903 [details] [diff] [review]
> Fix multiple hwc prepare calls with different layer lists.
> 
> Review of attachment 812903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +469,5 @@
> > +    }
> > +
> > +    // Full HWC Composition
> > +    Commit();
> > +
> 
> Why is the below only done on full hwc composition but not on partial
> composition? Could it be done at the end of Commit() ?
> 

idx denotes FB layer. During full HWC Composition, HAL sets releaseFenceFd on FB layer as well, which is redundant as FB layer is not used at all in Composition. So it is better to close it in current draw cycle to avoid any fd leak.
But in partial HWC Composition, FB layer gets used in composition, so we need to set releaseFenceFd on FB surface.
Commit() is common to both full and partial HWC Composition paths that's why this cannot be done at end of Commit().

Comment 11

[Diego Wilson [:diego]]

(In reply to Sushil from comment #10)
> idx denotes FB layer. During full HWC Composition, HAL sets releaseFenceFd
> on FB layer as well, which is redundant as FB layer is not used at all in
> Composition. So it is better to close it in current draw cycle to avoid any
> fd leak.
> But in partial HWC Composition, FB layer gets used in composition, so we
> need to set releaseFenceFd on FB surface.
> Commit() is common to both full and partial HWC Composition paths that's why
> this cannot be done at end of Commit().

Makes sense. Maybe a comment can added like "FB layer was empty so releasing fence fd".

Comment 12

[Sushil]

Attachment: Fix multiple hwc prepare calls with different layer lists.

Updated patch with last comments addressed.

Comment 13

[Diego Wilson [:diego]]

Comment on attachment 813192 [details] [diff] [review]
Fix multiple hwc prepare calls with different layer lists.

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

Looks good. I tested it on an ICS and JB targets.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 813192 [details] [diff] [review]
Fix multiple hwc prepare calls with different layer lists.

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

The GonkDisplay.h changes look fine. I think we'll have to change things a bit down the line, but this will do the trick for now.

::: widget/gonk/HwcComposer2D.cpp
@@ +523,2 @@
>      const hwc_rect_t r = {0, 0, mScreenRect.width, mScreenRect.height};
> +    hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = {NULL};

nullptr or 0

@@ +548,5 @@
>  
> +bool
> +HwcComposer2D::Commit()
> +{
> +    hwc_display_contents_1_t *displays[HWC_NUM_DISPLAY_TYPES] = {NULL};

nullptr or 0

Comment 15

[Sushil]

Attachment: Bug 919676 - Fix multiple hwc prepare calls with different layer lists. r=mwu

Comments from MWu addressed. Uploading the HG friendly patch.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/e02e1b107db7

Comment 17

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/e02e1b107db7

Comment 18

[Jason Smith [:jsmith]]

This causes a boot failure in bug 924129 on trunk right now. As such, this needs to be backed out to get the phone booting again.

Ryan - Can you back this out?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Backed out and new nightlies triggered on m-c.
https://hg.mozilla.org/mozilla-central/rev/64b497e6f593

Comment 20

[Diego Wilson [:diego]]

Attachment: Patch reland

Reland patch after fixing the null deref that caused bug 924129.

Comment 21

[Peter Chang[:pchang]]

(In reply to Diego Wilson [:diego] from comment #20)
> Created attachment 814412 [details] [diff] [review]
> Patch reland
> 
> Reland patch after fixing the null deref that caused bug 924129.

I had verified this patch on my leo device which had the booting problem as the same as buri.
It could boot to homescreen.

Comment 22

[Sushil]

Diego, looks like I should bring back "mHwcPrepared" flag which is safer than setting "mList->numHwLayers = 0" at places where it could be risky, like Bug # 924129 case. Will update in next patch.

Comment 23

[Sushil]

Thanks Diego for adding null check. Patch looks good. We can discuss and add 'mHwcPrepared' flag later on.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a13d52159c51

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a13d52159c51