869696 - Using Gralloc causes bad performance on the Geeksphone Peak

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Chris Lord [:cwiiis]]

Building B2G from master has serious performance issues (wrt framerate) on the Geeksphone Peak (and I've heard on the Keon, but I've not tested it) - however, building B2G with master Gaia but the gecko-18 branch of Gecko has very respectable performance, suggesting a gecko issue.

I've not debugged to see where this issue lies yet.

Comment 1

[Jason Smith [:jsmith]]

We haven't really started triaging koi yet. This isn't actionable in itself yet to be nomed yet (no clear scope associated with the bug). Pulling the koi? nom since this doesn't end up in our high priority QA bucket and doesn't have clear scope.

In order to get a regression window here, we're going to need accurate STR, expected results, and actual results that can be used to diagnose the issue to get a window on.

Chris - Can you provide a clear STR to use for the regression window?

Comment 2

[Chris Lord [:cwiiis]]

(In reply to Jason Smith [:jsmith] from comment #1)
> We haven't really started triaging koi yet. This isn't actionable in itself
> yet to be nomed yet (no clear scope associated with the bug). Pulling the
> koi? nom since this doesn't end up in our high priority QA bucket and
> doesn't have clear scope.
> 
> In order to get a regression window here, we're going to need accurate STR,
> expected results, and actual results that can be used to diagnose the issue
> to get a window on.
> 
> Chris - Can you provide a clear STR to use for the regression window?

STR:

1. Let phone boot
2. Swipe to everything.me
3. Swipe back to home

If you do this with the gecko-18 branch, performance is pretty good - on master, performance is *dramatically* worse. It should be pretty self-evident, especially if you can compare them side-by-side.

Note, this applies to just about every operation that results in an animation, but this is probaby the quickest and easiest one you can do.

Comment 3

[Chris Lord [:cwiiis]]

Issue doesn't exist in the Gecko aurora branch.

Comment 4

[Chris Lord [:cwiiis]]

This is bug 868556. Seems this hasn't hit the git mirror yet(?) but it's on hg m-c, so I assume it's coming.

Comment 5

[Chris Lord [:cwiiis]]

I had this wrong - it seems I get terrible performance on the Geeksphone Peak *unless* I revert bug 868556...

This seems pretty unintuitive to me, perhaps the path is broken on that device? I'll do some cursory investigation, any tips welcome.

Comment 6

[Chris Lord [:cwiiis]]

Attachment: Use an alternative method to unlock gralloc textures

I guess this is a driver issue, but this seems as good a way as any that achieves the same effect but doesn't cause slowness.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

This is terrifying.

This means that we're creating and destroying GL textures everytime we composite. It's interesting to hear that this is faster on one device, but I have to wonder how it perform on others before I can r+ this.

Could you try a few tweaks to the current code instead. This code just binds the EGLImage returned by GetNullEGLImage. This is defined in GLContextProviderEGL.cpp:

    EGLImage GetNullEGLImage() MOZ_OVERRIDE
    {
        if (!mNullGraphicBuffer.get()) {
            mNullGraphicBuffer
              = new android::GraphicBuffer(
                  1, 1,
                  PIXEL_FORMAT_RGB_565,
                  GRALLOC_USAGE_SW_READ_NEVER | GRALLOC_USAGE_SW_WRITE_NEVER);
            EGLint attrs[] = {
                LOCAL_EGL_NONE, LOCAL_EGL_NONE
            };
            mNullEGLImage = sEGLLibrary.fCreateImage(EGL_DISPLAY(),
                                                     EGL_NO_CONTEXT,
                                                     LOCAL_EGL_NATIVE_BUFFER_ANDROID,
                                                     mNullGraphicBuffer->getNativeBuffer(),
                                                     attrs);
        }
        return mNullEGLImage;
    }

Could you check that the fCreateImage path is only hit once? Could you check if changing GRALLOC_USAGE_SW_READ_NEVER to GRALLOC_USAGE_SW_READ_OFTEN makes it better? Could you check if changing the (1, 1) size to (64, 64) makes it better?

Alternatively, if we're really going down the route of your patch:
 - could you check at least another device to see how it's affected? And/or consider doing this only on this particular driver (as identified, maybe, by GL strings)?
 - could you get feedback directly from specialists, such as Michael Vines or Diego Wilson, so we get to understand why this performs better?

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 749423 [details] [diff] [review]
Use an alternative method to unlock gralloc textures

Cancelling review for now; need more investigation.

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Benoit Jacob [:bjacob] from comment #7)
> This is terrifying.
> 
> This means that we're creating and destroying GL textures everytime we
> composite. It's interesting to hear that this is faster on one device, but I
> have to wonder how it perform on others before I can r+ this.

Fair comment. I don't see it as any more terrifying than targeting and untargeting the eglimage on every frame though - textures ought to be pretty lightweight until they have resources associated with them, and given we retarget every frame anyway...

I'll get results for the Keon as well - these are the only two devices I have at the moment.

> Could you try a few tweaks to the current code instead. This code just binds
> the EGLImage returned by GetNullEGLImage. This is defined in
> GLContextProviderEGL.cpp:
> 
>     EGLImage GetNullEGLImage() MOZ_OVERRIDE
>     {
>         if (!mNullGraphicBuffer.get()) {
>             mNullGraphicBuffer
>               = new android::GraphicBuffer(
>                   1, 1,
>                   PIXEL_FORMAT_RGB_565,
>                   GRALLOC_USAGE_SW_READ_NEVER |
> GRALLOC_USAGE_SW_WRITE_NEVER);
>             EGLint attrs[] = {
>                 LOCAL_EGL_NONE, LOCAL_EGL_NONE
>             };
>             mNullEGLImage = sEGLLibrary.fCreateImage(EGL_DISPLAY(),
>                                                      EGL_NO_CONTEXT,
>                                                     
> LOCAL_EGL_NATIVE_BUFFER_ANDROID,
>                                                     
> mNullGraphicBuffer->getNativeBuffer(),
>                                                      attrs);
>         }
>         return mNullEGLImage;
>     }
> 
> Could you check that the fCreateImage path is only hit once? Could you check
> if changing GRALLOC_USAGE_SW_READ_NEVER to GRALLOC_USAGE_SW_READ_OFTEN makes
> it better? Could you check if changing the (1, 1) size to (64, 64) makes it
> better?

So I tried every combination of things here - changing size, format, flags, separately and together, it made no difference. I can confirm the path only gets hit once.

> Alternatively, if we're really going down the route of your patch:
>  - could you check at least another device to see how it's affected? And/or
> consider doing this only on this particular driver (as identified, maybe, by
> GL strings)?

This sounds like a reasonable thing to do - will look at that.

>  - could you get feedback directly from specialists, such as Michael Vines
> or Diego Wilson, so we get to understand why this performs better?

I don't know who these people are, but I'll go about finding out...

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

I'm told that passing the bit GraphicBuffer::USAGE_HW_TEXTURE in the GraphicBuffer constructor would be a good idea too, since we are binding this buffer to a OpenGL texture.

Comment 11

[Chris Lord [:cwiiis]]

(In reply to Benoit Jacob [:bjacob] from comment #10)
> I'm told that passing the bit GraphicBuffer::USAGE_HW_TEXTURE in the
> GraphicBuffer constructor would be a good idea too, since we are binding
> this buffer to a OpenGL texture.

Tried this, no difference - in the Android code, unless the hardware doesn't support cached buffers (whatever they are), all buffers effectively have this flag: http://gitorious.org/0xdroid/frameworks_base/blobs/36088840d796fa745be14384e0fa96443fbc2363/libs/surfaceflinger/Layer.cpp#line374 http://gitorious.org/0xdroid/frameworks_base/blobs/36088840d796fa745be14384e0fa96443fbc2363/libs/surfaceflinger/Layer.cpp#line120

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

So in fact, it seems that what we're doing in Unlock() might be completely useless. Can you try just making Unlock() return; without doing anything? Check adb logcat for genlock failures (also try a debug build, it will soon assert if locking fails). If you don't get any genlock failure and rendering looks correct... then Unlock() was a waste of time all along!

Here's the theory behind this. The GL gets a read lock on the gralloc buffer not exactly in Lock(), but when it subsequently starts drawing with it. Likewise, it would release that read lock when it's actually finished drawing with it, regardless of Unlock(). The reason why we'd hopefully not get a genlock failure the next time that we try to lock the gralloc buffer for write to draw content into it, is that the handle that we have to the gralloc buffer is different from the one that the GL internally has to it, so when we lock it, it actually waits for the GL's lock to be released (by contrast, if we try to lock it for write while _we_ have a read lock on it, with the same handle, that fails immediately).

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Try this!

Does this work?

Comment 14

[Chris Lord [:cwiiis]]

(In reply to Benoit Jacob [:bjacob] from comment #13)
> Created attachment 750431 [details] [diff] [review]
> Try this!
> 
> Does this work?

Unfortunately not - just returning in ::Unlock results in large pauses, occasional corruption and this on logcat:

E/libgenlock( 1263): perform_lock_unlock_operation: GENLOCK_IOC_LOCK failed (lockType0x1, err=Connection timed out fd=87)
E/msm7627a.gralloc( 1263): gralloc_lock: genlock_lock_buffer (lockType=0x2) failed
W/GraphicBufferMapper( 1263): lock(...) failed -22 (Invalid argument)

Comment 15

[Chris Lord [:cwiiis]]

bjacob sent me a patch to instrument libgenlock.so that aborts on failed locks. This is a stack trace for the first abort: http://www.pastebin.mozilla.org/2410230

I can confirm that the abort only happens when ::Unlock doesn't do anything - deleting the texture or retargeting the eglimage are both fine.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Oh, right --- I think I remember now, that in addition to the read lock that the GL itself places on the gralloc texture it's sampling from, what we do in Lock() has by itself the effect of placing a read lock on the gralloc buffer; that would be why Unlock() is actually needed.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Add Adreno (TM) 205 to renderers

This adds "Adreno (TM) 205" to the renderers, as this is the renderer string of the Peak, and I assume other affected devices. I don't have an "Adreno 205" device to see if that also needs the work-around.

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Use an alternative method to unlock gralloc textures on AdrenoTM205

The same patch, altered so it only affects AdrenoTM205.

I tried some other methods of unlocking the texture, but this was the only one that was fast. It'd be great to know a method of unlocking without retargeting/deleting anything, I suspect there are tangible gains to be had here.

Comment 19

[Chris Lord [:cwiiis]]

needinfo'ing mvines and dwilson - do you guys know of any faster way to get the GL driver to unlock the gralloc buffer besides retargeting the eglimage or deleting the texture?

If we could do it without retargeting or deleting, ::Lock could pretty much turn into a nullop, which would be great :)

Comment 20

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

I think it would be premature to generalize problems seen on the Peak to all A205 devices.  I suggest you try this on a Leo, Buri, or Ikura device first.  Unagi or Otoro could be used in a pinch but would not be ideal.

Comment 21

[Chris Lord [:cwiiis]]

(In reply to Michael Vines [:m1] [:evilmachines] from comment #20)
> I think it would be premature to generalize problems seen on the Peak to all
> A205 devices.  I suggest you try this on a Leo, Buri, or Ikura device first.
> Unagi or Otoro could be used in a pinch but would not be ideal.

The only other device I have is a Keon, can someone help with this testing? Also, is this method of unlocking really any worse than retargeting the eglimage?

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 750924 [details] [diff] [review]
Part 2 - Use an alternative method to unlock gralloc textures on AdrenoTM205

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

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +839,4 @@
>     * i.e. before the next time that we will try to acquire a write lock on the same buffer,
>     * because read and write locks on gralloc buffers are mutually exclusive.
>     */
> +  if (mGL->Renderer() == GLContext::RendererAdrenoTM205) {

Eh. I'll be honest, I would much much rather have us figure out what is actually slow with the current code on this driver... but alright, go for it.

Please at least add a comment.

Comment 23

[Chris Lord [:cwiiis]]

Comment added, pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1caf7322f918
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e9b99639f8


Another out-of-the-blue hypothesis: Perhaps retargeting causes some kind of flush? I wonder if deferring unlocking until after swap would make any difference?

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1caf7322f918
https://hg.mozilla.org/mozilla-central/rev/e0e9b99639f8

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

So here in the Taipei work week, people said exactly what Michael said in comment 20: that the Peak and Keon devices specifically are known to have special, bad drivers even though the GL strings are the same. So my r+'ing here was premature. Could you at least do the followup work of doing this for the Peak device specifically instead of basing this on GL strings? (Even better, of course, would be to do the follow-up work of going to the bottom of why this was slow on Peak).

Comment 26

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Chris Lord [:cwiiis] from comment #23)
> Another out-of-the-blue hypothesis: Perhaps retargeting causes some kind of
> flush? I wonder if deferring unlocking until after swap would make any
> difference?

That's worth looking into.

Comment 27

[Chris Lord [:cwiiis]]

Looks like this work-around was removed and gralloc is back to performing really terribly on the Peak. Let's just disable it and rely on the tiles backend work making it fast again.

Comment 28

[Chris Lord [:cwiiis]]

Attachment: Disable gralloc on peak

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 777066 [details] [diff] [review]
Disable gralloc on peak

It be nice to have this work if the peak gets updated drivers, but I'm guessing we don't have a way of getting reliable driver versions from the driver?

Comment 30

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/55768d72cb11

We don't, as far as I know, have a reliable way of getting driver versions from the driver and I'm not confident that we have enough employees testing on this device to stop it from breaking frequently anyway.

Given the decision is to go with the tiles layer backend long-term, I think this is a reasonable thing to do for now to give users better performance.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/55768d72cb11

Comment 32

[Geeksphone]

Disable gralloc makes that on 1.2 version says that the browser not support WebGL.

We tested enabling gralloc and seems to work fine without black screen on browser issue and performing very well for example on cubevid.

Regards.

Comment 33

[Julien Wajsberg [:julienw]]

Should we reenable gralloc on keon/peak ? This causes issues like bug 928398...

Comment 34

[Geeksphone]

No, this issue is because it's NOT enabled. See comment 3 in this bug.

Yes, you need to reenable gralloc, it's fixed here: https://bugzilla.mozilla.org/show_bug.cgi?id=912134

Comment 35

[Julien Wajsberg [:julienw]]

Geeksphone: yep, sorry, that's what I meant :) "this" was "gralloc not enabled" ;)

Comment 36

[Geeksphone]

Yes, that's correct.

We enabled it in internal builds and works flawlessly. It's time to reenable it.

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Alright, Chris, do you want to take care of that?

Comment 38

[Chris Lord [:cwiiis]]

I don't have a convenient B2G tree at the moment, but seeing as I'm blocking on tests, I'll take a look.

Comment 39

[Chris Lord [:cwiiis]]

Attachment: Enable gralloc on peak

Tested it, on a very quick test, nothing appears to break horribly.

Comment 40

[Julien Wajsberg [:julienw]]

Asking koi for the reenable fix, I think we can reenable it in 1.2 too, as the fixing bug (bug 912134) was koi+.

(triagers: the status is resolved/fixed because of the first patch that was to disable gralloc on peak)

Comment 41

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6a1e1f686d

Comment 42

[Preeti Raghunath(:Preeti)]

Please request the b2g26-approval for the same.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

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

Comment 44

[Chris Lord [:cwiiis]]

Comment on attachment 8333911 [details] [diff] [review]
Enable gralloc on peak

Requesting b2g26 approval, as requested.

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Possibly worse performance on Geeksphone Peak
Testing completed: Tested locally, Geeksphone have commented that it works fine
Risk to taking this patch (and alternatives if risky): Risk of bad behaviour on Geeksphone Peak due to flaky drivers/presumptuous code
String or UUID changes made by this patch: None

Comment 45

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #40)
> Asking koi for the reenable fix, I think we can reenable it in 1.2 too, as
> the fixing bug (bug 912134) was koi+.
> 
> (triagers: the status is resolved/fixed because of the first patch that was
> to disable gralloc on peak)

Forgot to add the blocking decision from triage here - this isn't a blocker, as this doesn't target a production device. We might consider taking this on an approval, however.

Comment 46

[Julien Wajsberg [:julienw]]

Jason, I thought it was too late for approval for 1.2?

Comment 47

[Julien Wajsberg [:julienw]]

(In reply to Chris Lord [:cwiiis] from comment #44)
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: Possibly worse performance on Geeksphone Peak

User impact if declined: Picking an image from the Gallery doesn't work on Peak.

> Testing completed: Tested locally, Geeksphone have commented that it works
> fine
> Risk to taking this patch (and alternatives if risky): Risk of bad behaviour
> on Geeksphone Peak due to flaky drivers/presumptuous code
> String or UUID changes made by this patch: None

Comment 48

[Preeti Raghunath(:Preeti)]

Too late for 1.2 so yes moving to 1.3

Comment 49

[bhavana bajaj [:bajaj]]

(In reply to Preeti Raghunath(:Preeti) from comment #48)
> Too late for 1.2 so yes moving to 1.3

Looks like this has already landed on m-c, so removing the blocking 1.3 flag here.

Comment 50

[bhavana bajaj [:bajaj]]

(In reply to Julien Wajsberg [:julienw] from comment #46)
> Jason, I thought it was too late for approval for 1.2?


Correct, we have crossed the phase of uplifting patches that are not blockers.

Comment 51

[Julien Wajsberg [:julienw]]

Bhavana, Preeti,

this patch is important for a basic functionality on Geeksphone Peak (see comment 47). As you can easily see on the patch, the patch will have zero implication on other phones. Therefore, how can we move forward on having this on the 1.2 branch?

Thanks,

Comment 52

[Julien Wajsberg [:julienw]]

Or should we say to Peak users: "please don't use the 1.2 branch on Peak, it won't work correctly"?

Comment 53

[bhavana bajaj [:bajaj]]

Firstly apologies on late response here, have been out.

Making an "exception" here on granting approval here which we typically don't do at this point in the cycle as this is a)impacts peak only b)regression causing breakage on an important basic functionality.

Comment 54

[Julien Wajsberg [:julienw]]

thanks!

Flagging Ryan for uplifting then!

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ed5db1fd8220