Closed Bug 947805 Opened 6 years ago Closed 6 years ago

Multiple hwc prepare calls lead to deadlock in HAL.

Categories

(Core Graveyard :: Widget: Gonk, defect, P1, blocker)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
mozilla29
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sushilchauhan, Assigned: sushilchauhan)

References

Details

(Whiteboard: [CR 583576])

Attachments

(1 file, 3 obsolete files)

Multiple hwc prepare calls lead to deadlock in HAL. Plus they are in-efficient, bad for H/W programming and can have side-effects on clean-up of resources in HAL and driver. So we need to avoid multiple mHwc prepare calls from HwcComposer2D.
In any use-case, if mHwc has been already prepared, then use GPU Composition. The updated FrameBuffer surface will get displayed and draw-lock in HAL will get unlocked.
Attachment #8344475 - Flags: review?(dwilson)
blocking-b2g: --- → 1.3?
Priority: -- → P2
Blocks: 930299
Severity: normal → blocker
blocking-b2g: 1.3? → 1.3+
Priority: P2 → P1
Component: General → Widget: Gonk
Product: Firefox OS → Core
Comment on attachment 8344475 [details] [diff] [review]
If mHwc is already prepared, then use GPU Composition.

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

::: widget/gonk/HwcComposer2D.cpp
@@ +563,4 @@
>      mList->hwLayers[idx].releaseFenceFd = -1;
>      mList->hwLayers[idx].planeAlpha = 0xFF;
>  
> +    if (mHwcPrepared) LOGE("Multiple hwc prepare calls!");

Please enclose the if-block contents in {} and break in multiple lines.

@@ +636,4 @@
>  
>      MOZ_ASSERT(Initialized());
>      if (mList) {
> +        if (mHwcPrepared) {

If more than one prepare happens, wouldn't it be safer to just completely abort by clearing the potentially bad "prepare" state?

I think this |if (mHwcPrepared)| block should right before the |if (!PrepareLayerList()| block and that it should set |mHwcPrepared = false| right before returning.

::: widget/gonk/HwcComposer2D.h
@@ +84,4 @@
>      nsTArray<int>           mPrevReleaseFds;
>      int                     mPrevRetireFence;
>      nsTArray<layers::LayerComposite*> mHwcLayerMap;
> +    bool                    mHwcPrepared;

Can it be called just mPrepared?
Attachment #8344475 - Flags: review?(dwilson)
(In reply to Diego Wilson [:diego] from comment #2)
> Comment on attachment 8344475 [details] [diff] [review]
> If mHwc is already prepared, then use GPU Composition.
> 
> Review of attachment 8344475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/HwcComposer2D.cpp
> @@ +636,4 @@
> >  
> >      MOZ_ASSERT(Initialized());
> >      if (mList) {
> > +        if (mHwcPrepared) {
> 
> If more than one prepare happens, wouldn't it be safer to just completely
> abort by clearing the potentially bad "prepare" state?

Yes, the intention is to safely clear the bad prepare state in HAL. That is why, we are totally avoiding the HWC Composition path and falling back to GPU. Hence, Render() will get called once GPU rendered content is ready, we will get the updated FrameBuffer surface and it will be composed on FB layer. There will be no hwc prepare call. Only mHwc set will be called to display updated FB surface, draw-lock in HAL will be unlocked and mHwcPrepared will be reset. We cannot drop the current frame completely by having neither GPU nor HWC Composition because hwc set is the only way to clear bad prepare state in HAL.

> 
> I think this |if (mHwcPrepared)| block should right before the |if
> (!PrepareLayerList()| block 
>
No, because then we will be doing |mList->numHwLayers = 0;| before it and this will cause an invalid array index in Render(): |mList->hwLayers[mList->numHwLayers - 1].handle = fbsurface->lastHandle;|.

> and that it should set |mHwcPrepared = false| right before returning.
>
Yes, I can move |mHwcPrepared = false;| at the end of Commit() but it cannot be moved to end of TryRender().
Addressed review comments.
Attachment #8344475 - Attachment is obsolete: true
Attachment #8344782 - Flags: review?(dwilson)
In a use-case, if mHwc is already prepared, then reset it with null hwc set to unlock the draw-lock in HAL.
Attachment #8344782 - Attachment is obsolete: true
Attachment #8344782 - Flags: review?(dwilson)
Attachment #8345620 - Flags: review?(dwilson)
Comment on attachment 8345620 [details] [diff] [review]
If hwcomposer is already prepared reset with null hwc set.

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

LGTM
Attachment #8345620 - Flags: review?(dwilson) → review+
Assignee: nobody → sushilchauhan
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Uploading HG friendly version of reviewed patch.
Attachment #8345620 - Attachment is obsolete: true
Attachment #8346105 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0946bb69e851
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [CR 583576]
Flags: in-moztrap-
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.