bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Gralloc gives us buffers of stride 288 when we ask for a buffer of width 256

NEW
Unassigned

Status

()

Core
Graphics
4 years ago
4 years ago

People

(Reporter: jrmuizel, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

(Reporter)

Description

4 years ago
This can cause substantial wastage. i.e. with 38 tiles we loose 1.2 MB
(Reporter)

Comment 1

4 years ago
Diego, is there something we can do to avoid this? 288 seems like a weird stride to require.
Flags: needinfo?(dwilson)
(Reporter)

Comment 2

4 years ago
Especially since strides of 128, 160, 192, 224, 288 and 320 all work.

Updated

4 years ago
Whiteboard: [MEMSHRINK]
What device is this, and what build?
(Reporter)

Comment 4

4 years ago
(In reply to Eric Rahm [:erahm] from comment #3)
> What device is this, and what build?

This is Flame. How do I determine what build it is?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> (In reply to Eric Rahm [:erahm] from comment #3)
> > What device is this, and what build?
> 
> This is Flame. How do I determine what build it is?

B2g-flash-tool [1] has a script called |check_versions.sh| that should do the job. Really I'm just wondering if this is 1.4, 2.0, or 2.1.

Taking a quick look at libgralloc [2] it's possible that libadreno is giving us a some weird values. But it really depends which gralloc_allocator is being created in |gralloc_open|. The standard AOSP version would definitely give you 256 back regardless of format.

[1] https://github.com/Mozilla-TWQA/B2G-flash-tool
[2] git://codeaurora.org/platform/hardware/qcom/display/libgralloc
Jeff, is there a way to put a warning in Gecko when something like this happens?  At least when we ask for a power of 2 and get something else back? Is it worth it?
(Reporter)

Comment 7

4 years ago
(In reply to Eric Rahm [:erahm] from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> > (In reply to Eric Rahm [:erahm] from comment #3)
> > > What device is this, and what build?
> > 
> > This is Flame. How do I determine what build it is?
> 
> B2g-flash-tool [1] has a script called |check_versions.sh| that should do
> the job. Really I'm just wondering if this is 1.4, 2.0, or 2.1.

I was seeing this on 2.1

> 
> Taking a quick look at libgralloc [2] it's possible that libadreno is giving
> us a some weird values. But it really depends which gralloc_allocator is
> being created in |gralloc_open|. The standard AOSP version would definitely
> give you 256 back regardless of format.

I stepped through the code and we're getting the 288 from the adreno helper.
(Reporter)

Updated

4 years ago
Flags: needinfo?(mvines)
Hi James,

Do you know why this is happening? Is this expected?
Flags: needinfo?(mvines)
Flags: needinfo?(jhicks)
Flags: needinfo?(dwilson)
Whiteboard: [MEMSHRINK] → [MemShrink:P1]
We would also want to make sure this doesn't happen on QRD.
Flags: needinfo?(milan)

Comment 10

4 years ago
(In reply to Diego Wilson [:diego] from comment #8)
> Hi James,
> 
> Do you know why this is happening? Is this expected?

What image format is giving that behavior?
Flags: needinfo?(jhicks)
ni? on Jeff for comment 10
Flags: needinfo?(jmuizelaar)
(Reporter)

Comment 12

4 years ago
(In reply to James Hicks from comment #10)
> (In reply to Diego Wilson [:diego] from comment #8)
> > Hi James,
> > 
> > Do you know why this is happening? Is this expected?
> 
> What image format is giving that behavior?

I believe it's android::PIXEL_FORMAT_RGBA_8888
Flags: needinfo?(jmuizelaar) → needinfo?(jhicks)
(Reporter)

Comment 13

4 years ago
I checked and the format going into gralloc is format = 1

Comment 14

4 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> I checked and the format going into gralloc is format = 1

I looked into the gralloc code, and this padding is expected behavior on Adreno 305 for cache performance reasons.
Flags: needinfo?(jhicks)
Should we then switch the tile size to 224x224 instead, as the padding doesn't seem to be needed there? This is a 12.5% memory penalty, and as we're probably around 30 tiles on active content, we're wasting 1mb in those scenarios.
Flags: needinfo?(milan)
(Reporter)

Comment 16

4 years ago
(In reply to James Hicks from comment #14)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #13)
> > I checked and the format going into gralloc is format = 1
> 
> I looked into the gralloc code, and this padding is expected behavior on
> Adreno 305 for cache performance reasons.

Can you explain a little more about when we can expect to hit this extra padding and what the cause is so we can avoid wasting memory on it?
Flags: needinfo?(jhicks)
Indeed, cache sizes are usually powers of two so this 288 number seems curious.

Updated

4 years ago
See Also: → bug 1069534
mvines, can someone answer the question from comment 16?
Flags: needinfo?(jhicks) → needinfo?(mvines)
(Reporter)

Comment 19

4 years ago
"No reason why they shouldn't allocate to the width we pad to; in theory land those are the sweet (or at least close enough) spots for performance.  We don't always hit the best perf in all spots since padding gets pretty expensive (memory wise) to do that."

So we should be able to use 288 at least on the flame.
Flags: needinfo?(mvines)
Tony, if we were to change the preference for the tile size, we'd want focused testing to make sure we didn't introduce any regressions.  Do you think we can arrange for that?  Is there a good time?
Blocks: 1062395
Flags: needinfo?(tchung)
(In reply to Milan Sreckovic [:milan] from comment #20)
> Tony, if we were to change the preference for the tile size, we'd want
> focused testing to make sure we didn't introduce any regressions.  Do you
> think we can arrange for that?  Is there a good time?

I'd like to understand more of what the testing request is, and a timeline on when you're trying to land this.  i'm assuming its 2.2 work, but the blocking bug you added concerns me for 2.1.   Also, if developers can run No-jun's automated image tests against it, and any existing platform reftests, would that suffice upfront?    We can do some exploratory testing around the problem, but it would really need to wait until we wrap up 2.1 FC time period first.
Flags: needinfo?(tchung)
Discussed offline with milan about it, I should have time to test things out if this is for 2.2
Some numbers for Flame.  Moving to 288x256 tiles saves us between 1MB and 1.3MB.  Moving to 288x288 saves us around 0.5MB (and in one case actually uses more memory, by 100KB).

If these are the types of memory savings we don't care about for 2.1, and we probably don't, then I'm not worried about testing this.  We will just flip the pref and tell people to be on the lookout.  I was asking in the context of 2.1, but if ~1MB doesn't help our OOM situation, then it's an easy decision.
Blocks: 1073549
No longer blocks: 1062395
OS: Mac OS X → Gonk (Firefox OS)
Alright so after a bit of brainstorming we have the following questions:
- Which devices and renders are affect by this?

Depending on the answer it would leave us with:
- Detecting the stride padding on startup.
- Hardcoding this based on the renderer string (not ideal if this is just driver dependant).
- Asking the OEMs to select the best values. I'm not a fan of this approach because it's passing on more complexity to them and it means that what we test vs. what they ship is more different.

Let me look into what is affected by this.
The OEMs/partners do sometimes override prefs; if we decide to go that route, here's an example: https://www.codeaurora.org/cgit/quic/lf/b2g/build/tree/gaia_settings/distribution

Updated

4 years ago
Depends on: 1094442
Tiles now pick a better size where appropriate now that bug 1094442 has landed. This bug can track why this is happening in general.
Whiteboard: [MemShrink:P1] → [MemShrink:P2]
You need to log in before you can comment on or make changes to this bug.