Closed Bug 944069 Opened 6 years ago Closed 6 years ago

Remove unnecessary wait during GPU Composition.

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, firefox28 fixed)

RESOLVED FIXED
mozilla28
blocking-b2g 1.3+
Tracking Status
firefox28 --- fixed

People

(Reporter: sushilchauhan, Assigned: sushilchauhan)

References

Details

(Keywords: perf, Whiteboard: [c= p= s=2013.12.06 u=1.3])

Attachments

(1 file, 2 obsolete files)

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.
Attachment #8339483 - Flags: review?(dwilson)
Assignee: nobody → sushilchauhan
Blocks: 930299
Status: UNCONFIRMED → ASSIGNED
blocking-b2g: --- → 1.3+
Ever confirmed: true
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.
Attachment #8339483 - Flags: review?(dwilson)
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 ?
(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
Addressed review comments.
Attachment #8339483 - Attachment is obsolete: true
Attachment #8339599 - Flags: review?(dwilson)
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.
Attachment #8339599 - Flags: review?(dwilson) → review+
Keywords: checkin-needed
Priority: P1 → P3
Will upload HG friendly patch.
Keywords: checkin-needed
Uploading HG friendly patch.
Attachment #8339599 - Attachment is obsolete: true
Attachment #8339654 - Flags: review+
Keywords: checkin-needed
Priority: P3 → P2
Component: General → Widget: Gonk
Product: Firefox OS → Core
https://hg.mozilla.org/mozilla-central/rev/cea5ec120ed7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Keywords: perf
Whiteboard: [c= p= s=2013.12.06 u=1.3]
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!
Flags: needinfo?(sushilchauhan)
Flags: needinfo?(dwilson)
(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.
Flags: needinfo?(sushilchauhan)
(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)
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.
Flags: needinfo?(dwilson)
(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.
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?
(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
Flags: in-moztrap-
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.