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

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sushilchauhan, Assigned: sushilchauhan)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Attachment #808732 - Flags: review?(pchang)
Attachment #808732 - Flags: review?(mwu)
Attachment #808732 - Flags: review?(mvines)
Attachment #808732 - Flags: review?(dwilson)
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)
Attachment #808732 - Flags: review?(mvines)
Dwilson, want to do the first review pass on this? I think you had some ideas about what direction this should be going.
Added a null check to previous patch.
Attachment #808732 - Attachment is obsolete: true
Attachment #808732 - Flags: review?(pchang)
Attachment #808732 - Flags: review?(mwu)
Attachment #808732 - Flags: review?(dwilson)
Attachment #808992 - Flags: review?(pchang)
Attachment #808992 - Flags: review?(mwu)
Attachment #808992 - Flags: review?(dwilson)
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.
Blocks: 901978
Blocks: 920654
Blocks: 915729
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().
Attachment #808992 - Flags: review?(dwilson) → review-
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.
Attachment #808992 - Attachment is obsolete: true
Attachment #808992 - Flags: review?(pchang)
Attachment #808992 - Flags: review?(mwu)
Attachment #810900 - Flags: review?(dwilson)
No longer blocks: 901978, 920654
Assignee: nobody → sushilchauhan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Attachment #810900 - Attachment is obsolete: true
Attachment #810900 - Flags: review?(dwilson)
Attachment #812903 - Flags: review?(dwilson)
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 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.
Attachment #812903 - Flags: feedback?(mwu)
(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().
(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".
Updated patch with last comments addressed.
Attachment #812903 - Attachment is obsolete: true
Attachment #812903 - Flags: review?(dwilson)
Attachment #812903 - Flags: feedback?(mwu)
Attachment #813192 - Flags: review?(dwilson)
Attachment #813192 - Flags: feedback?(mwu)
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.
Attachment #813192 - Flags: review?(dwilson)
Attachment #813192 - Flags: review+
Attachment #813192 - Flags: feedback?(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
Attachment #813192 - Flags: review+
Comments from MWu addressed. Uploading the HG friendly patch.
Attachment #813192 - Attachment is obsolete: true
Attachment #813780 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e02e1b107db7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 924129
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?
Flags: needinfo?(ryanvm)
Backed out and new nightlies triggered on m-c.
https://hg.mozilla.org/mozilla-central/rev/64b497e6f593
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
Posted patch Patch relandSplinter Review
Reland patch after fixing the null deref that caused bug 924129.
Attachment #814412 - Flags: review+
Attachment #813780 - Attachment is obsolete: true
(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.
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.
Thanks Diego for adding null check. Patch looks good. We can discuss and add 'mHwcPrepared' flag later on.
https://hg.mozilla.org/mozilla-central/rev/a13d52159c51
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.