944069 - Remove unnecessary wait during GPU Composition.

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect, P2)

Description

[Sushil]

Attachment: Remove unnecessary wait during GPU Composition.

During GPU Composition, FB layer release fence (which is coming from driver) is being set on Frame Buffer surface, so FB surface takes care of waiting on release fence to signal. Hence there is no need to wait on corresponding retire fence in HwcComposer2D in next draw cycle.

Comment 1

[Diego Wilson [:diego]]

Comment on attachment 8339483 [details] [diff] [review]
Remove unnecessary wait during GPU Composition.

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

::: widget/gonk/HwcComposer2D.cpp
@@ -492,4 @@
>  
>      // No composition on FB layer, so closing releaseFenceFd
>      close(mList->hwLayers[idx].releaseFenceFd);
> -    mList->hwLayers[idx].releaseFenceFd = -1;

I assume the line above doesn't have to do with the retire fence? You're just sneaking the line above as a bonus, right?

@@ +577,4 @@
>          // Denotes contents on display have been replaced.
>          // For buffer-sync, framework should not over-write
>          // prev buffers until we close prev releaseFenceFds
> +        sp<Fence> fence = new Fence(mPrevRetireFence);

Does mPrevRetireFence have to be inited to -1 in the constructor?

@@ +591,2 @@
>      for (uint32_t j=0; j < (mList->numHwLayers - 1); j++) {
> +        if (mList->hwLayers[j].compositionType == HWC_OVERLAY &&

Do we need the HWC_OVERLAY check? Just checking for a valid releaseFenceFd is simpler and more correct IMO.

@@ +595,4 @@
>          }
>      }
>  
> +    if (mList->retireFenceFd >= 0) {

So this is mainly a workaround to the fact the we're not honoring releaseFenceFds, is that correct? If so, it would be helpful to reference the releaseFenceFd bug 925444 in a source comment here as a reminder to remove it.

Comment 2

[Sushil]

Comment on attachment 8339483 [details] [diff] [review]
Remove unnecessary wait during GPU Composition.

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

::: widget/gonk/HwcComposer2D.cpp
@@ -492,4 @@
>  
>      // No composition on FB layer, so closing releaseFenceFd
>      close(mList->hwLayers[idx].releaseFenceFd);
> -    mList->hwLayers[idx].releaseFenceFd = -1;

Yes, it was unnecessary since we always reset releaseFenceFd of an HWC layer in PrepareLayerList() and FB layer in Prepare() in each draw cycle.

@@ +577,4 @@
>          // Denotes contents on display have been replaced.
>          // For buffer-sync, framework should not over-write
>          // prev buffers until we close prev releaseFenceFds
> +        sp<Fence> fence = new Fence(mPrevRetireFence);

Yes.

@@ +591,2 @@
>      for (uint32_t j=0; j < (mList->numHwLayers - 1); j++) {
> +        if (mList->hwLayers[j].compositionType == HWC_OVERLAY &&

We are interested in storing releaseFenceFds of OVERLAY layers only during full and partial HWC Composition, that is why HWC_OVERLAY check. But your comment is also valid, let me check.

@@ +595,4 @@
>          }
>      }
>  
> +    if (mList->retireFenceFd >= 0) {

Didn't get this. So do I need to add a comment here to remove it when Bug # 925444 is fixed ?

Comment 3

[Diego Wilson [:diego]]

(In reply to Sushil from comment #2)
> Didn't get this. So do I need to add a comment here to remove it when Bug #
> 925444 is fixed ?

If this is indeed a workaround then add something like

    //Workaround missing releaseFenceFd waits in gecko layers 
    //See Bug 925444

Comment 4

[Sushil]

Attachment: Remove unnecessary wait during GPU Composition.

Addressed review comments.

Comment 5

[Diego Wilson [:diego]]

Comment on attachment 8339599 [details] [diff] [review]
Remove unnecessary wait during GPU Composition.

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

Sanity tested on JB and ICS.

Comment 6

[Sushil]

Will upload HG friendly patch.

Comment 7

[Sushil]

Attachment: Bug 944069 - Remove unnecessary wait during GPU Composition. r=dwilson

Uploading HG friendly patch.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Sushil and Diego,

In comment 0:
"Hence there is no need to wait on corresponding retire fence in HwcComposer2D in next draw cycle"

https://bugzilla.mozilla.org/attachment.cgi?id=8339654&action=diff#a/widget/gonk/HwcComposer2D.cpp_sec4 ,

We wait for a fence mPrevReleaseFds[0] or the retire fence mPrevRetireFence.
I think the wait time of mPrevRetireFence is bigger than just one layer fence mPrevReleaseFds[0].
This patch waits the retire fence. I'm confused that how we save the wait time.
Thanks!

Comment 11

[Sushil]

(In reply to Jerry Shih[:jerry] from comment #10)
> Hi Sushil and Diego,
> 
> In comment 0:
> "Hence there is no need to wait on corresponding retire fence in
> HwcComposer2D in next draw cycle"
> 
> https://bugzilla.mozilla.org/attachment.cgi?id=8339654&action=diff#a/widget/
> gonk/HwcComposer2D.cpp_sec4 ,
> 
> We wait for a fence mPrevReleaseFds[0] or the retire fence mPrevRetireFence.
> I think the wait time of mPrevRetireFence is bigger than just one layer
> fence mPrevReleaseFds[0].

Signaling of retire fence denotes that contents on display have been replaced and driver is no longer reading the buffers. That's why we wait on retire fence otherwise we need to loop through HWC layer list and wait on the release fence of each HWC layer in the list (not just mPrevReleaseFds[0]) since driver is reading all of them. BTW, in the old code, mPrevReleaseFds[0] was used to store retire fence. I just added a new member "mPrevRetireFence" specifically for it.

> This patch waits the retire fence. I'm confused that how we save the wait time.

This patch saves the time only during GPU Composition. For HWC Composition, we need to wait on retire fence of previous draw cycle. This wait in HwcComposer2D is just an intermittent work-around until Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=925444 gets fixed.

Comment 12

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Sushil from comment #11)
> (In reply to Jerry Shih[:jerry] from comment #10)
> > This patch waits the retire fence. I'm confused that how we save the wait time.
> 
> This patch saves the time only during GPU Composition. For HWC Composition,
> we need to wait on retire fence of previous draw cycle. This wait in
> HwcComposer2D is just an intermittent work-around until Bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=925444 gets fixed.

Sorry, I'm still confused that how we saves gpu composition time.
The difference of patch is that we wait the fence from mPrevReleaseFds[0] to mPrevRetireFence.

If we use gpu for all layer composition, I think that we never block at fence->wait().
The retire fence's fd is -1.

The call stack:
HwcComposer2D::TryRender()
  ->TryHwComposition()
    ->fence->wait(1000)

Comment 13

[Diego Wilson [:diego]]

Hello Jerry,

Before this patch mPrevReleseFds[0] was always set to the previous retire fence and after this patch it's only held in mPrevRetireFence. So we've always waited for the previous retire fence on MDP composition.

As Sushil points out, the difference is only for GPU composition.

Before this patch we waited for the previous retire fence even in GPU comp because we always check mPrevReleaseFds. And mPrevReleaseFds always held at least the previous retire fence.

After this patch we do not wait for the previous retire fence anymore in GPU comp. This is because in GPU comp mPrevReleaseFds will be empty now that it doesn't contain the previous retire fence.

Comment 14

[Sushil]

(In reply to Jerry Shih[:jerry] from comment #12)
> (In reply to Sushil from comment #11)
> > (In reply to Jerry Shih[:jerry] from comment #10)
> Sorry, I'm still confused that how we saves gpu composition time.
> The difference of patch is that we wait the fence from mPrevReleaseFds[0] to
> mPrevRetireFence.
> 
> If we use gpu for all layer composition, I think that we never block at
> fence->wait().
> The retire fence's fd is -1.
> 

+    if (mList->retireFenceFd >= 0) {
+        if (!mPrevReleaseFds.IsEmpty()) {
+            mPrevRetireFence = mList->retireFenceFd;
+        } else { // GPU Composition
+            close(mList->retireFenceFd);
+        }

Yes, in ideal world we expect mList->retireFenceFd to be -1 in case of GPU Composition, so as per new code, if it is -1, we do not store and we do not wait in next cycle. But in my testing, I noticed that retire fence is not -1 in case of GPU Composition, so we were storing it to wait in next cycle (wait time was ~ 6 to 8 msecs) and it was unnecessary since FB layer is the only layer used during GPU Composition and we were setting release fence fd of FB layer on FB surface which should take care of waiting on it. So waiting on corresponding retire fence in next draw cycle was not required. As I had said, we will remove this "store and wait" stuff from HwcComposer2D once Bug # 925444 is resolved.

Comment 15

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Thanks, Diego and  Sushil.

> Yes, in ideal world we expect mList->retireFenceFd to be -1 in case of GPU
> Composition, so as per new code, if it is -1, we do not store and we do not
> wait in next cycle. But in my testing, I noticed that retire fence is not -1
> in case of GPU Composition.

If we use GPU Composition, we will not enter the Commit() function.
We just return at http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#490 .

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#481
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#494

So, we never meet the fence->wait() when we use gpu composition.
Is my thought right?

Comment 16

[Diego Wilson [:diego]]

(In reply to Jerry Shih[:jerry] from comment #15)
> If we use GPU Composition, we will not enter the Commit() function.
> We just return at
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.
> cpp#490 .

In GPU composition HwcComposer2d:TryRender() will fail and skip Commit(). However after the GPU composes to the back frame buffer it swaps that buffer using HwcComposer2d:Render() which then calls Commit():

http://dxr.mozilla.org/mozilla-central/source/gfx/gl/GLContextProviderEGL.cpp#527